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

frame.SampleNum() is wrong for the last frame of the stream #74

Open
MarkKremer opened this issue Aug 11, 2024 · 0 comments
Open

frame.SampleNum() is wrong for the last frame of the stream #74

MarkKremer opened this issue Aug 11, 2024 · 0 comments
Labels

Comments

@MarkKremer
Copy link
Collaborator

In #73 I added a commented out testcase to test that seeking to the last sample of the stream works.

At the moment of writing this doesn't work because frame.SampleNumber() doesn't return the correct value for the last frame of the stream. For a fixed-blocksize stream, the sample number is calculated by taking the frame number and multiplying it by the block size. However, from the spec:

The "blocking strategy" bit determines how to calculate the sample number of the first sample in the frame. If the bit is 0 (fixed-blocksize), the frame header encodes the frame number as above, and the frame's starting sample number will be the frame number times the blocksize. If it is 1 (variable-blocksize), the frame header encodes the frame's starting sample number itself. (In the case of a fixed-blocksize stream, only the last block may be shorter than the stream blocksize; its starting sample number will be calculated as the frame number times the previous frame's blocksize, or zero if it is the first frame).

So the sample number needs to be obtained some different way.

Side note: I found that the test file testdata/flac-test-files/subset/27 - old format variable blocksize file created with Flake 0.11.flac has a fixed block size, but stores sample numbers instead of frame numbers in the header.

Ignoring that test file, here are some thoughts about fixing it:

  • One option is to seek backwards in the Seek() function to get access to the previous frame, and always base the blocksize on that. This is most spec-like.
  • More efficient is to check the stream info. My assumption is that for a fixed stream, stream.Info.BlockSizeMax contains the block size of all frames except the last, and stream.Info.BlockSizeMin would match the last frame's block size. However, the spec states "(Minimum blocksize == maximum blocksize) implies a fixed-blocksize stream." so it may be worth checking that no files make the incorrect opposite implication and set both values to the same number for a fixed-blocksize stream even though the last frame is different.
  • This however does not fix frame.SampleNumber(). To make that work the Frame needs to store more or other information:
    • Instead of frame.Num being either the frame number or the frame start sample number, we could always store the sample number in it. This would be a breaking API change. During seeking, more information is available to calculate the field. During decoding, we'll need access to the previous frame or the stream info which isn't currently there. During encoding, we might be able to keep track of the current frame being encoded, or do the inverse calculation block number = frame.SampleNumber / stream.Info.BlockSizeMax if the stream info is already known at that point. I can imagine that there are some hurdles.
      • Moving the decoding to a dedicated Decoder struct (see under v2.x in Roadmap towards 2.x #33) could help because then the decoder has access to the stream info).
    • We could store both the frame number and sample number, but during seeking we don't know the frame number for non-fixed block sizes.
    • We could store frame.Num like it is now (containing either the block number or the sample number) in combination with a field that always contains the sample number. This would keep encoding easy but it seems like a weird API to expose to users.
    • We could just add a comment to frame.SampleNumber() stating this edge case, but that also doesn't seem like a good API. Although maybe it's OK for the low level API.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants