Bug 1411821 - use BufferReader instead of ByteReader in MP3 parser. r?kinetik draft
authorAlfredo.Yang <ayang@mozilla.com>
Fri, 20 Oct 2017 11:54:29 +0800
changeset 686707 3ce354cf4799d22657ac33d9df887bc3c1771c9c
parent 686573 3c3be72f4c9800b8a07f75050ccd40eb59e1f5bc
child 737441 23676d94f2683b5047b3c00be4dbb477cd92548c
push id86261
push userbmo:ayang@mozilla.com
push dateThu, 26 Oct 2017 09:04:35 +0000
reviewerskinetik
bugs1411821
milestone58.0a1
Bug 1411821 - use BufferReader instead of ByteReader in MP3 parser. r?kinetik MozReview-Commit-ID: BpfCZUG7C6c
dom/media/mp3/MP3Demuxer.cpp
dom/media/mp3/MP3FrameParser.cpp
dom/media/mp3/MP3FrameParser.h
media/libstagefright/binding/include/mp4_demuxer/BufferReader.h
--- a/dom/media/mp3/MP3Demuxer.cpp
+++ b/dom/media/mp3/MP3Demuxer.cpp
@@ -19,17 +19,17 @@ extern mozilla::LazyLogModule gMediaDemu
 #define MP3LOG(msg, ...) \
   MOZ_LOG(gMediaDemuxerLog, LogLevel::Debug, ("MP3Demuxer " msg, ##__VA_ARGS__))
 #define MP3LOGV(msg, ...) \
   MOZ_LOG(gMediaDemuxerLog, LogLevel::Verbose, ("MP3Demuxer " msg, ##__VA_ARGS__))
 
 using mozilla::media::TimeUnit;
 using mozilla::media::TimeInterval;
 using mozilla::media::TimeIntervals;
-using mp4_demuxer::ByteReader;
+using mp4_demuxer::BufferReader;
 
 namespace mozilla {
 
 // MP3Demuxer
 
 MP3Demuxer::MP3Demuxer(MediaResource* aSource) : mSource(aSource) { }
 
 bool
@@ -533,19 +533,20 @@ MP3TrackDemuxer::FindNextFrame()
 
     if ((mOffset - startOffset > maxSkippableBytes) ||
         (read = Read(buffer, mOffset, BUFFER_SIZE)) == 0) {
       MP3LOG("FindNext() EOS or exceeded maxSkippeableBytes without a frame");
       // This is not a valid MPEG audio stream or we've reached EOS, give up.
       break;
     }
 
-    ByteReader reader(buffer, read);
+    BufferReader reader(buffer, read);
     uint32_t bytesToSkip = 0;
-    foundFrame = mParser.Parse(&reader, &bytesToSkip);
+    auto res = mParser.Parse(&reader, &bytesToSkip);
+    foundFrame = res.isOk() ? res.unwrap() : false;
     frameHeaderOffset =
       mOffset + reader.Offset() - FrameParser::FrameHeader::SIZE;
 
     // If we've found neither an MPEG frame header nor an ID3v2 tag,
     // the reader shouldn't have any bytes remaining.
     MOZ_ASSERT(foundFrame || bytesToSkip || !reader.Remaining());
 
     if (foundFrame && mParser.FirstFrame().Length() &&
@@ -633,17 +634,17 @@ MP3TrackDemuxer::GetNextFrame(const Medi
   frame->mTimecode = frame->mTime;
   frame->mKeyframe = true;
 
   MOZ_ASSERT(!frame->mTime.IsNegative());
   MOZ_ASSERT(frame->mDuration.IsPositive());
 
   if (mNumParsedFrames == 1) {
     // First frame parsed, let's read VBR info if available.
-    ByteReader reader(frame->Data(), frame->Size());
+    BufferReader reader(frame->Data(), frame->Size());
     mParser.ParseVBRHeader(&reader);
     mFirstFrameOffset = frame->mOffset;
   }
 
   MP3LOGV("GetNext() End mOffset=%" PRIu64 " mNumParsedFrames=%" PRIu64
           " mFrameIndex=%" PRId64 " mTotalFrameLen=%" PRIu64
           " mSamplesPerFrame=%d mSamplesPerSecond=%d mChannels=%d",
           mOffset, mNumParsedFrames, mFrameIndex, mTotalFrameLen,
--- a/dom/media/mp3/MP3FrameParser.cpp
+++ b/dom/media/mp3/MP3FrameParser.cpp
@@ -6,25 +6,27 @@
 
 #include "MP3FrameParser.h"
 
 #include <algorithm>
 #include <inttypes.h>
 
 #include "mozilla/Assertions.h"
 #include "mozilla/EndianUtils.h"
+#include "mozilla/Pair.h"
+#include "mozilla/ResultExtensions.h"
 #include "VideoUtils.h"
 
 extern mozilla::LazyLogModule gMediaDemuxerLog;
 #define MP3LOG(msg, ...) \
   MOZ_LOG(gMediaDemuxerLog, LogLevel::Debug, ("MP3Demuxer " msg, ##__VA_ARGS__))
 #define MP3LOGV(msg, ...) \
   MOZ_LOG(gMediaDemuxerLog, LogLevel::Verbose, ("MP3Demuxer " msg, ##__VA_ARGS__))
 
-using mp4_demuxer::ByteReader;
+using mp4_demuxer::BufferReader;
 
 namespace mozilla {
 
 // FrameParser
 
 namespace frame_header {
 // FrameHeader mRaw byte offsets.
 static const int SYNC1 = 0;
@@ -88,29 +90,30 @@ FrameParser::ID3Header() const
 }
 
 const FrameParser::VBRHeader&
 FrameParser::VBRInfo() const
 {
   return mVBRHeader;
 }
 
-bool
-FrameParser::Parse(ByteReader* aReader, uint32_t* aBytesToSkip)
+Result<bool, nsresult>
+FrameParser::Parse(BufferReader* aReader, uint32_t* aBytesToSkip)
 {
   MOZ_ASSERT(aReader && aBytesToSkip);
   *aBytesToSkip = 0;
 
   if (!mID3Parser.Header().Size() && !mFirstFrame.Length()) {
     // No MP3 frames have been parsed yet, look for ID3v2 headers at file begin.
     // ID3v1 tags may only be at file end.
     // TODO: should we try to read ID3 tags at end of file/mid-stream, too?
     const size_t prevReaderOffset = aReader->Offset();
-    const uint32_t tagSize = mID3Parser.Parse(aReader);
-    if (tagSize) {
+    uint32_t tagSize;
+    MOZ_TRY_VAR(tagSize, mID3Parser.Parse(aReader));
+    if (!!tagSize) {
       // ID3 tag found, skip past it.
       const uint32_t skipSize = tagSize - ID3Parser::ID3Header::SIZE;
 
       if (skipSize > aReader->Remaining()) {
         // Skipping across the ID3v2 tag would take us past the end of the
         // buffer, therefore we return immediately and let the calling function
         // handle skipping the rest of the tag.
         MP3LOGV("ID3v2 tag detected, size=%d,"
@@ -123,17 +126,19 @@ FrameParser::Parse(ByteReader* aReader, 
       aReader->Read(skipSize);
     } else {
       // No ID3v2 tag found, rewinding reader in order to search for a MPEG
       // frame header.
       aReader->Seek(prevReaderOffset);
     }
   }
 
-  while (aReader->CanRead8() && !mFrame.ParseNext(aReader->ReadU8())) { }
+  for (auto res = aReader->ReadU8();
+       res.isOk() && !mFrame.ParseNext(res.unwrap()); res = aReader->ReadU8())
+  {}
 
   if (mFrame.Length()) {
     // MP3 frame found.
     if (!mFirstFrame.Length()) {
       mFirstFrame = mFrame;
     }
     // Indicate success.
     return true;
@@ -433,108 +438,123 @@ FrameParser::VBRHeader::Offset(float aDu
 
   if (rest > 0.0 && fullPer + 1 < mTOC.size()) {
     offset += rest * (mTOC.at(fullPer + 1) - offset);
   }
 
   return offset;
 }
 
-bool
-FrameParser::VBRHeader::ParseXing(ByteReader* aReader)
+Result<bool, nsresult>
+FrameParser::VBRHeader::ParseXing(BufferReader* aReader)
 {
   static const uint32_t XING_TAG = BigEndian::readUint32("Xing");
   static const uint32_t INFO_TAG = BigEndian::readUint32("Info");
 
   enum Flags
   {
     NUM_FRAMES = 0x01,
     NUM_BYTES = 0x02,
     TOC = 0x04,
     VBR_SCALE = 0x08
   };
 
   MOZ_ASSERT(aReader);
   const size_t prevReaderOffset = aReader->Offset();
 
   // We have to search for the Xing header as its position can change.
-  while (aReader->CanRead32() &&
-         aReader->PeekU32() != XING_TAG && aReader->PeekU32() != INFO_TAG) {
+  for (auto res = aReader->PeekU32();
+       res.isOk() && res.unwrap() != XING_TAG && res.unwrap() != INFO_TAG;) {
     aReader->Read(1);
+    res = aReader->PeekU32();
   }
 
-  if (aReader->CanRead32()) {
-    // Skip across the VBR header ID tag.
-    aReader->ReadU32();
-    mType = XING;
+  // Skip across the VBR header ID tag.
+  MOZ_TRY(aReader->ReadU32());
+  mType = XING;
+
+  uint32_t flags;
+  MOZ_TRY_VAR(flags, aReader->ReadU32());
+
+  if (flags & NUM_FRAMES) {
+    uint32_t frames;
+    MOZ_TRY_VAR(frames, aReader->ReadU32());
+    mNumAudioFrames = Some(frames);
   }
-  uint32_t flags = 0;
-  if (aReader->CanRead32()) {
-    flags = aReader->ReadU32();
-  }
-  if (flags & NUM_FRAMES && aReader->CanRead32()) {
-    mNumAudioFrames = Some(aReader->ReadU32());
-  }
-  if (flags & NUM_BYTES && aReader->CanRead32()) {
-    mNumBytes = Some(aReader->ReadU32());
+  if (flags & NUM_BYTES) {
+    uint32_t bytes;
+    MOZ_TRY_VAR(bytes, aReader->ReadU32());
+    mNumBytes = Some(bytes);
   }
   if (flags & TOC && aReader->Remaining() >= vbr_header::TOC_SIZE) {
     if (!mNumBytes) {
       // We don't have the stream size to calculate offsets, skip the TOC.
       aReader->Read(vbr_header::TOC_SIZE);
     } else {
       mTOC.clear();
       mTOC.reserve(vbr_header::TOC_SIZE);
+      uint8_t data;
       for (size_t i = 0; i < vbr_header::TOC_SIZE; ++i) {
-        mTOC.push_back(1.0f / 256.0f * aReader->ReadU8() * mNumBytes.value());
+        MOZ_TRY_VAR(data, aReader->ReadU8());
+        mTOC.push_back(1.0f / 256.0f * data * mNumBytes.value());
       }
     }
   }
-  if (flags & VBR_SCALE && aReader->CanRead32()) {
-    mScale = Some(aReader->ReadU32());
+  if (flags & VBR_SCALE) {
+    uint32_t scale;
+    MOZ_TRY_VAR(scale, aReader->ReadU32());
+    mScale = Some(scale);
   }
 
   aReader->Seek(prevReaderOffset);
   return mType == XING;
 }
 
-bool
-FrameParser::VBRHeader::ParseVBRI(ByteReader* aReader)
+Result<bool, nsresult>
+FrameParser::VBRHeader::ParseVBRI(BufferReader* aReader)
 {
   static const uint32_t TAG = BigEndian::readUint32("VBRI");
   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;
 
   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) == 0xFFE0);
+  auto sync = aReader->PeekU16();
+  if (sync.isOk()) { // To avoid compiler complains 'set but unused'.
+    MOZ_ASSERT((sync.unwrap() & 0xFFE0) == 0xFFE0);
+  }
   const size_t prevReaderOffset = aReader->Offset();
 
   // VBRI have a fixed relative position, so let's check for it there.
   if (aReader->Remaining() > MIN_FRAME_SIZE) {
     aReader->Seek(prevReaderOffset + OFFSET);
-    if (aReader->ReadU32() == TAG) {
+    uint32_t tag, frames;
+    MOZ_TRY_VAR(tag, aReader->ReadU32());
+    if (tag == TAG) {
       aReader->Seek(prevReaderOffset + FRAME_COUNT_OFFSET);
-      mNumAudioFrames = Some(aReader->ReadU32());
+      MOZ_TRY_VAR(frames, aReader->ReadU32());
+      mNumAudioFrames = Some(frames);
       mType = VBRI;
       aReader->Seek(prevReaderOffset);
       return true;
     }
   }
   aReader->Seek(prevReaderOffset);
   return false;
 }
 
 bool
-FrameParser::VBRHeader::Parse(ByteReader* aReader)
+FrameParser::VBRHeader::Parse(BufferReader* aReader)
 {
-  const bool rv = ParseVBRI(aReader) || ParseXing(aReader);
+  auto res = MakePair(ParseVBRI(aReader), ParseXing(aReader));
+  const bool rv = (res.first().isOk() && res.first().unwrap()) ||
+                  (res.second().isOk() && res.second().unwrap());
   if (rv) {
     MP3LOG("VBRHeader::Parse found valid VBR/CBR header: type=%s"
            " NumAudioFrames=%u NumBytes=%u Scale=%u TOC-size=%zu",
            vbr_header::TYPE_STR[Type()], NumAudioFrames().valueOr(0),
            NumBytes().valueOr(0), Scale().valueOr(0), mTOC.size());
   }
   return rv;
 }
@@ -569,17 +589,17 @@ FrameParser::Frame::ParseNext(uint8_t c)
 
 const FrameParser::FrameHeader&
 FrameParser::Frame::Header() const
 {
   return mHeader;
 }
 
 bool
-FrameParser::ParseVBRHeader(ByteReader* aReader)
+FrameParser::ParseVBRHeader(BufferReader* aReader)
 {
   return mVBRHeader.Parse(aReader);
 }
 
 // ID3Parser
 
 // Constants
 namespace id3_header {
@@ -594,22 +614,24 @@ static const int FLAGS_END = VERSION_END
 static const int SIZE_END = FLAGS_END + SIZE_LEN;
 
 static const uint8_t ID[ID_LEN] = {'I', 'D', '3'};
 
 static const uint8_t MIN_MAJOR_VER = 2;
 static const uint8_t MAX_MAJOR_VER = 4;
 } // namespace id3_header
 
-uint32_t
-ID3Parser::Parse(ByteReader* aReader)
+Result<uint32_t, nsresult>
+ID3Parser::Parse(BufferReader* aReader)
 {
   MOZ_ASSERT(aReader);
 
-  while (aReader->CanRead8() && !mHeader.ParseNext(aReader->ReadU8())) { }
+  for (auto res = aReader->ReadU8();
+       res.isOk() && !mHeader.ParseNext(res.unwrap()); res = aReader->ReadU8())
+  {}
 
   return mHeader.TotalTagSize();
 }
 
 void
 ID3Parser::Reset()
 {
   mHeader.Reset();
--- a/dom/media/mp3/MP3FrameParser.h
+++ b/dom/media/mp3/MP3FrameParser.h
@@ -3,17 +3,18 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef MP3_FRAME_PARSER_H_
 #define MP3_FRAME_PARSER_H_
 
 #include <vector>
 
 #include "mozilla/Maybe.h"
-#include "mp4_demuxer/ByteReader.h"
+#include "mozilla/Result.h"
+#include "mp4_demuxer/BufferReader.h"
 
 namespace mozilla {
 
 // ID3 header parser state machine used by FrameParser.
 // The header contains the following format (one byte per term):
 // 'I' 'D' '3' MajorVersion MinorVersion Flags Size1 Size2 Size3 Size4
 // For more details see http://id3.org/id3v2.3.0.
 class ID3Parser
@@ -76,19 +77,19 @@ public:
     // The current byte position in the parsed sequence. Reset via Reset and
     // incremented via Update.
     int mPos;
   };
 
   // Returns the parsed ID3 header. Note: check for validity.
   const ID3Header& Header() const;
 
-  // Parses contents of given ByteReader for a valid ID3v2 header.
+  // Parses contents of given BufferReader for a valid ID3v2 header.
   // Returns the total ID3v2 tag size if successful and zero otherwise.
-  uint32_t Parse(mp4_demuxer::ByteReader* aReader);
+  Result<uint32_t, nsresult> Parse(mp4_demuxer::BufferReader* aReader);
 
   // Resets the state to allow for a new parsing session.
   void Reset();
 
 private:
   // The currently parsed ID3 header. Reset via Reset, updated via Parse.
   ID3Header mHeader;
 };
@@ -222,30 +223,30 @@ public:
     // Returns the byte offset for the given duration percentage as a factor
     // (0: begin, 1.0: end).
     int64_t Offset(float aDurationFac) const;
 
     // 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(mp4_demuxer::ByteReader* aReader);
+    bool Parse(mp4_demuxer::BufferReader* aReader);
 
   private:
     // 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(mp4_demuxer::ByteReader* aReader);
+    Result<bool, nsresult> ParseXing(mp4_demuxer::BufferReader* aReader);
 
     // 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(mp4_demuxer::ByteReader* aReader);
+    Result<bool, nsresult> ParseVBRI(mp4_demuxer::BufferReader* aReader);
 
     // The total number of frames expected as parsed from a VBR header.
     Maybe<uint32_t> mNumAudioFrames;
 
     // The total number of bytes expected in the stream.
     Maybe<uint32_t> mNumBytes;
 
     // The VBR scale factor.
@@ -306,27 +307,27 @@ public:
   void ResetFrameData();
 
   // Clear the last parsed frame to allow for next frame parsing, i.e.:
   // - sets PrevFrame to CurrentFrame
   // - resets the CurrentFrame
   // - resets ID3Header if no valid header was parsed yet
   void EndFrameSession();
 
-  // Parses contents of given ByteReader for a valid frame header and returns
+  // Parses contents of given BufferReader for a valid frame header and returns
   // true if one was found. After returning, the variable passed to
   // 'aBytesToSkip' holds the amount of bytes to be skipped (if any) in order to
   // jump across a large ID3v2 tag spanning multiple buffers.
-  bool Parse(mp4_demuxer::ByteReader* aReader, uint32_t* aBytesToSkip);
+  Result<bool, nsresult> Parse(mp4_demuxer::BufferReader* aReader, uint32_t* aBytesToSkip);
 
-  // Parses contents of given ByteReader for a valid VBR header.
-  // The offset of the passed ByteReader needs to point to an MPEG frame begin,
+  // Parses contents of given BufferReader for a valid VBR header.
+  // The offset of the passed BufferReader 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(mp4_demuxer::ByteReader* aReader);
+  bool ParseVBRHeader(mp4_demuxer::BufferReader* aReader);
 
 private:
   // ID3 header parser.
   ID3Parser mID3Parser;
 
   // VBR header parser.
   VBRHeader mVBRHeader;
 
--- a/media/libstagefright/binding/include/mp4_demuxer/BufferReader.h
+++ b/media/libstagefright/binding/include/mp4_demuxer/BufferReader.h
@@ -187,22 +187,22 @@ public:
     auto ptr = Peek(1);
     if (!ptr) {
       NS_WARNING("Failed to peek data");
       return 0;
     }
     return *ptr;
   }
 
-  uint16_t PeekU16() const
+  mozilla::Result<uint16_t, nsresult> PeekU16() const
   {
     auto ptr = Peek(2);
     if (!ptr) {
       NS_WARNING("Failed to peek data");
-      return 0;
+      return mozilla::Err(NS_ERROR_FAILURE);
     }
     return mozilla::BigEndian::readUint16(ptr);
   }
 
   uint32_t PeekU24() const
   {
     auto ptr = Peek(3);
     if (!ptr) {
@@ -212,22 +212,22 @@ public:
     return ptr[0] << 16 | ptr[1] << 8 | ptr[2];
   }
 
   uint32_t Peek24() const
   {
     return (uint32_t)PeekU24();
   }
 
-  uint32_t PeekU32() const
+  mozilla::Result<uint32_t, nsresult> PeekU32()
   {
     auto ptr = Peek(4);
     if (!ptr) {
       NS_WARNING("Failed to peek data");
-      return 0;
+      return mozilla::Err(NS_ERROR_FAILURE);
     }
     return mozilla::BigEndian::readUint32(ptr);
   }
 
   int32_t Peek32() const
   {
     auto ptr = Peek(4);
     if (!ptr) {