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.

Sign up for GitHub

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

Jump to bottom

fix(soundsources): fix FFMPEG MP3 playback #11923 #12034

Draft
wants to merge 1 commit into
base: 2.4
Choose a base branch
from
Draft

fix(soundsources): fix FFMPEG MP3 playback #11923 #12034

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/test/soundproxy_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
#include <gtest/gtest.h>

#include <QTemporaryDir>
#include <QTemporaryFile>
#include <QtDebug>
Expand Down Expand Up @@ -176,6 +178,7 @@ TEST_F(SoundSourceProxyTest, open) {
const auto fileUrl = QUrl::fromLocalFile(filePath);
const auto providerRegistrations =
SoundSourceProxy::allProviderRegistrationsForUrl(fileUrl);
bool couldOpenFile = false;
for (const auto& providerRegistration : providerRegistrations) {
mixxx::AudioSourcePointer pAudioSource = openAudioSource(
filePath,
Expand All @@ -187,10 +190,13 @@ TEST_F(SoundSourceProxyTest, open) {
// skip test file
continue;
}
couldOpenFile = true;
Copy link
Contributor

@uklotzde uklotzde Sep 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

EXPECT_LT(0, pAudioSource->getSignalInfo().getChannelCount());
EXPECT_LT(0u, pAudioSource->getSignalInfo().getSampleRate());
EXPECT_FALSE(pAudioSource->frameIndexRange().empty());
}
// Ensure we can open the file with a least one provider.
EXPECT_TRUE(couldOpenFile);
}
}

Expand Down
Loading