1246851 - Add SurfacePipe, a safer and more maintainable API for writing image decoder output to surfaces

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

As frequent contributors to ImageLib know, we've had some serious issues lately with security bugs that originate from the way that image decoders write their output to surfaces. Decoders perform a variety of transformations when writing their output, including downscaling, deinterlacing, and color space conversion. Each decoder implementations these transformations separately, because with the current architecture this code cannot be shared, and because these transformations are complicated and can interact in various ways, we frequently introduce new bugs when maintaining them or when introducing new features. Because these transformations are interleaved with unrelated decoder-specific code, there is also no way to unit test them, so it's difficult to be sure that they behave reliably in all situations.

In this bug we'll change all that by introducing a new API for writing decoder output to surfaces. The new approach will use a pipeline of filters with a sink at the end. Each filter will implement one of the transformation mentioned above in isolation. Decoders will construct an appropriate filter pipeline and then simply write to it; the pipeline will do the rest. This design has the following advantages:

(1) Complicated code with many interdependencies and interactions is reduced to a series of simpler objects that are independent from one another.

(2) Each filter and sink can be tested and debugged independently, making it easier to verify correct behavior and to quickly isolate faults.

(3) All image decoders can share the same implementation of these filters.

(4) Each filter can be fuzzed independently.

(5) Buffer overflows will become more or less impossible to create in the decoders, since the pipeline will be in charge of when and where writes to memory take place.

(6) The pipeline design allows for the introduction of new transformations quickly and easily, or for reworking of transformations to use e.g. the GPU without lots of redundant work in each decoder.

In this bug we'll implement this design by creating SurfacePipe, SurfaceFilter, and SurfaceSink classes that interact in the obvious way. By constructing the pipelines using templates we are able to avoid any performance penalty. (I also experimented with an approach based on virtual methods that unfortunately caused some slowdown when compared to the existing code.) I've also included a massive new unit test suite for all of the new classes to ensure that we can be confident that they function correctly.

We'll eventually want more SurfaceFilters than the ones that are implemented in this bug; my initial goal is to convert the GIF decoder to use SurfacePipe, so what's implemented here is precisely what we need for that.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Add a new SurfacePipe API for writing to image surfaces in a safe and composable manner.

This patch introduces the basic types for the new API:

(1) SurfaceFilter: Each pipeline stage is an implementation of this class. The
most important methods are WritePixels() and WriteRows(), both of which take a
'generator function' (generally a lambda) that yields either pixels or rows
respectively until it can no longer continue or the surface is completely
written to. These methods are non-virtual for performance reasons. Subclasses
implement methods that advance to the new row, reset for the next progressive
pass, and the like. More details are given in the comments.

(2) SurfaceSink / PalettedSurfaceSink / NullSurfaceSink: Sinks are special
pipeline stages which must come at the end of the pipeline. (This is enforced by
the type system.) They're responsible for actually allocating surfaces and
writing to them after the preceding filters in the chain have performance
whatever transformations they need to perform. SurfaceSink is used for BGRA/BGRX
surfaces, while PalettedSurfaceSink is used for paletted surfaces.
NullSurfaceSink is a special sink which simulates a zero-size surface - i.e., it
is immediately finished, and does not accept any writes. It's used in the
implementation of SurfacePipe to ensure that a SurfacePipe is always valid, even
if it has not been given a SurfaceFilter chain.

(3) SurfacePipe: This is a wrapper for a SurfaceFilter chain, providing a single
type for client code to interact with. It exposes only methods that are
appropriate for calling from client code (i.e., an image decoder). In practice
SurfacePipe forwards everything to the SurfaceFilter chain it contains. The only
interesting thing is does internally is manage a NullSurfaceSink singleton (see
above); the singleton pattern is used to ensure that creating a SurfacePipe with
no filter chain does not require dynamic memory allocation, which would be
wasteful since client code will in practice always create an empty SurfacePipe
by default until it knows what kind of filter pipeline to create.

One thing that's important to note about this patch and the subsequent ones is
that a significant amount of complexity comes from support for paletted
surfaces. Paletted surfaces are *only* necessary to support the current approach
to GIF and PNG animation; they're useful to save memory because we decode every
frame in an animated image and store it in memory until the image is no longer
referenced by anything. The plan is to migrate GIF and PNG animation to the
media framework in the near future; this new implementation will use a streaming
decoding approach that will make using paletted surfaces for frames not only
non-useful but actively harmful, and we can remove everything related to
paletted surfaces from ImageLib at that time. So keep in mind: this complexity
is a temporary thing, and it'll get removed.

Comment 2

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add SurfaceFilter implementations for basic surface output operations.

This patch implements the SurfaceFilters we need for the GIF decoder.

They include:

(1) DownscalingFilter, a replacement for Downscaler that works as a SurfaceFilter.

(2) DeinterlacingFilter, a deinterlacer SurfaceFilter. It's currently focused on
the GIF deinterlacing algorithm, but it could be made to support other patterns
as well.

(3) RemoveFrameRectFilter, a SurfaceFilter that turns an image with a frame rect
into a simple image with transparency in the area outside the frame rect. This
SurfaceFilter allows other SurfaceFilters to ignore the existence of a frame
rect totally.

With RemoveFrameRectFilter, we never have frame rects on surfaces any more
unless they're an animation frame. This actually triggers a bug in
platform-specific code we use for tiling on OS X, so I've marked a test fuzzy on
that platform. The bug does not produce visible issues; the fuzz is very slight.

Comment 3

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Add a factory for constructing SurfacePipes.

This patch adds a factory for constructing SurfacePipes. Some template magic is
used to keep the amount of code required minimal.

Comment 4

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 4) - Add a test suite for SurfacePipes and SurfaceFilters.

This patch adds the test suite. It's pretty massive, unfortunately (well,
unfortunately from a review perspective). Hopefully things are relatively clear
but let me know if you need any explanation.

Comment 5

[Nicholas Nethercote [inactive]]

Comment on attachment 8717294 [details] [diff] [review]
(Part 1) - Add a new SurfacePipe API for writing to image surfaces in a safe and composable manner.

Review of attachment 8717294 [details] [diff] [review]:
-----------------------------------------------------------------

First of all, I love the overall idea of this bug and all the stuff you've described in comment 0.

Also, the code in this patch is really clean and readable, with good style and lots of nice comments, which is excellent.

There is a *lot* of machinery here. At this point I've only read the first patch and it all seems plausible, though I don't yet feel like I have a clear picture of how all the pieces fit together at a lower-level. Also I can't help wondering if it's possible to reduce the amount of abstraction and machinery :)

Many of my comments are nits. The remainder are mostly questions or explanations of things that I found unclear, and so might indicate where extra comments would help.

::: image/SurfacePipe.cpp
@@ +1,5 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
> + *
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

The correct modeline/license header is shown here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Mode_Line. Please use it for all the new files, in this patch and the subsequent patch.

@@ +38,5 @@
> +
> +nsresult
> +NullSurfaceSink::Configure(const NullSurfaceConfig& aConfig)
> +{
> +  ConfigureFilter(nullptr, 0, sizeof(uint32_t));

Could this also be uint8_t? I'm guessing the choice is arbitrary... if that's the case, a comment saying so would be nice.

@@ +129,5 @@
> +  uint32_t row = mFlipVertically
> +               ? mSurfaceSize.height - (mRow + 1)
> +               : mRow;
> +
> +  uint8_t* rowPtr = mImageData + row * mSurfaceSize.width * sizeof(uint32_t);

I don't understand why the pixel size is hardwired as uint32_t in SurfaceSink but hardwired as uint8_t in PalettedSurfaceSink. I'm probably missing something, but it feels like both those classes are generic enough that hardwiring pixel size isn't appropriate.

::: image/SurfacePipe.h
@@ +15,5 @@
> + *
> + * Writing to the SurfacePipe is done using lambdas that act as generator
> + * functions. Because the SurfacePipe machinery controls where the writes take
> + * place, a bug in an image decoder cannot cause a buffer overflow of the
> + * underlying surface. In particular, using WritePixels() a buffer overflow is

"when using" or "while using" reads better.

@@ +16,5 @@
> + * Writing to the SurfacePipe is done using lambdas that act as generator
> + * functions. Because the SurfacePipe machinery controls where the writes take
> + * place, a bug in an image decoder cannot cause a buffer overflow of the
> + * underlying surface. In particular, using WritePixels() a buffer overflow is
> + * totally impossible as long as the SurfacePipe code is correct.

Just "impossible" is enough :)

@@ +27,5 @@
> +
> +#include <mozilla/Maybe.h>
> +#include <mozilla/Move.h>
> +#include <mozilla/UniquePtr.h>
> +#include <mozilla/Variant.h>

These #includes should all use "" instead of <>.

@@ +52,5 @@
> +enum class WriteState : uint8_t
> +{
> +  NEED_MORE_DATA,  /// The lambda ran out of data.
> +
> +  FINISHED,        /// The lamda is done writing to the surface; future writes

s/lamda/lambda/

@@ +64,5 @@
> +
> +/**
> + * A typedef used to make the return value of WritePixels() lambdas (which may
> + * return either a pixel value or a WriteState) easier to specify. WriteRows()
> + * doesn't need such a typedef since WriteRows lambdas don't return a pixel value.

s/WriteRows/WriteRows()/, for consistency.

@@ +67,5 @@
> + * return either a pixel value or a WriteState) easier to specify. WriteRows()
> + * doesn't need such a typedef since WriteRows lambdas don't return a pixel value.
> + */
> +template <typename PixelType>
> +using NextPixel = Variant<PixelType, WriteState>;

I haven't seen this construct before! AFAICT it's called an "alias template", so use that instead of "typedef" in the comment above?

@@ +108,5 @@
> +   * to on every pass.
> +   *
> +   * @return a pointer to the buffer for the first row.
> +   */
> +  uint8_t* StartNextProgressivePass()

Is this the best name for this function? Would something like ResetToFirstRow() be more straightforward? (Ditto for DoStartNextProgressivePass()). Not sure, I'll let you decide.

@@ +119,5 @@
> +  /// @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 FinishedSurface() const { return mRowPointer == nullptr; }

Rename as HaveFinishedSurface(), perhaps? (I have a preference for bool funcs of the for IsX(), HasX(), HaveX(), etc.)

@@ +128,5 @@
> +   * been written to (i.e., FinishedSurface() returns true) or the lambda
> +   * returns a WriteState which WritePixels() will return to the caller.
> +   *
> +   * The template parameter PixelType must be uint8_t or uint32_t and must be in
> +   * agreement with the pixel size passed to ConfigureFilter().

"in agreement" -- I think this means the size matches, rather than the type matches? But it could be made clearer. (Nb: this same sentence appears three(?) more times in this file; please change all accordingly.)

@@ +136,5 @@
> +   *              return a NextPixel<PixelType> value.
> +   *
> +   * @return A WriteState value indicating the state that the lambda generator
> +   *         is in. WritePixels() itself will return WriteState::FINISHED if
> +   *         writing has finished, regardless of the lambda's internal state.

s/state that the lambda generator is in/lambda generator's state/ ?

@@ +149,5 @@
> +    while (!FinishedSurface()) {
> +      PixelType* rowPtr = reinterpret_cast<PixelType*>(mRowPointer);
> +
> +      for (; mCol < mRowWidth; ++mCol) {
> +        NextPixel<PixelType> result = aFunc();

Doing a function call per pixel seems heavyweight, but you said that performance is equivalent to what we already have, so I guess it's reasonable. (Actually, at this point in the review I don't have a clear idea why we need both WritePixels() and WriteRows(). I guess if WriteRows() is normally used then there will be many fewer function calls. Perhaps the comments above could explain why both are needed.)

@@ +168,5 @@
> +            return WriteState::FINISHED;
> +
> +          case WriteState::ERROR:
> +            // Note that we don't need to record this anywhere, because this
> +            // indicates an error in aFunc, but there's nothing wrong with our

s/but/and/

@@ +188,5 @@
> +  /**
> +   * Write rows to the surface one at a time by repeatedly calling a lambda
> +   * that yields rows. Writing continues until every row in the surface has
> +   * been written to (i.e., FinishedSurface() returns true) or the lambda
> +   * returns a WriteState which WritePixels() will return to the caller.

s/WritePixels()/WriteRows()/

@@ +200,5 @@
> +   *              indicates a WriteState to return to the caller, while
> +   *              Nothing() indicates that the lambda can generate more rows.
> +   *
> +   * @return A WriteState value indicating the state that the lambda generator
> +   *         is in. WriteRows() itself will return WriteState::FINISHED if

s/state that the lambda generator is in/lambda generator's state/ ?

@@ +217,5 @@
> +
> +    while (true) {
> +      PixelType* rowPtr = reinterpret_cast<PixelType*>(mRowPointer);
> +
> +      Maybe<WriteState> result = aFunc(rowPtr, mRowWidth);

At this point in the review I'm wondering what stops a buggy aFunc from overwriting memory it shouldn't.

@@ +276,5 @@
> +   */
> +  virtual Maybe<SurfaceInvalidRect> TakeInvalidRect() = 0;
> +
> +
> +protected:

No need for the two blank lines.

@@ +286,5 @@
> +   */
> +  virtual uint8_t* DoStartNextProgressivePass() = 0;
> +
> +
> +  //////////////////////////////////////////////////////////////////////////////

Ditto.

@@ +290,5 @@
> +  //////////////////////////////////////////////////////////////////////////////
> +  // Methods For Internal Use By Subclasses
> +  //////////////////////////////////////////////////////////////////////////////
> +
> +protected:

Remove this redundant "protected:" ?

@@ +314,5 @@
> +
> +private:
> +  uint8_t* mRowPointer;  /// Pointer to the current row or null if finished.
> +  uint32_t mRowWidth;    /// The width of each row.
> +  uint32_t mCol;         /// The current column we're writing to.

Is this 0-indexed or 1-indexed? Please say which it is in the comment.

@@ +329,5 @@
> +
> +/**
> + * NullSurfaceSink is a trivial SurfaceFilter implementation that behaves as if
> + * it were a zero-size SurfaceSink. It's used as the default filter chain for an
> + * unintialized SurfacePipe.

s/unintialized/uninitialized/

@@ +331,5 @@
> + * NullSurfaceSink is a trivial SurfaceFilter implementation that behaves as if
> + * it were a zero-size SurfaceSink. It's used as the default filter chain for an
> + * unintialized SurfacePipe.
> + *
> + * To avoid unnecessary allocate when creating SurfacePipe objects,

s/allocate/allocations/

@@ +374,5 @@
> +  { }
> +
> +  SurfacePipe(SurfacePipe&& aOther)
> +    : mHead(Move(aOther.mHead))
> +  { }

Is it worth deleting the non-moving copy constructor and assignment operator?

@@ +405,5 @@
> +   * Write pixels to the surface one at a time by repeatedly calling a lambda
> +   * that yields pixels. Writing continues until every pixel in the surface has
> +   * been written to (i.e., FinishedSurface() returns true) or the lambda
> +   * returns a WriteState which WritePixels() will return to the caller.
> +   *

I think this comment is identical to the one in SurfaceFilter. Maybe just cite that one here rather than repeating? You know they're going to get out of sync eventually...

@@ +427,5 @@
> +  /**
> +   * Write rows to the surface one at a time by repeatedly calling a lambda
> +   * that yields rows. Writing continues until every row in the surface has
> +   * been written to (i.e., FinishedSurface() returns true) or the lambda
> +   * returns a WriteState which WritePixels() will return to the caller.

Ditto about the repeated comment. (And again, s/WritePixels()/WriteRows()/.)

@@ +455,5 @@
> +  /**
> +   * @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).

Another duplicated comment.

@@ +499,5 @@
> +                              /// to since the last call to TakeInvalidRect().
> +  gfx::IntSize mSurfaceSize;  /// The size of the surface we allocated.
> +  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.

Please say if this is 0-indexed or 1-indexed in the comment.

@@ +511,5 @@
> +{
> +  using Filter = SurfaceSink;
> +  Decoder* mDecoder;     /// Which Decoder to use to allocate the surface.
> +  uint32_t mFrameNum;    /// Which frame of animation this surface is for.
> +  const gfx::IntSize& mTargetSize;  /// The size of the surface. 

Trailing whitespace.

@@ +543,5 @@
> +struct PalettedSurfaceConfig
> +{
> +  using Filter = PalettedSurfaceSink;
> +  Decoder* mDecoder;           /// Which Decoder to use to allocate the surface.
> +  uint32_t mFrameNum;          /// Which frame of animation this surface is for.

This struct has a lot of overlap with SurfaceConfig. Is it possible/worth making it inherit from SurfaceConfig?

@@ +544,5 @@
> +{
> +  using Filter = PalettedSurfaceSink;
> +  Decoder* mDecoder;           /// Which Decoder to use to allocate the surface.
> +  uint32_t mFrameNum;          /// Which frame of animation this surface is for.
> +  const gfx::IntSize& mTargetSize;  /// The logical size of the surface. 

Trailing whitespace.

@@ +576,5 @@
> +
> +  gfx::IntRect mFrameRect;  /// The surface subrect which contains data. Note
> +                            /// that the surface size we actually allocate is the
> +                            /// size of the frame rect, not the logical size of
> +                            /// the surface.

If you move this comment to the lines above mFrameRect it'll avoid the hanging indent.

Comment 6

[Seth Fowler [:seth] [:s2h]]

Thanks for the super-detailed review of part 1, Nick! Addressing your comments now.

Comment 7

[Seth Fowler [:seth] [:s2h]]

(In reply to Nicholas Nethercote [:njn] from comment #5)
> @@ +129,5 @@
> > +  uint32_t row = mFlipVertically
> > +               ? mSurfaceSize.height - (mRow + 1)
> > +               : mRow;
> > +
> > +  uint8_t* rowPtr = mImageData + row * mSurfaceSize.width * sizeof(uint32_t);
> 
> I don't understand why the pixel size is hardwired as uint32_t in
> SurfaceSink but hardwired as uint8_t in PalettedSurfaceSink. I'm probably
> missing something, but it feels like both those classes are generic enough
> that hardwiring pixel size isn't appropriate.

So this is actually the essential difference between SurfaceSink and PalettedSurfaceSink, other than the different configuration arguments. A pixel size of uint32_t implies BGRA/BGRX, while a pixel size of uint8_t implies a paletted surface. I'll update the comments for WritePixels() and WriteRows() to explain this better.

Comment 8

[Seth Fowler [:seth] [:s2h]]

(In reply to Nicholas Nethercote [:njn] from comment #5)
> @@ +108,5 @@
> > +   * to on every pass.
> > +   *
> > +   * @return a pointer to the buffer for the first row.
> > +   */
> > +  uint8_t* StartNextProgressivePass()
> 
> Is this the best name for this function? Would something like
> ResetToFirstRow() be more straightforward? (Ditto for
> DoStartNextProgressivePass()). Not sure, I'll let you decide.

Yeah, I'm not sure either, but ResetToFirstRow() does have the advantage of being more terse, so we may as well go with that unless someone has a better idea.

Comment 9

[Nicholas Nethercote [inactive]]

Comment on attachment 8717295 [details] [diff] [review]
(Part 2) - Add SurfaceFilter implementations for basic surface output operations.

Review of attachment 8717295 [details] [diff] [review]:
-----------------------------------------------------------------

r=me but I don't feel like I've done a great review here. (E.g. I've never heard the names Lanczos and Haeberli before now.) It may be worth getting a second pair of eyes (tnikkel?) on this one.

::: image/DownscalingFilter.h
@@ +7,5 @@
> +/**
> + * DownscalingSurfaceFilter is a SurfaceFilter implementation for use with
> + * SurfacePipe which performs Lanczos downscaling.
> + *
> + * It's separated from the other SurfaceFilters in this header file because some

This sentence is easy to misparse -- it sounds like there are other SurfaceFilters in this header file. I think you want something like "It's in its own header file, separate from the other SurfaceFilters, because..."

@@ +22,5 @@
> +#include <string.h>
> +
> +#include <mozilla/Maybe.h>
> +#include <mozilla/UniquePtr.h>
> +#include <mozilla/SSE.h>

Use "" instead of <>.

Also, put SSE.h before UniquePtr.h, for alphabetical ordering.

@@ +50,5 @@
> +struct DownscalingConfig
> +{
> +  template <typename Next> using Filter = DownscalingFilter<Next>;
> +  const gfx::IntSize& mOriginalSize;  /// The original size of the image.
> +  const gfx::IntSize& mTargetSize;    /// The size to downscale to.

Is there any significance to using references instead of straight copies? I wonder if the potential for puzzlement outweighs the saving from not having to copy what is a very small type. (I see now this is relevant for part 1, too.)

@@ +164,5 @@
> +
> +    // Allocate the buffer, which contains scanlines of the original image.
> +    // pad by 15 to handle overreads by the simd code
> +    mRowBuffer.reset(new (fallible) uint8_t[mOriginalSize.width *
> +                                            sizeof(uint32_t) + 15]);

This comment "pad by 15..." and computation occurs multiple times. Please pull it out into a helper function. Also, please capitalize "pad" and add a period to the end of the sentence.

@@ +169,5 @@
> +    if (MOZ_UNLIKELY(!mRowBuffer)) {
> +      return NS_ERROR_OUT_OF_MEMORY;
> +    }
> +    //
> +    // Clear the buffer to avoid writing uninitialized memory to the output.

Replace the "//" line with a blank line?

@@ +192,5 @@
> +
> +    if (MOZ_UNLIKELY(anyAllocationFailed)) {
> +      // We intentionally iterate through the entire array even if an allocation
> +      // fails, to ensure that all the pointers in it are either valid or nullptr.
> +      // That in turn ensures that ReleaseWindow() can clean up correctly.

This is a good comment, but it's in a strange spot. I'd put it just above the loop above.

@@ +223,5 @@
> +  {
> +    if (mInputRow >= mOriginalSize.height) {
> +      NS_WARNING("Advancing DownscalingFilter past the end of the input");
> +      return nullptr;
> +    }

It's not clear to me why this (and the one immediately below) is an NS_WARNING instead of a fatal assertion. Under what circumstance could it happen?

@@ +352,5 @@
> +  Next mNext;                       /// The next SurfaceFilter in the chain.
> +
> +  gfx::IntSize mOriginalSize;       /// The original size of the image.
> +  gfx::IntSize mTargetSize;         /// The size to downscale to.
> +  gfxSize mScale;                   /// The scale factors in each dimension.

It would be nice to mention that mScale is a function of mOriginalSize and mTargetSize.

@@ +364,5 @@
> +  int32_t mWindowCapacity;  /// How many rows the window contains.
> +
> +  int32_t mRowsInWindow;    /// How many rows we've buffered in the window.
> +  int32_t mInputRow;        /// The current row we're reading.
> +  int32_t mOutputRow;       /// The current row we're writing.

Are both of these 0-indexed?

::: image/SurfaceFilters.h
@@ +16,5 @@
> +#include <string.h>
> +
> +#include <mozilla/Likely.h>
> +#include <mozilla/Maybe.h>
> +#include <mozilla/UniquePtr.h>

Use "" instead of <>.

@@ +98,5 @@
> +
> +    // Clear the buffer to avoid writing uninitialized memory to the output.
> +    memset(mBuffer.get(), 0, mOriginalSize.width *
> +                             mOriginalSize.height *
> +                             sizeof(PixelType));

Factor out the size computation into a local variable, to avoid repetition?

@@ +319,5 @@
> +  }
> +
> +  Next mNext;                    /// The next SurfaceFilter in the chain.
> +
> +  gfx::IntSize mOriginalSize;    /// The original image size, before any filters.

Is it guaranteed to the the original image size? What if there's a downscaling filter earlier in the pipeline? (The same question applies to the Config and also  to RemoveFrameRectFilter and its Config.)

@@ +381,5 @@
> +    }
> +
> +    // Clamp mFrameRect to the original size of the image.
> +    gfx::IntRect originalRect(0, 0, aConfig.mOriginalSize.width,
> +                                 aConfig.mOriginalSize.height);

Pull that hanging indent back three spaces to align with the first '0'.

@@ +452,5 @@
> +        aRow += mFrameRect.x;
> +        aLength -= std::min(aLength, uint32_t(mFrameRect.x));
> +        uint32_t toWrite = std::min(aLength, uint32_t(mFrameRect.width));
> +        uint8_t* source = mBuffer.get()
> +                        - std::min(mUnclampedFrameRect.x, 0) * sizeof(uint32_t);

Put the '-' at the end of the first line, please!

Comment 10

[Nicholas Nethercote [inactive]]

I'll review parts 3 and 4 tomorrow.

Comment 11

[Seth Fowler [:seth] [:s2h]]

(In reply to Nicholas Nethercote [:njn] from comment #5)
> @@ +119,5 @@
> > +  /// @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 FinishedSurface() const { return mRowPointer == nullptr; }
> 
> Rename as HaveFinishedSurface(), perhaps? (I have a preference for bool
> funcs of the for IsX(), HasX(), HaveX(), etc.)

Renamed to IsSurfaceFinished().
 
> @@ +128,5 @@
> > +   * been written to (i.e., FinishedSurface() returns true) or the lambda
> > +   * returns a WriteState which WritePixels() will return to the caller.
> > +   *
> > +   * The template parameter PixelType must be uint8_t or uint32_t and must be in
> > +   * agreement with the pixel size passed to ConfigureFilter().
> 
> "in agreement" -- I think this means the size matches, rather than the type
> matches? But it could be made clearer. (Nb: this same sentence appears
> three(?) more times in this file; please change all accordingly.)

I think the change I made for the nit in comment 7 should take care of this too.

Comment 12

[Seth Fowler [:seth] [:s2h]]

(In reply to Nicholas Nethercote [:njn] from comment #5)
> Doing a function call per pixel seems heavyweight, but you said that
> performance is equivalent to what we already have, so I guess it's
> reasonable. (Actually, at this point in the review I don't have a clear idea
> why we need both WritePixels() and WriteRows(). I guess if WriteRows() is
> normally used then there will be many fewer function calls. Perhaps the
> comments above could explain why both are needed.)

Performance for WritePixels() especially relies on inlining; it's (perhaps obviously) only equivalent on opt builds. Inlining should be pretty reliable though given that the lambda generator functions are called in only one place. Also, there are no virtual function calls in the inner loop of WritePixels() (only AdvanceRow() is virtual); I went out of my way to ensure that to give the compiler as much information as possible.

The measurements I did that showed that this code performs as well as the old approach were actually with code that uses WritePixels(). (See bug 1247152.)

Comment 13

[Seth Fowler [:seth] [:s2h]]

(In reply to Nicholas Nethercote [:njn] from comment #5)
> (Actually, at this point in the review I don't have a clear idea
> why we need both WritePixels() and WriteRows(). I guess if WriteRows() is
> normally used then there will be many fewer function calls. Perhaps the
> comments above could explain why both are needed.)

Meant to clarify this in the preceding comment. They're just both provided because we can avoid copies in some cases using WriteRows(), and this is important for DownscalingFilter in particular. I'd actually prefer that code use WritePixels() whenever possible, because...

> @@ +217,5 @@
> > +
> > +    while (true) {
> > +      PixelType* rowPtr = reinterpret_cast<PixelType*>(mRowPointer);
> > +
> > +      Maybe<WriteState> result = aFunc(rowPtr, mRowWidth);
> 
> At this point in the review I'm wondering what stops a buggy aFunc from
> overwriting memory it shouldn't.

in the case of WriteRows(), nothing is preventing this. aFunc must respect the start/length combo given to ensure memory safety. (Though this at least should be easier to verify by inspection than the existing approach which involves pointer math over the surface's entire buffer.) That's why WritePixels() is much better; the lambda simply cannot make things go wrong, because it has no control over where the pixel it returns is written.

I'll add a comment clarifying that WritePixels() is vastly preferred.

Comment 14

[Seth Fowler [:seth] [:s2h]]

(In reply to Nicholas Nethercote [:njn] from comment #5)
> @@ +286,5 @@
> > +   */
> > +  virtual uint8_t* DoStartNextProgressivePass() = 0;
> > +
> > +
> > +  //////////////////////////////////////////////////////////////////////////////
> 
> Ditto.

If it's alright with you I'm going to leave these in, as the pattern of section titles written with a comment block like this, with two blank lines between sections, is quite widely used within ImageLib and IMO it's good. =)

Comment 15

[Nicholas Nethercote [inactive]]

> If it's alright with you I'm going to leave these in, as the pattern of
> section titles written with a comment block like this, with two blank lines
> between sections, is quite widely used within ImageLib and IMO it's good. =)

IME it's hard to maintain picky formatting like this as other people come to trample over your precious code... but I'm not concerned if you want to keep it.

Comment 16

[Seth Fowler [:seth] [:s2h]]

(In reply to Nicholas Nethercote [:njn] from comment #5)
> @@ +543,5 @@
> > +struct PalettedSurfaceConfig
> > +{
> > +  using Filter = PalettedSurfaceSink;
> > +  Decoder* mDecoder;           /// Which Decoder to use to allocate the surface.
> > +  uint32_t mFrameNum;          /// Which frame of animation this surface is for.
> 
> This struct has a lot of overlap with SurfaceConfig. Is it possible/worth
> making it inherit from SurfaceConfig?

It's probably possible, but I think I'd prefer the clarity of keeping these Config structs as simple as possible. (FWIW it'll be moot at some point in the near future anyway, as everything related to paletted surfaces is going to get removed relatively soon.)

Comment 17

[Seth Fowler [:seth] [:s2h]]

Thanks for the thorough review of part 2!

(In reply to Nicholas Nethercote [:njn] from comment #9)
> Is there any significance to using references instead of straight copies? I
> wonder if the potential for puzzlement outweighs the saving from not having
> to copy what is a very small type. (I see now this is relevant for part 1,
> too.)

I suppose not; I'll make that change.

> @@ +223,5 @@
> > +  {
> > +    if (mInputRow >= mOriginalSize.height) {
> > +      NS_WARNING("Advancing DownscalingFilter past the end of the input");
> > +      return nullptr;
> > +    }
> 
> It's not clear to me why this (and the one immediately below) is an
> NS_WARNING instead of a fatal assertion. Under what circumstance could it
> happen?

I've used NS_WARNING with a return, rather than MOZ_ASSERT, for several types of failures that have led to security bugs in the past. The motivation is to ensure that even if something does go wrong we don't do anything bad at runtime, even in release builds. We can probably turn these into fatal assertions in the future but I'm feeling a bit paranoid right now given the reset spate of security issues.

Comment 18

[Seth Fowler [:seth] [:s2h]]

(In reply to Nicholas Nethercote [:njn] from comment #9)
> @@ +319,5 @@
> > +  }
> > +
> > +  Next mNext;                    /// The next SurfaceFilter in the chain.
> > +
> > +  gfx::IntSize mOriginalSize;    /// The original image size, before any filters.
> 
> Is it guaranteed to the the original image size? What if there's a
> downscaling filter earlier in the pipeline? (The same question applies to
> the Config and also  to RemoveFrameRectFilter and its Config.)

You're right; in these patches we should s/OriginalSize/InputSize/ and s/TargetSize/OutputSize/ in most if not all cases. I'm too tired to do that tonight, but I'll do it tomorrow. (I believe I use both of these terms simultaneously in some helper functions in part 4, so that'll require a little head scratching, but I think it may be OK because original size really does mean original size there.)

I've fixed all of the other nits discussed above, so to make sure we're both in sync I'll go ahead and upload the modified patches even though I haven't made this change.

Comment 19

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Add a new SurfacePipe API for writing to image surfaces in a safe and composable manner.

Comment 20

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add SurfaceFilter implementations for basic surface output operations.

Comment 21

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Add a factory for constructing SurfacePipes.

Comment 22

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 4) - Add a test suite for SurfacePipes and SurfaceFilters.

Comment 23

[Nicholas Nethercote [inactive]]

Comment on attachment 8717798 [details] [diff] [review]
(Part 1) - Add a new SurfacePipe API for writing to image surfaces in a safe and composable manner.

Review of attachment 8717798 [details] [diff] [review]:
-----------------------------------------------------------------

Carrying over r+. I see a couple of trailing whitespaces that you should fix.

Comment 24

[Nicholas Nethercote [inactive]]

Comment on attachment 8717800 [details] [diff] [review]
(Part 3) - Add a factory for constructing SurfacePipes.

Review of attachment 8717800 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/SurfacePipeFactory.h
@@ +78,5 @@
> +   *               SurfacePipe; see the SurfacePipeFlags documentation for more
> +   *               information.
> +   *
> +   * @return A SurfacePipe if the parameters allowed one to be created
> +   * successfully, or Nothing() if the SurfacePipe could not be initialized.

Indent the second line. (And there's another one just like this lower down.)

@@ +103,5 @@
> +                          DeinterlacingConfig<uint32_t> { aFrameRect.Size(), progressiveDisplay },
> +                          RemoveFrameRectConfig { aOriginalSize, aFrameRect },
> +                          DownscalingConfig { aOriginalSize, aTargetSize, aFormat },
> +                          SurfaceConfig { aDecoder, aFrameNum, aTargetSize,
> +                                          aFormat, flipVertically });

*Lots* of repetition in this function. Things you can factor out into a local variable include:

- !aFrameRect.IsEqualEdges(...)
- The SurfaceConfig, which is used in all branches
- The DownscalingConfig, which is used in just the first half
- The RemoveFrameRectConfig and especially the DeinterlacingConfig are harder, but you can probably still do better there.

I think the pros of avoiding the repetition (less chance for typos/inconsistencies) make this worthwhile. In fact, it might be worth constructing all the configs at the top of the function. Some of them might not be used, but I imagine this function isn't hot and the improvement in readability would be large, e.g. clauses would look like this:

> return MakePipe(aFrameRect.Size(),
>                 deinterlacingConfig,
>                 removeFrameRectConfig,
>                 downscalingConfig,
>                 surfaceConfig);

@@ +120,5 @@
> +                          DownscalingConfig { aOriginalSize, aTargetSize, aFormat },
> +                          SurfaceConfig { aDecoder, aFrameNum, aTargetSize,
> +                                          aFormat, flipVertically });
> +        } else {
> +          return MakePipe(aOriginalSize,

You're violating the no-return-after-else rule from the style guide repeatedly here. I'm willing to let it slide in this case, though you could also make each clause assign to a variable |p| and then return that at the end.

@@ +195,5 @@
> +      return MakePalettedPipe(aFrameRect.Size(),
> +                              DeinterlacingConfig<uint8_t> { aFrameRect.Size(), progressiveDisplay },
> +                              PalettedSurfaceConfig { aDecoder, aFrameNum, aOriginalSize,
> +                                                      aFrameRect, aFormat, aPaletteDepth,
> +                                                      flipVertically });

I'd factor out the PalettedSurfaceConfig into a local variable.

@@ +218,5 @@
> +
> +    MOZ_ASSERT(pipe->IsValidPipeForInputSize(aInputSize));
> +    MOZ_ASSERT(!pipe->IsValidPalettedPipe());
> +
> +    return Some(SurfacePipe{Move(pipe)});

Put spaces inside the curly braces?

@@ +224,5 @@
> +
> +  template <typename... Configs>
> +  static Maybe<SurfacePipe>
> +  MakePalettedPipe(const nsIntSize& aInputSize, Configs... aConfigs)
> +  {

You said all the paletted stuff is going away, right? I wonder if it's worth adding XXX comments (perhaps with a bug number) to all the relevant places that will be deleted? That could be done in all the patches in this bug.

@@ +234,5 @@
> +
> +    MOZ_ASSERT(pipe->IsValidPipeForInputSize(aInputSize));
> +    MOZ_ASSERT(pipe->IsValidPalettedPipe());
> +
> +    return Some(SurfacePipe{Move(pipe)});

Ditto.

@@ +243,5 @@
> +
> +} // namespace image
> +} // namespace mozilla
> +
> +#endif

This line is missing the trailing "// mozilla_image_SurfacePipeFactory_h" comment.

Comment 25

[Nicholas Nethercote [inactive]]

Comment on attachment 8717801 [details] [diff] [review]
(Part 4) - Add a test suite for SurfacePipes and SurfaceFilters.

There are enough build system changes here that a build peer should double-check the moz.build changes: mshal, would you mind?

Comment 26

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Add a new SurfacePipe API for writing to image surfaces in a safe and composable manner.

These updated versions of this patch series are the same except that they
substitute the terminology "input rect" for "original rect" and "output rect"
for "target rect", as discussed above.

Comment 27

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add SurfaceFilter implementations for basic surface output operations.

Comment 28

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Add a factory for constructing SurfacePipes.

Comment 29

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 4) - Add a test suite for SurfacePipes and SurfaceFilters.

The updated version of this part additionally adds some helper functions in
Common.cpp to reduce code duplication in that file.

Comment 30

[Nicholas Nethercote [inactive]]

Comment on attachment 8718190 [details] [diff] [review]
(Part 4) - Add a test suite for SurfacePipes and SurfaceFilters.

Review of attachment 8718190 [details] [diff] [review]:
-----------------------------------------------------------------

Re-requesting mshal review of the moz.build changes.

Comment 31

[Seth Fowler [:seth] [:s2h]]

(In reply to Nicholas Nethercote [:njn] from comment #24)
> You said all the paletted stuff is going away, right? I wonder if it's worth
> adding XXX comments (perhaps with a bug number) to all the relevant places
> that will be deleted? That could be done in all the patches in this bug.

I haven't done this partially because I haven't yet taken the time to file bugs for all the preliminary steps but I'll go ahead and file an initial bug now and flesh it out later. I'll put a comment on CreatePalettedSurfacePipe() and PalettedSurfaceSink; I think that's probably enough.

Comment 32

[Seth Fowler [:seth] [:s2h]]

(In reply to Seth Fowler [:seth] [:s2h] from comment #31)
> (In reply to Nicholas Nethercote [:njn] from comment #24)
> > You said all the paletted stuff is going away, right? I wonder if it's worth
> > adding XXX comments (perhaps with a bug number) to all the relevant places
> > that will be deleted? That could be done in all the patches in this bug.
> 
> I haven't done this partially because I haven't yet taken the time to file
> bugs for all the preliminary steps but I'll go ahead and file an initial bug
> now and flesh it out later. I'll put a comment on
> CreatePalettedSurfacePipe() and PalettedSurfaceSink; I think that's probably
> enough.

Filed bug 1247520. I ended up adding comments on WritePixels() and WriteRows() too.

Comment 33

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Add a new SurfacePipe API for writing to image surfaces in a safe and composable manner.

Added comments as per comment 31 and comment 32.

Comment 34

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Add a factory for constructing SurfacePipes.

Addressed review comments. I'm not rerequesting review, but Nick, you may want
to take a quick look to see how you feel about the restructuring of the factory
functions. It's better but given C++'s feature set I think it can only be so
clean, unfortunately.

Comment 35

[Nicholas Nethercote [inactive]]

Comment on attachment 8718211 [details] [diff] [review]
(Part 3) - Add a factory for constructing SurfacePipes.

Review of attachment 8718211 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/SurfacePipeFactory.h
@@ +102,5 @@
> +    // Construct configurations for the SurfaceFilters.
> +    DeinterlacingConfig<uint32_t> deinterlacingConfig { aFrameRect.Size(),
> +                                                        progressiveDisplay };
> +    RemoveFrameRectConfig removeFrameRectConfig { aInputSize, aFrameRect };
> +    DownscalingConfig downscalingConfig { aInputSize, aOutputSize, aFormat };

This looks great! Thank you.

Is there any significance to the order you've chosen for these standard filters? Might be worth a comment explaining how you chose them. ("It's arbitrary" would be a reasonable comment, if it's true.)

Comment 36

[Nicholas Nethercote [inactive]]

Comment on attachment 8718190 [details] [diff] [review]
(Part 4) - Add a test suite for SurfacePipes and SurfaceFilters.

Review of attachment 8718190 [details] [diff] [review]:
-----------------------------------------------------------------

First of all, great job with writing such thorough tests. That's really great.

I'm giving f+ because there is *a lot* of repetition. I'd like to see at least some of it factored out before this lands. I've made a number of suggestions below. Also, a lot of the tests are doubled because there's a WritePixels() version and a WriteRows() version. Could that be avoided with templates? Some of those almost-duplicated functions are long.

Also, as usual, please remove all trailing whitespace. (It's worth setting up your text editor to highlight trailing whitespace.)

::: image/test/gtest/Common.cpp
@@ +130,5 @@
>    ASSERT_TRUE_OR_RETURN(data != nullptr, false);
>  
> +  int32_t rowLength = dataSurface->Stride();
> +  for (int32_t row = rect.y ; row < rect.YMost() ; ++row) {
> +    for (int32_t col = rect.x ; col < rect.XMost() ; ++col) {

Remove the spaces before the semi-colons.

Also, I think the old loop that did i+=4 is easier to read.

@@ +166,5 @@
> +
> +  // Check that the output rect itself is green.
> +  EXPECT_TRUE(RectIsSolidColor(surface, aRect, BGRAColor::Green(), aFuzz));
> +
> +  // Check that the area above the output rect is black.

s/black/transparent/, right?  (This is repeated three times below.)

Also, it's not clear from the comments that "above" includes "above and to the left" and "above and to the right". Likewise with "below".

@@ +212,5 @@
> +  EXPECT_EQ(inputWriteRect.width * inputWriteRect.height, count);
> +  
> +  // Assert that we're in the expected final state.
> +  EXPECT_TRUE(aFilter->IsSurfaceFinished());
> +  auto invalidRect = aFilter->TakeInvalidRect();

I wouldn't use |auto| here.

@@ +303,5 @@
> +  EXPECT_EQ(inputWriteRect.width * inputWriteRect.height, count);
> +
> +  // Assert that we're in the expected final state.
> +  EXPECT_TRUE(aFilter->IsSurfaceFinished());
> +  auto invalidRect = aFilter->TakeInvalidRect();

Ditto. (And this comment applies dozens more times.)

::: image/test/gtest/Common.h
@@ +107,5 @@
> +
> +/**
> + * @returns true if every pixel in the rect specified by @aRect is @aColor.
> + *
> + * @aRect must be totally contained with @aSurface.

What happens if it is not totally contained?

@@ +127,5 @@
> +/**
> + * Creates a pipeline of SurfaceFilters from a list of Config structs. The
> + * resulting pipeline is appropriate for use with normal (i.e., non-paletted)
> + * images. This is used during unit testing to simplify manual construction of
> + * filter pipelines.

Isn't this what SurfacePipeFactory is for?

@@ +205,5 @@
> + * Tests the result of calling WritePixels() using the provided SurfaceFilter
> + * pipeline. The pipeline must be a normal (i.e., non-paletted) pipeline.
> + *
> + * The arguments are specified in the an order intended to minimize the number
> + * of arguments that most test cases need to pass.

You have four functions in a row with long, almost identical comments. Having the latter three functions just cite this comment appropriately -- e.g. "like CheckWritePixels, but for checking WriteRows()" -- would be better, IMO.

::: image/test/gtest/TestDeinterlacingFilter.cpp
@@ +29,5 @@
> +  RefPtr<SourceBuffer> sourceBuffer = new SourceBuffer();
> +  RefPtr<Decoder> decoder =
> +    DecoderFactory::CreateAnonymousDecoder(decoderType, sourceBuffer,
> +                                           DefaultSurfaceFlags());
> +  ASSERT_TRUE(decoder != nullptr);

These eight lines are repeated many times in this patch. Please factor out into a helper function in Common.{h,cpp}.

@@ +216,5 @@
> +  WithDeinterlacingFilter(IntSize(1, 1), /* aProgressiveDisplay = */ true,
> +                          [](Decoder* aDecoder, SurfaceFilter* aFilter) {
> +    CheckWriteRows(aDecoder, aFilter,
> +                   /* aOutputRect = */ Some(IntRect(0, 0, 1, 1)),
> +                   /* aInputRect = */ Some(IntRect(0, 0, 1, 1)));

Is it worth testing the 0x0 case?

@@ +371,5 @@
> +    EXPECT_TRUE(aFilter->IsSurfaceFinished());
> +    auto invalidRect = aFilter->TakeInvalidRect();
> +    EXPECT_TRUE(invalidRect.isSome());
> +    EXPECT_EQ(IntRect(0, 0, 7, 7), invalidRect->mInputSpaceRect);
> +    EXPECT_EQ(IntRect(0, 0, 7, 7), invalidRect->mOutputSpaceRect);

This six line pattern occurs a lot across multiple files, only varying in the rectangle dimensions. Factor out?

@@ +484,5 @@
> +{
> +  uint32_t count = 0;
> +
> +  auto result = aFilter->WriteRows<uint32_t>([&](uint32_t* aRow, uint32_t aLength) {
> +    for (; aLength > 0 ; --aLength, ++aRow, ++count) {

No space before semicolon. (There are likely more of these that I haven't commented on. Please check the whole patch for them.)

::: image/test/gtest/TestDownscalingFilter.cpp
@@ +118,5 @@
> +}
> +
> +TEST(ImageDownscalingFilter, DownscalingFailsFor100_100to101_101)
> +{
> +  AssertConfiguringDownscalingFilterFails(IntSize(101, 101));

On all these AssertConfiguring*FilterFails() tests it would be useful to have a brief comment explaining why it fails, in this file and all the others.

@@ +134,5 @@
> +
> +TEST(ImageDownscalingFilter, DownscalingFailsFor100_100toMinus1_Minus1)
> +{
> +  AssertConfiguringDownscalingFilterFails(IntSize(-1, -1));
> +}

All these downscale examples (both valid and invalid) use the same scale factor for both height and width. Testing non-square scale factors seems worthwhile.

@@ +140,5 @@
> +TEST(ImageDownscalingFilter, WritePixelsOutput100_100to20_20)
> +{
> +  WithDownscalingFilter(IntSize(100, 100), IntSize(20, 20),
> +                        [](Decoder* aDecoder, SurfaceFilter* aFilter) {
> +    // Fill the image.

A brief description of the image (like you've done in the deinterlace tests) would be helpful.

@@ +167,5 @@
> +    RefPtr<SourceSurface> surface = currentFrame->GetSurface();
> +    EXPECT_TRUE(RowsAreSolidColor(surface, 0, 4, BGRAColor::Green(), /* aFuzz = */ 2));
> +    EXPECT_TRUE(RowsAreSolidColor(surface, 6, 3, BGRAColor::Red(), /* aFuzz = */ 3));
> +    EXPECT_TRUE(RowsAreSolidColor(surface, 11, 3, BGRAColor::Green(), /* aFuzz = */ 3));
> +    EXPECT_TRUE(RowsAreSolidColor(surface, 16, 4, BGRAColor::Red(), /* aFuzz = */ 3));

Why is fuzz needed even after skipping rows near the color boundaries?

@@ +227,5 @@
> +                               PalettedSurfaceConfig { decoder, 0, IntSize(20, 20),
> +                                                       IntRect(0, 0, 20, 20),
> +                                                       SurfaceFormat::B8G8R8A8, 8,
> +                                                       false });
> +  EXPECT_FALSE(bool(pipe));

A comment explaining why this fails would be useful.

::: image/test/gtest/TestDownscalingFilterNoSkia.cpp
@@ +56,5 @@
> +  auto pipe =
> +    MakeFilterPipeline(IntSize(100, 100),
> +                       DownscalingConfig { IntSize(100, 100), IntSize(50, 50),
> +                                           SurfaceFormat::B8G8R8A8 },
> +                       SurfaceConfig { decoder, 0, IntSize(100, 100),

Should this be IntSize(50, 50)?

::: image/test/gtest/TestRemoveFrameRectFilter.cpp
@@ +22,5 @@
> +WithRemoveFrameRectFilter(const IntSize& aOriginalSize,
> +                          const IntRect& aFrameRect,
> +                          Func aFunc)
> +{
> +  gfxPrefs::GetSingleton();

Is there any point calling GetSingleton() if you don't do anything with the return value?

@@ +67,5 @@
> +
> +  RawAccessFrameRef currentFrame = decoder->GetCurrentFrameRef();
> +  if (currentFrame) {
> +    currentFrame->Finish();
> +  }

Is this clean-up necessary given that the pipeline setup failed? If not, remove it here and lots of other places :)

@@ +75,5 @@
> +{
> +  WithRemoveFrameRectFilter(IntSize(100, 100),
> +                            IntRect(0, 0, 100, 100),
> +                            [](Decoder* aDecoder, SurfaceFilter* aFilter) {
> +    CheckWritePixels(aDecoder, aFilter,

This whole file is crying out for some repetition removal. Almost every test follows the same pattern. I would personally contemplate using macros to streamline it if it can't be done with normal C++.

@@ +124,5 @@
> +TEST(ImageRemoveFrameRectFilter, WritePixels100_100_to_Minus50_50_0_0)
> +{
> +  WithRemoveFrameRectFilter(IntSize(100, 100),
> +                            IntRect(-50, 50, 0, 0),
> +                            [](Decoder* aDecoder, SurfaceFilter* aFilter) {

The tests where aInputWriteRect and aOutputWriteRect are empty rectangles seem less interesting than the ones where those rectangles are non-empty. But the former outnumber the latter by about 2:1. Could some of the former be cut?

@@ +583,5 @@
> +                               PalettedSurfaceConfig { decoder, 0, IntSize(100, 100),
> +                                                       IntRect(0, 0, 50, 50),
> +                                                       SurfaceFormat::B8G8R8A8, 8,
> +                                                       false });
> +  EXPECT_FALSE(bool(pipe));

A comment explaining why this fails would be helpful.

::: image/test/gtest/TestSurfacePipeIntegration.cpp
@@ +31,5 @@
> +
> +  template <typename T>
> +  static SurfacePipe SurfacePipeFromPipeline(T&& aPipeline)
> +  {
> +    return SurfacePipe{Move(aPipeline)};

Spaces around braces.

@@ +91,5 @@
> +  RawAccessFrameRef currentFrame = decoder->GetCurrentFrameRef();
> +  currentFrame->Finish();
> +}
> +
> +TEST(ImageSurfacePipeIntegration, DeinterlaceDownscaleWritePixels)

Given that the test surface is all green, this isn't much of a test of the deinterlacing filter. I guess TestDeinterlacingFilter.cpp will test that more thoroughly? (I haven't read that file yet at this point in the review.)

@@ +108,5 @@
> +                       DeinterlacingConfig<uint32_t> { IntSize(100, 100),
> +                                                       /* mProgressiveDisplay = */ true },
> +                       DownscalingConfig { IntSize(100, 100), IntSize(25, 25),
> +                                           SurfaceFormat::B8G8R8A8 },
> +                       SurfaceConfig { decoder, 0, IntSize(25, 25),

It's unfortunate that the input and output dimensions need to be repeated for every stage of the pipeline. (In this example 100x100 is repeated 3 times, and 25x25 is repeated twice.) This issue pops up repeatedly. Would it be possible for pipeline stages to get the dimensions from the previous stage? (I haven't thought this through a lot, but it seems semi-plausible.)

@@ +176,5 @@
> +                   /* aOutputRect = */ Some(IntRect(0, 0, 20, 20)),
> +                   /* aInputRect = */ Some(IntRect(0, 0, 100, 100)),
> +                   /* aInputWriteRect = */ Some(IntRect(50, 50, 100, 50)),
> +                   /* aOutputWriteRect = */ Some(IntRect(10, 10, 10, 10)),
> +                   /* aFuzz = */ 0x33);

I admit to barely understanding this one. Is the 10x10 because the 100x100 first gets cut down to 50x50, and then downscaled to 20x20? 
And I don't know where the 0x33 value for aFuzz comes from.

@@ +374,5 @@
> +                               PalettedSurfaceConfig { decoder, 0, IntSize(100, 100),
> +                                                       IntRect(0, 0, 50, 50),
> +                                                       SurfaceFormat::B8G8R8A8, 8,
> +                                                       false });
> +  EXPECT_FALSE(bool(pipe));

A brief comment about why this configuration fails would be nice. Ditto for the function below.

::: image/test/gtest/TestSurfaceSink.cpp
@@ +16,5 @@
> +using namespace mozilla;
> +using namespace mozilla::gfx;
> +using namespace mozilla::image;
> +
> +template <bool FlipVertically, typename Func> void

Having a binary enum with named constants would be better -- WithSurfaceSink<FlipVertically> is much clearer than WithSurfaceSink<true>.

@@ +21,5 @@
> +WithSurfaceSink(Func aFunc)
> +{
> +  gfxPrefs::GetSingleton();
> +  DecoderType decoderType =
> +    DecoderFactory::GetDecoderType("image/gif");

This assignment fits one a single line within 80 chars.

@@ +47,5 @@
> +WithPalettedSurfaceSink(const IntRect& aFrameRect, Func aFunc)
> +{
> +  gfxPrefs::GetSingleton();
> +  DecoderType decoderType =
> +    DecoderFactory::GetDecoderType("image/gif");

Ditto. (And this exact code shows up in various other files in this patch.)

@@ +74,5 @@
> +{
> +  // Create the NullSurfaceSink.
> +  NullSurfaceSink sink;
> +  nsresult rv =
> +    sink.Configure(NullSurfaceConfig { });

Ditto.

@@ +97,5 @@
> +  EXPECT_TRUE(invalidRect.isNothing());
> +
> +  result = sink.WriteRows<uint32_t>([&](uint32_t* aRow, uint32_t aLength) {
> +    gotCalled = true;
> +    for (; aLength > 0 ; --aLength, ++aRow) {

No space before semicolons.

@@ +183,5 @@
> +    count = 0;
> +    result = aSink->WriteRows<uint32_t>([&](uint32_t* aRow, uint32_t aLength) {
> +      count++;
> +      for (; aLength > 0 ; --aLength, ++aRow) {
> +        *aRow = BGRAColor::Green().AsPixel();

Any reason you used red in the function above but green here?

Comment 37

[Seth Fowler [:seth] [:s2h]]

(In reply to Nicholas Nethercote [:njn] from comment #35)
> Is there any significance to the order you've chosen for these standard
> filters? Might be worth a comment explaining how you chose them. ("It's
> arbitrary" would be a reasonable comment, if it's true.)

It's arbitrary in the sense that any order will "work" (i.e. the machinery will behave correctly) but the particular order used here is probably the most logical - you want to deinterlace the raw rows from the original image, not already processed rows, and you probably want to downscale using the final version of the image that has the frame rect removed so the transparent pixels are accounted for when downscaling and pixels that don't appear in the final image are *not* accounted for.

I'll add a comment in there summarizing this.

Comment 38

[Seth Fowler [:seth] [:s2h]]

Thanks for the detailed feedback on part 4, Nick! I'm fading fast tonight but I'll make those changes tomorrow.

There were a couple of minor points I could address right away; see below.

(In reply to Nicholas Nethercote [:njn] from comment #36)
> @@ +127,5 @@
> > +/**
> > + * Creates a pipeline of SurfaceFilters from a list of Config structs. The
> > + * resulting pipeline is appropriate for use with normal (i.e., non-paletted)
> > + * images. This is used during unit testing to simplify manual construction of
> > + * filter pipelines.
> 
> Isn't this what SurfacePipeFactory is for?

We need to directly interact with the SurfaceFilters (rather than just the SurfacePipe abstraction) but we could probably just provide a friend function that pulled the SurfaceFilters out of a SurfacePipe. I actually did do that originally, and I'm sorry to say that I can't remember what made me change to this design, so there may be some hidden issue there that I'm forgetting. But I can try it again.

> @@ +167,5 @@
> > +    RefPtr<SourceSurface> surface = currentFrame->GetSurface();
> > +    EXPECT_TRUE(RowsAreSolidColor(surface, 0, 4, BGRAColor::Green(), /* aFuzz = */ 2));
> > +    EXPECT_TRUE(RowsAreSolidColor(surface, 6, 3, BGRAColor::Red(), /* aFuzz = */ 3));
> > +    EXPECT_TRUE(RowsAreSolidColor(surface, 11, 3, BGRAColor::Green(), /* aFuzz = */ 3));
> > +    EXPECT_TRUE(RowsAreSolidColor(surface, 16, 4, BGRAColor::Red(), /* aFuzz = */ 3));
> 
> Why is fuzz needed even after skipping rows near the color boundaries?

It's just the nature of Lanczos downscaling. The rows at the boundaries are quite far away from red or green, but there's a little bit of bleed even into rows that are further away. I can add a comment.

> ::: image/test/gtest/TestRemoveFrameRectFilter.cpp
> @@ +22,5 @@
> > +WithRemoveFrameRectFilter(const IntSize& aOriginalSize,
> > +                          const IntRect& aFrameRect,
> > +                          Func aFunc)
> > +{
> > +  gfxPrefs::GetSingleton();
> 
> Is there any point calling GetSingleton() if you don't do anything with the
> return value?

Yeah; we actually do this in various places in Gecko, not just in these tests. The actual reason for doing it is to ensure that gfxPrefs is initialized before we use it; not doing so can cause spurious test failures.

> @@ +124,5 @@
> > +TEST(ImageRemoveFrameRectFilter, WritePixels100_100_to_Minus50_50_0_0)
> > +{
> > +  WithRemoveFrameRectFilter(IntSize(100, 100),
> > +                            IntRect(-50, 50, 0, 0),
> > +                            [](Decoder* aDecoder, SurfaceFilter* aFilter) {
> 
> The tests where aInputWriteRect and aOutputWriteRect are empty rectangles
> seem less interesting than the ones where those rectangles are non-empty.
> But the former outnumber the latter by about 2:1. Could some of the former
> be cut?

The empty rectangle ones are more interesting than they appear, because much of the code in RemoveFrameRectFilter is really about seeking to the first row of actual data and from the last row of actual data to the end of the surface, and these are edge cases that are important to test. They actually found bugs in my initial implementation.

> @@ +91,5 @@
> > +  RawAccessFrameRef currentFrame = decoder->GetCurrentFrameRef();
> > +  currentFrame->Finish();
> > +}
> > +
> > +TEST(ImageSurfacePipeIntegration, DeinterlaceDownscaleWritePixels)
> 
> Given that the test surface is all green, this isn't much of a test of the
> deinterlacing filter. I guess TestDeinterlacingFilter.cpp will test that
> more thoroughly? (I haven't read that file yet at this point in the review.)

Yeah. The tests in this file mostly exist to make sure things don't explode when SurfaceFilters are composed.

> 
> @@ +108,5 @@
> > +                       DeinterlacingConfig<uint32_t> { IntSize(100, 100),
> > +                                                       /* mProgressiveDisplay = */ true },
> > +                       DownscalingConfig { IntSize(100, 100), IntSize(25, 25),
> > +                                           SurfaceFormat::B8G8R8A8 },
> > +                       SurfaceConfig { decoder, 0, IntSize(25, 25),
> 
> It's unfortunate that the input and output dimensions need to be repeated
> for every stage of the pipeline. (In this example 100x100 is repeated 3
> times, and 25x25 is repeated twice.) This issue pops up repeatedly. Would it
> be possible for pipeline stages to get the dimensions from the previous
> stage? (I haven't thought this through a lot, but it seems semi-plausible.)

I think this is a good idea! It would eliminate the need for some assertions, as well. I'll try implementing it and see how it looks.

One tweak: since we need to use the same Config structs no matter what, and SurfaceSinks can be used in isolation, we need to supply a size for SurfaceSinks, which means we'll always have a size present at the end of the pipeline. So we'd be better off having SurfaceFilters get their output size from the next stage than their input size from the previous stage.

Comment 39

[Michael Shal [:mshal]]

Comment on attachment 8718190 [details] [diff] [review]
(Part 4) - Add a test suite for SurfacePipes and SurfaceFilters.

>diff --git a/image/decoders/moz.build b/image/decoders/moz.build
>index 126bf67..73eecda 100644
>--- a/image/decoders/moz.build
>+++ b/image/decoders/moz.build
>-# Decoders need RasterImage.h
>+include('/ipc/chromium/chromium-config.mozbuild')
>+
> LOCAL_INCLUDES += [
>+    # Access to Skia headers for Downscaler.
>+    '/gfx/2d',
>+    # Decoders need ImageLib headers.
>     '/image',
> ]
> 
>+LOCAL_INCLUDES += CONFIG['SKIA_INCLUDES']

What is the purpose of these changes? I don't see anything else changed in image/decoders/* in this patch or any of the others on the bug.

Comment 40

[Seth Fowler [:seth] [:s2h]]

(In reply to Michael Shal [:mshal] from comment #39)
> Comment on attachment 8718190 [details] [diff] [review]
> (Part 4) - Add a test suite for SurfacePipes and SurfaceFilters.
> 
> >diff --git a/image/decoders/moz.build b/image/decoders/moz.build
> >index 126bf67..73eecda 100644
> >--- a/image/decoders/moz.build
> >+++ b/image/decoders/moz.build
> >-# Decoders need RasterImage.h
> >+include('/ipc/chromium/chromium-config.mozbuild')
> >+
> > LOCAL_INCLUDES += [
> >+    # Access to Skia headers for Downscaler.
> >+    '/gfx/2d',
> >+    # Decoders need ImageLib headers.
> >     '/image',
> > ]
> > 
> >+LOCAL_INCLUDES += CONFIG['SKIA_INCLUDES']
> 
> What is the purpose of these changes? I don't see anything else changed in
> image/decoders/* in this patch or any of the others on the bug.

Without them we run into problems with finding Skia-related headers. I haven't really spent any time seeing if this could be avoided but I suspect it has to do with moving the Skia-based downscaling code to live entirely in a header file. (Since it's now a template class.)

Comment 41

[Seth Fowler [:seth] [:s2h]]

CC'ing Blake as he expressed interest in this bug.

Comment 42

[Nicholas Nethercote [inactive]]

Comment on attachment 8718210 [details] [diff] [review]
(Part 1) - Add a new SurfacePipe API for writing to image surfaces in a safe and composable manner.

Review of attachment 8718210 [details] [diff] [review]:
-----------------------------------------------------------------

Still a couple of lines with trailing whitespace in this patch.

::: image/SurfacePipe.h
@@ +147,5 @@
> +   *              pixel in the surface each time it's called. The lambda must
> +   *              return a NextPixel<PixelType> value.
> +   *
> +
> +   * @return A WriteState value indicating the lambda generator's state.

Unnecessary blank line.

@@ +276,5 @@
> +  /**
> +   * @return true if the provided input size matches the configuration; used
> +   * to check that the SurfacePipe was configured correctly. Implementations
> +   * should recursively call IsValidPipeForInputSize() on the next filter in
> +   * the chain.

Indent lines 2--4 here.

Comment 43

[Seth Fowler [:seth] [:s2h]]

(In reply to Seth Fowler [:seth] [:s2h] from comment #38)
> We need to directly interact with the SurfaceFilters (rather than just the
> SurfacePipe abstraction) but we could probably just provide a friend
> function that pulled the SurfaceFilters out of a SurfacePipe. I actually did
> do that originally, and I'm sorry to say that I can't remember what made me
> change to this design, so there may be some hidden issue there that I'm
> forgetting. But I can try it again.

On reflection I'd prefer not to do this; it's better to keep the tests as simple as possible. (They're already complex enough!)

Comment 44

[Seth Fowler [:seth] [:s2h]]

(In reply to Nicholas Nethercote [:njn] from comment #36)
> @@ +75,5 @@
> > +{
> > +  WithRemoveFrameRectFilter(IntSize(100, 100),
> > +                            IntRect(0, 0, 100, 100),
> > +                            [](Decoder* aDecoder, SurfaceFilter* aFilter) {
> > +    CheckWritePixels(aDecoder, aFilter,
> 
> This whole file is crying out for some repetition removal. Almost every test
> follows the same pattern. I would personally contemplate using macros to
> streamline it if it can't be done with normal C++.

I'm not sure about this one. Do you think this would be a substantial gain? I could replace these tests with a function or macro that just accepts a list of IntSizes and IntRects, but that's practically all the existing code consists of anyway...

Comment 45

[Seth Fowler [:seth] [:s2h]]

(In reply to Nicholas Nethercote [:njn] from comment #36)
> @@ +183,5 @@
> > +    count = 0;
> > +    result = aSink->WriteRows<uint32_t>([&](uint32_t* aRow, uint32_t aLength) {
> > +      count++;
> > +      for (; aLength > 0 ; --aLength, ++aRow) {
> > +        *aRow = BGRAColor::Green().AsPixel();
> 
> Any reason you used red in the function above but green here?

Yep, we're testing progressive passes here. The red gets overwritten by green later in the test.

Comment 46

[Nicholas Nethercote [inactive]]

> I'm not sure about this one. Do you think this would be a substantial gain?
> I could replace these tests with a function or macro that just accepts a
> list of IntSizes and IntRects, but that's practically all the existing code
> consists of anyway...

There is lots duplication that can be easily removed in this patch. I've pointed out some specific items. Use your judgment as to exactly which changes you make, but there's definitely room for some. If I'm still not happy on re-review we can rinse and repeat :)

Comment 47

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Add a new SurfacePipe API for writing to image surfaces in a safe and composable manner.

This new version of part 1 modifies the internal SurfaceFilter API so that
SurfaceFilters get their output size from the input size of the next filter in
the chain, rather than the input and output size being specified for each
SurfaceFilter.

Rerequesting review since this is a somewhat substantial change. Primarily what
has changed is SurfaceFilter (which now stores an input size rather than just a
row width) and more specifically SurfaceFilter::ConfigureFilter(). The
SurfaceFilters/SurfaceSinks in this patch have been updated to call
ConfigureFilter() using the new signature and also to get their input size from
SurfaceFilter::InputSize() instead of storing it themselves. Also, the method
(previously used for assertions) IsFilterValidForInputSize() has been removed,
because there's now no way to construct a filter pipeline that violates this
requirement, which is very nice.

Thanks for suggesting this change, Nick. It's definitely for the better.

Comment 48

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add SurfaceFilter implementations for basic surface output operations.

Rerequesting review for part 2 for the same reasons as above; the API for
ConfigureFilter() has changed and each filter now calls
SurfaceFilter::InputSize() to obtain its input size instead of storing it
itself.

Comment 49

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Add a factory for constructing SurfacePipes.

Again, rerequesting review. The changes in this patch aren't so big. What has
changed is that the new, simpler *Config structs from parts 1 and 2 are used (so
output size no longer has to be specified for each filter) and that some
additional comments have been added, per the review comments.

Comment 50

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 4) - Add a test suite for SurfacePipes and SurfaceFilters.

Lots of changes here. I tried to address all the review comments above, though
there are a couple of cases where I didn't make the requested change. I've noted
all of those cases in comments above (the most substantial one is comment 44).

Mostly these changes are stylistic, in comments, or in factoring out some shared
code into functions. It's worth noting that, as requested, I've added new tests
for non-square downscaling factors to TestDownscalingFilter. That's the only
substantial new code.

Comment 51

[Seth Fowler [:seth] [:s2h]]

Note that nothing in bug 1247152 needs to be changed; the API improvements in part 1 are all internal and don't affect client code that uses SurfacePipe. (Which is good; part of the point of these patches is to keep the API the decoders use simple and isolate them from the complexity of the SurfaceFilters, so it's encouraging that a fairly substantial change didn't require touching the decoders at all.)

Comment 52

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=808770d1c5e6

Comment 53

[Nicholas Nethercote [inactive]]

Comment on attachment 8722624 [details] [diff] [review]
(Part 1) - Add a new SurfacePipe API for writing to image surfaces in a safe and composable manner.

Review of attachment 8722624 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

::: image/SurfacePipe.h
@@ +40,5 @@
> + * An invalid rect for a surface. Results are given both in the space of the
> + * original image (i.e., before any SurfaceFilters are applied) and in the space
> + * of the final surface (after all SurfaceFilters).
> + */
> +struct SurfaceInvalidRect

This comment mentions "original" and "final" but the data members use "Input" and "Output" -- make them consistent?

Comment 54

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7af42a55bf91

Comment 55

[Nicholas Nethercote [inactive]]

Comment on attachment 8722626 [details] [diff] [review]
(Part 3) - Add a factory for constructing SurfacePipes.

Review of attachment 8722626 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/SurfacePipeFactory.h
@@ +28,5 @@
> +template <typename Config, typename... Configs>
> +struct FilterPipeline<Config, Configs...>
> +{
> +  typedef typename Config::template Filter<typename FilterPipeline<Configs...>::Type> Type;
> +};

Ah, modern C++!

@@ +139,5 @@
> +      } else {  // (removeFrameRect is false)
> +        if (deinterlace) {
> +          return MakePipe(aInputSize, deinterlacingConfig, surfaceConfig);
> +        } else {  // (deinterlace is false)
> +          return MakePipe(aInputSize, surfaceConfig);

All the early returns are making my eye twitch. I know I said before it didn't matter... but I've changed my mind and I would prefer you to introduce a local variable |p| and return that at the end of the function. Thank you :)

@@ +191,5 @@
> +    if (deinterlace) {
> +      return MakePalettedPipe(aFrameRect.Size(), deinterlacingConfig,
> +                              palettedSurfaceConfig);
> +    } else {
> +      return MakePalettedPipe(aFrameRect.Size(), palettedSurfaceConfig);

Ditto.

@@ +206,5 @@
> +    if (NS_FAILED(rv)) {
> +      return Nothing();
> +    }
> +
> +    MOZ_ASSERT(pipe->IsValidPipeForInputSize(aInputSize));

Isn't IsValidPipeForInputSize() gone now? (Ah, I see you have debug compile errors on try because of this.)

@@ +209,5 @@
> +
> +    MOZ_ASSERT(pipe->IsValidPipeForInputSize(aInputSize));
> +    MOZ_ASSERT(!pipe->IsValidPalettedPipe());
> +
> +    return Some(SurfacePipe{ Move(pipe) });

Space before '{'.

@@ +225,5 @@
> +
> +    MOZ_ASSERT(pipe->IsValidPipeForInputSize(aInputSize));
> +    MOZ_ASSERT(pipe->IsValidPalettedPipe());
> +
> +    return Some(SurfacePipe{ Move(pipe) });

Ditto.

Comment 56

[Nicholas Nethercote [inactive]]

Comment on attachment 8722627 [details] [diff] [review]
(Part 4) - Add a test suite for SurfacePipes and SurfaceFilters.

Review of attachment 8722627 [details] [diff] [review]:
-----------------------------------------------------------------

Looking better! CreateTrivialDecoder() is a great improvement. r=me with the comments below addressed, thanks.

::: image/test/gtest/Common.cpp
@@ +183,5 @@
> +  // +---------------------------+
> +  // |             A             |
> +  // +---------+--------+--------+
> +  // |    B    |   C    |   D    |
> +  // +---------+--------+--------+

Nice ASCII art :)

@@ +233,5 @@
> +  int32_t count = 0;
> +  auto result = aFunc(count);
> +  EXPECT_EQ(WriteState::FINISHED, result);
> +  EXPECT_EQ(inputWriteRect.width * inputWriteRect.height, count);
> +  

Some trailing whitespace here. Please check all patches for other occurrences.

::: image/test/gtest/TestDeinterlacingFilter.cpp
@@ +34,5 @@
> +  
> +  aFunc(decoder, pipe.get());
> +
> +  RawAccessFrameRef currentFrame = decoder->GetCurrentFrameRef();
> +  currentFrame->Finish();

By my count there are more than 20 occurrences of this code. (Some of them null-check |currentFrame|.) It can be factored out into a separate function.

Also, I asked in the previous review if this code was necessary for the cases where the pipeline construction fails, but I don't see an answer to that. Is it?

@@ +270,5 @@
> +    EXPECT_TRUE(aFilter->IsSurfaceFinished());
> +    Maybe<SurfaceInvalidRect> invalidRect = aFilter->TakeInvalidRect();
> +    EXPECT_TRUE(invalidRect.isSome());
> +    EXPECT_EQ(IntRect(0, 0, 20, 20), invalidRect->mInputSpaceRect);
> +    EXPECT_EQ(IntRect(0, 0, 20, 20), invalidRect->mOutputSpaceRect);

By my count there are 14 occurrences of these six lines. If you factor it out it'll look something like this:

> AssertFinalStateOk(aFilter, IntRect(0, 0, 20, 20), IntRect(0, 0, 20, 20));

::: image/test/gtest/TestSurfacePipeIntegration.cpp
@@ +31,5 @@
> +
> +  template <typename T>
> +  static SurfacePipe SurfacePipeFromPipeline(T&& aPipeline)
> +  {
> +    return SurfacePipe{ Move(aPipeline) };

Space before '{'.

Comment 57

[Nicholas Nethercote [inactive]]

> ::: image/test/gtest/TestDeinterlacingFilter.cpp
> @@ +34,5 @@
> > +  
> > +  aFunc(decoder, pipe.get());
> > +
> > +  RawAccessFrameRef currentFrame = decoder->GetCurrentFrameRef();
> > +  currentFrame->Finish();
> 
> By my count there are more than 20 occurrences of this code. (Some of them
> null-check |currentFrame|.) It can be factored out into a separate function.

This comment was aimed at just the last two lines dealing with |currentFrame|. But now that I look again, all those WithFooFilter() functions are very similar, so bonus points if you can factor out more than just those two lines :)

Comment 58

[Seth Fowler [:seth] [:s2h]]

(In reply to Nicholas Nethercote [:njn] from comment #56)
> By my count there are more than 20 occurrences of this code. (Some of them
> null-check |currentFrame|.) It can be factored out into a separate function.

Yep. I should probably just make them all null-check. The ones that null-check now are the cases where the test may end up not allocating a surface depending on how it fails, but really that could probably happen in other cases that I happened not to hit during my testing.

> Also, I asked in the previous review if this code was necessary for the
> cases where the pipeline construction fails, but I don't see an answer to
> that. Is it?

Yeah, it is, depending on where in the pipeline the construction fails. The filters are constructed from the end backwards, so if the construction of the sink succeeds but some other stage in the pipeline fails, we may still have a surface that we need to mark as finished. (Assertions will fire if we don't; normally Decoder takes care of this, but since we're not calling the usual methods on Decoder in these tests it doesn't have a chance to.)

Comment 59

[Nathan Froyd [:froydnj]]

Comment on attachment 8722626 [details] [diff] [review]
(Part 3) - Add a factory for constructing SurfacePipes.

Review of attachment 8722626 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/SurfacePipeFactory.h
@@ +117,5 @@
> +                          removeFrameRectConfig, downscalingConfig,
> +                          surfaceConfig);
> +        } else {  // (deinterlace is false)
> +          return MakePipe(aFrameRect.Size(), removeFrameRectConfig,
> +                          downscalingConfig, surfaceConfig);

It's possible that all these early returns would make njn's eyes twitch less if they followed the coding style:

if (removeFrameRect) {
  if (deinterlace) {
    return ...
  }

  return ...
}

if (deinterlace) {
  return ...
}

return ...

Or maybe combining the booleans into a bitmask and switching off that?  Dunno.

Comment 60

[Nicholas Nethercote [inactive]]

> It's possible that all these early returns would make njn's eyes twitch less
> if they followed the coding style:
> 
> if (removeFrameRect) {
>   if (deinterlace) {
>     return ...
>   }
> 
>   return ...
> }

No! I actually like less than the current approach, because control paths of equal significance get different levels of indentation.

Comment 61

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Add a new SurfacePipe API for writing to image surfaces in a safe and composable manner.

This updated version of part 1 fixes a compiler warning about unused variables.

Comment 62

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add SurfaceFilter implementations for basic surface output operations.

This version of part 2 makes each filter check whether the rest of the pipeline
is a valid paletted pipeline or not, and if the answer is incompatible with the
filter, Configure() returns an error. Previously this was checked using
assertions. This reduces the amount of manual asserting that we need to do in
other parts of the code.

Comment 63

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Add a factory for constructing SurfacePipes.

Because of the changes in part 3, there's no need to distinguish between
SurfacePipeFactory::MakePipe() and SurfacePipeFactory::MakePalettedPipe() (the
only difference was in the assertions they made), so the latter is removed in
this version of the patch.

Comment 64

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 4) - Add a test suite for SurfacePipes and SurfaceFilters.

All the review comments above are addressed. The resulting patch is well over a
hundred lines shorter; nice!

Comment 65

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 4) - Add a test suite for SurfacePipes and SurfaceFilters.

Fixed some additional whitespace issues.

Comment 66

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c06ccba43070

Comment 67

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=475cf058848b

Comment 68

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Add a new SurfacePipe API for writing to image surfaces in a safe and composable manner.

Fixed a compiler warning.

Comment 69

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 4) - Add a test suite for SurfacePipes and SurfaceFilters.

Fixed some compiler warnings.

Comment 70

[Seth Fowler [:seth] [:s2h]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/de2f01e26094f9c4bb9a42421ee07219278dcb4d
Bug 1246851 (Part 1) - Add a new SurfacePipe API for writing to image surfaces in a safe and composable manner. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/e24b148198373a064a9544099882accbf2f8ca46
Bug 1246851 (Part 2) - Add SurfaceFilter implementations for basic surface output operations. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/db31b23b3652f29a4c136c188379b6e4d3ae0684
Bug 1246851 (Part 3) - Add a factory for constructing SurfacePipes. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5e9eb7e2244ee2d0b41e0129e45acd642a7b4dd
Bug 1246851 (Part 4) - Add a test suite for SurfacePipes and SurfaceFilters. r=njn

Comment 71

[Nicholas Nethercote [inactive]]

\o/

Comment 72

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/de2f01e26094
https://hg.mozilla.org/mozilla-central/rev/e24b14819837
https://hg.mozilla.org/mozilla-central/rev/db31b23b3652
https://hg.mozilla.org/mozilla-central/rev/f5e9eb7e2244

Comment 73

[Seth Fowler [:seth] [:s2h]]

(In reply to Nicholas Nethercote [:njn] from comment #71)
> \o/

Thanks so much for your help in getting this thing reviewed and landed! I know it was a monster. =)