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 what is allowed to change between frames #109

Open
wader opened this issue Oct 20, 2021 · 5 comments · May be fixed by #138
Open

Document what is allowed to change between frames #109

wader opened this issue Oct 20, 2021 · 5 comments · May be fixed by #138

Comments

@wader
Copy link

@wader wader commented Oct 20, 2021

For example can one assume that for a valid FLAC stream sample rate, channel configuration and channel count must be the same for all frames and also must match what is specified in streaminfo?

@ktmf01
Copy link
Collaborator

@ktmf01 ktmf01 commented Oct 20, 2021

This is something that might be difficult to pin down. While it is impossible to decode a stream where the number of channels changes to WAV or similar formats, I've found this comment in the libFLAC code recently that suggests it was at some point intended to be valid. Here's another one.

Perhaps I should create a file that has some of these mid-stream changes, and try to see what flac and ffmpeg make of them?

@JeromeMartinez
Copy link
Collaborator

@JeromeMartinez JeromeMartinez commented Oct 20, 2021

Shouldn't STREAMINFO be just informational (when sample rate code, sample size are not 0)?
FFmpeg does not decode a file without STREAMINFO but flac command line does (but aborts if there is a change...) so it seems legit to change the "mandatory" word.

IMO all should be allowed to change between frames (good for streaming!) and it should be documented that STREAMINFO is for quick information and hash only. Especially when tools e.g. FFmpeg don't change the STREAMINFO part when they mix to e.g. Matroska with a limitation to the duration (not the full input file) which make the sample count and MD5 wrong.

Actually, I am reluctant to limit the spec if the issue is the tools rather than the format itself i.e. if a streamed FLAC file can support sample rate change it should be considered as valid. Maybe adding this limitations (no change between frames, STREAMINFO and "fLaC" magic value present) to the "Subset" part i.e. "Subset" part is for maximum compatibility (beside the crashes found in players due to memory allocations and so on...) else we try to go as far as possible (e.g. make it streamable) with the bitstream?

About the mix in e.g. WAV (or Matroska): it is a limitation of the container, not a limitation of FLAC, limitation of some containers should not impact FLAC (another container could handle the feature), for example AAC in TS has no problem to change channels etc, why not FLAC?

@ktmf01
Copy link
Collaborator

@ktmf01 ktmf01 commented Oct 20, 2021

IMO all should be allowed to change between frames (good for streaming!) and it should be documented that STREAMINFO is for quick information and hash only. Especially when tools e.g. FFmpeg don't change the STREAMINFO part when they mix to e.g. Matroska with a limitation to the duration (not the full input file) which make the sample count and MD5 wrong.

I do see a potential problem here: with a fixed blocksize stream, a transition has to happen on a block boundary, which isn't really lossless behaviour. Perhaps this should only be allowed on variable blocksize streams? Blocksizes smaller than 16 are only allowed on the last block, but perhaps this should also be allowed in variable blocksize streams upon such a change in parameters?

@JeromeMartinez
Copy link
Collaborator

@JeromeMartinez JeromeMartinez commented Oct 20, 2021

In practice we have tons of issues there :), because it looks like the format was built with linear compression and no cut in mind (= write once, never remux) and AFAIK tools which do a cut don't change the CODED NUMBER and don't check for compatibility if STREAMINFO is same but BLOCKING STRATEGY is not (they just concatenate).
And actually it seems to not be a problem for most decoders (ignoring the CODED NUMBER?).

Usage goes farer than spec, so it is spec (and flac command tool) vs practice, I think we should document practice there (CODED NUMBER to be ignored? using "MAY" wording?). And put all about a "perfect stream" in the "Subset" or somewhere about the difference between a "native" encoding and a remux/cut/streaming.

@ktmf01
Copy link
Collaborator

@ktmf01 ktmf01 commented Feb 15, 2022

What about adding this to the section dissecting the frame header

Each frame header stores the audio sample rate, number of bits per sample and number of channels independently of other frame headers. This was done to simplify streaming of FLAC files over the internet (for example in internet radio) but it also allows these properties to change mid-stream. Because not all environments in which FLAC decoders are used are able to cope with changes to these parameters during playback, a decoder MAY choose to stop decoding on such a change. A decoder that does not check for such a change could be vulnerable to buffer overflows. See also the section on security considerations.

And this to the coded number section

In case the coded number is a frame number, it MUST be equal to the number of frames preceding the current frame. In case the coded number is a sample number, it MUST be equal to the number of samples preceding the current frame. In a stream where these requirements are not met, seeking is not (reliably) possible.

A decoder that relies on the coded number during seeking could be vulnerable to buffer overflows or seeking could get stuck in an infinite loops in case it seeks in a stream where the coded numbers are non-consecutive or otherwise invalid. See also the section on security considerations.

@ktmf01 ktmf01 linked a pull request Feb 23, 2022 that will close this issue
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