Bug 1262338 (Part 1) - Ensure that SurfaceFilter always knows when AdvanceRow() is called. r=njn
authorSeth Fowler <mark.seth.fowler@gmail.com>
Wed, 25 May 2016 22:48:29 -0700
changeset 340153 f5442ff12b331819dbcc1121053e68231763cb6c
parent 340152 fe87fe3e36a93ef95ba922fcfaea60736f992e4b
child 340154 bea7a677b4191bb1bc5b017fdd73924c77f8b5ba
push id1183
push userraliiev@mozilla.com
push dateMon, 05 Sep 2016 20:01:49 +0000
treeherdermozilla-release@3148731bed45 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnjn
bugs1262338
milestone49.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 1262338 (Part 1) - Ensure that SurfaceFilter always knows when AdvanceRow() is called. r=njn
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.