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

flac: Allow different sample rate between frames #287

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Gusted
Copy link

@Gusted Gusted commented Jun 10, 2024

FLAC encodes for each frame the sample rate of that frame, it is therefore to be expected that the sample rate changes between frames and is valid per FLAC's test vectors. This patch removes the sample rate check in strict_frame_header_check and updates the sample rate in the SignalSpec of the AudioBuffer after decoding a frame.

Ref: https://github.com/ietf-wg-cellar/flac-test-files/blob/main/uncommon/01%20-%20changing%20samplerate.flac

FLAC encodes for each frame the sample rate of that frame, it is
therefore to be expected that the sample rate changes between frames and
is valid per FLAC's test vectors. This patch removes the sample rate
check in `strict_frame_header_check` and updates the sample rate in the
SignalSpec of the AudioBuffer after decoding a frame.

Ref: https://github.com/ietf-wg-cellar/flac-test-files/blob/main/uncommon/01%20-%20changing%20samplerate.flac
@Gusted Gusted force-pushed the flac-change-sample-rate branch from 30d7e8d to 301ef9c Compare June 10, 2024 18:05
@Gusted Gusted changed the title flac: Allow diffrent sample rate between frames flac: Allow different sample rate between frames Jun 10, 2024
@Gusted
Copy link
Author

Gusted commented Jun 10, 2024

There is a test vector that can be used to verify this, but I don't see how the flac crate is tested, so any help would be appreciated.

@dedobbin
Copy link
Contributor

dedobbin commented Aug 1, 2024

Hey ! First of all, thanks a lot for providing the test file, it helps a lot.

I played around with it for a bit, it seems to me the sample rate is not applied correctly and i think this would require a deeper change.

symphonia-play does play the entire file, but the pitch goes up and down, implying the change in samplerate is not 'picked up'.
ffplay does actually plays it without pitch changes, respecting the changing rate, but when comparing the samples between ffmpeg and symphonia(-check) the samples are different. I find that a bit odd since sample rate should not have effect on how the samples are read, perhaps ffmpeg does a little trick there, but haven't looked deeper into it.

Also side note, vlc just stops playing when the samplerate changes, and (as to be expected) audacity does only apply the initial sample rate.

So yeah to make this work properly it seems to me a deeper change is needed, if this is desirable/viable, i hope @pdeljanov can say something about that based on this information.

@pdeljanov
Copy link
Owner

Thanks @Gusted for your PR.

Unfortunately, I am not a fan of making the AudioBuffer specification mutable since that means channels also becomes mutable. Changing channels without reallocating the underlying buffer accordingly (which would now be possible) would violate the invariants of AudioBuffer.

I think a fix for this may be best targeted for 0.6 (see the dev-0.6 branch). Not only does 0.6 completely rewrite the audio interfaces, but I think we also need to better specify the ResetRequired error since this is a case where the user should be notified that the audio format has changed.

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.

3 participants