[macOS testing required] Fix cue shift/offset for SoundSourceCoreAudio

[uklotzde]

As discussed on Zulip: https://mixxx.zulipchat.com/#narrow/stream/109171-development/topic/Cue.20shift.2Foffset

All unit tests pass, but extensive manual testing is required! Existing cue points might be shifted for some MP3 files.

Related:
digital-dj-tools/dj-data-converter#3

[daschuer]

Thank you for doing this remote. LGTM.

Do you have a clue how many tracks are effected?
Does this fix a random sample shifting issue during the track? Is it a constant offset during the whole track? Is this offset constant for a single track?

In the last case this PR introduces a real issue while fixing a theoretical bug only.

We should be sure that this is worth to merge and how to inform about side effects.

Is the chroma for musicbrains lookups effected.

[uklotzde]

It's a constant offset encoded depending on the file and read as leadingFrames. Only a few milliseconds of silence at the beginning, chroma and MusicBrainz lookups should not be affected. Might occur for both MP3 and M4A files.

[uklotzde]

We should only merge this PR after confirming that the frame index is now in fact calculated correctly!

Another question is if we should merge this after confirmation? We need to if cue points should be portable within Mixxx across different platforms. And we need to if we would like to synchronize cue points with external sources, given that those sources apply the same rules as we do.

At least we don't seem to be the only ones who didn't get it right in the first place. Thanks to some recent efforts in the open-source community this becomes apparent.

[uklotzde]

Testing:

• Use a set of MP3/M4A test files and let Mixxx calculate the intro/outro points
• Start with an empty database and repeat this procedure on both Linux and macOS
• Compare the calculated sample offsets obtained from both platforms

Links to the test files used in https://mixxx.zulipchat.com/user_uploads/2380/juWR0xjOUUpYXwUAhU4lnsY0/mixxx_cue_shift.ods can be found in the linked issues and the new FFmpeg branch.

We also need to check which files are affected by this decoding fix to give an advice in the CHANGELOG.
Not fixing the offset issue is not an option in the long run if we plan to import cue points from external libraries.

I'm not able to perform any tests on macOS and don't care as long as Apple is refusing to offer support for cross-platform development.

[ferranpujolcamins]

Where can I find the first file specified in the test spreadsheet?

[daschuer]

@ferranpujolcamins are you in charge to test this?

[ferranpujolcamins]

Yes, I'm in touch with @uklotzde

[uklotzde]

Tests are failing for the additional files added during the FFmpeg integration.

[uklotzde]

Tests should be fixed. The offset calculations seem to be correct, but we need some more testing in the wild. I've improved the error reporting to detect unexpected behavior.

[Holzhaus]

Merge?

[Be-ing]

I think this still needs some manual testing from a macOS user? @ferranpujolcamins, @benis?

[Holzhaus]

I addition to manual testing, can we automate this somehow so that the CI can warn us if something breaks? Couldn't we take some public domain/Creative-Commons-licensed track, encode it with a bunch of different encoders and then write tests for it?

[daschuer]

We have already automated tests for the different sound sources.
https://github.com/mixxxdj/mixxx/tree/master/src/test/soundFileFormats
They can however not replace a manual "smoke test"

[ferranpujolcamins]

@uklotzde How should we test exactly? Same test than the other time?

[uklotzde]

We currently only check for consistency when decoding a single file. Consistently decoding the wrong signal would pass the test, because we don't have any reference files with their expected decoded signal.

Decoding the same file with different decoders could be an option, but this only allows a limited number of combinations and we would have to assume that those decoders are actually available. Moreover, our test files don't reflect properly the multitude of encoders, versions, and settings that occur in the wild.

[uklotzde]

@ferranpujolcamins The developer docs don't clearly state if and how to account for the leading or trailing frames of the converter. Debug assertions should reveal any false assumptions about the total file length.

If cue points of .mp3 and .m4a files with leading frames > 0 don't change then we are safe. A log message for printing this information could be added temporarily to tryOpen() for identifying such files.

[benis]

I don't think I can help unfortunately, I wiped my Mac recently and haven't bothered to set up my development environment again. I wasn't getting much of a response when I asked for assistance with things and I ended up losing interest.

[daschuer]

Can we just merge this and wait for feedback during the beta phase?

[uklotzde]

I'm confident that the changes do not introduce any major issues. The logging has been improved, so we hopefully get more detailed reports in case of unexpected behavior.

Feedback and a LGTM from one or more macOS user(s) would be desirable.

[Holzhaus]

Since it looks like there are no MacOS testers around, shall we merge?

[Holzhaus]

Merge?

[daschuer]

Yes.