Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use ogg_page_granulepos to get number of samples written for Ogg FLAC #743

Merged
merged 3 commits into from
Sep 26, 2024

Conversation

ziplantil
Copy link
Contributor

The granule position of the Ogg page will tell us how many audio frames are included in the page that we receive from libogg. Therefore, to count the number of frames per page, we simply check the granule position and subtract it from the granule position of the previous page.

A signed 64-bit integer type is used for the granule position, as libogg also uses one as the return type of ogg_page_granulepos. An unsigned 32-bit integer is sufficient to contain the number of frames, as the granule position of a page is determined by the last complete packet in that page, the number of frames in each packet is limited by the 32-bit unsigned integer type internally in libflac, and an Ogg page, at most 65,307 bytes, can only contain at most ~20 million frames (tested experimentally with pure silence).

The granule position of the Ogg page will tell us how many audio frames are
included in the page that we receive from libogg. Therefore, to count the
number of frames per page, we simply check the granule position and subtract it
from the granule position of the previous page.

A signed 64-bit integer type is used for the granule position, as libogg also
uses one as the return type of ogg_page_granulepos. An unsigned 32-bit integer
is sufficient to contain the number of frames, as the granule position of a
page is determined by the last complete packet in that page, the number of
frames in each packet is limited by the 32-bit unsigned integer type internally
in libflac, and an Ogg page, at most 65,307 bytes, can only contain at most
~20 million frames (tested experimentally with pure silence
with maximum compression level).
@ktmf01
Copy link
Collaborator

ktmf01 commented Sep 21, 2024

I don't think this works when granulepos is UINT64_MAX (which is when a page only contains no packet ends). Also, I don't see the advance of doing it this way? Finally, I think the use of the word 'frames' is confusing, it usually means something else in FLAC context.

@ziplantil
Copy link
Contributor Author

ziplantil commented Sep 21, 2024

I don't think this works when granulepos is UINT64_MAX (which is when a page only contains no packet ends).

That is a relatively easy thing to fix.

I think the use of the word 'frames' is confusing, it usually means something else in FLAC context.

Good point, it could just be changed to samples instead.

Also, I don't see the advance of doing it this way?

It is the more 'correct' approach, because this actually checks how many samples are supposed to be present in each Ogg page.

However, reading the code again, it might indeed not actually be necessary, since we are actually only feeding one packet per each call before we loop over the pages that can be output.

Therefore, this implementation and the previous one will be equivalent when the packets are small enough to fit within a page (i.e. when we are not submitting more than one page for each packet). With large packets, if we fix the -1 case, the only difference will be when the correct number of samples for that packet can actually be provided (for the first page in the old implementation, or for the last page in this one).

I would personally prefer the last one, since even though the samples parameter is merely meant to be advisory, that value could be used by client code and be used for timing purposes. Having the number of samples be provided with the first page (when the packet is not yet completely available), as opposed to the last, could lead to issues then.

A granule position of -1 on a page means that no packets finish on that page.
If we encounter this value, tell the callback that the page contains 0
samples.

Change the terminology to samples instead of frames, as the term 'frame'
has another meaning in context of FLAC.
@ziplantil ziplantil force-pushed the ziplantil/ogg-flac-page-granule-pos branch from 6f4c0a4 to 9d7a63e Compare September 21, 2024 19:50
@ktmf01
Copy link
Collaborator

ktmf01 commented Sep 24, 2024

If I understand this correctly, the main difference is in when samples are counted towards a page: previously samples in an incomplete packet were counted in the first page containing that packet, and with this change, samples in an incomplete packet are counted in the last page contained that packet.

Could you take a look at the commit I added?

@ziplantil
Copy link
Contributor Author

The commit looks good to me.

@ktmf01
Copy link
Collaborator

ktmf01 commented Sep 26, 2024

@ziplantil I see you're using an email address to sign your commits with, that is hidden in github. Therefore, when I squash your commits with the github interface, they get signed with a @users.noreply.github.com email address.

Do you have a preference? If you'd rather have the squashed commit signed with your regular email, I will squash these commits manually, no problem.

@ziplantil
Copy link
Contributor Author

I do not have a preference - whichever is easier for you.

@ktmf01 ktmf01 merged commit 9dd1b0a into xiph:master Sep 26, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants