Bug 1262338 (Part 1) - Ensure that SurfaceFilter always knows when AdvanceRow() is called. r=njn a=abillings
authorSeth Fowler <mark.seth.fowler@gmail.com>
Wed, 25 May 2016 22:48:29 -0700
changeset 335036 b9ab03b42c5de945ad520bccc1483e9da0a9d751
parent 335035 abce57bf965ba789f1ae52cf5865d60177132385
child 335037 ca85cebb809dde03109b92b670a3d34907a24ae4
push id1146
push userCallek@gmail.com
push dateMon, 25 Jul 2016 16:35:44 +0000
treeherdermozilla-release@a55778f9cd5a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnjn, abillings
bugs1262338
milestone48.0a2
Bug 1262338 (Part 1) - Ensure that SurfaceFilter always knows when AdvanceRow() is called. r=njn a=abillings
image/DownscalingFilter.h
image/SurfaceFilters.h
image/SurfacePipe.cpp
image/SurfacePipe.h
--- a/image/DownscalingFilter.h
+++ b/image/DownscalingFilter.h
@@ -67,27 +67,27 @@ struct DownscalingConfig
  * manually constructs an instance and attempts to actually use it. Callers
  * should avoid this by ensuring that they do not request downscaling in
  * non-Skia builds.
  */
 template <typename Next>
 class DownscalingFilter final : public SurfaceFilter
 {
 public:
-  uint8_t* AdvanceRow() override { MOZ_CRASH(); return nullptr; }
   Maybe<SurfaceInvalidRect> TakeInvalidRect() override { return Nothing(); }
 
   template <typename... Rest>
   nsresult Configure(const DownscalingConfig& aConfig, Rest... aRest)
   {
     return NS_ERROR_FAILURE;
   }
 
 protected:
   uint8_t* DoResetToFirstRow() override { MOZ_CRASH(); return nullptr; }
+  uint8_t* DoAdvanceRow() override { MOZ_CRASH(); return nullptr; }
 };
 
 #else
 
 /**
  * DownscalingFilter performs Lanczos downscaling, taking image input data at one size
  * and outputting it rescaled to a different size.
  *
@@ -210,17 +210,29 @@ public:
     if (invalidRect) {
       // Compute the input space invalid rect by scaling.
       invalidRect->mInputSpaceRect.ScaleRoundOut(mScale.width, mScale.height);
     }
 
     return invalidRect;
   }
 
-  uint8_t* AdvanceRow() override
+protected:
+  uint8_t* DoResetToFirstRow() override
+  {
+    mNext.ResetToFirstRow();
+
+    mInputRow = 0;
+    mOutputRow = 0;
+    mRowsInWindow = 0;
+
+    return GetRowPointer();
+  }
+
+  uint8_t* DoAdvanceRow() override
   {
     if (mInputRow >= mInputSize.height) {
       NS_WARNING("Advancing DownscalingFilter past the end of the input");
       return nullptr;
     }
 
     if (mOutputRow >= mNext.InputSize().height) {
       NS_WARNING("Advancing DownscalingFilter past the end of the output");
@@ -255,28 +267,16 @@ public:
     }
 
     mInputRow++;
 
     return mInputRow < mInputSize.height ? GetRowPointer()
                                          : nullptr;
   }
 
-protected:
-  uint8_t* DoResetToFirstRow() override
-  {
-    mNext.ResetToFirstRow();
-
-    mInputRow = 0;
-    mOutputRow = 0;
-    mRowsInWindow = 0;
-
-    return GetRowPointer();
-  }
-
 private:
   uint8_t* GetRowPointer() const { return mRowBuffer.get(); }
 
   static uint32_t PaddedWidthInBytes(uint32_t aLogicalWidth)
   {
     // Convert from width in BGRA/BGRX pixels to width in bytes, padding by 15
     // to handle overreads by the SIMD code inside Skia.
     return aLogicalWidth * sizeof(uint32_t) + 15;
--- a/image/SurfaceFilters.h
+++ b/image/SurfaceFilters.h
@@ -114,17 +114,27 @@ public:
     return sizeof(PixelType) == 1 && mNext.IsValidPalettedPipe();
   }
 
   Maybe<SurfaceInvalidRect> TakeInvalidRect() override
   {
     return mNext.TakeInvalidRect();
   }
 
-  uint8_t* AdvanceRow() override
+protected:
+  uint8_t* DoResetToFirstRow() override
+  {
+    mNext.ResetToFirstRow();
+    mPass = 0;
+    mInputRow = 0;
+    mOutputRow = InterlaceOffset(mPass);;
+    return GetRowPointer(mOutputRow);
+  }
+
+  uint8_t* DoAdvanceRow() override
   {
     if (mPass >= 4) {
       return nullptr;  // We already finished all passes.
     }
     if (mInputRow >= InputSize().height) {
       return nullptr;  // We already got all the input rows we expect.
     }
 
@@ -206,26 +216,16 @@ public:
     mOutputRow = nextOutputRow;
 
     // We'll actually write to the first Haeberli output row, then copy it until
     // we reach the last Haeberli output row. The assertions above make sure
     // this always includes mOutputRow.
     return GetRowPointer(nextHaeberliOutputRow);
   }
 
-protected:
-  uint8_t* DoResetToFirstRow() override
-  {
-    mNext.ResetToFirstRow();
-    mPass = 0;
-    mInputRow = 0;
-    mOutputRow = InterlaceOffset(mPass);;
-    return GetRowPointer(mOutputRow);
-  }
-
 private:
   static uint32_t InterlaceOffset(uint32_t aPass)
   {
     MOZ_ASSERT(aPass < 4, "Invalid pass");
     static const uint8_t offset[] = { 0, 4, 2, 1 };
     return offset[aPass];
   }
 
@@ -408,17 +408,63 @@ public:
     return NS_OK;
   }
 
   Maybe<SurfaceInvalidRect> TakeInvalidRect() override
   {
     return mNext.TakeInvalidRect();
   }
 
-  uint8_t* AdvanceRow() override
+protected:
+  uint8_t* DoResetToFirstRow() override
+  {
+    uint8_t* rowPtr = mNext.ResetToFirstRow();
+    if (rowPtr == nullptr) {
+      mRow = InputSize().height;
+      return nullptr;
+    }
+
+    mRow = mUnclampedFrameRect.y;
+
+    // Advance the next pipeline stage to the beginning of the frame rect,
+    // outputting blank rows.
+    if (mFrameRect.y > 0) {
+      int32_t rowsToWrite = mFrameRect.y;
+      mNext.template WriteRows<uint32_t>([&](uint32_t* aRow, uint32_t aLength)
+                                           -> Maybe<WriteState> {
+        memset(aRow, 0, aLength * sizeof(uint32_t));
+        rowsToWrite--;
+        return rowsToWrite > 0 ? Nothing()
+                               : Some(WriteState::NEED_MORE_DATA);
+      });
+    }
+
+    // We're at the beginning of the frame rect now, so return if we're either
+    // ready for input or we're already done.
+    rowPtr = mBuffer ? mBuffer.get() : mNext.CurrentRowPointer();
+    if (!mFrameRect.IsEmpty() || rowPtr == nullptr) {
+      // Note that the pointer we're returning is for the next row we're
+      // actually going to write to, but we may discard writes before that point
+      // if mRow < mFrameRect.y.
+      return AdjustRowPointer(rowPtr);
+    }
+
+    // We've finished the region specified by the frame rect, but the frame rect
+    // is empty, so we need to output the rest of the image immediately. Advance
+    // to the end of the next pipeline stage's buffer, outputting blank rows.
+    mNext.template WriteRows<uint32_t>([](uint32_t* aRow, uint32_t aLength) {
+      memset(aRow, 0, aLength * sizeof(uint32_t));
+      return Nothing();
+    });
+
+    mRow = InputSize().height;
+    return nullptr;  // We're done.
+  }
+
+  uint8_t* DoAdvanceRow() override
   {
     uint8_t* rowPtr = nullptr;
 
     const int32_t currentRow = mRow;
     mRow++;
 
     if (currentRow < mFrameRect.y) {
       // This row is outside of the frame rect, so just drop it on the floor.
@@ -472,64 +518,16 @@ public:
     mNext.template WriteRows<uint32_t>([&](uint32_t* aRow, uint32_t aLength) {
       memset(rowPtr, 0, aLength * sizeof(uint32_t));
       return Nothing();
     });
 
     return nullptr;  // We're done.
   }
 
-protected:
-  uint8_t* DoResetToFirstRow() override
-  {
-    uint8_t* rowPtr = mNext.ResetToFirstRow();
-    if (rowPtr == nullptr) {
-      mRow = InputSize().height;
-      return nullptr;
-    }
-
-    mRow = mUnclampedFrameRect.y;
-
-    // Advance the next pipeline stage to the beginning of the frame rect,
-    // outputting blank rows.
-    if (mFrameRect.y > 0) {
-      int32_t rowsToWrite = mFrameRect.y;
-      mNext.template WriteRows<uint32_t>([&](uint32_t* aRow, uint32_t aLength)
-                                           -> Maybe<WriteState> {
-        memset(aRow, 0, aLength * sizeof(uint32_t));
-        rowsToWrite--;
-        return rowsToWrite > 0 ? Nothing()
-                               : Some(WriteState::NEED_MORE_DATA);
-      });
-    }
-
-    // We're at the beginning of the frame rect now, so return if we're either
-    // ready for input or we're already done.
-    rowPtr = mBuffer ? mBuffer.get() : mNext.CurrentRowPointer();
-    if (!mFrameRect.IsEmpty() || rowPtr == nullptr) {
-      // Note that the pointer we're returning is for the next row we're
-      // actually going to write to, but we may discard writes before that point
-      // if mRow < mFrameRect.y.
-      return AdjustRowPointer(rowPtr);
-    }
-
-    // We've finished the region specified by the frame rect, but the frame rect
-    // is empty, so we need to output the rest of the image immediately. Advance
-    // to the end of the next pipeline stage's buffer, outputting blank rows.
-    int32_t rowsWritten = 0;
-    mNext.template WriteRows<uint32_t>([&](uint32_t* aRow, uint32_t aLength) {
-      rowsWritten++;
-      memset(aRow, 0, aLength * sizeof(uint32_t));
-      return Nothing();
-    });
-
-    mRow = InputSize().height;
-    return nullptr;  // We're done.
-  }
-
 private:
   uint8_t* AdjustRowPointer(uint8_t* aNextRowPointer) const
   {
     if (mBuffer) {
       MOZ_ASSERT(aNextRowPointer == mBuffer.get());
       return aNextRowPointer;  // No adjustment needed for an intermediate buffer.
     }
 
--- a/image/SurfacePipe.cpp
+++ b/image/SurfacePipe.cpp
@@ -65,17 +65,17 @@ AbstractSurfaceSink::TakeInvalidRect()
 uint8_t*
 AbstractSurfaceSink::DoResetToFirstRow()
 {
   mRow = 0;
   return GetRowPointer();
 }
 
 uint8_t*
-AbstractSurfaceSink::AdvanceRow()
+AbstractSurfaceSink::DoAdvanceRow()
 {
   if (mRow >= uint32_t(InputSize().height)) {
     return nullptr;
   }
 
   // If we're vertically flipping the output, we need to flip the invalid rect. Since we're
   // dealing with an axis-aligned rect, only the y coordinate needs to change.
   int32_t invalidY = mFlipVertically
--- a/image/SurfacePipe.h
+++ b/image/SurfacePipe.h
@@ -114,16 +114,30 @@ public:
    */
   uint8_t* ResetToFirstRow()
   {
     mCol = 0;
     mRowPointer = DoResetToFirstRow();
     return mRowPointer;
   }
 
+  /**
+   * Called by WritePixels() and WriteRows() to advance this filter to the next
+   * row.
+   *
+   * @return a pointer to the buffer for the next row, or nullptr to indicate
+   *         that we've finished the entire surface.
+   */
+  uint8_t* AdvanceRow()
+  {
+    mCol = 0;
+    mRowPointer = DoAdvanceRow();
+    return mRowPointer;
+  }
+
   /// @return a pointer to the buffer for the current row.
   uint8_t* CurrentRowPointer() const { return mRowPointer; }
 
   /// @return true if we've finished writing to the surface.
   bool IsSurfaceFinished() const { return mRowPointer == nullptr; }
 
   /// @return the input size this filter expects.
   gfx::IntSize InputSize() const { return mInputSize; }
@@ -186,19 +200,17 @@ public:
             // Note that we don't need to record this anywhere, because this
             // indicates an error in aFunc, and there's nothing wrong with our
             // machinery. The caller can recover as needed and continue writing to
             // the row.
             return WriteState::FAILURE;
         }
       }
 
-      // We've finished the row.
-      mRowPointer = AdvanceRow();
-      mCol = 0;
+      AdvanceRow();  // We've finished the row.
     }
 
     // We've finished the entire surface.
     return WriteState::FINISHED;
   }
 
   /**
    * Write rows to the surface one at a time by repeatedly calling a lambda
@@ -239,18 +251,17 @@ public:
       return WriteState::FINISHED;  // Already done.
     }
 
     while (true) {
       PixelType* rowPtr = reinterpret_cast<PixelType*>(mRowPointer);
 
       Maybe<WriteState> result = aFunc(rowPtr, mInputSize.width);
       if (result != Some(WriteState::FAILURE)) {
-        mCol = 0;
-        mRowPointer = AdvanceRow();  // We've finished the row.
+        AdvanceRow();  // We've finished the row.
       }
 
       if (IsSurfaceFinished()) {
         break;
       }
 
       if (result == Some(WriteState::FINISHED)) {
         // Make sure that IsSurfaceFinished() returns true so the caller can't
@@ -271,41 +282,40 @@ public:
   //////////////////////////////////////////////////////////////////////////////
   // Methods Subclasses Should Override
   //////////////////////////////////////////////////////////////////////////////
 
   /// @return true if this SurfaceFilter can be used with paletted surfaces.
   virtual bool IsValidPalettedPipe() const { return false; }
 
   /**
-   * Called by WritePixels() and WriteRows() to advance this filter to the next
-   * row.
-   *
-   * @return a pointer to the buffer for the next row, or nullptr to indicate
-   *         that we've finished the entire surface.
-   */
-  virtual uint8_t* AdvanceRow() = 0;
-
-  /**
    * @return a SurfaceInvalidRect representing the region of the surface that
    *         has been written to since the last time TakeInvalidRect() was
    *         called, or Nothing() if the region is empty (i.e. nothing has been
    *         written).
    */
   virtual Maybe<SurfaceInvalidRect> TakeInvalidRect() = 0;
 
 protected:
 
   /**
    * Called by ResetToFirstRow() to actually perform the reset. It's legal to
    * throw away any previously written data at this point, as all rows must be
    * written to on every pass.
    */
   virtual uint8_t* DoResetToFirstRow() = 0;
 
+  /**
+   * Called by AdvanceRow() to actually advance this filter to the next row.
+   *
+   * @return a pointer to the buffer for the next row, or nullptr to indicate
+   *         that we've finished the entire surface.
+   */
+  virtual uint8_t* DoAdvanceRow() = 0;
+
 
   //////////////////////////////////////////////////////////////////////////////
   // Methods For Internal Use By Subclasses
   //////////////////////////////////////////////////////////////////////////////
 
   /**
    * Called by subclasses' Configure() methods to initialize the configuration
    * of this filter. After the filter is configured, calls ResetToFirstRow().
@@ -355,20 +365,20 @@ public:
   /// Returns the singleton instance of NullSurfaceSink.
   static NullSurfaceSink* Singleton();
 
   virtual ~NullSurfaceSink() { }
 
   nsresult Configure(const NullSurfaceConfig& aConfig);
 
   Maybe<SurfaceInvalidRect> TakeInvalidRect() override { return Nothing(); }
-  uint8_t* AdvanceRow() override { return nullptr; }
 
 protected:
   uint8_t* DoResetToFirstRow() override { return nullptr; }
+  uint8_t* DoAdvanceRow() override { return nullptr; }
 
 private:
   static UniquePtr<NullSurfaceSink> sSingleton;  /// The singleton instance.
 };
 
 
 /**
  * SurfacePipe is the public API that decoders should use to interact with a
@@ -473,20 +483,20 @@ public:
   AbstractSurfaceSink()
     : mImageData(nullptr)
     , mImageDataLength(0)
     , mRow(0)
     , mFlipVertically(false)
   { }
 
   Maybe<SurfaceInvalidRect> TakeInvalidRect() override final;
-  uint8_t* AdvanceRow() override final;
 
 protected:
   uint8_t* DoResetToFirstRow() override final;
+  uint8_t* DoAdvanceRow() override final;
   virtual uint8_t* GetRowPointer() const = 0;
 
   gfx::IntRect mInvalidRect;  /// The region of the surface that has been written
                               /// to since the last call to TakeInvalidRect().
   uint8_t*  mImageData;       /// A pointer to the beginning of the surface data.
   uint32_t  mImageDataLength; /// The length of the surface data.
   uint32_t  mRow;             /// The row to which we're writing. (0-indexed)
   bool      mFlipVertically;  /// If true, write the rows from top to bottom.