Switch VBR header parsing to use a ByteReader as well instead of directly accessing the buffer

This also contains two small fixes to the VBR header parsing logic itself:
    - VBRI parsing was previously broken because the offset wasn't calculated correctly.
    - Xing-style headers can use a VBR header ID of "Info" as well.

2 files changed, 55 insertions, 33 deletions

diff --git a/dom/media/MP3Demuxer.cpp b/dom/media/MP3Demuxer.cpp
index 9ba80c32f..643416be4 100644
--- a/dom/media/MP3Demuxer.cpp
+++ b/dom/media/MP3Demuxer.cpp
@@ -391,7 +391,9 @@ MP3TrackDemuxer::GetNextFrame(const MediaByteRange& aRange) {
   if (mNumParsedFrames == 1) {
     // First frame parsed, let's read VBR info if available.
     // TODO: read info that helps with seeking (bug 1163667).
-    mParser.ParseVBRHeader(frame->Data(), frame->Data() + frame->Size());
+    ByteReader reader(frame->Data(), frame->Size());
+    mParser.ParseVBRHeader(&reader);
+    reader.DiscardRemaining();
     mFirstFrameOffset = frame->mOffset;
   }
 
@@ -779,9 +781,11 @@ FrameParser::VBRHeader::NumFrames() const {
 }
 
 bool
-FrameParser::VBRHeader::ParseXing(const uint8_t* aBeg, const uint8_t* aEnd) {
+FrameParser::VBRHeader::ParseXing(ByteReader* aReader) {
   static const uint32_t TAG = BigEndian::readUint32("Xing");
+  static const uint32_t TAG2 = BigEndian::readUint32("Info");
   static const uint32_t FRAME_COUNT_OFFSET = 8;
+  static const uint32_t FRAME_COUNT_SIZE = 4;
 
   enum Flags {
     NUM_FRAMES = 0x01,
@@ -790,51 +794,61 @@ FrameParser::VBRHeader::ParseXing(const uint8_t* aBeg, const uint8_t* aEnd) {
     VBR_SCALE = 0x08
   };
 
-  if (!aBeg || !aEnd || aBeg >= aEnd) {
-    return false;
-  }
+  MOZ_ASSERT(aReader);
+  const size_t prevReaderOffset = aReader->Offset();
 
   // We have to search for the Xing header as its position can change.
-  for (; aBeg + sizeof(TAG) < aEnd; ++aBeg) {
-    if (BigEndian::readUint32(aBeg) != TAG) {
+  while (aReader->Remaining() >= FRAME_COUNT_OFFSET + FRAME_COUNT_SIZE) {
+    if (aReader->PeekU32() != TAG && aReader->PeekU32() != TAG2) {
+      aReader->Read(1);
       continue;
     }
+    // Skip across the VBR header ID tag.
+    aReader->Read(sizeof(TAG));
 
-    const uint32_t flags = BigEndian::readUint32(aBeg + sizeof(TAG));
-    if (flags & NUM_FRAMES && aBeg + FRAME_COUNT_OFFSET < aEnd) {
-      mNumFrames = BigEndian::readUint32(aBeg + FRAME_COUNT_OFFSET);
+    const uint32_t flags = aReader->ReadU32();
+    if (flags & NUM_FRAMES) {
+      mNumFrames = aReader->ReadU32();
     }
     mType = XING;
+    aReader->Seek(prevReaderOffset);
     return true;
   }
+  aReader->Seek(prevReaderOffset);
   return false;
 }
 
 bool
-FrameParser::VBRHeader::ParseVBRI(const uint8_t* aBeg, const uint8_t* aEnd) {
+FrameParser::VBRHeader::ParseVBRI(ByteReader* aReader) {
   static const uint32_t TAG = BigEndian::readUint32("VBRI");
-  static const uint32_t OFFSET = 32 - FrameParser::FrameHeader::SIZE;
+  static const uint32_t OFFSET = 32 + FrameParser::FrameHeader::SIZE;
   static const uint32_t FRAME_COUNT_OFFSET = OFFSET + 14;
   static const uint32_t MIN_FRAME_SIZE = OFFSET + 26;
 
-  if (!aBeg || !aEnd || aBeg >= aEnd) {
-    return false;
-  }
+  MOZ_ASSERT(aReader);
+  // ParseVBRI assumes that the ByteReader offset points to the beginning of a frame,
+  // therefore as a simple check, we look for the presence of a frame sync at that position.
+  MOZ_ASSERT(aReader->PeekU16() & 0xFFE0);
+  const size_t prevReaderOffset = aReader->Offset();
 
-  const int64_t frameLen = aEnd - aBeg;
   // VBRI have a fixed relative position, so let's check for it there.
-  if (frameLen > MIN_FRAME_SIZE &&
-      BigEndian::readUint32(aBeg + OFFSET) == TAG) {
-    mNumFrames = BigEndian::readUint32(aBeg + FRAME_COUNT_OFFSET);
-    mType = VBRI;
-    return true;
+  if (aReader->Remaining() > MIN_FRAME_SIZE) {
+    aReader->Seek(prevReaderOffset + OFFSET);
+    if (aReader->ReadU32() == TAG) {
+      aReader->Seek(prevReaderOffset + FRAME_COUNT_OFFSET);
+      mNumFrames = aReader->ReadU32();
+      mType = VBRI;
+      aReader->Seek(prevReaderOffset);
+      return true;
+    }
   }
+  aReader->Seek(prevReaderOffset);
   return false;
 }
 
 bool
-FrameParser::VBRHeader::Parse(const uint8_t* aBeg, const uint8_t* aEnd) {
-  return ParseVBRI(aBeg, aEnd) || ParseXing(aBeg, aEnd);
+FrameParser::VBRHeader::Parse(ByteReader* aReader) {
+  return ParseVBRI(aReader) || ParseXing(aReader);
 }
 
 // FrameParser::Frame
@@ -868,8 +882,8 @@ FrameParser::Frame::Header() const {
 }
 
 bool
-FrameParser::ParseVBRHeader(const uint8_t* aBeg, const uint8_t* aEnd) {
-  return mVBRHeader.Parse(aBeg, aEnd);
+FrameParser::ParseVBRHeader(ByteReader* aReader) {
+  return mVBRHeader.Parse(aReader);
 }
 
 // ID3Parser
diff --git a/dom/media/MP3Demuxer.h b/dom/media/MP3Demuxer.h
index 6abe86658..2b20c4254 100644
--- a/dom/media/MP3Demuxer.h
+++ b/dom/media/MP3Demuxer.h
@@ -216,18 +216,24 @@ public:
     // Returns the total number of frames expected in the stream/file.
     int64_t NumFrames() const;
 
-    // Parses given buffer [aBeg, aEnd) for a valid VBR header.
+    // Parses contents of given ByteReader for a valid VBR header.
+    // The offset of the passed ByteReader needs to point to an MPEG frame begin,
+    // as a VBRI-style header is searched at a fixed offset relative to frame begin.
     // Returns whether a valid VBR header was found in the range.
-    bool Parse(const uint8_t* aBeg, const uint8_t* aEnd);
+    bool Parse(mp4_demuxer::ByteReader* aReader);
 
   private:
-    // Parses given buffer [aBeg, aEnd) for a valid Xing header.
+    // Parses contents of given ByteReader for a valid Xing header.
+    // The initial ByteReader offset will be preserved.
     // Returns whether a valid Xing header was found in the range.
-    bool ParseXing(const uint8_t* aBeg, const uint8_t* aEnd);
+    bool ParseXing(mp4_demuxer::ByteReader* aReader);
 
-    // Parses given buffer [aBeg, aEnd) for a valid VBRI header.
+    // Parses contents of given ByteReader for a valid VBRI header.
+    // The initial ByteReader offset will be preserved. It also needs to point
+    // to the beginning of a valid MPEG frame, as VBRI headers are searched
+    // at a fixed offset relative to frame begin.
     // Returns whether a valid VBRI header was found in the range.
-    bool ParseVBRI(const uint8_t* aBeg, const uint8_t* aEnd);
+    bool ParseVBRI(mp4_demuxer::ByteReader* aReader);
 
     // The total number of frames expected as parsed from a VBR header.
     int64_t mNumFrames;
@@ -292,9 +298,11 @@ public:
   // ID3v2 tag spanning multiple buffers.
   bool Parse(mp4_demuxer::ByteReader* aReader, uint32_t* aBytesToSkip);
 
-  // Parses given buffer [aBeg, aEnd) for a valid VBR header.
+  // Parses contents of given ByteReader for a valid VBR header.
+  // The offset of the passed ByteReader needs to point to an MPEG frame begin,
+  // as a VBRI-style header is searched at a fixed offset relative to frame begin.
   // Returns whether a valid VBR header was found.
-  bool ParseVBRHeader(const uint8_t* aBeg, const uint8_t* aEnd);
+  bool ParseVBRHeader(mp4_demuxer::ByteReader* aReader);
 
 private:
   // ID3 header parser.