fix(soundsources): fix FFMPEG MP3 playback #11923

[Swiftb0y]

fixes #11923

fix adopted from uklotzde@e0f00c1
Thank you Uwe

[Holzhaus]

SoundSourceProxyTest.firstSoundTest fails now.

[daschuer]

This is the failing test:

2023-09-26T22:35:24.4704302Z /home/runner/work/mixxx/mixxx/src/test/soundproxy_test.cpp:862: Failure
2023-09-26T22:35:24.4704660Z Expected equality of these values:
2023-09-26T22:35:24.4704940Z   firstSoundSample
2023-09-26T22:35:24.4705178Z     Which is: 1168
2023-09-26T22:35:24.4705434Z   ref.firstSoundSample
2023-09-26T22:35:24.4705686Z     Which is: 1166
2023-09-26T22:35:24.4706274Z /home/runner/work/mixxx/mixxx/src/test/id3-test-data/cover-test-itunes-12.3.0-aac.m4a FFmpeg 4.4.2-0ubuntu0.22.04.1
2023-09-26T22:35:24.4706758Z [mov,mp4,m4a,3gp,3g2,mj2 @ 0x563071354d40] stream 0, timescale not set
2023-09-26T22:35:24.4707545Z info [0x563071259d30] Opened file "/home/runner/work/mixxx/mixxx/src/test/id3-test-data/cover-test-ffmpeg-aac.m4a" using provider "FFmpeg 4.4.2-0ubuntu0.22.04.1"
2023-09-26T22:35:24.4708111Z /home/runner/work/mixxx/mixxx/src/test/soundproxy_test.cpp:862: Failure
2023-09-26T22:35:24.4708454Z Expected equality of these values:
2023-09-26T22:35:24.4708731Z   firstSoundSample
2023-09-26T22:35:24.4708980Z     Which is: 1166
2023-09-26T22:35:24.4709220Z   ref.firstSoundSample
2023-09-26T22:35:24.4709469Z     Which is: 1112
2023-09-26T22:35:24.4710035Z /home/runner/work/mixxx/mixxx/src/test/id3-test-data/cover-test-ffmpeg-aac.m4a FFmpeg 4.4.2-0ubuntu0.22.04.1

The new offset does no longer match the CoreAudio version, the proposed fix is probably not correct.

[uklotzde]

FFmpeg documentation is scarce. The Chromium sources suggest that the start time depends on the codec:

https://chromium.googlesource.com/chromium/src.git/+/master/media/filters/ffmpeg_demuxer.cc#104

[JoergAtGithub]

This just reverts the bugfix from #11879. We need a fix, which fixes both bugs.

[Swiftb0y]

This just reverts the bugfix from #11879. We need a fix, which fixes both bugs.

What would that fix be then? av_seek_frame fails for mp3 files with a negative seekIndex. Just clamp for mp3 files? conditionally retry seeking with a clamped value if it fails with a negative index?

[Swiftb0y]

I'm not terribly familiar with all the details of audio decoding so I'd appreciate some guidance here. Fact is that mp3 playback is currently broken for -DFFMPEG=ON -DMAD=OFF builds (on linux at least).

[daschuer]

The issue is that the offset of lib123 and ffmpeg was anyway broken before. Maybe the additional failure is just a symptom of that original issue.

If we want to offer a migration path to the floating point library that involves a bit less CPU during encoding, we need to fix that.

If I remember correctly the ffmpeg implementation plays the initial Mp3 Info Frame that has no usable audio data.
This is handled her in case of libmad:

mixxx/src/sources/soundsourcemp3.cpp

Line 329 in 0119d85

mp3InfoTagSkipped = true;

I am afraid this is not trivial and needs a good amount of testing. If we do it wrong the beats and cues are off.

I am personally not interested to move to lib123, because even if the offset issue is fixed, it puts all my metadata on a risk in case of differently handled decoding issues.

[Swiftb0y]

you're confusing me, what does soundsourcemp3 have to do with soundsourceffmpeg?

I am personally not interested to move to lib123, because even if the offset issue is fixed, it puts all my metadata on a risk in case of differently handled decoding issues.

I understand that, but for people like me (and like @uklotzde too) that have been using ffmpeg as their only mp3 decoder, the current solution is even worse as we're unable to play mp3 files at all. Switching away from ffmpeg is not an option for the same reason as you're not interested in switching to ffmpeg. I'm not particularly keen on fixing the differences in decoder offset either, I'm only interested in restoring the previous (possibly "wrong" offset) behavior that didn't break ffmpeg.

As far as I can tell, that behavior is just that the seekIndex was clamped to not be negative, right?

[Swiftb0y]

fyi, I replaced the current commit on the branch with a different commit that just ensures all audio test files can be opened. Fortunately it succeeds on CI but is obviously broken for me (highlighting the issue I'm experiencing).

[uklotzde]

Using a negative seek index that is not justified by the decoded packet/frame data is undefined behavior. It seems to work for decoding AAC files, but fails for MP3 (and maybe others).

Unfortunately the negative seek offset cannot be obtained even when decoding the first packet. If you set VERBOSE_DEBUG_LOG to true you could examine the pts time stamps of the test files, you will notice that they are all 0. So even the Chromium patch doesn't seem to fix that.

The previous "fix" was wrong and slipped through unnoticed just because the tests were insufficient.

[uklotzde]

I would like to emphasize that my intention is not to accuse anyone! Platform-independent decoding and portable offsets are still unsolved problems and I don't have any solution at hand.

Extending the firstSoundTest by covering all decoders instead of only the primary one shows no clear winner.

[uklotzde]

MAD will open the file successfully and FFmpeg is skipped. No errors, unless you disable all other MP3 decoders that have a higher priority than FFmpeg.

FFmpeg has the lowest priority and is only ever used as a fallback if anything else fails. Although I personally consider it as the must reliable decoder for MP3, MP4, and ALAC (at least on Linux/Windows).

Unfortunately, the lazy test behavior was needed for the AAC/ALAC files that both share the same .m4a file extension.

[Swiftb0y]

MAD will open the file successfully and FFmpeg is skipped. No errors, unless you disable all other MP3 decoders that have a higher priority than FFmpeg.

Yes, that was the intention here. It's not super important what decoder is used for what format, its only important that we can open the file with at least one. The other tests are care about the actual encoder and what offset it introduces/requires.

Although I personally consider it as the must reliable decoder for MP3, MP4, and ALAC (at least on Linux/Windows).

I agree, thats why I compile with ffmpeg and without MAD.

Unfortunately, the lazy test behavior was needed for the AAC/ALAC files that both share the same .m4a file extension.

What do you mean with "lazy test behavior"? The fallback approach?

[uklotzde]

Most tests don't fail if opening fails. You don't notice if a file that could previously be opened can no longer be decoded. Otherwise you would need to explicitly which combinations are expected to fail and should be skipped. This would be preferable to detect regressions.

[daschuer]

I understand that, but for people like me (and like @uklotzde too) that have been using ffmpeg as their only mp3 decoder, the current solution is even worse as we're unable to play mp3 files at all.

Ok, so I think we have an issue here with your private build, not with an official Mixxx build.

The test per platform (not per encoding library) has been installed to exactly detect theses issues. If one is changing to ffmpeg the test should fail until the offset issue is fixed. This prevents us from package maintainers or other user building Mixxx with a shifted offset invalidating all their beats and cues. This also will detect if ffmpeg swithes to a different library for a certain audio format. This is unfortunately out of our control with ffmpeg.

So you may apply a fix that restores the broken lib123 offset, that your cues and beats are still valid.
Than you also need to skip the offset test in your local build. We cannot make this offset the standard for all users.

[Swiftb0y]

Ok, so I think we have an issue here with your private build, not with an official Mixxx build.

Yes, thats true. That's also why I'm not looking for a perfect fix.

The test per platform (not per encoding library) has been installed to exactly detect theses issues. If one is changing to ffmpeg the test should fail until the offset issue is fixed.

So you may apply a fix that restores the broken lib123 offset, that your cues and beats are still valid.

I assume you mean libmpg123 which I assume is used by ffmpeg in the background? As I said, I'm not super familiar with the encoder infrastructure but I assume clamping the seekIndex will be enough, right?

[daschuer]

Just test it.
If the fix introduces an offset and invalidates your cues, can be checked via the "first sound moved warning" in the log output.

[uklotzde]

Just test it. If the fix introduces an offset and invalidates your cues, can be checked via the "first sound moved warning" in the log output.

Breaking any decoder that is used as a fallback is not a "private issue". It does not work as before, even if only in rare cases that probably doesn't affect many users.

[Swiftb0y]

Just test it.
If the fix introduces an offset and invalidates your cues, can be checked via the "first sound moved warning" in the log output.

Just clamping the seekIndex seems to work:

debug [CachingReaderWorker 1] First sound found at the previously stored position

I don't have AACs to test against. What should I do about the failing test. While obviously biased, I agree with @uklotzde's point that we should probably properly fix this.

[daschuer]

The test should not fail in case of AACs and you should not see any moved first sound warning when playing mp3s. Than this bug can be considered as fixed.

The failing mp3 test should continue to fail, because the ffmpeg mp3 implementation is still broken. Every user who is trying that should see that failed test. I suggest to just disable the test in your case.

[github-actions]

This PR is marked as stale because it has been open 90 days with no activity.

[github-actions]

This PR is marked as stale because it has been open 90 days with no activity.