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

lp1445885: Fix handling of files with wrong suffix #4615

Merged
merged 26 commits into from
Jan 25, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
71f45b5
Fix 1:n mapping between file types and file suffixes
uklotzde Jan 7, 2022
4853a3a
Detect and report file type updates
uklotzde Jan 7, 2022
699a490
Fix import of inaccessible or missing files
uklotzde Jan 7, 2022
74d7632
Test that wrong file suffixes are fixed on re-import
uklotzde Jan 7, 2022
703fbfb
Delete obsolete "aif" file type
uklotzde Jan 7, 2022
00d4f9a
Move changelog entry from 2.3.2 to 2.4.0
uklotzde Jan 7, 2022
eabfcca
Add code comments
uklotzde Jan 7, 2022
5937340
Fix regex of supported file names
uklotzde Jan 7, 2022
5bfa42c
DirectoryDAOTest: Simplify code
uklotzde Jan 7, 2022
e40fab9
Add test for file types with corresponding suffixes
uklotzde Jan 7, 2022
adf9faf
Improve code comment as requested
uklotzde Jan 7, 2022
5b500ee
Add more file/type suffix mapping tests
uklotzde Jan 8, 2022
49df712
Merge branch 'main' of git@github.com:mixxxdj/mixxx.git into soundsou…
uklotzde Jan 8, 2022
f57cf30
Add remaining file types to suffix mapping test
uklotzde Jan 8, 2022
db15d6f
Maintain a s_fileTypeByMimeType for a reliable file type lookup
daschuer Jan 9, 2022
8fb2185
Don't add duplicates to s_fileTypeByMimeType
daschuer Jan 10, 2022
cb52de8
Add getFileTypeByMimeType() helper
daschuer Jan 10, 2022
9b2518e
Remove duplicate mime type entries. We need a strict 1 to many relati…
daschuer Jan 10, 2022
f63a72b
Adjust SoundSource::getTypeFromFile() for using getFileTypeByMineType
daschuer Jan 10, 2022
d3b504d
Add special case handling for opus files that the SounsourceOpus is used
daschuer Jan 10, 2022
2a44a94
Fix test getTypeFromMissingFile()
daschuer Jan 10, 2022
3ed94d0
Improve the code around the "audio/x-opus+ogg" mime type
daschuer Jan 16, 2022
babfce5
Merge remote-tracking branch 'upstream/main' into soundsource-file-type
daschuer Jan 23, 2022
2f77c85
Bypass mime type lookup from content for .flac files
uklotzde Jan 24, 2022
212489b
Merge pull request #73 from uklotzde/soundsource-file-type
daschuer Jan 24, 2022
8a9b24b
Use QLatin1String()
daschuer Jan 24, 2022
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@
* SoundSourceMP3: Log recoverable errors as info instead of warning [#4365](https://github.com/mixxxdj/mixxx/pull/4365)
* Fix type detection of AIFF files [#4364](https://github.com/mixxxdj/mixxx/pull/4364)
* AAC encoder: Fixed a memory leak [#4386](https://github.com/mixxxdj/mixxx/pull/4386) [#4408](https://github.com/mixxxdj/mixxx/pull/4408)
* Improve robustness of file type detection by considering the actual MIME type of the content. [lp:1445885](https://bugs.launchpad.net/mixxx/+bug/1445885) [#4356](https://github.com/mixxxdj/mixxx/pull/4356) [#4357](https://github.com/mixxxdj/mixxx/pull/4357)

### Audio Engine

Expand Down
6 changes: 6 additions & 0 deletions res/linux/org.mixxx.Mixxx.metainfo.xml
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,12 @@
#4386
#4408
</li>
<li>
Improve robustness of file type detection by considering the actual MIME type of the content.
lp:1445885
#4356
#4357
</li>
</ul>
<p>
Audio Engine
Expand Down
11 changes: 6 additions & 5 deletions src/coreservices.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -399,16 +399,17 @@ void CoreServices::initialize(QApplication* pApp) {
QSet<QString>::fromList(prev_plugins_list);
#endif

const QList<QString> curr_plugins_list = SoundSourceProxy::getSupportedFileExtensions();
QSet<QString> curr_plugins =
const QList<QString> supportedFileSuffixes = SoundSourceProxy::getSupportedFileSuffixes();
auto curr_plugins =
#if QT_VERSION >= QT_VERSION_CHECK(5, 14, 0)
QSet<QString>(curr_plugins_list.begin(), curr_plugins_list.end());
QSet<QString>(supportedFileSuffixes.begin(), supportedFileSuffixes.end());
#else
QSet<QString>::fromList(curr_plugins_list);
QSet<QString>::fromList(supportedFileSuffixes);
#endif

rescan = rescan || (prev_plugins != curr_plugins);
pConfig->set(ConfigKey("[Library]", "SupportedFileExtensions"), curr_plugins_list.join(","));
pConfig->set(ConfigKey("[Library]", "SupportedFileExtensions"),
supportedFileSuffixes.join(","));

// Scan the library directory. Do this after the skinloader has
// loaded a skin, see Bug #1047435
Expand Down
14 changes: 10 additions & 4 deletions src/sources/metadatasourcetaglib.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -86,14 +86,20 @@ class AiffFile : public TagLib::RIFF::AIFF::File {

std::pair<MetadataSourceTagLib::ImportResult, QDateTime>
MetadataSourceTagLib::afterImport(ImportResult importResult) const {
return std::make_pair(importResult,
MetadataSource::getFileSynchronizedAt(QFile(m_fileName)));
const auto sourceSynchronizedAt =
MetadataSource::getFileSynchronizedAt(QFile(m_fileName));
DEBUG_ASSERT(sourceSynchronizedAt.isValid() ||
importResult != ImportResult::Succeeded);
return std::make_pair(importResult, sourceSynchronizedAt);
}

std::pair<MetadataSourceTagLib::ExportResult, QDateTime>
MetadataSourceTagLib::afterExport(ExportResult exportResult) const {
return std::make_pair(exportResult,
MetadataSource::getFileSynchronizedAt(QFile(m_fileName)));
const auto sourceSynchronizedAt =
MetadataSource::getFileSynchronizedAt(QFile(m_fileName));
DEBUG_ASSERT(sourceSynchronizedAt.isValid() ||
exportResult != ExportResult::Succeeded);
return std::make_pair(exportResult, sourceSynchronizedAt);
}

std::pair<MetadataSource::ImportResult, QDateTime>
Expand Down
81 changes: 38 additions & 43 deletions src/sources/soundsource.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#include <QMimeDatabase>
#include <QMimeType>

#include "sources/soundsourceproxy.h"
#include "util/logger.h"

namespace mixxx {
Expand All @@ -21,20 +22,6 @@ inline QUrl validateLocalFileUrl(QUrl url) {
return url;
}

inline QString fileTypeFromSuffix(const QString& suffix) {
const QString fileType = suffix.toLower().trimmed();
if (fileType.isEmpty()) {
// Always return a default-constructed, null string instead
// of an empty string which might either be null or "".
return QString{};
}
// Map shortened suffix "aif" to "aiff" for disambiguation
if (fileType == QStringLiteral("aif")) {
return QStringLiteral("aiff");
}
return fileType;
}

} // anonymous namespace

//static
Expand All @@ -45,45 +32,53 @@ QString SoundSource::getTypeFromUrl(const QUrl& url) {

//static
QString SoundSource::getTypeFromFile(const QFileInfo& fileInfo) {
const QString fileSuffix = fileInfo.suffix();
const QString fileType = fileTypeFromSuffix(fileSuffix);
DEBUG_ASSERT(!fileType.isEmpty() || fileType == QString{});
const QMimeType mimeType = QMimeDatabase().mimeTypeForFile(
const QString fileSuffix = fileInfo.suffix().toLower().trimmed();

if (fileSuffix == QLatin1String("opus")) {
// Bypass the insufficient mime type lookup from content for opus files
// Files with "opus" suffix are of mime type "audio/x-opus+ogg" or "audio/opus".
// In case of "audio/x-opus+ogg" only the container format "audio/ogg"
// is detected, which will be decoded with the SoundSourceOggVobis()
// but we want SoundSourceOpus(). "audio/opus" files without ogg
// container are detected as "text/plain". They are not yet supported by Mixxx.
return fileSuffix;
}
if (fileSuffix == QLatin1String("flac")) {
// Bypass the insufficient mime type lookup from content for FLAC files.
// Legacy FLAC files may contain an ID3 tag (written by ExactAudioCopy and
// others) that causes these files to be identified as "audio/mpeg" instead
// of "audio/flac". Most decoders and TagLib are able to ignore and skip
// the non-standard ID3 data.
// https://mixxx.zulipchat.com/#narrow/stream/109171-development/topic/mimetype.20sometimes.20wrong
return fileSuffix;
}

QMimeType mimeType = QMimeDatabase().mimeTypeForFile(
fileInfo, QMimeDatabase::MatchContent);
// According to the documentation mimeTypeForFile always returns a valid
// type, using the generic type application/octet-stream as a fallback.
// This might also occur for missing files as seen on Qt 5.12.
if (!mimeType.isValid() ||
mimeType.name() == QStringLiteral("application/octet-stream")) {
qWarning()
<< "Unknown MIME type for file" << fileInfo.filePath();
return fileType;
}
const QString preferredSuffix = mimeType.preferredSuffix();
if (preferredSuffix.isEmpty()) {
DEBUG_ASSERT(mimeType.suffixes().isEmpty());
qInfo()
<< "MIME type" << mimeType
<< "has no preferred suffix";
return fileType;
}
const QString preferredFileType = fileTypeFromSuffix(preferredSuffix);
if (fileType == preferredFileType || mimeType.suffixes().contains(fileSuffix)) {
return fileType;
if (!mimeType.isValid() || mimeType.isDefault()) {
qInfo() << "Unable to detect MIME type from file" << fileInfo.filePath();
mimeType = QMimeDatabase().mimeTypeForFile(
fileInfo, QMimeDatabase::MatchExtension);
if (!mimeType.isValid() || mimeType.isDefault()) {
return fileSuffix;
}
}
const QString fileType = SoundSourceProxy::getFileTypeByMimeType(mimeType);
if (fileType.isEmpty()) {
qWarning() << "No file type registered for MIME type" << mimeType;
return fileSuffix;
}
if (fileType != fileSuffix && !mimeType.suffixes().contains(fileSuffix)) {
qWarning()
<< "Using type" << preferredFileType
<< "according to the detected MIME type" << mimeType
<< "of file" << fileInfo.filePath();
} else {
qWarning()
<< "Using type" << preferredFileType
<< "instead of" << fileType
<< "Using type" << fileType
<< "instead of" << fileSuffix
<< "according to the detected MIME type" << mimeType
<< "of file" << fileInfo.filePath();
}
return preferredFileType;
return fileType;
}

SoundSource::SoundSource(const QUrl& url, const QString& type)
Expand Down
6 changes: 3 additions & 3 deletions src/sources/soundsourcecoreaudio.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ constexpr SINT kMp3MaxSeekPrefetchFrames =
const QString SoundSourceProviderCoreAudio::kDisplayName = QStringLiteral("Apple Core Audio");

//static
const QStringList SoundSourceProviderCoreAudio::kSupportedFileExtensions = {
const QStringList SoundSourceProviderCoreAudio::kSupportedFileTypes = {
QStringLiteral("aac"),
QStringLiteral("m4a"),
QStringLiteral("mp4"),
Expand All @@ -41,8 +41,8 @@ const QStringList SoundSourceProviderCoreAudio::kSupportedFileExtensions = {
};

SoundSourceProviderPriority SoundSourceProviderCoreAudio::getPriorityHint(
const QString& supportedFileExtension) const {
Q_UNUSED(supportedFileExtension)
const QString& supportedFileType) const {
Q_UNUSED(supportedFileType)
// On macOS SoundSourceCoreAudio is the preferred decoder for all
// supported audio formats.
return SoundSourceProviderPriority::Higher;
Expand Down
8 changes: 4 additions & 4 deletions src/sources/soundsourcecoreaudio.h
Original file line number Diff line number Diff line change
Expand Up @@ -55,18 +55,18 @@ class SoundSourceCoreAudio
class SoundSourceProviderCoreAudio : public SoundSourceProvider {
public:
static const QString kDisplayName;
static const QStringList kSupportedFileExtensions;
static const QStringList kSupportedFileTypes;

QString getDisplayName() const override {
return kDisplayName;
}

QStringList getSupportedFileExtensions() const override {
return kSupportedFileExtensions;
QStringList getSupportedFileTypes() const override {
return kSupportedFileTypes;
}

SoundSourceProviderPriority getPriorityHint(
const QString& supportedFileExtension) const override;
const QString& supportedFileType) const override;

SoundSourcePointer newSoundSource(const QUrl& url) override {
return newSoundSourceFromUrl<SoundSourceCoreAudio>(url);
Expand Down
21 changes: 10 additions & 11 deletions src/sources/soundsourceffmpeg.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,7 @@ SoundSourceProviderFFmpeg::SoundSourceProviderFFmpeg() {
std::call_once(initFFmpegLibFlag, initFFmpegLib);
}

QStringList SoundSourceProviderFFmpeg::getSupportedFileExtensions() const {
QStringList SoundSourceProviderFFmpeg::getSupportedFileTypes() const {
QStringList list;
QStringList disabledInputFormats;

Expand All @@ -365,27 +365,26 @@ QStringList SoundSourceProviderFFmpeg::getSupportedFileExtensions() const {
list.append("aac");
continue;
} else if (!strcmp(pavInputFormat->name, "aiff")) {
list.append("aif");
list.append("aiff");
continue;
} else if (!strcmp(pavInputFormat->name, "mp3")) {
list.append("mp3");
continue;
} else if (!strcmp(pavInputFormat->name, "mp4")) {
} else if (!strcmp(pavInputFormat->name, "mp4") ||
!strcmp(pavInputFormat->name, "m4v")) {
list.append("mp4");
continue;
} else if (!strcmp(pavInputFormat->name, "m4v")) {
list.append("m4v");
continue;
} else if (!strcmp(pavInputFormat->name, "mov,mp4,m4a,3gp,3g2,mj2")) {
} else if (!strcmp(pavInputFormat->name,
"mov,mp4,m4a,3gp,3g2,mj2")) {
list.append("mov");
list.append("mp4");
list.append("m4a");
list.append("3gp");
list.append("3g2");
list.append("mj2");
continue;
} else if (!strcmp(pavInputFormat->name, "opus") || !strcmp(pavInputFormat->name, "libopus")) {
} else if (!strcmp(pavInputFormat->name, "opus") ||
!strcmp(pavInputFormat->name, "libopus")) {
list.append("opus");
continue;
} else if (!strcmp(pavInputFormat->name, "wav")) {
Expand Down Expand Up @@ -453,10 +452,10 @@ QStringList SoundSourceProviderFFmpeg::getSupportedFileExtensions() const {
}

SoundSourceProviderPriority SoundSourceProviderFFmpeg::getPriorityHint(
const QString& supportedFileExtension) const {
Q_UNUSED(supportedFileExtension)
const QString& supportedFileType) const {
Q_UNUSED(supportedFileType)
// TODO: Increase priority to Default or even Higher for all
// supported and tested file extension?
// supported and tested file types?
// Currently it is only used as a fallback after all other
// SoundSources failed to open a file or are otherwise unavailable.
return SoundSourceProviderPriority::Lowest;
Expand Down
4 changes: 2 additions & 2 deletions src/sources/soundsourceffmpeg.h
Original file line number Diff line number Diff line change
Expand Up @@ -197,10 +197,10 @@ class SoundSourceProviderFFmpeg : public SoundSourceProvider {
return kDisplayName;
}

QStringList getSupportedFileExtensions() const override;
QStringList getSupportedFileTypes() const override;

SoundSourceProviderPriority getPriorityHint(
const QString& supportedFileExtension) const override;
const QString& supportedFileType) const override;

SoundSourcePointer newSoundSource(const QUrl& url) override {
return newSoundSourceFromUrl<SoundSourceFFmpeg>(url);
Expand Down
6 changes: 3 additions & 3 deletions src/sources/soundsourceflac.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -75,13 +75,13 @@ constexpr SINT kBitsPerSampleDefault = 0;
const QString SoundSourceProviderFLAC::kDisplayName = QStringLiteral("Xiph.org libFLAC");

//static
const QStringList SoundSourceProviderFLAC::kSupportedFileExtensions = {
const QStringList SoundSourceProviderFLAC::kSupportedFileTypes = {
QStringLiteral("flac"),
};

SoundSourceProviderPriority SoundSourceProviderFLAC::getPriorityHint(
const QString& supportedFileExtension) const {
Q_UNUSED(supportedFileExtension)
const QString& supportedFileType) const {
Q_UNUSED(supportedFileType)
// This reference decoder is supposed to produce more accurate
// and reliable results than any other DEFAULT provider.
return SoundSourceProviderPriority::Higher;
Expand Down
8 changes: 4 additions & 4 deletions src/sources/soundsourceflac.h
Original file line number Diff line number Diff line change
Expand Up @@ -59,18 +59,18 @@ class SoundSourceFLAC final : public SoundSource {
class SoundSourceProviderFLAC : public SoundSourceProvider {
public:
static const QString kDisplayName;
static const QStringList kSupportedFileExtensions;
static const QStringList kSupportedFileTypes;

QString getDisplayName() const override {
return kDisplayName;
}

QStringList getSupportedFileExtensions() const override {
return kSupportedFileExtensions;
QStringList getSupportedFileTypes() const override {
return kSupportedFileTypes;
}

SoundSourceProviderPriority getPriorityHint(
const QString& supportedFileExtension) const override;
const QString& supportedFileType) const override;

SoundSourcePointer newSoundSource(const QUrl& url) override {
return newSoundSourceFromUrl<SoundSourceFLAC>(url);
Expand Down
6 changes: 3 additions & 3 deletions src/sources/soundsourcem4a.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -189,14 +189,14 @@ inline bool startsWithADTSHeader(
const QString SoundSourceProviderM4A::kDisplayName = QStringLiteral("Nero FAAD2");

//static
const QStringList SoundSourceProviderM4A::kSupportedFileExtensions = {
const QStringList SoundSourceProviderM4A::kSupportedFileTypes = {
QStringLiteral("m4a"),
QStringLiteral("mp4"),
};

QStringList SoundSourceProviderM4A::getSupportedFileExtensions() const {
QStringList SoundSourceProviderM4A::getSupportedFileTypes() const {
if (faad2::LibLoader::Instance()->isLoaded()) {
return kSupportedFileExtensions;
return kSupportedFileTypes;
} else {
return QStringList(); // none available
}
Expand Down
4 changes: 2 additions & 2 deletions src/sources/soundsourcem4a.h
Original file line number Diff line number Diff line change
Expand Up @@ -80,13 +80,13 @@ class SoundSourceM4A : public SoundSource {
class SoundSourceProviderM4A : public SoundSourceProvider {
public:
static const QString kDisplayName;
static const QStringList kSupportedFileExtensions;
static const QStringList kSupportedFileTypes;

QString getDisplayName() const override {
return kDisplayName;
}

QStringList getSupportedFileExtensions() const override;
QStringList getSupportedFileTypes() const override;

SoundSourcePointer newSoundSource(const QUrl& url) override;
};
Expand Down
6 changes: 3 additions & 3 deletions src/sources/soundsourcemediafoundation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -49,16 +49,16 @@ const QString SoundSourceProviderMediaFoundation::kDisplayName =
QStringLiteral("Microsoft Media Foundation");

//static
const QStringList SoundSourceProviderMediaFoundation::kSupportedFileExtensions = {
const QStringList SoundSourceProviderMediaFoundation::kSupportedFileTypes = {
QStringLiteral("aac"),
QStringLiteral("m4a"),
QStringLiteral("m4v"),
QStringLiteral("mp4"),
};

SoundSourceProviderPriority SoundSourceProviderMediaFoundation::getPriorityHint(
const QString& supportedFileExtension) const {
Q_UNUSED(supportedFileExtension)
const QString& supportedFileType) const {
Q_UNUSED(supportedFileType)
// On Windows SoundSourceMediaFoundation is the preferred decoder for all
// supported audio formats.
return SoundSourceProviderPriority::Higher;
Expand Down
Loading