Support Vorbis comments from a multiplexed Ogg stream (#1370)

Ogg::File::readPages() read pages from all logical bitstreams and
indexed their packets into one global list. In a multiplexed file the
Theora and Vorbis packets got interleaved, so packet 0 was a Theora
header rather than the expected Vorbis type-3 comment header.

Before reading packets, the Vorbis reader now selects the Vorbis
logical bitstream (the one whose first packet is the Vorbis
identification header), so it reads the correct stream regardless of
position. Packet parsing is scoped to the selected bitstream, so
packets from other codecs in the same file are ignored.

tests/data/multiplex.ogg is generated using

ffmpeg -hide_banner -y \
  -f lavfi -i "color=c=navy:s=500x500:r=1:d=2" \
  -f lavfi -i "sine=frequency=440:sample_rate=48000:duration=2" \
  -map 0:v -map 1:a \
  -c:v libtheora -q:v 3 \
  -c:a libvorbis -ac 2 -q:a 2 \
  -flags +bitexact -fflags +bitexact \
  -metadata:s:a:0 TITLE="Paper Lights" \
  -metadata:s:a:0 encoder= -metadata:s:v:0 encoder= \
  -f ogg tests/data/multiplex.ogg

taglib/ogg/oggfile.cpp

@@ -57,6 +57,16 @@ public:
   std::unique_ptr<PageHeader> firstPageHeader;
   std::unique_ptr<PageHeader> lastPageHeader;
   Map<unsigned int, ByteVector> dirtyPackets;
+
+  // File offset of the next page to read while scanning the file.  This tracks
+  // the physical position independently of the logical stream's pages so that
+  // pages of other multiplexed streams can be skipped.
+  offset_t currentPageOffset { -1 };
+
+  // Serial number of the logical bitstream packets are read from.  In a
+  // multiplexed Ogg stream only pages of this stream are considered.
+  unsigned int streamSerialNumber { 0 };
+  bool streamSerialNumberSet { false };
 };
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -171,6 +181,38 @@ Ogg::File::File(IOStream *stream) :
 {
 }
 
+bool Ogg::File::selectStream(const ByteVector &magic)
+{
+  // All beginning-of-stream pages of a (possibly multiplexed) Ogg stream
+  // appear at the very start of the file, before any secondary pages.  Inspect
+  // each one's first packet and lock onto the first logical bitstream whose
+  // identification header matches magic.
+
+  offset_t offset = find("OggS");
+  if(offset < 0)
+    return false;
+
+  while(true) {
+    Page page(this, offset);
+    if(!page.header()->isValid())
+      return false;
+
+    // Once the beginning-of-stream pages are exhausted there are no more
+    // logical bitstreams to discover.
+    if(!page.header()->firstPageOfStream())
+      return false;
+
+    const ByteVectorList packets = page.packets();
+    if(!packets.isEmpty() && packets.front().startsWith(magic)) {
+      d->streamSerialNumber = page.header()->streamSerialNumber();
+      d->streamSerialNumberSet = true;
+      return true;
+    }
+
+    offset += page.size();
+  }
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // private members
 ////////////////////////////////////////////////////////////////////////////////
@@ -178,37 +220,53 @@ Ogg::File::File(IOStream *stream) :
 bool Ogg::File::readPages(unsigned int i)
 {
   while(true) {
-    unsigned int packetIndex;
-    offset_t offset;
 
-    if(d->pages.isEmpty()) {
-      packetIndex = 0;
-      offset = find("OggS");
-      if(offset < 0)
-        return false;
-    }
-    else {
+    // If we've already indexed the page containing packet i, we're done.
+
+    if(!d->pages.isEmpty()) {
       const Page *page = d->pages.back();
-      packetIndex = nextPacketIndex(page);
-      offset = page->fileOffset() + page->size();
-
-      // Enough pages have been fetched.
-      if(packetIndex > i) {
+      if(nextPacketIndex(page) > i)
         return true;
-      }
-      else if(page->header()->lastPageOfStream()) {
+      if(page->header()->lastPageOfStream())
+        return false;
+    }
+
+    // Locate the first page in the file if we haven't started scanning yet.
+
+    if(d->currentPageOffset < 0) {
+      d->currentPageOffset = find("OggS");
+      if(d->currentPageOffset < 0)
+        return false;
+    }
+
+    // Read pages until we find the next one belonging to our logical bitstream,
+    // skipping pages of other streams in a multiplexed Ogg stream.
+
+    Page *nextPage;
+    while(true) {
+      nextPage = new Page(this, d->currentPageOffset);
+      if(!nextPage->header()->isValid()) {
+        delete nextPage;
         return false;
       }
-    }
 
-    // Read the next page and add it to the page list.
+      d->currentPageOffset += nextPage->size();
+
+      const unsigned int serial = nextPage->header()->streamSerialNumber();
+      if(!d->streamSerialNumberSet) {
+        d->streamSerialNumber = serial;
+        d->streamSerialNumberSet = true;
+      }
+
+      if(serial == d->streamSerialNumber)
+        break;
 
-    auto nextPage = new Page(this, offset);
-    if(!nextPage->header()->isValid()) {
       delete nextPage;
-      return false;
     }
 
+    const unsigned int packetIndex
+      = d->pages.isEmpty() ? 0 : nextPacketIndex(d->pages.back());
+
     nextPage->setFirstPacketIndex(packetIndex);
     d->pages.append(nextPage);
   }
@@ -295,4 +353,5 @@ void Ogg::File::writePacket(unsigned int i, const ByteVector &packet)
   // Discard all the pages to keep them up-to-date by fetching them again.
 
   d->pages.clear();
+  d->currentPageOffset = -1;
 }

taglib/ogg/oggfile.h

@@ -105,6 +105,20 @@ namespace TagLib {
        */
       File(IOStream *stream);
 
+      /*!
+       * Restricts packet parsing to the first logical bitstream whose first
+       * packet begins with \a magic.  This is needed for multiplexed Ogg
+       * streams, such as an Ogg Vorbis stream muxed with an Ogg Theora video
+       * stream carrying cover art, where packets of the individual logical
+       * bitstreams are interleaved.  Must be called before any packet is
+       * requested.
+       *
+       * Returns \c true if a matching logical bitstream was found and selected.
+       * If no match is found, the file falls back to the first logical
+       * bitstream in the file.
+       */
+      bool selectStream(const ByteVector &magic);
+
     private:
       /*!
        * Reads the pages from the beginning of the file until enough to compose

taglib/ogg/vorbis/vorbisfile.cpp

@@ -41,8 +41,10 @@ public:
 namespace TagLib {
   /*!
    * Vorbis headers can be found with one type ID byte and the string "vorbis" in
-   * an Ogg stream.  0x03 indicates the comment header.
+   * an Ogg stream.  0x01 indicates the identification header and 0x03 indicates
+   * the comment header.
    */
+  static constexpr char vorbisIdentificationHeaderID[] = { 0x01, 'v', 'o', 'r', 'b', 'i', 's', 0 };
   static constexpr char vorbisCommentHeaderID[] = { 0x03, 'v', 'o', 'r', 'b', 'i', 's', 0 };
 } // namespace TagLib
 
@@ -119,6 +121,10 @@ bool Vorbis::File::save()
 
 void Vorbis::File::read(bool readProperties)
 {
+  // Select the Vorbis logical bitstream in case the file is a multiplexed Ogg
+  // stream (e.g. Vorbis audio muxed with a Theora video stream for cover art).
+  selectStream(vorbisIdentificationHeaderID);
+
   ByteVector commentHeaderData = packet(1);
 
   if(commentHeaderData.mid(0, 7) != vorbisCommentHeaderID) {

tests/test_ogg.cpp

@@ -32,6 +32,7 @@
 #include "oggfile.h"
 #include "vorbisfile.h"
 #include "oggpageheader.h"
+#include "plainfile.h"
 #include <cppunit/extensions/HelperMacros.h>
 #include "utils.h"
 
@@ -47,6 +48,7 @@ class TestOGG : public CppUnit::TestFixture
   CPPUNIT_TEST(testDictInterface1);
   CPPUNIT_TEST(testDictInterface2);
   CPPUNIT_TEST(testAudioProperties);
+  CPPUNIT_TEST(testMultiplexed);
   CPPUNIT_TEST(testPageChecksum);
   CPPUNIT_TEST(testPageGranulePosition);
   CPPUNIT_TEST_SUITE_END();
@@ -200,6 +202,45 @@ public:
     CPPUNIT_ASSERT_EQUAL(0, f.audioProperties()->bitrateMinimum());
   }
 
+  void testMultiplexed()
+  {
+    // A multiplexed Ogg stream where the Vorbis logical bitstream is not the
+    // first one (it is preceded by a Theora video stream). The Vorbis comment
+    // and audio properties must still be read from the Vorbis stream.
+    ScopedFileCopy copy("multiplex", ".ogg");
+    string filename = copy.fileName();
+    {
+      Vorbis::File f(filename.c_str());
+      CPPUNIT_ASSERT(f.isValid());
+      CPPUNIT_ASSERT(f.tag());
+      CPPUNIT_ASSERT_EQUAL(String("Paper Lights"), f.tag()->title());
+      CPPUNIT_ASSERT(f.audioProperties());
+      CPPUNIT_ASSERT_EQUAL(2, f.audioProperties()->channels());
+      CPPUNIT_ASSERT_EQUAL(48000, f.audioProperties()->sampleRate());
+
+      f.tag()->setTitle("Changed Title");
+      CPPUNIT_ASSERT(f.save());
+    }
+    {
+      Vorbis::File f(filename.c_str());
+      CPPUNIT_ASSERT(f.isValid());
+      CPPUNIT_ASSERT_EQUAL(String("Changed Title"), f.tag()->title());
+
+      f.tag()->setTitle("Paper Lights");
+      CPPUNIT_ASSERT(f.save());
+    }
+    {
+      Vorbis::File f(filename.c_str());
+      CPPUNIT_ASSERT(f.isValid());
+      CPPUNIT_ASSERT_EQUAL(String("Paper Lights"), f.tag()->title());
+    }
+
+    // Check if the modified file is byte for byte equal to the original file
+    const ByteVector origData = PlainFile(TEST_FILE_PATH_C("multiplex.ogg")).readAll();
+    const ByteVector fileData = PlainFile(filename.c_str()).readAll();
+    CPPUNIT_ASSERT(origData == fileData);
+  }
+
   void testPageChecksum()
   {
     ScopedFileCopy copy("empty", ".ogg");