Fix handling of lowercase 'metadata_block_picture' fields in Vorbis comments.

Also refactored some redundant code for parsing pictures.

diff --git a/taglib/ogg/xiphcomment.cpp b/taglib/ogg/xiphcomment.cpp
index d6b8e11c0504430aca12b4e9f75408fd3078a950..f56bf810c841f8cc2d4529cb0abbe449ad26f18f 100644 (file)
--- a/taglib/ogg/xiphcomment.cpp
+++ b/taglib/ogg/xiphcomment.cpp
@@ -447,85 +447,69 @@ void Ogg::XiphComment::parse(const ByteVector &data)
     const unsigned int commentLength = data.toUInt(pos, false);
     pos += 4;
 
-    ByteVector entry = data.mid(pos, commentLength);
-
+    const ByteVector entry = data.mid(pos, commentLength);
     pos += commentLength;
 
     // Don't go past data end
+
     if(pos > data.size())
       break;
 
-    // Handle Pictures separately
-    if(entry.startsWith("METADATA_BLOCK_PICTURE=")) {
+    // Check for field separator
 
-      // We need base64 encoded data including padding
-      if((entry.size() - 23) > 3 && ((entry.size() - 23) % 4) == 0) {
+    const int sep = entry.find('=');
+    if(sep < 1) {
+      debug("Ogg::XiphComment::parse() - Discarding a field. Separator not found.");
+      continue;
+    }
 
-        // Decode base64 picture data
-        ByteVector picturedata = ByteVector::fromBase64(entry.mid(23));
-        if(picturedata.size()) {
+    // Parse the key
 
-          // Decode Flac Picture
-          FLAC::Picture * picture = new FLAC::Picture();
-          if(picture->parse(picturedata)) {
+    const String key = String(entry.mid(0, sep), String::UTF8).upper();
+    if(!checkKey(key)) {
+      debug("Ogg::XiphComment::parse() - Discarding a field. Invalid key.");
+      continue;
+    }
 
-            d->pictureList.append(picture);
+    if(key == "METADATA_BLOCK_PICTURE" || key == "COVERART") {
 
-            // continue to next field
-            continue;
-          }
-          else {
-            delete picture;
-            debug("Failed to decode FlacPicture block");
-          }
-        }
-        else {
-          debug("Failed to decode base64 encoded data");
-        }
-      }
-      else {
-        debug("Invalid base64 encoded data");
-      }
-    }
+      // Handle Pictures separately
 
-    // Handle old picture standard
-    if(entry.startsWith("COVERART=")) {
+      const ByteVector picturedata = ByteVector::fromBase64(entry.mid(sep + 1));
+      if(picturedata.isEmpty()) {
+        debug("Ogg::XiphComment::parse() - Discarding a field. Invalid base64 data");
+        continue;
+      }
 
-      if((entry.size() - 9) > 3 && ((entry.size() - 9) % 4) == 0) {
+      if(key[0] == L'M') {
 
-        // Decode base64 picture data
-        ByteVector picturedata = ByteVector::fromBase64(entry.mid(9));
-        if (picturedata.size()) {
+        // Decode FLAC Picture
 
-          // Assume it's some type of image file
-          FLAC::Picture * picture = new FLAC::Picture();
-          picture->setData(picturedata);
-          picture->setMimeType("image/");
-          picture->setType(FLAC::Picture::Other);
+        FLAC::Picture * picture = new FLAC::Picture();
+        if(picture->parse(picturedata)) {
           d->pictureList.append(picture);
-
-          // continue to next field
-          continue;
         }
         else {
-          debug("Failed to decode base64 encoded data");
+          delete picture;
+          debug("Ogg::XiphComment::parse() - Failed to decode FLAC Picture block");
         }
       }
       else {
-        debug("Invalid base64 encoded data");
+
+        // Assume it's some type of image file
+
+        FLAC::Picture * picture = new FLAC::Picture();
+        picture->setData(picturedata);
+        picture->setMimeType("image/");
+        picture->setType(FLAC::Picture::Other);
+        d->pictureList.append(picture);
       }
     }
+    else {
 
-    // Check for field separator
-    int sep = entry.find('=');
-    if(sep < 1) {
-      debug("Discarding invalid comment field.");
-      continue;
-    }
+      // Parse the text
 
-    // Parse key and value
-    String key = String(entry.mid(0, sep), String::UTF8);
-    String value = String(entry.mid(sep + 1), String::UTF8);
-    addField(key, value, false);
+      addField(key, String(entry.mid(sep + 1), String::UTF8), false);
+    }
   }
 }

diff --git a/tests/data/lowercase-fields.ogg b/tests/data/lowercase-fields.ogg
new file mode 100644 (file)
index 0000000..0ddd493
Binary files /dev/null and b/tests/data/lowercase-fields.ogg differ

diff --git a/tests/test_xiphcomment.cpp b/tests/test_xiphcomment.cpp
index d2a28a1f7dbf9f1e491fa7c07dd7863614585364..64ae0b219243ff53e4c48813d5c3fde215493bd8 100644 (file)
--- a/tests/test_xiphcomment.cpp
+++ b/tests/test_xiphcomment.cpp
@@ -47,6 +47,7 @@ class TestXiphComment : public CppUnit::TestFixture
   CPPUNIT_TEST(testClearComment);
   CPPUNIT_TEST(testRemoveFields);
   CPPUNIT_TEST(testPicture);
+  CPPUNIT_TEST(testLowercaseFields);
   CPPUNIT_TEST_SUITE_END();
 
 public:
@@ -192,6 +193,23 @@ public:
     }
   }
 
+  void testLowercaseFields()
+  {
+    const ScopedFileCopy copy("lowercase-fields", ".ogg");
+    {
+      Vorbis::File f(copy.fileName().c_str());
+      List<FLAC::Picture *> lst = f.tag()->pictureList();
+      CPPUNIT_ASSERT_EQUAL(String("TEST TITLE"), f.tag()->title());
+      CPPUNIT_ASSERT_EQUAL(String("TEST ARTIST"), f.tag()->artist());
+      CPPUNIT_ASSERT_EQUAL((unsigned int)1, lst.size());
+      f.save();
+    }
+    {
+      Vorbis::File f(copy.fileName().c_str());
+      CPPUNIT_ASSERT(f.find("METADATA_BLOCK_PICTURE") > 0);
+    }
+  }
+
 };
 
 CPPUNIT_TEST_SUITE_REGISTRATION(TestXiphComment);