Merge pull request #4357 from uklotzde/soundsource-mimetype

SoundSource: Fix file type detection
mixxxdj · Oct 5, 2021 · f2ec8a2 · f2ec8a2
2 parents b237b50 + 66eb947
commit f2ec8a2
Show file tree

Hide file tree

Showing 4 changed files with 67 additions and 11 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,9 @@
 # Changelog
 
+## [2.3.2](https://launchpad.net/mixxx/+milestone/2.3.2) (Unreleased)
+
+* 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)
+
 ## [2.3.1](https://launchpad.net/mixxx/+milestone/2.3.1) (2021-09-29)
 
 * Added mapping for the Numark DJ2GO2 Touch controller [#4108](https://github.com/mixxxdj/mixxx/pull/4108) [#4287](https://github.com/mixxxdj/mixxx/pull/4287)

diff --git a/res/linux/org.mixxx.Mixxx.metainfo.xml b/res/linux/org.mixxx.Mixxx.metainfo.xml
@@ -98,6 +98,18 @@
   Do not edit it manually.
   -->
   <releases>
+    <release version="2.3.2" type="development">
+      <description>
+ <ul>
+  <li>
+   Improve robustness of file type detection by considering the actual MIME type of the content.
+   lp:1445885
+   #4356
+   #4357
+  </li>
+ </ul>
+</description>
+    </release>
     <release version="2.3.1" type="stable" date="2021-09-29" timestamp="1632873600">
       <description>
  <ul>

diff --git a/src/sources/soundsource.cpp b/src/sources/soundsource.cpp
@@ -21,6 +21,16 @@ 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{};
+    }
+    return fileType;
+}
+
 } // anonymous namespace
 
 //static
@@ -32,37 +42,43 @@ QString SoundSource::getTypeFromUrl(const QUrl& url) {
 //static
 QString SoundSource::getTypeFromFile(const QFileInfo& fileInfo) {
     const QString fileSuffix = fileInfo.suffix();
-    DEBUG_ASSERT(!fileSuffix.isEmpty() || fileSuffix == QString{});
+    const QString fileType = fileTypeFromSuffix(fileSuffix);
+    DEBUG_ASSERT(!fileType.isEmpty() || fileType == QString{});
     const QMimeType mimeType = QMimeDatabase().mimeTypeForFile(fileInfo);
-    if (!mimeType.isValid()) {
+    // 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 fileSuffix;
+        return fileType;
     }
     const QString preferredSuffix = mimeType.preferredSuffix();
     if (preferredSuffix.isEmpty()) {
         DEBUG_ASSERT(mimeType.suffixes().isEmpty());
         qInfo()
                 << "MIME type" << mimeType
                 << "has no preferred suffix";
-        return fileSuffix;
+        return fileType;
     }
-    if (fileSuffix == preferredSuffix || mimeType.suffixes().contains(fileSuffix)) {
-        return fileSuffix;
+    const QString preferredFileType = fileTypeFromSuffix(preferredSuffix);
+    if (fileType == preferredFileType || mimeType.suffixes().contains(fileSuffix)) {
+        return fileType;
     }
-    if (fileSuffix.isEmpty()) {
+    if (fileType.isEmpty()) {
         qWarning()
-                << "Using type" << preferredSuffix
+                << "Using type" << preferredFileType
                 << "according to the detected MIME type" << mimeType
                 << "of file" << fileInfo.filePath();
     } else {
         qWarning()
-                << "Using type" << preferredSuffix
-                << "instead of" << fileSuffix
+                << "Using type" << preferredFileType
+                << "instead of" << fileType
                 << "according to the detected MIME type" << mimeType
                 << "of file" << fileInfo.filePath();
     }
-    return preferredSuffix;
+    return preferredFileType;
 }
 
 SoundSource::SoundSource(const QUrl& url, const QString& type)

diff --git a/src/test/soundproxy_test.cpp b/src/test/soundproxy_test.cpp
@@ -713,25 +713,49 @@ TEST_F(SoundSourceProxyTest, getTypeFromFile) {
     // Generate file names for the temporary file
     const QString filePathWithoutSuffix =
             mixxxtest::generateTemporaryFileName("file_with_no_file_suffix");
+    const QString filePathWithEmptySuffix =
+            mixxxtest::generateTemporaryFileName("file_with_empty_suffix.");
     const QString filePathWithUnknownSuffix =
             mixxxtest::generateTemporaryFileName("file_with.unknown_suffix");
     const QString filePathWithWrongSuffix =
             mixxxtest::generateTemporaryFileName("file_with_wrong_suffix.wav");
+    const QString filePathWithUppercaseAndLeadingTrailingWhitespaceSuffix =
+            mixxxtest::generateTemporaryFileName("file_with_uppercase_suffix. MP3 ");
 
     // Create the temporary files by copying an existing file
     const QString validFilePath = kTestDir.absoluteFilePath(QStringLiteral("empty.mp3"));
     mixxxtest::copyFile(validFilePath, filePathWithoutSuffix);
+    mixxxtest::copyFile(validFilePath, filePathWithEmptySuffix);
     mixxxtest::copyFile(validFilePath, filePathWithUnknownSuffix);
     mixxxtest::copyFile(validFilePath, filePathWithWrongSuffix);
+    mixxxtest::copyFile(validFilePath, filePathWithUppercaseAndLeadingTrailingWhitespaceSuffix);
 
     ASSERT_STREQ(qPrintable("mp3"), qPrintable(mixxx::SoundSource::getTypeFromFile(validFilePath)));
+
     EXPECT_STREQ(qPrintable("mp3"),
             qPrintable(mixxx::SoundSource::getTypeFromFile(
                     filePathWithoutSuffix)));
+    EXPECT_STREQ(qPrintable("mp3"),
+            qPrintable(mixxx::SoundSource::getTypeFromFile(
+                    filePathWithEmptySuffix)));
     EXPECT_STREQ(qPrintable("mp3"),
             qPrintable(mixxx::SoundSource::getTypeFromFile(
                     filePathWithUnknownSuffix)));
     EXPECT_STREQ(qPrintable("mp3"),
             qPrintable(mixxx::SoundSource::getTypeFromFile(
                     filePathWithWrongSuffix)));
+    EXPECT_STREQ(qPrintable("mp3"),
+            qPrintable(mixxx::SoundSource::getTypeFromFile(
+                    filePathWithUppercaseAndLeadingTrailingWhitespaceSuffix)));
+}
+
+TEST_F(SoundSourceProxyTest, getTypeFromMissingFile) {
+    const QFileInfo missingFileWithUppercaseSuffixAndLeadingTrailingWhitespaceSuffix =
+            kTestDir.absoluteFilePath(QStringLiteral("missing_file. AIFF "));
+
+    ASSERT_FALSE(missingFileWithUppercaseSuffixAndLeadingTrailingWhitespaceSuffix.exists());
+
+    EXPECT_STREQ(qPrintable("aiff"),
+            qPrintable(mixxx::SoundSource::getTypeFromFile(
+                    missingFileWithUppercaseSuffixAndLeadingTrailingWhitespaceSuffix)));
 }