Add support for Animated PNG

third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp

 /*
  * Copyright (C) 2006 Apple Computer, Inc.
  * Copyright (C) Research In Motion Limited 2009-2010. All rights reserved.
  *
  * Portions are Copyright (C) 2001 mozilla.org
  *
  * Other contributors:
  *   Stuart Parmenter <stuart@mozilla.com>
  *
  * This library is free software; you can redistribute it and/or
@@ skipping 20 matching lines @@
  * licenses (the MPL or the GPL) and not to allow others to use your
  * version of this file under the LGPL, indicate your decision by
  * deletingthe provisions above and replace them with the notice and
  * other provisions required by the MPL or the GPL, as the case may be.
  * If you do not delete the provisions above, a recipient may use your
  * version of this file under any of the LGPL, the MPL or the GPL.
  */
 
 #include "platform/image-decoders/png/PNGImageDecoder.h"
 
-#include "platform/image-decoders/png/PNGImageReader.h"
 #include "png.h"
-#include "wtf/PtrUtil.h"
 #include <memory>

[Noel Gordon, 2017/02/21 13:53:54]

Remove these two #include, PNGImageDecoder.h provides them and I won't cry.

[scroggo_chromium, 2017/02/23 22:09:52]

Done.

 
 namespace blink {
 
 PNGImageDecoder::PNGImageDecoder(AlphaOption alphaOption,
                                  const ColorBehavior& colorBehavior,
                                  size_t maxDecodedBytes,
                                  size_t offset)
     : ImageDecoder(alphaOption, colorBehavior, maxDecodedBytes),
-      m_offset(offset) {}
+      m_offset(offset),
+      m_currentFrame(0),
+      // It might be more logical to default to cAnimationNone, but BitmapImage

[Noel Gordon, 2017/02/21 13:53:55]

// It would be logical to default ... 

[scroggo_chromium, 2017/02/23 22:09:53]

Done.

+      // uses that as a signal to never check again, meaning the actual count
+      // will never be respected.
+      m_repetitionCount(cAnimationLoopOnce),
+      m_hasAlphaChannel(false),
+      m_currentBufferSawAlpha(false) {}
 
 PNGImageDecoder::~PNGImageDecoder() {}
 
+bool PNGImageDecoder::setFailed() {
+  m_reader.reset();
+  return ImageDecoder::setFailed();
+}
+
+size_t PNGImageDecoder::decodeFrameCount() {
+  parse(PNGImageReader::PNGParseQuery::PNGMetaDataQuery);

[Noel Gordon, 2017/02/21 13:53:54]

parse(ParseQuery::MetaData);

[scroggo_chromium, 2017/02/23 22:09:53]

Done.

+  return failed() ? m_frameBufferCache.size() : m_reader->frameCount();
+}
+
+void PNGImageDecoder::decode(size_t index) {
+  parse(PNGImageReader::PNGParseQuery::PNGMetaDataQuery);

[Noel Gordon, 2017/02/21 13:53:55]

parse(ParseQuery::MetaData);

[scroggo_chromium, 2017/02/23 22:09:52]

Done.

+
+  if (failed())
+    return;
+
+  updateAggressivePurging(index);
+
+  Vector<size_t> framesToDecode = findFramesToDecode(index);
+  for (auto i = framesToDecode.rbegin(); i != framesToDecode.rend(); i++) {
+    m_currentFrame = *i;
+    if (!m_reader->decode(*m_data, *i)) {
+      setFailed();
+      return;
+    }
+
+    // If this returns false, we need more data to continue decoding.
+    if (!postDecodeProcessing(*i))
+      break;
+  }
+
+  // It is also a fatal error if all data is received and we have decoded all
+  // frames available but the file is truncated.
+  if (index >= m_frameBufferCache.size() - 1 && isAllDataReceived() &&
+      m_reader && !m_reader->parseCompleted())
+    setFailed();
+}
+

[Noel Gordon, 2017/02/21 13:53:55]

Move the parse() routine definition to here.

[scroggo_chromium, 2017/02/23 22:09:53]

Done. For my edification, why is here better?

[Noel Gordon, 2017/02/27 07:45:31]

It's then adjacent to the functions that use it (saving me a lot of scrolling up
and down when reviewing this code :)

+void PNGImageDecoder::clearFrameBuffer(size_t frameIndex) {

[Noel Gordon, 2017/02/21 13:53:55]

frameIndex -> index.

[scroggo_chromium, 2017/02/23 22:09:53]

Done.

+  if (m_reader)
+    m_reader->clearDecodeState(frameIndex);
+  ImageDecoder::clearFrameBuffer(frameIndex);
+}
+
+bool PNGImageDecoder::onInitFrameBuffer(size_t index) {
+  if (PNG_INTERLACE_ADAM7 ==

[Noel Gordon, 2017/02/21 13:53:54]

Let's prefer the early return:

  png_byte interlaceType =
      png_get_interlace_type(m_reader->pngPtr(), m_reader->infoPtr());
  if (PNG_INTERLACE_ADAM7 != interlaceType)
    return;
  
  unsigned colorChannels = m_hasAlphaChannel ? 4 : 3;

[scroggo_chromium, 2017/02/23 22:09:52]

Done.

+      png_get_interlace_type(m_reader->pngPtr(), m_reader->infoPtr())) {
+    unsigned colorChannels = m_hasAlphaChannel ? 4 : 3;
+    m_reader->createInterlaceBuffer(colorChannels * size().width() *

[Noel Gordon, 2017/02/21 13:53:55]

colorChannels * size().area() ?

[scroggo_chromium, 2017/02/23 22:09:52]

Done.

+                                    size().height());
+    return m_reader->interlaceBuffer();

[Noel Gordon, 2017/02/21 13:53:55]

If this failed, setFailed() will be called in ImageDecoder code

   if (!onInitFrameBuffer())
      return setFailed();

looking at the changes to ImageDecoder.  That is troublesome (see below).

[scroggo_chromium, 2017/02/23 22:09:52]

setFailed() can be called in other ways from initFrameBuffer, too. As you point
out below, this is bad when we try to access parts of PNGImageReader afterwards.

GIFImageReader is in a similar situation - it calls initFrameBuffer, which may
delete the reader itself. AFAICT, this is safe because it just returns if that
happens. But maybe it would be safer to just make the caller call setFailed?
(That is what I have done in the next patch set.)

[Noel Gordon, 2017/02/27 07:45:31]

Yeah, possible security problems.
Yes I think if callers are required to do their own failure handling, eg.,
calling setFailed() or whatever their specific decoder needs, that'd be safer
(as this PNG decoder example pointed out).

+  }
+  return true;
+}
+
+bool PNGImageDecoder::canReusePreviousFrameBuffer(size_t frameIndex) const {

[Noel Gordon, 2017/02/21 13:53:55]

frameIndex -> index.

[scroggo_chromium, 2017/02/23 22:09:53]

Done.

+  DCHECK(frameIndex < m_frameBufferCache.size());
+  return m_frameBufferCache[frameIndex].getDisposalMethod() !=
+         ImageFrame::DisposeOverwritePrevious;
+}
+
+void PNGImageDecoder::parse(PNGImageReader::PNGParseQuery query) {

[Noel Gordon, 2017/02/21 13:53:55]

void PNGImageDecoder::parse(ParseQuery query) {

[scroggo_chromium, 2017/02/23 22:09:54]

Done.

+  if (failed() || (m_reader && m_reader->parseCompleted()))
+    return;
+
+  if (!m_reader)
+    m_reader = WTF::makeUnique<PNGImageReader>(this, m_offset);
+
+  if (!m_reader->parse(*m_data, query))
+    setFailed();
+}
+
+void PNGImageDecoder::setRepetitionCount(int repetitionCount) {
+  m_repetitionCount = repetitionCount;
+}
+
+int PNGImageDecoder::repetitionCount() const {
+  return failed() ? cAnimationLoopOnce : m_repetitionCount;
+}
+
+void PNGImageDecoder::initializeNewFrame(size_t index) {
+  const PNGImageReader::FrameInfo& frameInfo = m_reader->frameInfo(index);
+  ImageFrame* buffer = &m_frameBufferCache[index];

[Noel Gordon, 2017/02/21 13:53:55]

ImageFrame& buffer = m_frameBufferCache[index];

[scroggo_chromium, 2017/02/23 22:09:53]

Done.

+
+  DCHECK(IntRect(IntPoint(), size()).contains(frameInfo.frameRect));
+  buffer->setOriginalFrameRect(frameInfo.frameRect);

[Noel Gordon, 2017/02/21 13:53:54]

"buffer->X" to "buffer.X" all through here.

[scroggo_chromium, 2017/02/23 22:09:52]

Done.

+  buffer->setDuration(frameInfo.duration);

[Noel Gordon, 2017/02/21 13:53:54]

Space before this line.

[scroggo_chromium, 2017/02/23 22:09:53]

Done.

+  buffer->setDisposalMethod(frameInfo.disposalMethod);
+  buffer->setAlphaBlendSource(frameInfo.alphaBlend);
+  buffer->setRequiredPreviousFrameIndex(

[Noel Gordon, 2017/02/21 13:53:54]

Space before 151.

  size_t previousFrameIndex = findRequiredPreviousFrame(index, false);
  buffer.setRequiredPreviousFrameIndex(previousFrameIndex);
}

[scroggo_chromium, 2017/02/23 22:09:53]

Done.

+      findRequiredPreviousFrame(index, false));
+}
+
 inline float pngFixedToFloat(png_fixed_point x) {
   return ((float)x) * 0.00001f;
 }
 
 inline sk_sp<SkColorSpace> readColorSpace(png_structp png, png_infop info) {
   if (png_get_valid(png, info, PNG_INFO_sRGB)) {
     return SkColorSpace::MakeNamed(SkColorSpace::kSRGB_Named);
   }
 
   png_charp name = nullptr;
@@ skipping 37 matching lines @@
       // However, the current behavior matches Safari and Firefox.
     }
   }
 
   return nullptr;
 }
 
 void PNGImageDecoder::headerAvailable() {
   png_structp png = m_reader->pngPtr();
   png_infop info = m_reader->infoPtr();
-  png_uint_32 width = png_get_image_width(png, info);
-  png_uint_32 height = png_get_image_height(png, info);
 
-  // Protect against large PNGs. See http://bugzil.la/251381 for more details.
-  const unsigned long maxPNGSize = 1000000UL;
-  if (width > maxPNGSize || height > maxPNGSize) {
-    longjmp(JMPBUF(png), 1);
-    return;
-  }
-
-  // Set the image size now that the image header is available.
-  if (!setSize(width, height)) {
-    longjmp(JMPBUF(png), 1);
-    return;
-  }
-
+  png_uint_32 width, height;
   int bitDepth, colorType, interlaceType, compressionType, filterType, channels;
   png_get_IHDR(png, info, &width, &height, &bitDepth, &colorType,
                &interlaceType, &compressionType, &filterType);
 
   // The options we set here match what Mozilla does.
 
   // Expand to ensure we use 24-bit for RGB and 32-bit for RGBA.
   if (colorType == PNG_COLOR_TYPE_PALETTE ||
       (colorType == PNG_COLOR_TYPE_GRAY && bitDepth < 8))
     png_set_expand(png);
 
   png_bytep trns = 0;
   int trnsCount = 0;

[Noel Gordon, 2017/02/21 13:53:54]

Are |trns| and |trnsCount| used anywhere?

[scroggo_chromium, 2017/02/23 22:09:52]

For completeness, this has been addressed in crrev.com/2716533002

   if (png_get_valid(png, info, PNG_INFO_tRNS)) {
     png_get_tRNS(png, info, &trns, &trnsCount, 0);
     png_set_expand(png);
   }
 
   if (bitDepth == 16)
     png_set_strip_16(png);
 
   if (colorType == PNG_COLOR_TYPE_GRAY ||
       colorType == PNG_COLOR_TYPE_GRAY_ALPHA)
     png_set_gray_to_rgb(png);
 
-  if ((colorType & PNG_COLOR_MASK_COLOR) && !ignoresColorSpace()) {
-    // We only support color profiles for color PALETTE and RGB[A] PNG.
-    // Supporting color profiles for gray-scale images is slightly tricky, at
-    // least using the CoreGraphics ICC library, because we expand gray-scale
-    // images to RGB but we do not similarly transform the color profile. We'd
-    // either need to transform the color profile or we'd need to decode into a
-    // gray-scale image buffer and hand that to CoreGraphics.
-    sk_sp<SkColorSpace> colorSpace = readColorSpace(png, info);
-    if (colorSpace) {
-      setEmbeddedColorSpace(colorSpace);
+  // Only set the size and the color space of the image once. Since non-first

[Noel Gordon, 2017/02/21 13:53:54]

// Only set the size and the color space of the image once since non-first
// frames also use this method: there is no per-frame color space, and the
// image size is determined from the header width and height.

[scroggo_chromium, 2017/02/23 22:09:53]

Done.

+  // frames also use this method, we don't want them to override the size of
+  // the image to the size of their frame rect. Frames also don't specify their
+  // own color space, so we need to set it only once.
+  if (!isDecodedSizeAvailable()) {
+    // Protect against large PNGs. See http://bugzil.la/251381 for more details.
+    const unsigned long maxPNGSize = 1000000UL;
+    if (width > maxPNGSize || height > maxPNGSize) {
+      longjmp(JMPBUF(png), 1);
+      return;
+    }
+
+    if (!setSize(width, height)) {

[Noel Gordon, 2017/02/21 13:53:55]

// Set the image size now that the image header is available.
if (!setSize(width, height)) {

[scroggo_chromium, 2017/02/23 22:09:52]

Done.

+      longjmp(JMPBUF(png), 1);
+      return;
+    }
+
+    if ((colorType & PNG_COLOR_MASK_COLOR) && !ignoresColorSpace()) {
+      // We only support color profiles for color PALETTE and RGB[A] PNG.
+      // Supporting color profiles for gray-scale images is slightly tricky, at
+      // least using the CoreGraphics ICC library, because we expand gray-scale
+      // images to RGB but we do not similarly transform the color profile. We'd
+      // either need to transform the color profile or we'd need to decode into
+      // a
+      // gray-scale image buffer and hand that to CoreGraphics.
+      sk_sp<SkColorSpace> colorSpace = readColorSpace(png, info);
+      if (colorSpace) {

[Noel Gordon, 2017/02/21 13:53:54]

Is sk_sp<T> convertible to bool?  If so, we can use that fact, and we should
std::move colorSpace into the callee

if (sk_sp<SkColorSpace> colorSpace = readColorSpace(png, info))
  setEmbeddedColorSpace(std::move(colorSpace));

since colorSpace is not retained / used by any code herein after this point
(those crazy color kids).

[scroggo_chromium, 2017/02/23 22:09:52]

Addressed by crrev.com/2716533002

+        setEmbeddedColorSpace(colorSpace);
+      }
     }
   }
 

[Noel Gordon, 2017/02/21 13:53:55]

Add a DCHECK(isDecodedSizeAvailable()) around here?

[scroggo_chromium, 2017/02/23 22:09:53]

Done.

[Noel Gordon, 2017/02/27 07:45:31]

One second thoughts, this is in the middle of some the color handling code.  I
figure there is more work to do in that code, so maybe move this DCHECK to after
all the color-specific code, sorry.

[scroggo_chromium, 2017/02/27 20:31:05]

As I look for a better place to put it, I think this is the best, even though
it's in between color-specific code. The block above checks for the decoded
size, so it looks like this:

  if (!isDecodedSizeAvailable()) {
    // Do some work, including ensuring that the decoded size
    // is available. If not, signal an error and do not continue.
  }

  DCHECK(isDecodedSizeAvailable());

I guess either way we're splitting up something. Maybe this belongs just above
png_read_update_info?
No worries. Now is the time to address it!

[Noel Gordon, 2017/03/01 09:11:34]

Yeah, seem so.
Yeah, anywhere after the color-code. Try a few places and see how you feel about
it, ditch it if it's PITA.
 
Thx.

[scroggo_chromium, 2017/03/03 21:56:40]

Done.

   if (!hasEmbeddedColorSpace()) {
     // TODO (msarett):
     // Applying the transfer function (gamma) should be handled by
     // SkColorSpaceXform.  Here we always convert to a transfer function that
     // is a 2.2 exponential.  This is a little strange given that the dst
     // transfer function is not necessarily a 2.2 exponential.
     // TODO (msarett):

[Noel Gordon, 2017/02/21 13:53:55]

Comment lines 277-280: remove.  Fixed long ago.

[scroggo_chromium, 2017/02/23 22:09:52]

Acknowledged.

[Noel Gordon, 2017/02/27 07:45:31]

And also addressed by crrev.com/2716533002

     // Often, PNGs that specify their transfer function with the gAMA tag will
     // also specify their gamut with the cHRM tag.  We should read this tag
     // and do a full color space transformation if it is present.
     const double inverseGamma = 0.45455;
     const double defaultGamma = 2.2;
     double gamma;
     if (!ignoresColorSpace() && png_get_gAMA(png, info, &gamma)) {
       const double maxGamma = 21474.83;
       if ((gamma <= 0.0) || (gamma > maxGamma)) {
         gamma = inverseGamma;
         png_set_gAMA(png, info, gamma);
       }
       png_set_gamma(png, defaultGamma, gamma);
     } else {
       png_set_gamma(png, defaultGamma, inverseGamma);
     }
   }
 
   // Tell libpng to send us rows for interlaced pngs.
   if (interlaceType == PNG_INTERLACE_ADAM7)
     png_set_interlace_handling(png);
 
   // Update our info now.

[Noel Gordon, 2017/02/21 13:53:54]

// Update our info now (so we can read color channel info).
png_read_update_info(png, info);

channels = png_get_channels(png, info);
DCHECK(channels == 3 || channels == 4);
m_hasAlphaChannel = (channels == 4);

[scroggo_chromium, 2017/02/23 22:09:53]

Done.

   png_read_update_info(png, info);
   channels = png_get_channels(png, info);
-  ASSERT(channels == 3 || channels == 4);
+  DCHECK(channels == 3 || channels == 4);
 
-  m_reader->setHasAlpha(channels == 4);
-
-  if (m_reader->decodingSizeOnly()) {
-// If we only needed the size, halt the reader.
-#if PNG_LIBPNG_VER_MAJOR > 1 || \
-    (PNG_LIBPNG_VER_MAJOR == 1 && PNG_LIBPNG_VER_MINOR >= 5)
-    // Passing '0' tells png_process_data_pause() not to cache unprocessed data.
-    m_reader->setReadOffset(m_reader->currentBufferSize() -
-                            png_process_data_pause(png, 0));
-#else
-    m_reader->setReadOffset(m_reader->currentBufferSize() - png->buffer_size);
-    png->buffer_size = 0;
-#endif
-  }
+  m_hasAlphaChannel = (channels == 4);
+  m_currentBufferSawAlpha = false;

[Noel Gordon, 2017/02/21 13:53:54]

Should this m_currentBufferSawAlpha = false be moved into onInitFrame?  That's
what GIF does to init per-frame alpha tracking I think?

[scroggo_chromium, 2017/02/23 22:09:52]

Done.

 }
 
 void PNGImageDecoder::rowAvailable(unsigned char* rowBuffer,
                                    unsigned rowIndex,
                                    int) {
-  if (m_frameBufferCache.isEmpty())
+  if (m_currentFrame >= m_frameBufferCache.size())
     return;
 
-  // Initialize the framebuffer if needed.
-  ImageFrame& buffer = m_frameBufferCache[0];
-  if (buffer.getStatus() == ImageFrame::FrameEmpty) {
-    png_structp png = m_reader->pngPtr();
-    if (!buffer.setSizeAndColorSpace(size().width(), size().height(),
-                                     colorSpaceForSkImages())) {
-      longjmp(JMPBUF(png), 1);
-      return;
-    }
+  // When a client calls ImageDecoder::setMemoryAllocator *before* decoding the
+  // frame count, the first frame won't get initialized correctly. The call will
+  // resize |m_frameBufferCache| to 1, and therefore ImageDecoder::frameCount
+  // will *not* call initializeNewFrame for the first frame, whether it is a
+  // static image or the first frame of an animated image. Amongst others, the
+  // frame rect will not be set. If this is the case, initialize the frame here.
+  if (m_frameBufferCache[0].originalFrameRect().size() == IntSize(0, 0))

[Noel Gordon, 2017/02/21 13:53:54]

m_frameBufferCache[0].originalFrameRect().size().isZero() ?

The proceeding comment is a sad story.  Is there any way round the problem it
speaks of?  Maybe FIXME: for now.

[scroggo_chromium, 2017/02/23 22:09:53]

This was caught by the following tests:
DeferredImageDecoderTest.decodeOnOtherThread
DeferredImageDecoderTest.drawIntoSkPicture
DeferredImageDecoderTest.frameOpacity

which use a PNG image, which is why we did not catch this before now. Using a
similar GIF fails frameOpacity - when we try to correct the alpha value,
originalFrameRect does not cover the full image, so we continue to assume the
frame has alpha.

Rather than working around the problem, there is a better fix - make
setMemoryAllocator (/its caller) call frameCount() rather than manually
resizing.

I've made the change in the next patch set, but it is probably worth landing as
a separate change.

[Noel Gordon, 2017/02/27 07:45:31]

OK good, we have tests.
Just in passing, it looks to me like the GIF decoder's alpha tracking is
incorrect - it set's the frame status to FrameComplete _and then updates the
frame alpha state_.  Same bug I saw in this new PNG code.
OK, sounds better, I will look at the fix when we get there.

[scroggo_chromium, 2017/02/27 20:31:05]

Agreed - GIF decoder should update its alpha state before setting its status to
FrameComplete, both logically, and because notifyPixelsChanged will not get
called after the pixels have changed.

I'm not sure that makes a difference in practice, though. notifyPixelsChanged
does a few things:

- call onNotifyPixelsChanged, a virtual method that does nothing and currently
has no overrides
- set the gen ID to 0, so the next time it is requested, a new ID is created.
The gen ID should not be requested in between the two calls, so for its purposes
it should not matter which order they happened in.
- notify listeners - this seems to tell caches to boot out the old gen ID

What would be dangerous is if we called notifyPixelsChanged, then someone else
tried to draw the bitmap (calling getGenerationID to get a newly generated ID),
and *then* we changed the alpha state, but I don't think that can happen.

I have fixed the bug in GIF (but it does not fix the bug in frameOpacity).

[Noel Gordon, 2017/03/01 09:19:35]

Nod.
Yes, I don't think it happens.  Less to worry though if we stick to the simple
rule: set the alpha you want and then set FrameComplete.

That happens in the this PNG code now, and in the GIF code too with your change,
all looks fine to me.
Odd.  I checked out this CL, it has the GIF fix, and merged it into ToT Chrome
on OSX, built and ran the tests, and frameOpacity passes for me.

I tried the same on Windows. After fixing a few compilation bugs, same result:
frameOpacity passes for me.

Not sure why it's not passing for you.  Anyho, I think it'd help here if you
synced your client up to ToT, merged in any changes, upload and send the result
to the try bots to iron out any compilation problems.

With that, I could patch the CL in with ease, and could re-test frameOpacity to
ensure I'm testing the same thing as you.  Possible?

[scroggo_chromium, 2017/03/03 21:56:40]

A separate change fixes frameOpacity, which is also in this CL, so it does pass
for me. Without the changes to ImageFrameGenerator/ImageDecoder around
setMemoryAllocator, my updated frameOpacity test fails.
I can sync up to ToT. FWIW, I like to do that as infrequently as possible during
a review, since it can create extra work (rebasing, recompiling etc) that could
otherwise be done only once. It can also make it hard to see what I changed, but
I've made the rebase its own patch set.

[Noel Gordon, 2017/03/06 03:06:45]

OK, the "separate change" change can be landed here since you have included it,
or as a separate change, whichever you prefer.
OK thanks, the bots do have various compilation errors for that rebase patch set
so there's still work to do there.

+    initializeNewFrame(0);
 
-    unsigned colorChannels = m_reader->hasAlpha() ? 4 : 3;
-    if (PNG_INTERLACE_ADAM7 ==
-        png_get_interlace_type(png, m_reader->infoPtr())) {
-      m_reader->createInterlaceBuffer(colorChannels * size().width() *
-                                      size().height());
-      if (!m_reader->interlaceBuffer()) {
-        longjmp(JMPBUF(png), 1);
-        return;
-      }
-    }
+  if (!initFrameBuffer(m_currentFrame))

[Noel Gordon, 2017/02/21 13:53:54]

First issue: code in diff-left short circuited real work here by calling

if (buffer.getStatus() == ImageFrame::FrameEmpty) {
   ...
}

so it was done once per image frame.  Now we have

  if (!initFrameBuffer(m_currentFrame))

being called to do real work on _each row_ of an image frame.  Sadness w.r.t to
PNG decode perf: is there some way to FIXME this?

[scroggo_chromium, 2017/02/23 22:09:54]

What is the real work you're concerned about here? The function call?
(initFrameBuffer makes the same check to short circuit.) We could avoid that by
modifying this line to say

  if (buffer.getStatus() == ImageFrame::FrameEmpty
      && !initFrameBuffer(m_currentFrame))

Another possibility - call initFrameBuffer before we ever start processing
IDAT/fdAT. Then we can even

  DCHECK_EQ(FramePartial, buffer.getStatus());

With this approach, we'll need to catch the empty frame a different way.
Currently, frameComplete checks the Status - if it's FrameEmpty, we know
rowAvailable was never called. Instead, we can set a boolean for whether
rowAvailable has been called. (We can even take that further and keep track of
the rows decoded to know whether the image is truly complete, which would assist
in fixing the last three images in [1], though it deviates from the current
behavior for static PNGs.)

This also addresses the issue brought up elsewhere, that initFrameBuffer may
delete the reader.

WDYT?

[1] https://philip.html5.org/tests/apng/tests.html

[scroggo_chromium, 2017/02/24 14:57:25]

I moved initFrameBuffer into decode in patch set 8. This follows
WEBPImageDecoder's lead, and means rowAvailable does not need to do any work for
initializing the frame.

[Noel Gordon, 2017/02/27 07:45:31]

Right.  So you changed initFrameBuffer to not call setFailed() anymore (that's
good), the so the caller now has to do the error handling, so we get:

if (buffer.getStatus() == ImageFrame::FrameEmpty) {
  if (!initFrameBuffer(m_currentFrame))
    longjmp(JMPBUF(m_reader->pngPtr()), 1);
}

Given that, then there is little reason for PNGImageDecoder::onInitFrameBuffer
to even exist.  You could grab it's body and put it all herein.

if (buffer.getStatus() == ImageFrame::FrameEmpty) {
  if (!initFrameBuffer(m_currentFrame))
    longjmp(JMPBUF(m_reader->pngPtr()), 1);

  + what onInitFrameBuffer was doing
}

and we could remove PNGImageDecoder::onInitFrameBuffer, right?

Also to catch the empty frame case in frameComplete(), you can check for
FrameEmpty as you were doing, no need to add a new variable to track that case.

[scroggo_chromium, 2017/02/27 20:31:05]

Yes.
Agreed. The new variable is only used/necessary in patch set 8, which takes a
different approach to the initFrameBuffer problems.

[Noel Gordon, 2017/03/01 09:26:49]

Acknowledged.

+    longjmp(JMPBUF(m_reader->pngPtr()), 1);

[Noel Gordon, 2017/02/21 13:53:54]

Second issue here: initFrameBuffer() calls onInitFrame(), and the latter can
fail and call setFailed() as I noted earlier, right?

That means m_reader is NULL here and we crash the renderer via
m_reader->pngPtr() == NULL->pngPtr().

You have to save the JUMBUF locally before calling initFrameBuffer(), and use
that saved JMPBUF to longjmp() out if you want to avoid a crash.  Even so, this
would again have some perf impact: is there some way to FIXME this?

[scroggo_chromium, 2017/02/23 22:09:52]

Some options, suggested in above responses:
- stop calling setFailed in initFrameBuffer (as is done in the next patch set)
- call initFrameBuffer outside of rowAvailable

WDYT?

[Noel Gordon, 2017/02/27 07:45:31]

good
see above, have rowAvailable handle all the frame init stuff in a FrameEmpty
short-circuit.

[scroggo_chromium, 2017/02/27 20:31:05]

Sounds fine to me. Done in the latest patch set.

[Noel Gordon, 2017/03/01 09:26:49]

Ok good.  nits: use size.area() and move the colorChannels local inside the if
statement ?

 if (PNG_INTERLACE_ADAM7 ==
     png_get_interlace_type(png, m_reader->infoPtr())) {
   unsigned colorChannels = m_reader->hasAlpha() ? 4 : 3;
   m_reader->createInterlaceBuffer(colorChannels * size().area());	
   if (!m_reader->interlaceBuffer()) {	
      longjmp(JMPBUF(png), 1);	
      return;	
    }	
  }

[scroggo_chromium, 2017/03/03 21:56:40]

I reverted back to the original code, since after removing onInitFrameBuffer I
wasn't really changing this piece.

Done in next patch set.

[Noel Gordon, 2017/02/21 13:53:55]

Third issue here: diff-left calls buffer.setHasAlpha(false);  I did not see that
call anywhere through here or in the functions called.   Did I miss it, or do we
not need to call it anymore?

[scroggo_chromium, 2017/02/23 22:09:53]

Alpha is tracked differently in the multi-frame code. The code on the left calls
setHasAlpha(false) at the beginning, and then setHasAlpha(true) if a non-opaque
pixel is seen. This means that while the frame's status is FramePartial,
m_hasAlpha is false, although the SkBitmap still has an alpha type that
recognizes that there is alpha (since the image has not been completely
decoded).

In the new code, m_hasAlpha remains true (set in zeroFillFrameRect), which
matches the SkBitmap. After the frame is completely decoded, m_hasAlpha may be
set to false in correctAlphaWhenFrameBufferSawNoAlpha, which also takes into
account the frame rect and the frame that the current frame depends on. This
behavior matches GIF and allows us to share code.

Either way, we end up with the correct value once the frame is FrameComplete
(almost - see [8]). But there is a difference, as you point out - while an
ImageFrame is FramePartial, its m_hasAlpha (and therefore hasAlpha()) returns
true, whereas the old code returned false (if a transparent pixel has not yet
been seen). It is not obvious to me why the old code is desirable. The
setHasAlpha(false) after zeroFillFrameRect was introduced here [1] (EDIT: Can be
traced further back to [7]), at which point it was the same in other decoders
(e.g. JPEG). But JPEG was changed later to fix a bug [2] (it now calls
setHasAlpha(true); that is not actually necessary, since this already happens in
zeroFillFrameRect). (Without digging too much into each case, note that the rest
of the decoders make different decisions about what to do during FramePartial:

BMP - set to false, like current PNG
GIF - leave alone, so it remains true
WEBP - set to true
.)

In today's code, I don't think there's a difference. m_hasAlpha appears to only
be read (besides for the purpose of copying) when the status is FrameComplete.
e.g.

- computeAlphaType [3]
- WebGLImageConversion::ImageExtractor::extractImage (by way of hasAlpha) [4]

ImageDecoder::frameHasAlphaAtIndex [5] is an interesting case. It is only called
by DeferredImageDecoder [6], which calls it on m_actualDecoder. m_actualDecoder
is never used for decoding, so the default version of frameIsCompleteIndex
(which just checks whether the status is FrameComplete) always returns false (no
decoding means the frame will always be FrameEmpty). So for the existing
PNGImageDecoder (which does not override frameIsCompleteAtIndex), no callers
know the difference between true or false m_hasAlpha during FramePartial.
(Taking that a step further, that call to hasAlpha is also on m_actualDecoder.
Now that frameIsCompleteAtIndex may return true for an animated PNG, hasAlpha
might be called. But again, since m_actualDecoder is not used for decoding,
hasAlpha will return its initial value of true.)

[1]
https://chromium.googlesource.com/chromium/src/+/f09863db44eed54d690e52815adb...
[2]
https://chromium.googlesource.com/chromium/src/+/08f8f9ac56eb0d6bf6d876e2dffd...
[3]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image...
[4]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...
[5]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image...
[6]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...

[7]
https://chromium.googlesource.com/chromium/src/+/d963d0f9e6daae10178b3c30e008...
From that CL's description:

        Various minor cleanups to the Skia files.  Mostly non-functional, except
        for two specific changes:
        * JPEGs and PNGs were always marked as transparent; now they are only
          marked as transparent when they actually are.  I doubt this has much
          of an effect but in theory it could be used to optimize their display.

This does not lead me to think this is an important feature to preserve.

[8] The old code actually has a bug. Since we mark the ImageFrame as *not*
having alpha, and then only correct it if we find alpha, we can get into a
situation where we've left the ImageFrame thinking it's opaque when we still
have the transparent pixels from the zeroFillFrameRect call.
https://philip.html5.org/tests/apng/058.png is a good example of this. The IDAT
is incomplete, so we only decode the first 32 out of 64 rows - the last 32 are
still transparent.

And, now that I think about it some more, I realize that I have retained this
bug. libpng does not care that not all of the rows were in the IDAT (depending
on the state of the zstream, libpng may report a warning or an error, but
neither happens in this case), so we "correct" m_hasAlpha to false after we did
not see any transparent pixels.

[Noel Gordon, 2017/02/27 07:45:31]

modulo the bug in the GIF code, this paragraph describes the correct behavior.
 
The new code returns the correct value, right? 

 The
The fix in [2] is correct, and the setHasAlpha(true) is redundant but safe.  The
change log describes what can happen when we get the alpha wrong.  The logic of
the change in [2] applies to all image decoder types while
we are decoding their frames.
 
So the result is we do report the correct alpha state, even though the decoders
you listed above seem all over the place wrt to setting initial alpha during
frame initialization.  They could be cleaned up.  Main point here is

"m_hasAlpha appears to only be read (besides for the purpose of copying) when
the status is FrameComplete."
nod.
nod.

They are both correct.
 
m_actualDecoder is used for decoding the image header on the renderer main
thread, but when we decide to decode the image off the main thread,
m_actualDecoder gets deleted IIRC, and the image frame generator is asked for
frame state.  Is the alpha state is being incorrectly reported from a frame
generator?  That seems like a DeferredImageDecoder issue if so.
alpha state detection you mean?
 
That's correct, the too few IDAT rows problem ...
... could report the wrong alpha, but the too few IDAT rows case is also an
invalid APNG which we should not be decoding, right?

[scroggo_chromium, 2017/02/27 20:31:05]

Could you be more specific? Once the frame is complete, both the new and the old
code return the correct value (again - except in the case of [8]).

While the frame is FramePartial, it's not entirely clear what "correct" is.
hasAlpha and setHasAlpha do not provide any documentation for what they mean,
but I could imagine two interpretations:

a) Report/set whether the ImageFrame has alpha. This is true if the portion of
the image that has been decoded has alpha OR if some rows have not been decoded
and therefore they are still transparent.
b) Report/set whether the portion of the image decoded so far has alpha. This
ignores any rows that have not yet been decoded.

The decoder implementations do not agree on the interpretation to use.
Agreed. By the same logic, my change here is correct.
(Yes. My point here is that when choosing between (a) and (b) above, there is no
difference for most callers.)
m_actualDecoder does not get deleted until all the data has been received:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...

Until then, DeferredImageDecoder gets its information from m_actualDecoder.
For the purposes of this discussion, I'm concerned about the case where the data
is not all received. (As it happens, we do have issues around the opacity once
the image is fully received :( See crbug.com/593430.)
Setting m_hasAlpha to false until a pixel is seen with alpha (i.e.
interpretation (b)). Your original question is essentially "what happened to
buffer.setHasAlpha(false)?" I've tried to explain that though the new code is
different, it changes the interpretation to (a), which preserves the interesting
bit - reporting the proper alpha state when the image is complete.
According to https://philip.html5.org/tests/apng/tests.html, yes. I do not see
anything in the specification (https://wiki.mozilla.org/APNG_Specification), but
it does seem reasonable to state that fewer IDAT rows than the number of rows
reported by the frame is an error, and per the spec we should not decode.

OTOH, this same exact case also happens for a static PNG image. libpng does not
report it as an error, and neither does today's Chromium code. I have not dug
into the history here - maybe this is an oversight, and maybe this is on
purpose.

(I drew attention to this in the CL description:
"""
- The final three images draw incorrectly. They have incorrectly sized IDATs/
fdATs. These could be respected by checking the warning that libpng sends,
although we currently ignore this for static PNGs anyway.
"""
although I was not quite correct - libpng does not always send a warning in the
case of too few rows, so we'd need to check ourselves, unless we modify libpng.)

This CL attempts to keep the behavior for decoding static PNG images unchanged.
Since this is a similar issue for static and animated images, I went the same
way for animated ones. But we could treat the two cases differently, or we could
go ahead and change the behavior for static PNGs, too. 

The third to last image in the test suite is particularly interesting - the fcTL
tells us that the default image does not fill the image. So we should either
treat it as a static PNG, or report a failure. In this case, the fcTL is
"correct" - the IDAT is truncated (it only has the number of rows reported by
the fcTL chunk), but libpng does not appear to mind. So if we revert to drawing
the default image, we run into the same problem.

[Noel Gordon, 2017/03/01 11:29:46]

Yeap, sorry, I was replying to this part:

"But there is a difference, as you point out - while an ImageFrame is
FramePartial, its m_hasAlpha (and therefore hasAlpha()) returns true, whereas
the old code returned false (if a transparent pixel has
not yet been seen).  It is not obvious to me why the old code is desirable."

The old PNG was single frame, and to find out about frame alpha since there is
no direct public access to frame.hasAlpha() for external users, they have to
call frameHasAlphaAtIndex().  That returns true until frameIsCompleteAtIndex(),
and thereafter returns frame[0].hasAlpha().  Seems correct to me.

In the new APNG code, how does frameHasAlphaAtIndex() work?  Since
frameIsCompleteAtIndex() is been overridden and now returns "the frame fully
received" if the index > 0 (moulo any bugs in it), and that means
frame[index].hasAlpha() could be returned.  The frame's state is either
<= FramePartial or FrameComplete.  The return is true now while not
FrameComplete, and the actual frame alpha for FrameComplete.  Seems correct to
me.

To answer my own question ...
... seems like the answer is yes.
Indeed correct, and by the same logic, frames with state <= framePartial have
alpha, period.  The correct answer to your question about possible
interpretations is a).  The code mentions this in a few places, ImageFrame.cpp
mentions it using JPEG as the example.
Yes, this is a HTML spec requirement.  The initial (index == 0) frame of a
static image, or animated image, can be drawn by canvas elements (canvas 2d and
WebGL).  They check FrameComplete status before they attempt to draw, and fail
the draw if FrameComplete is not true.

This means that an overridden frameIsCompleteAtIndex(index) in the new PNG
decoder needs to do the right thing for index == 0, and return the decode
complete status, not whether the frame is fully received.

I'm not sure the frameIsCompleteAtIndex() code here is doing the right thing
yet.  A layout test (draw an animated PNG to <canvas>) might help sort that out.
Ah yes, I forgotten that m_actualDecoder needs to hang around longer.
(yes, the deferred chicken+egg bug crbug.com/593430).
 
Nothing to worry about here.  BitmapImage explains that in it public API
currentFrameKnownToBeOpaque() http://bit.ly/2lbsEBT

"frameHasAlphaAtIndex() conservatively returns false" is a roundabout way of
saying that m_actualDecoder will in the normal case, always return true to this
request.  Make sense to me since the callers of currentFrameKnownToBeOpaque()
are all on the main thread.

So no issues with m_actualDecoder being used, which was your concern above,
since that is expected behavior for a BitmapImage in the age of deferred
(non-main-thread) image decoding.
Right, got it.
Sounds correct to me: no need for the buffer.setHasAlpha(false) anymore.
Yeah these cases seems difficult: might be a way to fix them, libpng might have
a #define to enable more reporting, but maybe not worth chasing if the result is
we change behavior for static PNG.

[scroggo_chromium, 2017/03/03 21:56:40]

Happy to provide a layout test. Is there a particular concern you have with the
code? And what do you have in mind? Something like:
- provide enough data for part of the second frame, verify that we still draw
the first frame?
Let's take them individually:
a) Too many rows:
We just need to catch the warnings here [1][2] and consider it a failure. We
know whether we're dealing with a static PNG or an animated one, so we could
behave differently if we want to.
[1]
https://cs.chromium.org/chromium/src/third_party/libpng/pngpread.c?q=pngpread...
[2]
https://cs.chromium.org/chromium/src/third_party/libpng/pngpread.c?q=pngpread...

b) Too few rows:
This is trickier, but not terribly hard. In some cases, libpng will report a
warning [3] or an error [4] if there is not enough IDAT data, but note that in
the latter case, it will silently move on to the next chunk if the
PNG_FLAG_ZSTREAM_ENDED flag is set. (I have not found any #defines that made a
difference here.) In order to handle this properly, we would need to keep track
of the rows received, and if we do not get enough, signal an error. Again, we
could handle it differently depending on whether it's a static PNG or not. But
the code would likely be ugly, especially for interlaced images.

[3]
https://cs.chromium.org/chromium/src/third_party/libpng/pngpread.c?q=pngpread...
[4]
https://cs.chromium.org/chromium/src/third_party/libpng/pngpread.c?q=pngpread...

[Noel Gordon, 2017/03/06 03:06:45]

scroggo_chromium wrote:
Suggest you push these cases off into another bug, as I expect that that cases
will take some time and fiddling to get right.

[Noel Gordon, 2017/03/06 03:06:45]

Yes, and I filed crbug.com/698487 about it, PTAL.

[scroggo_chromium, 2017/03/07 20:25:36]

Filed https://bugs.chromium.org/p/chromium/issues/detail?id=698808

[Noel Gordon, 2017/03/08 15:36:01]

Yeap looked, good description of the issue therein, thanks for filing the bug.

-    buffer.setStatus(ImageFrame::FramePartial);
-    buffer.setHasAlpha(false);
-
-    // For PNGs, the frame always fills the entire image.
-    buffer.setOriginalFrameRect(IntRect(IntPoint(), size()));
-  }
+  ImageFrame& buffer = m_frameBufferCache[m_currentFrame];
+  const IntRect& frameRect = buffer.originalFrameRect();

[Noel Gordon, 2017/02/21 13:53:55]

There is a nice DCHECK you used elsewhere to check that frameRect is contained
in the image size().  Would it make sense to add it here, to remind readers that
frameRect has been clipped to the image size()?

[scroggo_chromium, 2017/02/23 22:09:53]

Done.

 
   /* libpng comments (here to explain what follows).
-     *
-     * this function is called for every row in the image. If the
-     * image is interlacing, and you turned on the interlace handler,
-     * this function will be called for every row in every pass.
-     * Some of these rows will not be changed from the previous pass.
-     * When the row is not changed, the new_row variable will be NULL.
-     * The rows and passes are called in order, so you don't really
-     * need the row_num and pass, but I'm supplying them because it
-     * may make your life easier.
-     */
+   *
+   * this function is called for every row in the image. If the
+   * image is interlacing, and you turned on the interlace handler,
+   * this function will be called for every row in every pass.
+   * Some of these rows will not be changed from the previous pass.
+   * When the row is not changed, the new_row variable will be NULL.
+   * The rows and passes are called in order, so you don't really
+   * need the row_num and pass, but I'm supplying them because it
+   * may make your life easier.
+   */
 
-  // Nothing to do if the row is unchanged, or the row is outside
-  // the image bounds: libpng may send extra rows, ignore them to
-  // make our lives easier.
+  // Nothing to do if the row is unchanged, or the row is outside the image

[Noel Gordon, 2017/02/21 13:53:55]

// Nothing to do if the row is unchanged, or the row is outside the image
// bounds. In the case that a frame presents more data than the indicated
// frame size, ignore the extra rows and use the frame size as the source
// of truth. libpng can send extra rows: ignore them too, this to prevent
// memory writes outside of the image bounds (security).

[scroggo_chromium, 2017/02/23 22:09:54]

Done.

+  // bounds.  In the case that a frame presents more data than the indicated
+  // frame size, ignore the extra rows and thus use the frame size as the
+  // source of truth. libpng may also send extra rows. Ignore those as well.
+  // This prevents us from trying to write outside of the image bounds.
   if (!rowBuffer)
     return;
-  int y = rowIndex;
-  if (y < 0 || y >= size().height())
+  int y = rowIndex + frameRect.y();
+  DCHECK_GE(y, 0);
+  if (y >= size().height())
     return;
 
   /* libpng comments (continued).
-     *
-     * For the non-NULL rows of interlaced images, you must call
-     * png_progressive_combine_row() passing in the row and the
-     * old row.  You can call this function for NULL rows (it will
-     * just return) and for non-interlaced images (it just does the
-     * memcpy for you) if it will make the code easier. Thus, you
-     * can just do this for all cases:
-     *
-     *    png_progressive_combine_row(png_ptr, old_row, new_row);
-     *
-     * where old_row is what was displayed for previous rows. Note
-     * that the first pass (pass == 0 really) will completely cover
-     * the old row, so the rows do not have to be initialized. After
-     * the first pass (and only for interlaced images), you will have
-     * to pass the current row, and the function will combine the
-     * old row and the new row.
-     */
+   *
+   * For the non-NULL rows of interlaced images, you must call
+   * png_progressive_combine_row() passing in the row and the
+   * old row.  You can call this function for NULL rows (it will
+   * just return) and for non-interlaced images (it just does the
+   * memcpy for you) if it will make the code easier. Thus, you
+   * can just do this for all cases:
+   *
+   *    png_progressive_combine_row(png_ptr, old_row, new_row);
+   *
+   * where old_row is what was displayed for previous rows. Note
+   * that the first pass (pass == 0 really) will completely cover
+   * the old row, so the rows do not have to be initialized. After
+   * the first pass (and only for interlaced images), you will have
+   * to pass the current row, and the function will combine the
+   * old row and the new row.
+   */
 
-  bool hasAlpha = m_reader->hasAlpha();
+  bool hasAlpha = m_hasAlphaChannel;
   png_bytep row = rowBuffer;
 
   if (png_bytep interlaceBuffer = m_reader->interlaceBuffer()) {
     unsigned colorChannels = hasAlpha ? 4 : 3;
     row = interlaceBuffer + (rowIndex * colorChannels * size().width());
     png_progressive_combine_row(m_reader->pngPtr(), row, rowBuffer);
   }
 
   // Write the decoded row pixels to the frame buffer. The repetitive
   // form of the row write loops is for speed.
-  ImageFrame::PixelData* const dstRow = buffer.getAddr(0, y);
+  ImageFrame::PixelData* const dstRow = buffer.getAddr(frameRect.x(), y);
   unsigned alphaMask = 255;
-  int width = size().width();
+  int width = frameRect.width();
 
   png_bytep srcPtr = row;
   if (hasAlpha) {
     // Here we apply the color space transformation to the dst space.
     // It does not really make sense to transform to a gamma-encoded
     // space and then immediately after, perform a linear premultiply.
     // Ideally we would pass kPremul_SkAlphaType to xform->apply(),
     // instructing SkColorSpaceXform to perform the linear premultiply
     // while the pixels are a linear space.
     // We cannot do this because when we apply the gamma encoding after
     // the premultiply, we will very likely end up with valid pixels
     // where R, G, and/or B are greater than A.  The legacy drawing
     // pipeline does not know how to handle this.
     if (SkColorSpaceXform* xform = colorTransform()) {
       SkColorSpaceXform::ColorFormat colorFormat =
           SkColorSpaceXform::kRGBA_8888_ColorFormat;
       xform->apply(colorFormat, dstRow, colorFormat, srcPtr, size().width(),
                    kUnpremul_SkAlphaType);
       srcPtr = (png_bytep)dstRow;

[Noel Gordon, 2017/02/21 13:53:55]

nit and not your doing, mind: C-style cast not allowed per blink style.

[scroggo_chromium, 2017/02/23 22:09:53]

Addressed in crrev.com/2716533002

     }
 
-    if (buffer.premultiplyAlpha()) {
-      for (auto *dstPixel = dstRow; dstPixel < dstRow + width;
-           dstPixel++, srcPtr += 4) {
-        buffer.setRGBAPremultiply(dstPixel, srcPtr[0], srcPtr[1], srcPtr[2],
-                                  srcPtr[3]);
-        alphaMask &= srcPtr[3];
+    if (m_frameBufferCache[m_currentFrame].getAlphaBlendSource() ==
+        ImageFrame::BlendAtopBgcolor) {
+      if (buffer.premultiplyAlpha()) {
+        for (auto *dstPixel = dstRow; dstPixel < dstRow + width;
+             dstPixel++, srcPtr += 4) {
+          buffer.setRGBAPremultiply(dstPixel, srcPtr[0], srcPtr[1], srcPtr[2],
+                                    srcPtr[3]);
+          alphaMask &= srcPtr[3];
+        }
+      } else {
+        for (auto *dstPixel = dstRow; dstPixel < dstRow + width;
+             dstPixel++, srcPtr += 4) {
+          buffer.setRGBARaw(dstPixel, srcPtr[0], srcPtr[1], srcPtr[2],
+                            srcPtr[3]);
+          alphaMask &= srcPtr[3];
+        }
       }
     } else {
-      for (auto *dstPixel = dstRow; dstPixel < dstRow + width;
-           dstPixel++, srcPtr += 4) {
-        buffer.setRGBARaw(dstPixel, srcPtr[0], srcPtr[1], srcPtr[2], srcPtr[3]);
-        alphaMask &= srcPtr[3];
+      // Now, the blend method is ImageFrame::BlendAtopPreviousFrame. Since the
+      // frame data of the previous frame is copied at initFrameBuffer, we can
+      // blend the pixel of this frame, stored in |srcPtr|, over the previous
+      // pixel stored in |dstPixel|.
+      if (buffer.premultiplyAlpha()) {
+        for (auto *dstPixel = dstRow; dstPixel < dstRow + width;
+             dstPixel++, srcPtr += 4) {
+          buffer.blendRGBAPremultiplied(dstPixel, srcPtr[0], srcPtr[1],
+                                        srcPtr[2], srcPtr[3]);
+          alphaMask &= srcPtr[3];
+        }
+      } else {
+        for (auto *dstPixel = dstRow; dstPixel < dstRow + width;
+             dstPixel++, srcPtr += 4) {
+          buffer.blendRGBARaw(dstPixel, srcPtr[0], srcPtr[1], srcPtr[2],
+                              srcPtr[3]);
+          alphaMask &= srcPtr[3];
+        }
       }
     }
+
+    if (alphaMask != 255 && !m_currentBufferSawAlpha)
+      m_currentBufferSawAlpha = true;
+
   } else {
     for (auto *dstPixel = dstRow; dstPixel < dstRow + width;
          dstPixel++, srcPtr += 3) {
       buffer.setRGBARaw(dstPixel, srcPtr[0], srcPtr[1], srcPtr[2], 255);
     }
 
     // We'll apply the color space xform to opaque pixels after they have been
     // written to the ImageFrame, purely because SkColorSpaceXform supports
     // RGBA (and not RGB).
     if (SkColorSpaceXform* xform = colorTransform()) {
       xform->apply(xformColorFormat(), dstRow, xformColorFormat(), dstRow,
                    size().width(), kOpaque_SkAlphaType);
     }
   }
 
-  if (alphaMask != 255 && !buffer.hasAlpha())
-    buffer.setHasAlpha(true);
-
   buffer.setPixelsChanged(true);
 }
 
+bool PNGImageDecoder::frameIsCompleteAtIndex(size_t index) const {
+  if (!m_reader)
+    return false;
+
+  // For non-animated images, return whether the status of the frame is
+  // ImageFrame::FrameComplete with ImageDecoder::frameIsCompleteAtIndex.
+  // This matches the behavior of WEBPImageDecoder.
+  if (index == 0 && m_reader->parseCompleted() && m_reader->frameCount() == 1)

[Noel Gordon, 2017/02/21 13:53:55]

Comparisons to 0: use !index.

if (!index && m_reader->parseCompleted() && ...
  return ImageDecoder::frameIsCompleteAtIndex(index);

The next sentence I read is "// For first frames of animated images," but I
think the if above can be false and so we can fall into the code below too, aka
for normal PNG images.  Any problems with that, should I worry?

[scroggo_chromium, 2017/02/23 22:09:53]

Good point. I see two ways this condition could return false for a normal PNG
image:
- index > 0 for a normal PNG image
  - solution: drop the check for index, which is handled by ImageDecoder anyway
- we have not yet parsed the size
  - solution: check for isDecodedSizeAvailable. If false, the frame is not
complete.
    I also had to rearrange PNGImageReader::parse to ensure that once the size
is
    available in a static PNG m_parseCompleted is set to true.

Are there others I have missed?
I generally worry about this method. Special-casing a static image matches the
behavior of WEBP (mostly - IIUC, WEBP will treat a single frame but animated
image the same as an animated image; I don't know whether that is possible for
WEBP, but APNG can have an acTL chunk and an fcTL chunk but only one frame), and
the existing code for PNG. But it is different from GIF, which treats a single
frame image the same as an animated one. (Note that in APNG, unlike GIF, there
is no need to look ahead to see if there are more frames, since there either was
no acTL or it indicated one frame. So in order to know where the end of the
frame is, we would have to parse ahead just to find it, whereas GIF learns this
info on the way to looking for more frames.)

If we drop the if above (or missed a case), PNG will behave no differently for
some clients (assuming I initialize m_frameInfo[0].byteLength, which I do in the
latest patch set). It will always return false, the same as when it is called by
DeferredImageDecoder [1][2], where the status will never be FrameComplete,
because m_actualDecoder was never used to decode.


[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...
[2]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...

[Noel Gordon, 2017/02/27 07:45:31]

I expect that the following

bool PNGImageDecoder::frameIsCompleteAtIndex(size_t index) const {
  return ImageDecoder::frameIsCompleteAtIndex(index)
}

would still work.  Your image will animate, etc?

Now, I have no clue why animated image types (GIF say) require this routine to
become "frame is fully received at index", and thus all the
firstFrameFullyReceived() code that resulted here.
  

[scroggo_chromium, 2017/02/27 20:31:05]

This method was originally intended to mean "frame is fully received at index",
*not* "frame's status is FrameComplete at index". It was introduced in [1].
Although it was called by ImageSource::frameIsCompleteAtIndex, took the place of
code that checks for FrameComplete, and has a name that sure sounds like it does
that, the intent of [1] was to "know" that a frame was complete (/should be
complete) *without decoding*. In fact, hclam@ didn't even expect/plan for it to
be called for non-animated images [2], and the default implementation just
returned false. Pushback from pkasting@ convinced hclam@ to provide a default
implementation, though it was originally to return isAllDataReceived(), rather
than FrameComplete-ness [3].

So returning "frame is fully received at index" follows the original intent of
the method. This code even more closely follows the WEBP implementation [4],
which treats static and animated images differently (at your suggestion [5]).

Looking at these old CLs, both you[6] and pkasting@[7] seemed to have concerns
about frameIsCompleteAtIndex, but nothing has been done to clean it up. (I
encouraged aleksandar.stojiljkovic@ to split apart the notions of complete
versus received when he was modifying code that called frameIsCompleteAtIndex,
and he started [8] but ultimately dropped it.) 

[1] https://codereview.chromium.org/14317005
[2]
https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/image...
[3] https://codereview.chromium.org/14317005/#ps23001
[4]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image...
[5]
https://chromiumcodereview.appspot.com/13980003/diff/125001/Source/core/platf...
[6] https://chromiumcodereview.appspot.com/13980003#msg36
[7]
https://codereview.chromium.org/14317005/diff/23001/Source/core/platform/imag...
[8] https://chromiumcodereview.appspot.com/1962563002/

+    return ImageDecoder::frameIsCompleteAtIndex(index);
+
+  // For first frames of animated images, PNGImageReader exposes whether it is
+  // fully received through firstFrameFullyReceived().  Non-first frames are

[Noel Gordon, 2017/02/21 13:53:55]

Let's break this comment into two parts.

  // For first frames ...
  // fully received through firstFrameFullyReceived().
  if (!index)
     return m_reader.firstFrameFullyReceived();

  // Non-first frames are reported by |m_reader| ...
  // ... below the frame count.
  return index < m_reader->frameCount();
  
   

[scroggo_chromium, 2017/02/23 22:09:53]

Done.

+  // reported by |m_reader| once it has parsed all data for that frame, so we
+  // can simply return if the index is below the reported frame count.
+  if (index == 0)
+    return m_reader->firstFrameFullyReceived();
+  return (index < m_reader->frameCount());
+}
+
+float PNGImageDecoder::frameDurationAtIndex(size_t index) const {
+  return (index < m_frameBufferCache.size()

[Noel Gordon, 2017/02/21 13:53:55]

This ternary if statement is unclear due to 80-column line-limit foo-bar-ness:
make it 

  if (index < m_frameBufferCache.size())
    return m_frameBufferCache[index].duration();
  return 0;

[scroggo_chromium, 2017/02/23 22:09:52]

Done.

+              ? m_frameBufferCache[index].duration()
+              : 0);
+}
+
 void PNGImageDecoder::complete() {

[Noel Gordon, 2017/02/21 13:53:54]

complete -> frameComplete

[scroggo_chromium, 2017/02/23 22:09:53]

Done.

-  if (m_frameBufferCache.isEmpty())
+  if (m_currentFrame >= m_frameBufferCache.size())
     return;
 
-  m_frameBufferCache[0].setStatus(ImageFrame::FrameComplete);
-}
+  if (m_reader->interlaceBuffer())
+    m_reader->clearInterlaceBuffer();
 
-inline bool isComplete(const PNGImageDecoder* decoder) {
-  return decoder->frameIsCompleteAtIndex(0);
-}
+  ImageFrame& buffer = m_frameBufferCache[m_currentFrame];
+  if (buffer.getStatus() == ImageFrame::FrameEmpty)
+    longjmp(JMPBUF(m_reader->pngPtr()), 1);
 
-void PNGImageDecoder::decode(bool onlySize) {
-  if (failed())
-    return;
+  buffer.setStatus(ImageFrame::FrameComplete);

[Noel Gordon, 2017/02/21 13:53:55]

This latches the decoded image frame alpha into the underlying SkBitmap backing
the decoded frame ...

 
-  if (!m_reader)
-    m_reader = WTF::makeUnique<PNGImageReader>(this, m_offset);
-
-  // If we couldn't decode the image but have received all the data, decoding
-  // has failed.
-  if (!m_reader->decode(*m_data, onlySize) && isAllDataReceived())
-    setFailed();
-
-  // If decoding is done or failed, we don't need the PNGImageReader anymore.
-  if (isComplete(this) || failed())
-    m_reader.reset();
+  if (!m_currentBufferSawAlpha)
+    correctAlphaWhenFrameBufferSawNoAlpha(m_currentFrame);

[Noel Gordon, 2017/02/21 13:53:55]

This code can change the image frame alpha state by the looks, and since we have
already called buffer.setStatus(ImageFrame::FrameComplete), the SkBitmap backing
will never be told, it seems to me.

Perhaps do this code first, and do the ImageFrame::FrameComplete step last.

[scroggo_chromium, 2017/02/23 22:09:54]

Done.

 }
 
 }  // namespace blink