search

pdeljanov / Symphonia

Pure Rust multimedia format demuxing, tag reading, and audio decoding library
Mozilla Public License 2.0
2.42k stars 144 forks source link

flac: Allow different sample rate between frames #287

Open Gusted opened 5 months ago

Gusted commented 5 months ago

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 commented 5 months ago

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 commented 3 months ago

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 commented 3 months ago

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.