Bug 1315554 - Part 5. Add method to clone a SourceBufferIterator when decoding. r=tnikkel
☠☠ backed out by 2df50fe08bc6 ☠ ☠
authorAndrew Osmond <aosmond@mozilla.com>
Sat, 22 Jul 2017 00:14:59 -0400
changeset 370280 6fa3ad97ce9c28ac9e66adf75c37142b0a9e21e1
parent 370279 e67f6df41836b70373f7337ae392533ac9c8c7bf
child 370281 e39309b6fe7f29b2ecfb16bf3c17c710b74d11f4
push id92837
push useraosmond@gmail.com
push dateSat, 22 Jul 2017 04:15:12 +0000
treeherdermozilla-inbound@abc949687bdc [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstnikkel
bugs1315554
milestone56.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 1315554 - Part 5. Add method to clone a SourceBufferIterator when decoding. r=tnikkel
image/SourceBuffer.cpp
image/SourceBuffer.h
image/StreamingLexer.h
image/test/gtest/TestSourceBuffer.cpp
--- a/image/SourceBuffer.cpp
+++ b/image/SourceBuffer.cpp
@@ -37,16 +37,17 @@ SourceBufferIterator::operator=(SourceBu
     mOwner->OnIteratorRelease();
   }
 
   mOwner = Move(aOther.mOwner);
   mState = aOther.mState;
   mData = aOther.mData;
   mChunkCount = aOther.mChunkCount;
   mByteCount = aOther.mByteCount;
+  mRemainderToRead = aOther.mRemainderToRead;
 
   return *this;
 }
 
 SourceBufferIterator::State
 SourceBufferIterator::AdvanceOrScheduleResume(size_t aRequestedBytes,
                                               IResumable* aConsumer)
 {
@@ -58,16 +59,35 @@ SourceBufferIterator::AdvanceOrScheduleR
   }
 
   // The range of data [mOffset, mOffset + mNextReadLength) has just been read
   // by the caller (or at least they don't have any interest in it), so consume
   // that data.
   MOZ_ASSERT(mData.mIterating.mNextReadLength <= mData.mIterating.mAvailableLength);
   mData.mIterating.mOffset += mData.mIterating.mNextReadLength;
   mData.mIterating.mAvailableLength -= mData.mIterating.mNextReadLength;
+
+  // An iterator can have a limit imposed on it to read only a subset of a
+  // source buffer. If it is present, we need to mimic the same behaviour as
+  // the owning SourceBuffer.
+  if (MOZ_UNLIKELY(mRemainderToRead != SIZE_MAX)) {
+    MOZ_ASSERT(mData.mIterating.mNextReadLength <= mRemainderToRead);
+    mRemainderToRead -= mData.mIterating.mNextReadLength;
+
+    if (MOZ_UNLIKELY(mRemainderToRead == 0)) {
+      mData.mIterating.mNextReadLength = 0;
+      SetComplete(NS_OK);
+      return COMPLETE;
+    }
+
+    if (MOZ_UNLIKELY(aRequestedBytes > mRemainderToRead)) {
+      aRequestedBytes = mRemainderToRead;
+    }
+  }
+
   mData.mIterating.mNextReadLength = 0;
 
   if (MOZ_LIKELY(mState == READY)) {
     // If the caller wants zero bytes of data, that's easy enough; we just
     // configured ourselves for a zero-byte read above!  In theory we could do
     // this even in the START state, but it's not important for performance and
     // breaking the ability of callers to assert that the pointer returned by
     // Data() is non-null doesn't seem worth it.
@@ -513,24 +533,24 @@ SourceBuffer::SizeOfIncludingThisWithCom
 
     n += chunkSize;
   }
 
   return n;
 }
 
 SourceBufferIterator
-SourceBuffer::Iterator()
+SourceBuffer::Iterator(size_t aReadLength)
 {
   {
     MutexAutoLock lock(mMutex);
     mConsumerCount++;
   }
 
-  return SourceBufferIterator(this);
+  return SourceBufferIterator(this, aReadLength);
 }
 
 void
 SourceBuffer::OnIteratorRelease()
 {
   MutexAutoLock lock(mMutex);
 
   MOZ_ASSERT(mConsumerCount > 0, "Consumer count doesn't add up");
--- a/image/SourceBuffer.h
+++ b/image/SourceBuffer.h
@@ -72,36 +72,38 @@ class SourceBufferIterator final
 public:
   enum State {
     START,    // The iterator is at the beginning of the buffer.
     READY,    // The iterator is pointing to new data.
     WAITING,  // The iterator is blocked and the caller must yield.
     COMPLETE  // The iterator is pointing to the end of the buffer.
   };
 
-  explicit SourceBufferIterator(SourceBuffer* aOwner)
+  explicit SourceBufferIterator(SourceBuffer* aOwner, size_t aReadLimit)
     : mOwner(aOwner)
     , mState(START)
     , mChunkCount(0)
     , mByteCount(0)
+    , mRemainderToRead(aReadLimit)
   {
     MOZ_ASSERT(aOwner);
     mData.mIterating.mChunk = 0;
     mData.mIterating.mData = nullptr;
     mData.mIterating.mOffset = 0;
     mData.mIterating.mAvailableLength = 0;
     mData.mIterating.mNextReadLength = 0;
   }
 
   SourceBufferIterator(SourceBufferIterator&& aOther)
     : mOwner(Move(aOther.mOwner))
     , mState(aOther.mState)
     , mData(aOther.mData)
     , mChunkCount(aOther.mChunkCount)
     , mByteCount(aOther.mByteCount)
+    , mRemainderToRead(aOther.mRemainderToRead)
   { }
 
   ~SourceBufferIterator();
 
   SourceBufferIterator& operator=(SourceBufferIterator&& aOther);
 
   /**
    * Returns true if there are no more than @aBytes remaining in the
@@ -174,16 +176,29 @@ public:
   }
 
   /// @return a count of the chunks we've advanced through.
   uint32_t ChunkCount() const { return mChunkCount; }
 
   /// @return a count of the bytes in all chunks we've advanced through.
   size_t ByteCount() const { return mByteCount; }
 
+  /// @return the source buffer which owns the iterator.
+  SourceBuffer* Owner() const
+  {
+    MOZ_ASSERT(mOwner);
+    return mOwner;
+  }
+
+  /// @return the current offset from the beginning of the buffer.
+  size_t Position() const
+  {
+    return mByteCount - mData.mIterating.mAvailableLength;
+  }
+
 private:
   friend class SourceBuffer;
 
   SourceBufferIterator(const SourceBufferIterator&) = delete;
   SourceBufferIterator& operator=(const SourceBufferIterator&) = delete;
 
   bool HasMore() const { return mState != COMPLETE; }
 
@@ -203,16 +218,21 @@ private:
 
   State SetReady(uint32_t aChunk, const char* aData,
                  size_t aOffset, size_t aAvailableLength,
                  size_t aRequestedBytes)
   {
     MOZ_ASSERT(mState != COMPLETE);
     mState = READY;
 
+    // Prevent the iterator from reporting more data than it is allowed to read.
+    if (aAvailableLength > mRemainderToRead) {
+      aAvailableLength = mRemainderToRead;
+    }
+
     // Update state.
     mData.mIterating.mChunk = aChunk;
     mData.mIterating.mData = aData;
     mData.mIterating.mOffset = aOffset;
     mData.mIterating.mAvailableLength = aAvailableLength;
 
     // Update metrics.
     mChunkCount++;
@@ -241,29 +261,37 @@ private:
 
   /**
    * This union contains our iteration state if we're still iterating (for
    * states START, READY, and WAITING) and the status the SourceBuffer was
    * completed with if we're in state COMPLETE.
    */
   union {
     struct {
-      uint32_t mChunk;
-      const char* mData;
-      size_t mOffset;
-      size_t mAvailableLength;
-      size_t mNextReadLength;
-    } mIterating;
+      uint32_t mChunk;   // Index of the chunk in SourceBuffer.
+      const char* mData; // Pointer to the start of the chunk.
+      size_t mOffset;    // Current read position of the iterator relative to
+                         // mData.
+      size_t mAvailableLength; // How many bytes remain unread in the chunk,
+                               // relative to mOffset.
+      size_t mNextReadLength; // How many bytes the last iterator advance
+                              // requested to be read, so that we know much
+                              // to increase mOffset and reduce mAvailableLength
+                              // by when the next advance is requested.
+    } mIterating;        // Cached info of the chunk currently iterating over.
     struct {
-      nsresult mStatus;
-    } mAtEnd;
+      nsresult mStatus;  // Status code indicating if we read all the data.
+    } mAtEnd;            // State info after iterator is complete.
   } mData;
 
-  uint32_t mChunkCount;  // Count of chunks we've advanced through.
-  size_t mByteCount;     // Count of bytes in all chunks we've advanced through.
+  uint32_t mChunkCount;  // Count of chunks observed, including current chunk.
+  size_t mByteCount;     // Count of readable bytes observed, including unread
+                         // bytes from the current chunk.
+  size_t mRemainderToRead; // Count of bytes left to read if there is a maximum
+                           // imposed by the caller. SIZE_MAX if unlimited.
 };
 
 /**
  * SourceBuffer is a parallel data structure used for storing image source
  * (compressed) data.
  *
  * SourceBuffer is a single producer, multiple consumer data structure. The
  * single producer calls Append() to append data to the buffer. In parallel,
@@ -314,18 +342,21 @@ public:
   /// Memory reporting.
   size_t SizeOfIncludingThisWithComputedFallback(MallocSizeOf) const;
 
 
   //////////////////////////////////////////////////////////////////////////////
   // Consumer methods.
   //////////////////////////////////////////////////////////////////////////////
 
-  /// Returns an iterator to this SourceBuffer.
-  SourceBufferIterator Iterator();
+  /**
+   * Returns an iterator to this SourceBuffer, which cannot read more than the
+   * given length.
+   */
+  SourceBufferIterator Iterator(size_t aReadLength = SIZE_MAX);
 
 
   //////////////////////////////////////////////////////////////////////////////
   // Consumer methods.
   //////////////////////////////////////////////////////////////////////////////
 
   /**
    * The minimum chunk capacity we'll allocate, if we don't know the correct
--- a/image/StreamingLexer.h
+++ b/image/StreamingLexer.h
@@ -388,16 +388,62 @@ public:
       // zero bytes.
       MOZ_ASSERT_UNREACHABLE("Truncated state makes no sense");
       return;
     }
 
     SetTransition(aStartState);
   }
 
+  /**
+   * From the given SourceBufferIterator, aIterator, create a new iterator at
+   * the same position, with the given read limit, aReadLimit. The read limit
+   * applies after adjusting for the position. If the given iterator has been
+   * advanced, but required buffering inside StreamingLexer, the position
+   * of the cloned iterator will be at the beginning of buffered data; this
+   * should match the perspective of the caller.
+   */
+  SourceBufferIterator Clone(SourceBufferIterator& aIterator,
+                             size_t aReadLimit) const
+  {
+    // In order to advance to the current position of the iterator from the
+    // perspective of the caller, we need to take into account if we are
+    // buffering data.
+    size_t pos = aIterator.Position();
+    if (!mBuffer.empty()) {
+      pos += aIterator.Length();
+      MOZ_ASSERT(pos > mBuffer.length());
+      pos -= mBuffer.length();
+    }
+
+    size_t readLimit = aReadLimit;
+    if (aReadLimit != SIZE_MAX) {
+      readLimit += pos;
+    }
+
+    SourceBufferIterator other = aIterator.Owner()->Iterator(readLimit);
+
+    // Since the current iterator has already advanced to this point, we
+    // know that the state can only be READY or COMPLETE. That does not mean
+    // everything is stored in a single chunk, and may require multiple Advance
+    // calls to get where we want to be.
+    DebugOnly<SourceBufferIterator::State> state;
+    do {
+      state = other.Advance(pos);
+      MOZ_ASSERT(state != SourceBufferIterator::WAITING);
+      MOZ_ASSERT(pos >= other.Length());
+      pos -= other.Length();
+    } while (pos > 0);
+
+    // Force the data pointer to be where we expect it to be.
+    state = other.Advance(0);
+    MOZ_ASSERT(state != SourceBufferIterator::WAITING);
+    return other;
+  }
+
   template <typename Func>
   LexerResult Lex(SourceBufferIterator& aIterator,
                   IResumable* aOnResume,
                   Func aFunc)
   {
     if (mTransition.NextStateIsTerminal()) {
       // We've already reached a terminal state. We never deliver any more data
       // in this case; just return the terminal state again immediately.
--- a/image/test/gtest/TestSourceBuffer.cpp
+++ b/image/test/gtest/TestSourceBuffer.cpp
@@ -803,8 +803,60 @@ TEST_F(ImageSourceBuffer, ExpectLengthDo
 
   // Check that we can't advance.
   CheckIteratorMustWait(iterator, mExpectNoResume);
 
   // Call ExpectLength(). If this triggers a resume, |mExpectNoResume| will
   // ensure that the test fails.
   mSourceBuffer->ExpectLength(1000);
 }
+
+TEST_F(ImageSourceBuffer, CompleteSuccessWithSameReadLength)
+{
+  SourceBufferIterator iterator = mSourceBuffer->Iterator(1);
+
+  // Write a single byte to the buffer and complete the buffer. (We have to
+  // write at least one byte because completing a zero length buffer always
+  // fails; see the ZeroLengthBufferAlwaysFails test.)
+  CheckedAppendToBuffer(mData, 1);
+  CheckedCompleteBuffer(iterator, 1);
+
+  // We should be able to advance once (to read the single byte) and then should
+  // reach the COMPLETE state with a successful status.
+  CheckedAdvanceIterator(iterator, 1);
+  CheckIteratorIsComplete(iterator, 1);
+}
+
+TEST_F(ImageSourceBuffer, CompleteSuccessWithSmallerReadLength)
+{
+  // Create an iterator limited to one byte.
+  SourceBufferIterator iterator = mSourceBuffer->Iterator(1);
+
+  // Write two bytes to the buffer and complete the buffer. (We have to
+  // write at least one byte because completing a zero length buffer always
+  // fails; see the ZeroLengthBufferAlwaysFails test.)
+  CheckedAppendToBuffer(mData, 2);
+  CheckedCompleteBuffer(iterator, 2);
+
+  // We should be able to advance once (to read the single byte) and then should
+  // reach the COMPLETE state with a successful status, because our iterator is
+  // limited to a single byte, rather than the full length.
+  CheckedAdvanceIterator(iterator, 1);
+  CheckIteratorIsComplete(iterator, 1);
+}
+
+TEST_F(ImageSourceBuffer, CompleteSuccessWithGreaterReadLength)
+{
+  // Create an iterator limited to one byte.
+  SourceBufferIterator iterator = mSourceBuffer->Iterator(2);
+
+  // Write a single byte to the buffer and complete the buffer. (We have to
+  // write at least one byte because completing a zero length buffer always
+  // fails; see the ZeroLengthBufferAlwaysFails test.)
+  CheckedAppendToBuffer(mData, 1);
+  CheckedCompleteBuffer(iterator, 1);
+
+  // We should be able to advance once (to read the single byte) and then should
+  // reach the COMPLETE state with a successful status. Our iterator lets us
+  // read more but the underlying buffer has been completed.
+  CheckedAdvanceIterator(iterator, 1);
+  CheckIteratorIsComplete(iterator, 1);
+}