Bug 1298710 - Remove ByteReader::DiscardRemaining and AutoByteReader - r=jya
authorGerald Squelart <gsquelart@mozilla.com>
Sun, 04 Sep 2016 18:33:30 +1000
changeset 312635 ce2a0cbdb4e80f88c9a12f63dcb49176525d3dd2
parent 312634 0768e4f23ebf7ccfc7529498979e2a16dc2b39f4
child 312649 00c15597bb8d2e2c6615fe0b8ae616a42b558694
push id30650
push userryanvm@gmail.com
push dateSun, 04 Sep 2016 17:35:16 +0000
treeherdermozilla-central@ce2a0cbdb4e8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjya
bugs1298710
milestone51.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1298710 - Remove ByteReader::DiscardRemaining and AutoByteReader - r=jya DiscardRemaning was needed to prevent debug-time assertion that the buffer was read completely or explicitly discarded. However this required extra work in cases where buffer didn't need to be read to the end. And also it could cause crashes (in debug versions) if a buffer was not fully read, be it because the parser was incorrect or because the media file itself was wrong (though possibly still readable despite that). Finding parser issues is still possible by manually instrumenting ByteReader during development. And reading media file with small recoverable errors is a bonus. MozReview-Commit-ID: 2RUYzaYAeRW
dom/media/MP3Demuxer.cpp
dom/media/flac/FlacFrameParser.cpp
dom/media/mediasource/ContainerParser.cpp
dom/media/platforms/agnostic/WAVDecoder.cpp
media/libstagefright/binding/AnnexB.cpp
media/libstagefright/binding/H264.cpp
media/libstagefright/binding/include/mp4_demuxer/Box.h
media/libstagefright/binding/include/mp4_demuxer/ByteReader.h
--- a/dom/media/MP3Demuxer.cpp
+++ b/dom/media/MP3Demuxer.cpp
@@ -488,17 +488,16 @@ MP3TrackDemuxer::FindNextFrame() {
     ByteReader reader(buffer, read);
     uint32_t bytesToSkip = 0;
     foundFrame = mParser.Parse(&reader, &bytesToSkip);
     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());
-    reader.DiscardRemaining();
 
     if (foundFrame && mParser.FirstFrame().Length() &&
         !VerifyFrameConsistency(mParser.FirstFrame(), mParser.CurrentFrame())) {
       // We've likely hit a false-positive, ignore it and proceed with the
       // search for the next valid frame.
       foundFrame = false;
       mOffset = frameHeaderOffset + 1;
       mParser.EndFrameSession();
@@ -579,17 +578,16 @@ MP3TrackDemuxer::GetNextFrame(const Medi
 
   MOZ_ASSERT(frame->mTime >= 0);
   MOZ_ASSERT(frame->mDuration > 0);
 
   if (mNumParsedFrames == 1) {
     // First frame parsed, let's read VBR info if available.
     ByteReader reader(frame->Data(), frame->Size());
     mParser.ParseVBRHeader(&reader);
-    reader.DiscardRemaining();
     mFirstFrameOffset = frame->mOffset;
   }
 
   MP3LOGV("GetNext() End mOffset=%" PRIu64 " mNumParsedFrames=%" PRIu64
           " mFrameIndex=%" PRId64 " mTotalFrameLen=%" PRIu64
           " mSamplesPerFrame=%d mSamplesPerSecond=%d mChannels=%d",
           mOffset, mNumParsedFrames, mFrameIndex, mTotalFrameLen,
           mSamplesPerFrame, mSamplesPerSecond, mChannels);
--- a/dom/media/flac/FlacFrameParser.cpp
+++ b/dom/media/flac/FlacFrameParser.cpp
@@ -5,17 +5,17 @@
 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "FlacFrameParser.h"
 #include "mp4_demuxer/ByteReader.h"
 #include "nsTArray.h"
 #include "OggCodecState.h"
 #include "VideoUtils.h"
 
-using mp4_demuxer::AutoByteReader;
+using mp4_demuxer::ByteReader;
 
 namespace mozilla
 {
 
 #define OGG_FLAC_METADATA_TYPE_STREAMINFO 0x7F
 #define FLAC_STREAMINFO_SIZE   34
 
 #define BITMASK(x) ((1ULL << x)-1)
@@ -57,17 +57,17 @@ FlacFrameParser::HeaderBlockLength(const
 
 bool
 FlacFrameParser::DecodeHeaderBlock(const uint8_t* aPacket, size_t aLength)
 {
   if (aLength < 4 || aPacket[0] == 0xff) {
     // Not a header block.
     return false;
   }
-  AutoByteReader br(aPacket, aLength);
+  ByteReader br(aPacket, aLength);
 
   mPacketCount++;
 
   if (aPacket[0] == 'f') {
     if (mPacketCount != 1 || memcmp(br.Read(4), "fLaC", 4) ||
         br.Remaining() != FLAC_STREAMINFO_SIZE + 4) {
       return false;
     }
@@ -205,21 +205,21 @@ FlacFrameParser::IsHeaderBlock(const uin
 
   // If we detect either a ogg or plain flac header, then it must be valid.
   if (aLength < 4 || aPacket[0] == 0xff) {
     // A header is at least 4 bytes.
     return false;
   }
   if (aPacket[0] == 0x7f) {
     // Ogg packet
-    AutoByteReader br(aPacket + 1, aLength - 1);
+    ByteReader br(aPacket + 1, aLength - 1);
     const uint8_t* signature = br.Read(4);
     return signature && !memcmp(signature, "FLAC", 4);
   }
-  AutoByteReader br(aPacket, aLength - 1);
+  ByteReader br(aPacket, aLength - 1);
   const uint8_t* signature = br.Read(4);
   if (signature && !memcmp(signature, "fLaC", 4)) {
     // Flac start header, must have STREAMINFO as first metadata block;
     if (!br.CanRead8()) {
       return false;
     }
     uint32_t blockType = br.ReadU8() & 0x7f;
     return blockType == FLAC_METADATA_TYPE_STREAMINFO;
--- a/dom/media/mediasource/ContainerParser.cpp
+++ b/dom/media/mediasource/ContainerParser.cpp
@@ -394,17 +394,16 @@ private:
           break;
         }
         if (reader.Remaining() < size - 8) {
           // Incomplete atom.
           break;
         }
         reader.Read(size - 8);
       }
-      reader.DiscardRemaining();
     }
 
     bool StartWithInitSegment()
     {
       return mInitOffset.isSome() &&
         (mMediaOffset.isNothing() || mInitOffset.ref() < mMediaOffset.ref());
     }
     bool StartWithMediaSegment()
--- a/dom/media/platforms/agnostic/WAVDecoder.cpp
+++ b/dom/media/platforms/agnostic/WAVDecoder.cpp
@@ -113,18 +113,16 @@ WaveDataDecoder::DoDecode(MediaRawData* 
           int32_t v = aReader.ReadLE24();
           buffer[i * mInfo.mChannels + j] =
               Int24bitToAudioSample<AudioDataValue>(v);
         }
       }
     }
   }
 
-  aReader.DiscardRemaining();
-
   int64_t duration = frames / mInfo.mRate;
 
   mCallback->Output(new AudioData(aOffset,
                                   aTstampUsecs,
                                   duration,
                                   frames,
                                   Move(buffer),
                                   mInfo.mChannels,
--- a/media/libstagefright/binding/AnnexB.cpp
+++ b/media/libstagefright/binding/AnnexB.cpp
@@ -94,17 +94,16 @@ AnnexB::ConvertExtraDataToAnnexB(const m
   const uint8_t* ptr = reader.Read(5);
   if (ptr && ptr[0] == 1) {
     // Append SPS then PPS
     ConvertSPSOrPPS(reader, reader.ReadU8() & 31, annexB);
     ConvertSPSOrPPS(reader, reader.ReadU8(), annexB);
 
     // MP4Box adds extra bytes that we ignore. I don't know what they do.
   }
-  reader.DiscardRemaining();
 
   return annexB.forget();
 }
 
 void
 AnnexB::ConvertSPSOrPPS(ByteReader& aReader, uint8_t aCount,
                         mozilla::MediaByteBuffer* aAnnexB)
 {
@@ -325,17 +324,16 @@ AnnexB::HasSPS(const mozilla::MediaByteB
   }
 
   ByteReader reader(aExtraData);
   const uint8_t* ptr = reader.Read(5);
   if (!ptr || !reader.CanRead8()) {
     return false;
   }
   uint8_t numSps = reader.ReadU8() & 0x1f;
-  reader.DiscardRemaining();
 
   return numSps > 0;
 }
 
 bool
 AnnexB::ConvertSampleTo4BytesAVCC(mozilla::MediaRawData* aSample)
 {
   MOZ_ASSERT(IsAVCC(aSample));
--- a/media/libstagefright/binding/H264.cpp
+++ b/media/libstagefright/binding/H264.cpp
@@ -417,34 +417,30 @@ H264::DecodeSPSFromExtraData(const mozil
   ByteReader reader(aExtraData);
 
   if (!reader.Read(5)) {
     return false;
   }
 
   if (!(reader.ReadU8() & 0x1f)) {
     // No SPS.
-    reader.DiscardRemaining();
     return false;
   }
   uint16_t length = reader.ReadU16();
 
   if ((reader.PeekU8() & 0x1f) != 7) {
     // Not a SPS NAL type.
-    reader.DiscardRemaining();
     return false;
   }
 
   const uint8_t* ptr = reader.Read(length);
   if (!ptr) {
     return false;
   }
 
-  reader.DiscardRemaining();
-
   RefPtr<mozilla::MediaByteBuffer> rawNAL = new mozilla::MediaByteBuffer;
   rawNAL->AppendElements(ptr, length);
 
   RefPtr<mozilla::MediaByteBuffer> sps = DecodeNALUnit(rawNAL);
 
   if (!sps) {
     return false;
   }
--- a/media/libstagefright/binding/include/mp4_demuxer/Box.h
+++ b/media/libstagefright/binding/include/mp4_demuxer/Box.h
@@ -68,17 +68,17 @@ MOZ_RAII
 class BoxReader
 {
 public:
   explicit BoxReader(Box& aBox)
     : mBuffer(aBox.Read())
     , mReader(mBuffer.Elements(), mBuffer.Length())
   {
   }
-  AutoByteReader* operator->() { return &mReader; }
+  ByteReader* operator->() { return &mReader; }
 
 private:
   nsTArray<uint8_t> mBuffer;
-  AutoByteReader mReader;
+  ByteReader mReader;
 };
 }
 
 #endif
--- a/media/libstagefright/binding/include/mp4_demuxer/ByteReader.h
+++ b/media/libstagefright/binding/include/mp4_demuxer/ByteReader.h
@@ -43,30 +43,23 @@ public:
     MOZ_ASSERT(!mPtr && !mRemaining);
     mPtr = aData.Elements();
     mRemaining = aData.Length();
     mLength = mRemaining;
   }
 
   ~ByteReader()
   {
-    NS_ASSERTION(!mRemaining, "Not all bytes have been processed");
   }
 
   size_t Offset()
   {
     return mLength - mRemaining;
   }
 
-  // Make it explicit if we're not using the extra bytes.
-  void DiscardRemaining()
-  {
-    mRemaining = 0;
-  }
-
   size_t Remaining() const { return mRemaining; }
 
   bool CanRead8() { return mRemaining >= 1; }
 
   uint8_t ReadU8()
   {
     auto ptr = Read(1);
     if (!ptr) {
@@ -346,28 +339,11 @@ public:
   }
 
 private:
   const uint8_t* mPtr;
   size_t mRemaining;
   size_t mLength;
 };
 
-// A ByteReader that automatically discards remaining data before destruction.
-class MOZ_RAII AutoByteReader : public ByteReader
-{
-public:
-  AutoByteReader(const uint8_t* aData, size_t aSize)
-    : ByteReader(aData, aSize)
-  {
-  }
-  ~AutoByteReader()
-  {
-    ByteReader::DiscardRemaining();
-  }
-
-  // Prevent unneeded DiscardRemaining() calls.
-  void DiscardRemaining() = delete;
-};
-
 } // namespace mp4_demuxer
 
 #endif