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

Document how total samples in stream should be used #110

Closed
wader opened this issue Oct 20, 2021 · 5 comments · Fixed by #134
Closed

Document how total samples in stream should be used #110

wader opened this issue Oct 20, 2021 · 5 comments · Fixed by #134

Comments

@wader
Copy link

@wader wader commented Oct 20, 2021

I've seen FLAC files in the wild where non-streaminfo-block-size aligned number samples in a stream is signalled using total samples but the last frame does not have a variable block size. For example ffmpeg currently seems to decode all frame samples in this case and ignores total samples.

Also maybe good to make it clear what the MD5 sum should be based on, the total samples or all frame samples?

@ktmf01
Copy link
Collaborator

@ktmf01 ktmf01 commented Oct 20, 2021

As far as I know, total samples is just informational, and can be zero as well, meaning it is unknown. The blocksize in the frame header of the last block signals the length of the last block. Still, this needs to be clarified indeed.

@wader
Copy link
Author

@wader wader commented Oct 20, 2021

Ok. Would it be an error if a non-zero total samples does not match up with actual number of samples in all frames?

@ktmf01
Copy link
Collaborator

@ktmf01 ktmf01 commented Feb 14, 2022

I'm not sure anymore whether anything should be written on this topic except for a generic statement that the handling of non-conformant streams is left undefined and up to the decoder.

Currently libFLAC stops decoding after reaching the number of samples set in streaminfo, but ffmpeg doesn't.

@JeromeMartinez
Copy link
Collaborator

@JeromeMartinez JeromeMartinez commented Feb 14, 2022

On one side I consider that such field should be informational only because I see no reason the decoder should stop if there is more content, on the other side issue is that the "reference" decoder stops...

With FFV1 we faced some issues with old versions of the reference decoders and provided warnings. Maybe same here?

  • Consider the field as relevant for the MD5 hash value else informational
  • Warning that "some decoders" may stop the decoding after the count of decoded samples reaches the value in this field.

@ktmf01
Copy link
Collaborator

@ktmf01 ktmf01 commented Feb 15, 2022

The reference decoder stops because of performance reasons which may not be really relevant anymore

https://github.com/xiph/flac/blob/a2fe43f64e7ce8057fb274c64996568d69b301b8/src/libFLAC/stream_decoder.c#L1966-L1967

 	/* If we know the total number of samples in the stream, stop if we've read that many. */
	/* This will stop us, for example, from wasting time trying to sync on an ID3V1 tag. */

What about adding a general statement to the description of the streaminfo block, like this

The streaminfo metadata block contains technical information about the FLAC stream relevant for decoding. Decoder behavior in case of incorrect or incomplete information is left unspecified (i.e. up to the decoder implementation). A decoder MAY choose to stop further decoding in case the information supplied by the streaminfo metadata block turns out to be incorrect or invalid. A decoder accepting information from the streaminfo block (most significantly the maximum framesize, maximum blocksize, number of audio channels, number of bits per sample and total number of samples) without doing further checks during decoding of audio frames could be vulnerable to buffer overflows. See also the section on security considerations.

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 a pull request may close this issue.

3 participants