Closed Bug 1255107 Opened 8 years ago Closed 8 years ago

Use SurfacePipe in the PNG decoder

Categories

(Core :: Graphics: ImageLib, defect)

Product:
Core
Component:
Graphics: ImageLib
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: seth, Assigned: seth)

References

(Blocks 2 open bugs)

Details

(Whiteboard: gfx-noted)

Attachments

(3 files, 5 obsolete files)

(Part 1) - Add a RowHasPixels() utility function to allow tests to compare rows to an arbitrary sequence of BGRAColor values.
5.08 KB, patch
Details | Diff | Splinter Review
(Part 2) - Add a SurfaceFilter for bilinear interpolation for ADAM7 interlaced images. f=glennrp,r=njn
47.37 KB, patch
Details | Diff | Splinter Review
(Part 3) - Use SurfacePipe in the PNG decoder.
46.35 KB, patch
Details | Diff | Splinter Review 
SurfacePipe is intended to eliminate the possibility of output buffer overflows in ImageLib, so we want to use it for all decoders. That includes PNG.
Blocks: 1255109
Depends on: 1279117
Blocks: 1280552
This is just a quick helper that we'll use in the tests for part 2. We need to
be able to generate an expected sequence of pixel values and test against it;
RowHasPixels() makes it easy to do that.
Attachment #8763279 - Flags: review?(n.nethercote)
This is unfortunately by far the biggest patch in this series.

PNG images use an interlacing scheme called ADAM7 which is based upon 8x8 blocks
rather than rows as in GIF. (Either read the patch or check Wikipedia for more
information.) To make interlaced PNGs look nicer when we haven't finished
downloading them, we perform bilinear interpolation on the partially decoded
image. Essentially we interpolate horizontally between pixels that have their
final values and then interpolate vertically to fill in rows with no final pixel
values. Right now, this is done as a post processing step on every second pass
over an interlaced image.

In a SurfacePipe world we don't want any post processing steps like this,
because we don't want anything but the SurfacePipe itself to directly write to
the surface. So this patch replaces the existing bilinear interpolation code in
nsPNGDecoder.cpp with a new SurfaceFilter that does the same job.

The new SurfaceFilter operates in a streaming fashion (obviously enough), so it
adds no additional passes over the image compared to not performing bilinear
interpolation. It also supports interpolation for every ADAM7 pass instead of
only even passes like the existing code. Finally, the current design has the
flaw that if we draw the image between the end of one even ADAM7 pass and the
end of the next even ADAM7 pass, we'll draw a version of the image that doesn't
have any bilinear interpolation applied; this SurfaceFilter version doesn't have
that issue.

This is a classic scenario in which SIMD would help and I've filed a followup
bug about using SIMD to accelerate this.

Writing this SurfaceFilter was not hard but writing unit tests without
reimplementing the entire thing in the tests was quite tricky and I think I've
only half-succeeded. Still, the nice thing about programmatic generation of unit
tests is that it's very easy to generate a lot of tests to test a lot of edge
cases quickly, and I caught many subtle bugs in ADAM7InterpolatingFilter using
these tests. Nevertheless these tests are going to be kinda a pain to review and
I apologize. =(

Glenn, if you have any feedback on ADAM7InterpolatingFilter I'd love to hear it.
If you haven't been following the SurfacePipe work up till now it may help to
take a look at SurfacePipe.h and SurfacePipeFactory.h to get an idea of how this
stuff fits together.
Attachment #8763281 - Flags: review?(n.nethercote)
Attachment #8763281 - Flags: feedback?(glennrp+bmo)
This is the patch that actually switches us over to use SurfacePipe in the PNG
decoder. It's mostly pretty straightforward. The main changes happen in
row_callback() and the new method WriteRow(). I also updated CreateFrame()'s
signature since we need to know whether the frame is interlaced, and I took that
opportunity to generally clean it up a little; this of course propagated to
callsites. In a few cases where I had to touch a line anyway I also replaced
C-style casts with C++-style casts.

It's worth noting that the color management stuff will also eventually be
handled by SurfacePipe, but compared to all other SurfacePipe-related changes
I'm relatively nervous about that one because it seems a bit more difficult to
test thoroughly. For that reason I'm going to put off the color management stuff
for later so that it can be backed out independently.
Attachment #8763282 - Flags: review?(n.nethercote)
Blocks: 1255106
No longer depends on: 1255106
The approach looks OK but I can't test it.  Some dependency is missing or perhaps rebasing is needed:

 3:06.89 /home/glennrp/FF/d/image/decoders/nsPNGDecoder.cpp:928:1:   required from here
 3:06.89 /home/glennrp/FF/d/image/SurfaceFilters.h:718:7: error: ‘class mozilla::image::SurfaceFilter’ has no member named ‘WritePixelsToRow
Comment on attachment 8763279 [details] [diff] [review]
(Part 1) - Add a RowHasPixels() utility function to allow tests to compare rows to an arbitrary sequence of BGRAColor values.

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

::: image/test/gtest/Common.cpp
@@ +226,5 @@
> +  ASSERT_GE_OR_RETURN(aRow, 0, false);
> +
> +  IntSize surfaceSize = aSurface->GetSize();
> +  ASSERT_EQ_OR_RETURN(aPixels.size(), size_t(surfaceSize.width), false);
> +  ASSERT_LT_OR_RETURN(aRow, surfaceSize.height, false); 

Trailing whitespace.

@@ +229,5 @@
> +  ASSERT_EQ_OR_RETURN(aPixels.size(), size_t(surfaceSize.width), false);
> +  ASSERT_LT_OR_RETURN(aRow, surfaceSize.height, false); 
> +
> +  RefPtr<DataSourceSurface> dataSurface = aSurface->GetDataSurface();
> +  ASSERT_TRUE_OR_RETURN(dataSurface != nullptr, false);

Just use |dataSurface| or |!!dataSurface|?

@@ +231,5 @@
> +
> +  RefPtr<DataSourceSurface> dataSurface = aSurface->GetDataSurface();
> +  ASSERT_TRUE_OR_RETURN(dataSurface != nullptr, false);
> +
> +  ASSERT_EQ_OR_RETURN(dataSurface->Stride(), surfaceSize.width * 4, false);

I don't understand this line.
Attachment #8763279 - Flags: review?(n.nethercote) → review+
Assignee: nobody → seth
Comment on attachment 8763281 [details] [diff] [review]
(Part 2) - Add a SurfaceFilter for bilinear interpolation for ADAM7 interlaced images. f=glennrp,r=njn

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

Seems ok to me, though I'm no expert -- I'd never heard of ADAM7 before.

Lots of comments below but they're mostly nits.

::: image/SurfaceFilters.h
@@ +538,5 @@
> +};
> +
> +/**
> +
> + * ADAM7InterpolatingFilter performs bilinear interpolation over an ADAM7

Unnecessary blank line in the comment.

@@ +622,5 @@
> +protected:
> +  uint8_t* DoResetToFirstRow() override
> +  {
> +    mRow = 0;
> +    mPass = std::min(mPass + 1, 7);

This suggests that we might do additional passes, and in those cases we want to treat them the same as pass 7. Is that right? Can that actually happen? Seems like it shouldn't happen.

(Later: I see now that the tests do an extra pass to see nothing has changed. Is this really a useful thing to test?)

@@ +636,5 @@
> +  }
> +
> +  uint8_t* DoAdvanceRow() override
> +  {
> +    MOZ_ASSERT(0 < mPass && mPass <= 7, "Invalid pass");

I haven't read the entire patch yet, but I'm unclear about what the valid values are for mPass. The diagram above uses 1..7, but the constructor initializes mPass to 0. Please add a comment to mPass's declaration indicating all the used values and which ones are valid vs. invalid.

@@ +711,5 @@
> +
> +      // We iterate through the previous and current important row every time we
> +      // write out an interpolated row, so we need to copy the pointers.
> +      uint8_t* prevRowBytes = aPreviousRow;
> +      uint8_t* curRowBytes = aCurrentRow;

I'd call this currRowBytes. IMO that name is clearer, and it has the benefit of matching the length of prevRowBytes.

@@ +716,5 @@
> +
> +      // Write out the interpolated pixels. Interpolation is componentwise.
> +      aNext.template WritePixelsToRow<uint32_t>([&]{
> +        uint32_t pixel = 0;
> +        uint8_t* component = reinterpret_cast<uint8_t*>(&pixel);

I'd use |auto| here to avoid duplicating the type.

@@ +720,5 @@
> +        uint8_t* component = reinterpret_cast<uint8_t*>(&pixel);
> +        *component++ = InterpolateByte(*prevRowBytes++, *curRowBytes++, weight);
> +        *component++ = InterpolateByte(*prevRowBytes++, *curRowBytes++, weight);
> +        *component++ = InterpolateByte(*prevRowBytes++, *curRowBytes++, weight);
> +        *component++ = InterpolateByte(*prevRowBytes++, *curRowBytes++, weight);

Does this work for both little- and big-endian platforms?

@@ +731,5 @@
> +  {
> +    const size_t finalPixelStride = FinalPixelStride(aPass);
> +    const size_t finalPixelStrideBytes = finalPixelStride * sizeof(uint32_t);
> +    const size_t lastFinalPixel = LastFinalPixel(aWidth, aPass);
> +    const size_t lastFinalPixelBytes = lastFinalPixel * sizeof(uint32_t);

"lastFinal" reads strangely. "last" means "last in the row". "Final" means... "after all processing is done"? Not sure if this can be improved, though might be worth thinking about.

@@ +764,5 @@
> +        }
> +      }
> +    }
> +
> +    // For the pixels after the last final pixel in the row, there isn't a

"last final pixel" again.

@@ +835,5 @@
> +  }
> +
> +  static const float* InterpolationWeights(int32_t aStride)
> +  {
> +    // Precalculated interpolation weights.

Mention the dummy value of 1.0f used for the non-existent pass 0 in each of these arrays, as in the above comments.

@@ +852,5 @@
> +        MOZ_CRASH();
> +    }
> +  }
> +
> +  Next mNext;                        /// The next SurfaceFilter in the chain.

Indent this comment by 1 more char to match those below.

@@ +855,5 @@
> +
> +  Next mNext;                        /// The next SurfaceFilter in the chain.
> +
> +  UniquePtr<uint8_t[]> mPreviousRow;  /// The last row with meaningful pixels
> +                                      /// values that got written to.

s/pixels/pixel/

::: image/SurfacePipeFactory.h
@@ +104,5 @@
>        !aFrameRect.IsEqualEdges(nsIntRect(0, 0, aInputSize.width, aInputSize.height));
>  
> +    // Don't interpolate if we're sure we won't show this surface to the user
> +    // until it's completely decoded, as after all ADAM7 passes are complete no
> +    // interpolation is needed anyway.

The "as after all" is hard to parse. Maybe "...completely decoded, because no interpolation is needed once all ADAM7 passes are complete." ?

@@ +136,5 @@
>                            downscalingConfig, surfaceConfig);
> +        } else if (adam7Interpolate) {
> +          pipe = MakePipe(interpolatingConfig, removeFrameRectConfig,
> +                          downscalingConfig, surfaceConfig);
> +        } else {  // (deinterlace and adam7Interpolate are false)

I think all the comments on the else blocks are overkill here, but I don't mind if you want to keep them.

::: image/test/gtest/TestADAM7InterpolatingFilter.cpp
@@ +40,5 @@
> +void
> +AssertConfiguringADAM7InterpolatingFilterFails(const IntSize& aSize)
> +{
> +  RefPtr<Decoder> decoder = CreateTrivialDecoder();
> +  ASSERT_TRUE(decoder != nullptr);

You used |ASSERT_TRUE(bool(decoder));| in the previous function, which is probably better.

@@ +60,5 @@
> +{
> +  return BGRAColor(InterpolateByte(aColorA.mBlue,  aColorB.mBlue,  aWeight),
> +                   InterpolateByte(aColorA.mGreen, aColorB.mGreen, aWeight),
> +                   InterpolateByte(aColorA.mRed,   aColorB.mRed,   aWeight),
> +                   InterpolateByte(aColorA.mAlpha, aColorB.mAlpha, aWeight));

I'd use aColor1 and aColor2 to avoid possible confusion of A and B with "alpha" and "blue".

@@ +67,5 @@
> +enum class ShouldInterpolate
> +{
> +  eYes,
> +  eNo
> +};

I think it would be reasonable to use a bool instead of this type :)

@@ +95,5 @@
> +
> +  // We cycle through the vector of colors forever (subject to the above
> +  // constraint about the end of the row).
> +  BGRAColor colorA = aColors[finalPixelA % aColors.size()];
> +  BGRAColor colorB = aColors[finalPixelB % aColors.size()];

Same as above w.r.t. the "A" and "B" here.

@@ +114,5 @@
> +vector<float>&
> +InterpolationWeights(int32_t aStride)
> +{
> +  // Precalculated interpolation weights.
> +  static vector<float> stride8Weights =

I think this function is identical to the one of the same name within ADAM7InterpolatingFilter. Can the duplication be avoided?

@@ +328,5 @@
> +  // transparent pixels.
> +  return IsImportantRow(aRow, aPass) ? aColors[aRow % aColors.size()]
> +                                     : BGRAColor::Transparent();
> +
> +}

Extraneous blank line.

@@ +447,5 @@
> +{
> +  // This test writes 8 passes of green pixels (the seven ADAM7 passes, plus one
> +  // extra to make sure nothing goes on if we write too much input) and verifies
> +  // that the output is a solid green surface each time. Because all the pixels
> +  // are the same color, interpolation doesn't matter; we interpolation

"we test interpolation"

@@ +541,5 @@
> +    BGRAColor::Green(), BGRAColor::Blue(), BGRAColor::Red(), BGRAColor::Blue(),
> +    BGRAColor::Red(), BGRAColor::Green(), BGRAColor::Blue(), BGRAColor::Red(),
> +    BGRAColor::Green(), BGRAColor::Red(), BGRAColor::Red(), BGRAColor::Blue(),
> +    BGRAColor::Blue(), BGRAColor::Blue(), BGRAColor::Red(), BGRAColor::Green(),
> +    BGRAColor::Green(), BGRAColor::Blue(), BGRAColor::Red(), BGRAColor::Blue()

I'd consider aligning each column vertically to improve readability, here and below.
Attachment #8763281 - Flags: review?(n.nethercote) → review+
(In reply to Glenn Randers-Pehrson from comment #4)
> The approach looks OK but I can't test it.  Some dependency is missing or
> perhaps rebasing is needed:
> 
>  3:06.89 /home/glennrp/FF/d/image/decoders/nsPNGDecoder.cpp:928:1:  
> required from here
>  3:06.89 /home/glennrp/FF/d/image/SurfaceFilters.h:718:7: error: ‘class
> mozilla::image::SurfaceFilter’ has no member named ‘WritePixelsToRow

Yep. You'll need the patch in bug 1279117 as well.
Comment on attachment 8763282 [details] [diff] [review]
(Part 3) - Use SurfacePipe in the PNG decoder.

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

Not sure I completely got my head around the entirety of this patch, but AFAICT it looks reasonable. Minor comments below.

::: image/decoders/nsPNGDecoder.cpp
@@ -630,4 @@
>      if (NS_FAILED(rv)) {
>        png_longjmp(decoder->mPNG, 5); // NS_ERROR_OUT_OF_MEMORY
>      }
> -    MOZ_ASSERT(decoder->mImageData, "Should have a buffer now");

Why is this assert no longer necessary/useful?

@@ +757,5 @@
>    if (decoder->interlacebuf) {
> +      uint32_t width = uint32_t(decoder->mFrameRect.width);
> +
> +      // We'll output the deinterlaced version of the row.
> +      rowToWrite = decoder->interlacebuf + (row_num * decoder->mChannels * width);

This block has 4-char indentation. It should have 2-char indentation.

@@ +769,5 @@
> +  if (row_num + 1 == height && pass < 6) {
> +    // We've reached the end of a pass, but there are more passes coming. Get
> +    // the SurfacePipe ready for the next pass. Note that we don't do this if
> +    // we're on pass 6 (the 7th and final pass, since libpng numbers them
> +    // starting from zero) to ensure that we don't accidentally write any extra

If PNG numbers them 0..6, would it make things simpler for us to do the same in the previous patch? (Various things would need updating, including the two copies of the diagram.)

@@ -948,4 @@
>    if (NS_FAILED(rv)) {
>      png_longjmp(decoder->mPNG, 5); // NS_ERROR_OUT_OF_MEMORY
>    }
> -  MOZ_ASSERT(decoder->mImageData, "Should have a buffer now");

As above: Why is this assert no longer necessary/useful?
Attachment #8763282 - Flags: review?(n.nethercote) → review+
Thanks for the review! Going through the comments now.

(In reply to Nicholas Nethercote [:njn] from comment #5)
> @@ +231,5 @@
> > +
> > +  RefPtr<DataSourceSurface> dataSurface = aSurface->GetDataSurface();
> > +  ASSERT_TRUE_OR_RETURN(dataSurface != nullptr, false);
> > +
> > +  ASSERT_EQ_OR_RETURN(dataSurface->Stride(), surfaceSize.width * 4, false);
>
> I don't understand this line.

See here for an explanation of DataSourceSurface::Stride():

https://dxr.mozilla.org/mozilla-central/source/gfx/2d/2D.h#458

tldr: The Stride() is the size of a row of pixels in the DataSourceSurface in bytes. We're just sanity checking here that the value is what we expect: four times the width of the surface. (Four times because the surface is 32bpp.) If we got this wrong, the math in the loops below doesn't work.
About interlace pass numbering: the PNG specification describes them as 1..7, while libpng implements them as 0..6, plus the unused Pass 7.  Confusingly, in the same paragraph the PNG specification talks about pixel numbering from 0..width-1 and 0..height-1.  The pixel-numbering and pass-numbering conventions used in the PNG specification are the same as those used in the GIF specification; the PNG authors simply followed suit.
(In reply to Glenn Randers-Pehrson from comment #10)
> About interlace pass numbering: the PNG specification describes them as
> 1..7, while libpng implements them as 0..6, plus the unused Pass 7. 
> Confusingly, in the same paragraph the PNG specification talks about pixel
> numbering from 0..width-1 and 0..height-1.  The pixel-numbering and
> pass-numbering conventions used in the PNG specification are the same as
> those used in the GIF specification; the PNG authors simply followed suit.

I went back and forth about which scheme to use, but in the end resources on the web seem to consistently number the interlacing passes as 1..7, and it happened that using that scheme made the implementation of ADAM7InterpolationFilter::DoResetToFirstRow() feel cleaner to me, so I went with the 1..7 scheme. The only real downside is that all of the arrays of hardcoded constants end up having an extra unused value which I felt the need to explain in comments every time.
(In reply to Nicholas Nethercote [:njn] (on vacation until July 11) from comment #6)
> This suggests that we might do additional passes, and in those cases we want
> to treat them the same as pass 7. Is that right? Can that actually happen?
> Seems like it shouldn't happen.
> 
> (Later: I see now that the tests do an extra pass to see nothing has
> changed. Is this really a useful thing to test?)

It's not useful per se, but I wanted to make sure that nothing outright exploded if a bug in the PNG decoder caused us to write extra passes somehow. This way it won't do any damage. (Other than maybe causing visually incorrect output.)
(In reply to Nicholas Nethercote [:njn] (on vacation until July 11) from comment #6)
> @@ +720,5 @@
> > +        uint8_t* component = reinterpret_cast<uint8_t*>(&pixel);
> > +        *component++ = InterpolateByte(*prevRowBytes++, *curRowBytes++, weight);
> > +        *component++ = InterpolateByte(*prevRowBytes++, *curRowBytes++, weight);
> > +        *component++ = InterpolateByte(*prevRowBytes++, *curRowBytes++, weight);
> > +        *component++ = InterpolateByte(*prevRowBytes++, *curRowBytes++, weight);
> 
> Does this work for both little- and big-endian platforms?

Should be fine, yeah. The math is all per-byte and all bytes are treated identically.
Comment on attachment 8763281 [details] [diff] [review]
(Part 2) - Add a SurfaceFilter for bilinear interpolation for ADAM7 interlaced images. f=glennrp,r=njn

Works for me and is an improvement over previous behavior.
Attachment #8763281 - Flags: feedback?(glennrp+bmo) → feedback+
(In reply to Nicholas Nethercote [:njn] (on vacation until July 11) from comment #6)
> @@ +136,5 @@
> >                            downscalingConfig, surfaceConfig);
> > +        } else if (adam7Interpolate) {
> > +          pipe = MakePipe(interpolatingConfig, removeFrameRectConfig,
> > +                          downscalingConfig, surfaceConfig);
> > +        } else {  // (deinterlace and adam7Interpolate are false)
> 
> I think all the comments on the else blocks are overkill here, but I don't
> mind if you want to keep them.

I don't like them either, but the current if/else nesting is extreme enough that any little bit of assistance to the reader seems worth it. I plan to revisit this design soon and probably switch to having individual factory functions per-decoder, because there are going to be two more SurfaceFilters at a minimum and the current approach is going to become totally unreadable.
(In reply to Glenn Randers-Pehrson from comment #14)
> Comment on attachment 8763281 [details] [diff] [review]
> (Part 2) - Add a SurfaceFilter for bilinear interpolation for ADAM7
> interlaced images. f=glennrp,r=njn
> 
> Works for me and is an improvement over previous behavior.

Thanks for looking at this, Glenn. I feel more confident knowing that you've looked it over.
(In reply to Nicholas Nethercote [:njn] (on vacation until July 11) from comment #8)
> ::: image/decoders/nsPNGDecoder.cpp
> @@ -630,4 @@
> >      if (NS_FAILED(rv)) {
> >        png_longjmp(decoder->mPNG, 5); // NS_ERROR_OUT_OF_MEMORY
> >      }
> > -    MOZ_ASSERT(decoder->mImageData, "Should have a buffer now");
> 
> Why is this assert no longer necessary/useful?
> 
> @@ -948,4 @@
> >    if (NS_FAILED(rv)) {
> >      png_longjmp(decoder->mPNG, 5); // NS_ERROR_OUT_OF_MEMORY
> >    }
> > -  MOZ_ASSERT(decoder->mImageData, "Should have a buffer now");
> 
> As above: Why is this assert no longer necessary/useful?

The plan is that once every image decoder uses SurfacePipe (i.e., soon), Decoder::mImageData and the related fields will go away, and it will be the SurfacePipe itself that allocates and owns imgFrame objects. I've been monitoring progress towards this goal by checking how many references to mImageData turn up in a search of |image/decoders|. However, it's not important to remove uses in e.g. asserts right now, so I'll restore these checks for now.
(In reply to Nicholas Nethercote [:njn] (on vacation until July 11) from comment #8)
> @@ +769,5 @@
> > +  if (row_num + 1 == height && pass < 6) {
> > +    // We've reached the end of a pass, but there are more passes coming. Get
> > +    // the SurfacePipe ready for the next pass. Note that we don't do this if
> > +    // we're on pass 6 (the 7th and final pass, since libpng numbers them
> > +    // starting from zero) to ensure that we don't accidentally write any extra
> 
> If PNG numbers them 0..6, would it make things simpler for us to do the same
> in the previous patch? (Various things would need updating, including the
> two copies of the diagram.)

See comment 11. tldr: 1..7 is more consistent with the spec and various online resources, and in a happy coincidence it made the code a little nicer in one place in ADAM7InterpolatingFilter. It definitely has the tradeoff of requiring explanatory comments in several places, though.
Attachment #8763279 - Attachment is obsolete: true
Attachment #8763281 - Attachment is obsolete: true
As above.
Attachment #8763282 - Attachment is obsolete: true
Glenn, I can work around this so it need not block this bug, but I wanted to let you know of a surprising behavior of libpng. Take a look at this log from one of the failures above:

REFTEST TEST-LOAD | file:///Users/mfowler/Code/Work/streaming-gif/image/test/reftest/pngsuite-oddsizes/s03i3p01.png | 4 / 36 (11%)
++DOMWINDOW == 14 (0x132aa2000) [pid = 53146] [serial = 14] [outer = 0x130626000]
*** Creating PNG decoder for image 0x130dfcaa0. Metadata? n
*** Destroying PNG decoder. Metadata? y
*** Creating PNG decoder for image 0x130dfcaa0. Metadata? n
*** row_callback: 0 pass 0
*** row_callback: 1 pass 0
*** row_callback: 2 pass 0
*** Resetting pipeline for row 2 pass 0
*** row_callback: 0 pass 2
*** row_callback: 1 pass 2
*** row_callback: 2 pass 2
*** Resetting pipeline for row 2 pass 2
*** row_callback: 0 pass 3
*** row_callback: 1 pass 3
*** row_callback: 2 pass 3
*** Resetting pipeline for row 2 pass 3
*** row_callback: 0 pass 4
*** row_callback: 1 pass 4
*** row_callback: 2 pass 4
*** Resetting pipeline for row 2 pass 4
*** row_callback: 0 pass 5
*** row_callback: 1 pass 5
*** row_callback: 2 pass 5
*** Resetting pipeline for row 2 pass 5
*** row_callback: 0 pass 6
*** row_callback: 1 pass 6
*** row_callback: 2 pass 6
*** Destroying PNG decoder. Metadata? n

Note that row_callback never gets called for pass 1 at all. I assume this is because pass 1 does not modify any pixels in this image, but I would've expected row_callback to be called with new_row set to null.
Flags: needinfo?(glennrp+bmo)
This updated version of part 3 resolves the issues that showed up in the try
job:

- |result| in nsPNGDecoder::WriteRow() is now marked DebugOnly to get rid of a
  warning that it's unused in opt builds.

- The way we advance through passes for interlaced images has been changed to
  work around the issue discussed in comment 23.
Attachment #8764098 - Attachment is obsolete: true
(In reply to Seth Fowler [:seth] [:s2h] from comment #23)

> Note that row_callback never gets called for pass 1 at all. I assume this is
> because pass 1 does not modify any pixels in this image, but I would've
> expected row_callback to be called with new_row set to null.

It's expected.  See the Caution note in the PNG spec "some passes will
be entirely empty".  Also see the libpng manpage "Because interlacing may skip a
pass you cannot be sure that the preceding pass is just 'pass-1'."
Flags: needinfo?(glennrp+bmo)
OK, there was still an issue with PNG animations in the previous try job. The
problem is that for BGRA frames, SurfacePipe does not allow the output imgFrame
to have a frame rect. It achieves this by replacing any frame rect that may
exist with 0-alpha pixels surrounding the part of the frame that has data. For
single-frame images, this is definitely the right thing to do, and it doesn't
affect GIF animations since GIF animation frames are paletted, but PNG produces
BGRA animation frames, which means it runs into this issue. It wouldn't be a
problem even there except that PNG supports both OVER and SOURCE blend modes for
compositing animation frames, and with SOURCE, the 0-alpha pixels will overwrite
parts of the previous frame that shouldn't be overwritten.

I've solved this by piping through the frame rect to FrameAnimator as the "blend
rect" and using that instead of the frame rect from the imgFrame when available.
The modifications to the compositing code in FrameAnimator that I made were very
minimal and conservative because I don't want to risk introducing regressions by
getting clever. (And after all, that's not the area this patch is focused on.)
The plan is to replace FrameAnimator's blending code with SurfacePipe-based
compositing very soon, so this code is on the verge of disappearing anyway.

Locally this fixes all the issues for me; if it looks good on try, it's ready to
go.
Attachment #8764470 - Attachment is obsolete: true
Marking this as checkin-needed since it's ready to go but open trees are rarely a thing these days.
Keywords: checkin-needed
Pushed by mfowler@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c56b42022377
(Part 1) - Add a RowHasPixels() utility function to allow tests to compare rows to an arbitrary sequence of BGRAColor values. r=njn
https://hg.mozilla.org/integration/mozilla-inbound/rev/212067476b41
(Part 2) - Add a SurfaceFilter for bilinear interpolation for ADAM7 interlaced images. f=glennrp,r=njn
https://hg.mozilla.org/integration/mozilla-inbound/rev/39ba4da73c6c
(Part 3) - Use SurfacePipe in the PNG decoder. r=njn
Keywords: checkin-needed
Pushed by mfowler@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7e8b25f0e25
(Followup) - Add an explicit cast to a usage of DebugOnly to work around MSVC. r=me
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: