Add support for Animated PNG

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

Index: third_party/WebKit/Source/platform/image-decoders/png/PNGImageReader.cpp
diff --git a/third_party/WebKit/Source/platform/image-decoders/png/PNGImageReader.cpp b/third_party/WebKit/Source/platform/image-decoders/png/PNGImageReader.cpp
index 1dad2dfa390be727ea58096df995693eda511df7..43184c4f5759f929b0a6f0492a4d478451a2d239 100644
--- a/third_party/WebKit/Source/platform/image-decoders/png/PNGImageReader.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/png/PNGImageReader.cpp
@@ -38,11 +38,12 @@
 
 #include "platform/image-decoders/png/PNGImageReader.h"
 
+#include <memory>
+#include "platform/image-decoders/FastSharedBufferReader.h"
 #include "platform/image-decoders/SegmentReader.h"
 #include "platform/image-decoders/png/PNGImageDecoder.h"
-#include "png.h"
 #include "wtf/PtrUtil.h"
-#include <memory>
+#include "zlib.h"
 
 namespace {
 
@@ -61,8 +62,8 @@ void PNGAPI pngRowAvailable(png_structp png,
   imageDecoder(png)->rowAvailable(row, rowIndex, state);
 }
 
-void PNGAPI pngComplete(png_structp png, png_infop) {
-  imageDecoder(png)->complete();
+void PNGAPI pngFrameComplete(png_structp png, png_infop) {
+  imageDecoder(png)->frameComplete();
 }
 
 void PNGAPI pngFailed(png_structp png, png_const_charp) {
@@ -73,45 +74,638 @@ void PNGAPI pngFailed(png_structp png, png_const_charp) {
 
 namespace blink {
 
-PNGImageReader::PNGImageReader(PNGImageDecoder* decoder, size_t readOffset)
-    : m_decoder(decoder),
-      m_readOffset(readOffset),
-      m_currentBufferSize(0),
-      m_decodingSizeOnly(false),
-      m_hasAlpha(false) {
+PNGImageReader::PNGImageReader(PNGImageDecoder* decoder, size_t initialOffset)
+    : m_width(0),
+      m_height(0),
+      m_decoder(decoder),
+      m_initialOffset(initialOffset),
+      m_readOffset(initialOffset),
+      m_progressiveDecodeOffset(0),
+      m_idatOffset(0),
+      m_idatIsPartOfAnimation(false),
+      m_expectIdats(true),
+      m_isAnimated(false),
+      m_parsedSignature(false),
+      m_parsedIHDR(false),
+      m_parseCompleted(false),
+      m_reportedFrameCount(0),
+      m_nextSequenceNumber(0),
+      m_fctlNeedsDatChunk(false),
+      m_ignoreAnimation(false) {
   m_png = png_create_read_struct(PNG_LIBPNG_VER_STRING, 0, pngFailed, 0);
   m_info = png_create_info_struct(m_png);
-  png_set_progressive_read_fn(m_png, m_decoder, pngHeaderAvailable,
-                              pngRowAvailable, pngComplete);
+  png_set_progressive_read_fn(m_png, m_decoder, nullptr, pngRowAvailable,
+                              pngFrameComplete);
 }
 
 PNGImageReader::~PNGImageReader() {
   png_destroy_read_struct(m_png ? &m_png : 0, m_info ? &m_info : 0, 0);
   DCHECK(!m_png && !m_info);
+}
 
-  m_readOffset = 0;
+// This method reads from the FastSharedBufferReader, starting at offset,
+// and returns |length| bytes in the form of a pointer to a const png_byte*.
+// This function is used to make it easy to access data from the reader in a
+// png friendly way, and pass it to libpng for decoding.
+//
+// Pre-conditions before using this:
+// - |reader|.size() >= |readOffset| + |length|
+// - |buffer|.size() >= |length|
+// - |length| <= |kBufferSize|
+//
+// The reason for the last two precondition is that currently the png signature
+// plus IHDR chunk (8B + 25B = 33B) is the largest chunk that is read using this
+// method. If the data is not consecutive, it is stored in |buffer|, which must
+// have the size of (at least) |length|, but there's no need for it to be larger
+// than |kBufferSize|.
+static constexpr size_t kBufferSize = 33;
+const png_byte* readAsConstPngBytep(const FastSharedBufferReader& reader,
+                                    size_t readOffset,
+                                    size_t length,
+                                    char* buffer) {
+  DCHECK(length <= kBufferSize);
+  return reinterpret_cast<const png_byte*>(
+      reader.getConsecutiveData(readOffset, length, buffer));
 }
 
-bool PNGImageReader::decode(const SegmentReader& data, bool sizeOnly) {
-  m_decodingSizeOnly = sizeOnly;
+// This is used as a value for the byteLength of a frameInfo struct to
+// indicate that it is the first frame and its byteLength is not yet known. 1
+// is a safe value since the byteLength field of a frame is at least 12, in
+// the case of an empty fdAT or IDAT chunk.
+static constexpr size_t kFirstFrameIndicator = 1;
+
+// Return false on a fatal error
+bool PNGImageReader::decode(SegmentReader& data, size_t index) {
+  if (index >= m_frameInfo.size())
+    return true;
+
+  const FastSharedBufferReader reader(&data);
+
+  if (!m_isAnimated) {
+    if (setjmp(JMPBUF(m_png)))
+      return false;
+    DCHECK_EQ(0u, index);
+    m_progressiveDecodeOffset += processData(
+        reader, m_frameInfo[0].startOffset + m_progressiveDecodeOffset, 0);
+    return true;
+  }
+
+  DCHECK(m_isAnimated);
+
+  if (index) {

[Noel Gordon, 2017/03/06 05:21:47]

Optional: would understanding be aided by having all this code in a helper
routine?  e.g, 

bool PNGImageReader::shouldDecodeWithNewPNG(size_t index) const {
  ...
}

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

By itself, I did not think so, but I created a couple of local booleans inside
the method that I think make it clearer. Let me know what you think.

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

Yeah, the local bools are active code and help explain.  Better I think.

+    // If a progressive decode is in progress for frame zero,
+    if (m_progressiveDecodeOffset ||
+        // or the frame does not match IHDR,
+        m_frameInfo[index].frameRect != IntRect({}, {m_width, m_height})) {
+      // decode as a new PNG, to start over/use a modified IHDR.
+      clearDecodeState(0);
+    }
+  } else {
+    // For frame zero, only start over with a modified IHDR if a decode is
+    // not in progress.
+    if (!m_progressiveDecodeOffset &&
+        m_frameInfo[index].frameRect != IntRect({}, {m_width, m_height}))
+      clearDecodeState(0);
+  }
+
+  const bool decodeAsNewPNG = !m_png;
+  if (decodeAsNewPNG) {
+    DCHECK(!m_info);
+    m_png = png_create_read_struct(PNG_LIBPNG_VER_STRING, 0, pngFailed, 0);
+    m_info = png_create_info_struct(m_png);
+    png_set_progressive_read_fn(m_png, m_decoder, pngHeaderAvailable,
+                                pngRowAvailable, pngFrameComplete);
+  }
 
-  // We need to do the setjmp here. Otherwise bad things will happen.
   if (setjmp(JMPBUF(m_png)))
-    return m_decoder->setFailed();
+    return false;
+
+  if (decodeAsNewPNG)
+    startFrameDecoding(reader, index);
+
+  if (!index && (!firstFrameFullyReceived() || m_progressiveDecodeOffset)) {
+    const bool decodedFrameCompletely = progressivelyDecodeFirstFrame(reader);
+    if (!decodedFrameCompletely)
+      return true;
+
+    m_progressiveDecodeOffset = 0;
+  } else {
+    decodeFrame(reader, index);
+  }
+
+  static png_byte IEND[12] = {0, 0, 0, 0, 'I', 'E', 'N', 'D', 174, 66, 96, 130};
+  png_process_data(m_png, m_info, IEND, 12);
+  png_destroy_read_struct(&m_png, &m_info, 0);
+  DCHECK(!m_png && !m_info);
+
+  return true;
+}
+
+void PNGImageReader::startFrameDecoding(const FastSharedBufferReader& reader,
+                                        size_t index) {
+  // If the frame is the size of the whole image, just re-process all header
+  // data up to the first frame.
+  const IntRect& frameRect = m_frameInfo[index].frameRect;
+  if (frameRect.location() == IntPoint() &&

[Noel Gordon, 2017/03/06 05:21:47]

I think you want 

  if (frameRect == IntRect(IntPoint(), m_decoder->size()))

here, right?

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

Done.

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

I should've also suggested this

if (frameRect == IntRect(0, 0, m_width, m_height))

to use here.  We need to know we have the image width / height at this point in
the code.  Use whichever one works best, your call.

[scroggo_chromium, 2017/03/08 20:53:22]

I have a slight preference for IntRect(0, 0, m_width, m_height), but I do not
have a strong preference. They are slightly different - the former calls a
method that returns ints, and m_width and m_height are png_uint_32.

[Noel Gordon, 2017/03/13 12:16:17]

No problems with int/uint conversion here: headerAvailable() has been called,
and it restricts the maximum image width and height, right?  IntRect(0, 0,
m_width, m_height) would better match the rest of the code.  Suggest you go with
IntRect(0, 0, m_width, m_height) here [1].

[1] This brings up another point about how m_width / m_height are set, which we
can discuss in the next round.

+      frameRect.size() == m_decoder->size()) {
+    processData(reader, m_initialOffset, m_idatOffset);
+    return;
+  }
+
+  // Process the IHDR chunk, but change the width and height so it reflects
+  // the frame's width and height. ImageDecoder will apply the x,y offset.
+  char readBuffer[kBufferSize];
+
+  // |headerSize| is equal to |kBufferSize|, but adds more semantic insight.
+  constexpr size_t headerSize = 33;
+  png_byte header[headerSize];
+  const png_byte* chunk =
+      readAsConstPngBytep(reader, m_initialOffset, headerSize, readBuffer);
+  memcpy(header, chunk, headerSize);

[Noel Gordon, 2017/03/06 05:21:47]

"more semantic insight."  This memcpy seems superfluous and so is having an
extra png_byte header[headerSize] when we could jsut read the same data from the
chunk, this additional code seems to add unnecessary work, not insight.  How
about ...

  constexpr size_t headerSize = kBufferSize;
  char readBuffer[headerSize];
  const png_byte* chunk =
      readAsConstPngBytep(reader, m_initialOffset, headerSize, readBuffer);

  png_byte* header = const_cast<png_byte*>(chunk);
  png_save_uint_32(header + 16, frameRect.width());
  ...

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

Done. We still need to memcpy if the FastSharedBufferReader returned a pointer
to its own memory.

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

Scanned the new code here, scratching my head wondering why the rest of the code
does not do similarly when we use the readAsConstPngBytep API.  I might be
missing something.

[scroggo_chromium, 2017/03/08 20:53:21]

FastSharedBufferReader returns a pointer, which is either to its own memory (if
the requested block was contiguous) or to the client owned pointer (readBuffer,
in this case). If we're just reading it (as in most cases in this file), either
one is fine. But if we intend to modify it, like we do down below with
png_save_uint_32, we need to make sure we're acting on our own memory, rather
than the memory owned by the FastSharedBufferReader.

[Noel Gordon, 2017/03/13 12:16:17]

Ah we modify it, gotcha.  Agree you need to memcpy, makes sense to me now.

+
+  png_save_uint_32(header + 16, frameRect.width());
+  png_save_uint_32(header + 20, frameRect.height());
+  // IHDR has been modified, so tell libpng to ignore CRC errors.
+  png_set_crc_action(m_png, PNG_CRC_QUIET_USE, PNG_CRC_QUIET_USE);
+  png_process_data(m_png, m_info, header, headerSize);
+
+  // Process the rest of the header chunks.
+  processData(reader, m_initialOffset + headerSize, m_idatOffset - headerSize);
+}
+
+// Determine if the bytes 4 to 7 of |chunk| indicate that it is a |tag| chunk.
+// - The length of |chunk| must be >= 8
+// - The length of |tag| must be = 4
+static inline bool isChunk(const png_byte* chunk, const char* tag) {
+  return memcmp(chunk + 4, tag, 4) == 0;
+}
+
+bool PNGImageReader::progressivelyDecodeFirstFrame(
+    const FastSharedBufferReader& reader) {
+  char readBuffer[8];  // large enough to identify a chunk.
+  size_t offset = m_frameInfo[0].startOffset;
+
+  // Loop while there is enough data to do progressive decoding.
+  while (reader.size() >= offset + 8) {
+    // At the beginning of each loop, the offset is at the start of a chunk.
+    const png_byte* chunk = readAsConstPngBytep(reader, offset, 8, readBuffer);
+    const png_uint_32 length = png_get_uint_32(chunk);
+    DCHECK(length <= PNG_UINT_31_MAX);
+
+    // When an fcTL or IEND chunk is encountered, the frame data has ended.
+    // Return true, since all frame data is decoded.
+    if (isChunk(chunk, "fcTL") || isChunk(chunk, "IEND"))
+      return true;
+
+    // If this chunk was already decoded, move on to the next.
+    if (m_progressiveDecodeOffset >= offset + length + 12) {
+      offset += length + 12;
+      continue;
+    }
+
+    // At this point, three scenarios are possible:
+    // 1) Some bytes of this chunk were already decoded in a previous call.
+    //    Continue from there.
+    // 2) This is an fdAT chunk. Convert it to an IDAT chunk to decode.
+    // 3) This is any other chunk, most likely an IDAT chunk.
+    size_t endOffsetChunk = offset + length + 12;
+
+    if (m_progressiveDecodeOffset >= offset + 8) {
+      offset = m_progressiveDecodeOffset;
+    } else if (isChunk(chunk, "fdAT")) {
+      processFdatChunkAsIdat(length);
+      // Skip the sequence number.
+      offset += 12;
+    } else {
+      png_process_data(m_png, m_info, const_cast<png_byte*>(chunk), 8);
+      offset += 8;
+    }
+
+    size_t bytesLeftInChunk = endOffsetChunk - offset;
+    size_t bytesDecoded = processData(reader, offset, bytesLeftInChunk);
+    m_progressiveDecodeOffset = offset + bytesDecoded;
+    if (bytesDecoded < bytesLeftInChunk)
+      return false;
+    offset += bytesDecoded;
+  }
+
+  return false;
+}
+
+void PNGImageReader::processFdatChunkAsIdat(png_uint_32 fdatLength) {
+  // An fdAT chunk is build up as follows:
+  // - |length| (4B)
+  // - fdAT tag (4B)
+  // - sequence number (4B)
+  // - frame data (|length| - 4B)
+  // - CRC (4B)
+  // Thus, to reformat this into an IDAT chunk, do the following:
+  // - write |length| - 4 as the new length, since the sequence number
+  //   must be removed.
+  // - change the tag to IDAT.
+  // - omit the sequence number from the data part of the chunk.
+  png_byte chunkIDAT[] = {0, 0, 0, 0, 'I', 'D', 'A', 'T'};
+  png_save_uint_32(chunkIDAT, fdatLength - 4);
+  // The CRC is incorrect when applied to the modified fdAT.
+  png_set_crc_action(m_png, PNG_CRC_QUIET_USE, PNG_CRC_QUIET_USE);
+  png_process_data(m_png, m_info, chunkIDAT, 8);
+}
+
+void PNGImageReader::decodeFrame(const FastSharedBufferReader& reader,
+                                 size_t index) {
+  size_t offset = m_frameInfo[index].startOffset;
+  size_t endOffset = offset + m_frameInfo[index].byteLength;
+  char readBuffer[8];
+
+  while (offset < endOffset) {
+    const png_byte* chunk = readAsConstPngBytep(reader, offset, 8, readBuffer);
+    const png_uint_32 length = png_get_uint_32(chunk);
+    DCHECK(length <= PNG_UINT_31_MAX);
+
+    if (isChunk(chunk, "fdAT")) {
+      processFdatChunkAsIdat(length);
+      // The frame data and the CRC span |length| bytes, so skip the
+      // sequence number and process |length| bytes to decode the frame.
+      processData(reader, offset + 12, length);
+    } else {
+      png_process_data(m_png, m_info, const_cast<png_byte*>(chunk), 8);
+      processData(reader, offset + 8, length + 4);
+    }
+    offset += 12 + length;
+  }
+}
+
+// Compute the CRC and compare to the stored value.
+static bool checkCrc(const FastSharedBufferReader& reader,
+                     size_t chunkStart,
+                     size_t chunkLength) {
+  const size_t kSizeNeededForfcTL = 26 + 4;
+  char readBuffer[kSizeNeededForfcTL];
+  DCHECK(chunkLength + 4 <= kSizeNeededForfcTL);
+  const png_byte* chunk =
+      readAsConstPngBytep(reader, chunkStart + 4, chunkLength + 4, readBuffer);
+
+  char crcBuffer[4];
+  const png_byte* crcLoc =

[Noel Gordon, 2017/03/06 05:21:48]

abbv.  crcLoc -> crcPosition.

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

Done.

+      readAsConstPngBytep(reader, chunkStart + 8 + chunkLength, 4, crcBuffer);
+  const png_uint_32 crc = png_get_uint_32(crcLoc);

[Noel Gordon, 2017/03/06 05:21:47]

s/const//

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

Done.

+  const png_uint_32 actual = crc32(crc32(0, Z_NULL, 0), chunk, chunkLength + 4);

[Noel Gordon, 2017/03/06 05:21:47]

return crc == crc32(crc32(0, Z_NULL, 0), chunk, ... ?

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

Done.

+  return actual == crc;
+}
+
+bool PNGImageReader::checkSequenceNumber(const png_byte* position) {
+  png_uint_32 sequenceNumber = png_get_uint_32(position);

[Noel Gordon, 2017/03/06 05:21:48]

sequenceNumber -> sequence and reformat the code here.

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

Done.

+  if (sequenceNumber != m_nextSequenceNumber ||
+      sequenceNumber > PNG_UINT_31_MAX)
+    return false;
+
+  ++m_nextSequenceNumber;
+  return true;
+}
+
+// Return false if there was a fatal error; true otherwise.
+bool PNGImageReader::parse(SegmentReader& data, ParseQuery query) {
+  if (m_parseCompleted)
+    return true;
+
+  const FastSharedBufferReader reader(&data);
+
+  if (!parseSize(reader))
+    return false;
 
+  if (!m_decoder->isDecodedSizeAvailable())
+    return true;
+
+  // For non animated images (identified by no acTL chunk before the IDAT),
+  // there is no need to continue parsing.
+  if (!m_isAnimated) {
+    DCHECK(m_frameInfo.isEmpty());

[Noel Gordon, 2017/03/06 05:21:48]

Move this DCHECK to line before the m_frameInfo.push_back() ?

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

Done.

+    FrameInfo frame;
+    frame.startOffset = m_readOffset;
+    // This should never be read in this case, but initialize just in case.
+    frame.byteLength = kFirstFrameIndicator;
+    frame.duration = 0;
+    frame.frameRect = IntRect(IntPoint(), {m_width, m_height});
+    frame.disposalMethod = ImageFrame::DisposalMethod::DisposeKeep;
+    frame.alphaBlend = ImageFrame::AlphaBlendSource::BlendAtopBgcolor;
+    m_frameInfo.push_back(frame);
+    m_parseCompleted = true;
+    return true;
+  }
+
+  if (query == ParseQuery::Size)
+    return true;
+
+  DCHECK_EQ(ParseQuery::MetaData, query);
+  DCHECK(m_isAnimated);
+
+  // Loop over the data and manually register all frames. Nothing is passed to
+  // libpng for processing. A frame is registered on the next fcTL chunk or
+  // when the IEND chunk is found. This ensures that only complete frames are
+  // reported, unless there is an error in the stream.
+  char readBuffer[kBufferSize];
+  while (reader.size() >= m_readOffset + 8) {
+    const png_byte* chunk =
+        readAsConstPngBytep(reader, m_readOffset, 8, readBuffer);
+    const size_t length = png_get_uint_32(chunk);
+    if (length > PNG_UINT_31_MAX)
+      return false;
+
+    const bool IDAT = isChunk(chunk, "IDAT");
+    if (IDAT && !m_expectIdats)
+      return false;
+
+    const bool fdAT = isChunk(chunk, "fdAT");
+    if (fdAT && m_expectIdats)
+      return false;
+
+    if (fdAT || (IDAT && m_idatIsPartOfAnimation)) {
+      m_fctlNeedsDatChunk = false;
+      if (m_newFrame.startOffset == 0) {

[Noel Gordon, 2017/03/06 05:21:47]

Comparison to 0.

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

Done.

+        // Beginning of a new frame's data.
+        m_newFrame.startOffset = m_readOffset;
+
+        if (m_frameInfo.isEmpty()) {
+          // This is the first frame. Report it immediately so it can be
+          // decoded progressively.
+          m_newFrame.byteLength = kFirstFrameIndicator;
+          m_frameInfo.push_back(m_newFrame);
+        }
+      }
+
+      if (fdAT) {
+        if (reader.size() < m_readOffset + 8 + 4)
+          return true;
+        const png_byte* sequenceNumPosition =

[Noel Gordon, 2017/03/06 05:21:47]

sequenceNumPosition -> sequencePosition

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

Done.

+            readAsConstPngBytep(reader, m_readOffset + 8, 4, readBuffer);
+        if (!checkSequenceNumber(sequenceNumPosition))
+          return false;
+      }
+
+    } else if (isChunk(chunk, "fcTL") || isChunk(chunk, "IEND")) {
+      // This marks the end of the previous frame.
+      if (m_newFrame.startOffset != 0) {

[Noel Gordon, 2017/03/06 05:21:48]

Comparsion to 0.  !m_newFrame.startOffset

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

Done.

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

Bug on my part suggesting !m_newFrame.startOffset.  Wrong, please re-check your
change here, should be
 
if (m_newFrame.startOffset != 0) -> if (m_newFrame.startOffset)

please, and my apologies.

[scroggo_chromium, 2017/03/08 20:53:22]

Thanks. I noticed the bug and corrected :)

+        m_newFrame.byteLength = m_readOffset - m_newFrame.startOffset;
+        if (m_frameInfo[0].byteLength == kFirstFrameIndicator) {
+          m_frameInfo[0].byteLength = m_newFrame.byteLength;
+        } else {
+          m_frameInfo.push_back(m_newFrame);
+          if (isChunk(chunk, "fcTL")) {
+            if (m_frameInfo.size() >= m_reportedFrameCount)
+              return false;
+          } else {
+            // IEND
+            if (m_frameInfo.size() != m_reportedFrameCount)
+              return false;
+          }
+        }
+
+        m_newFrame.startOffset = 0;
+      }
+
+      if (reader.size() < m_readOffset + 12 + length)
+        return true;
+
+      if (isChunk(chunk, "IEND")) {
+        m_parseCompleted = true;
+        return true;
+      }
+
+      if (length != 26 || !checkCrc(reader, m_readOffset, length))
+        return false;
+
+      chunk = readAsConstPngBytep(reader, m_readOffset + 8, length, readBuffer);
+      if (!parseFrameInfo(chunk))
+        return false;
+
+      m_expectIdats = false;
+    } else if (isChunk(chunk, "acTL")) {
+      // There should only be one acTL chunk, and it should be before the
+      // IDAT chunk.
+      return false;
+    }
+    m_readOffset += 12 + length;
+  }
+  return true;
+}
+
+// If |length| == 0, read until the stream ends. Return number of bytes
+// processed.
+size_t PNGImageReader::processData(const FastSharedBufferReader& reader,
+                                   size_t offset,
+                                   size_t length) {
   const char* segment;
-  while (size_t segmentLength = data.getSomeData(segment, m_readOffset)) {
-    m_readOffset += segmentLength;
-    m_currentBufferSize = m_readOffset;
+  size_t totalProcessedBytes = 0;
+  while (reader.size() > offset) {
+    size_t segmentLength = reader.getSomeData(segment, offset);
+    if (length > 0 && segmentLength + totalProcessedBytes > length)
+      segmentLength = length - totalProcessedBytes;
+
     png_process_data(m_png, m_info,
-                     reinterpret_cast<png_bytep>(const_cast<char*>(segment)),
+                     reinterpret_cast<png_byte*>(const_cast<char*>(segment)),
                      segmentLength);
-    if (sizeOnly ? m_decoder->isDecodedSizeAvailable()
-                 : m_decoder->frameIsCompleteAtIndex(0))
+    offset += segmentLength;
+    totalProcessedBytes += segmentLength;
+    if (totalProcessedBytes == length)
+      return length;
+  }
+  return totalProcessedBytes;
+}
+
+// Process up to the start of the IDAT with libpng.
+// Return false for a fatal error. True otherwise.
+bool PNGImageReader::parseSize(const FastSharedBufferReader& reader) {
+  if (m_decoder->isDecodedSizeAvailable())
+    return true;
+
+  char readBuffer[kBufferSize];
+
+  if (setjmp(JMPBUF(m_png)))
+    return false;
+
+  if (!m_parsedSignature) {
+    if (reader.size() < m_readOffset + 8)
       return true;
+
+    const png_byte* chunk =
+        readAsConstPngBytep(reader, m_readOffset, 8, readBuffer);
+    png_process_data(m_png, m_info, const_cast<png_byte*>(chunk), 8);
+    m_readOffset += 8;
+    m_parsedSignature = true;
+    // Initialize the newFrame by setting the readOffset to 0.

[Noel Gordon, 2017/03/06 05:21:47]

Comment does not match the code, maybe throw it.

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

Done.

+    m_newFrame.startOffset = 0;
   }
 
-  return false;
+  // This loop peeks at the chunk tag until the IDAT chunk is found. APNG
+  // chunks are handled manually. Other tags are passed on to libpng for general
+  // parsing. Peek at chunks by looking at the first 8 bytes, which contain the
+  // length and the chunk tag.
+  for (png_uint_32 length = 0; reader.size() >= m_readOffset + 8;
+       m_readOffset += length + 12) {
+    const png_byte* chunk =
+        readAsConstPngBytep(reader, m_readOffset, 8, readBuffer);
+    length = png_get_uint_32(chunk);
+
+    // If we encounter the IDAT chunk, we're done with the png header

[Noel Gordon, 2017/03/06 05:21:47]

Superfluous comment.

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

Done.

+    if (isChunk(chunk, "IDAT")) {
+      // Done with header chunks.
+      m_idatOffset = m_readOffset;
+      m_fctlNeedsDatChunk = false;
+      if (m_ignoreAnimation)
+        m_isAnimated = false;
+      if (!m_isAnimated || 1 == m_reportedFrameCount)
+        m_decoder->setRepetitionCount(cAnimationNone);
+      // Call headerAvailable manually. IDAT will be supplied (possibly

[Noel Gordon, 2017/03/06 05:21:48]

I'm not sure this comment adds value either.

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

Done.

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

Thanks, Function-level comment says IDAT, no need to keep mentioning IDAT a
further 3 times in comment.

[scroggo_chromium, 2017/03/08 20:53:22]

I only see one left after responding to the last round of comments. ("This loop
peeks at the chunk tag until the IDAT chunk is found.") Removed.

[Noel Gordon, 2017/03/13 12:16:17]

"removed" -> nod, that's the spirit :)

+      // modified) when decoding the frame.
+      m_decoder->headerAvailable();
+      return true;
+    }
+
+    // 12 is the length, tag and crc part of the chunk, which are all 4B.
+    if (reader.size() < m_readOffset + length + 12)
+      break;
+
+    if (isChunk(chunk, "acTL")) {
+      if (m_ignoreAnimation)
+        continue;
+      if (m_isAnimated || length != 8 || !m_parsedIHDR ||
+          !checkCrc(reader, m_readOffset, 8)) {
+        m_ignoreAnimation = true;
+        continue;
+      }
+      chunk = readAsConstPngBytep(reader, m_readOffset + 8, length, readBuffer);
+      m_reportedFrameCount = png_get_uint_32(chunk);
+      if (!m_reportedFrameCount || m_reportedFrameCount > PNG_UINT_31_MAX) {
+        m_ignoreAnimation = true;
+        continue;
+      }
+      png_uint_32 repetitionCount = png_get_uint_32(chunk + 4);
+      if (repetitionCount > PNG_UINT_31_MAX) {
+        m_ignoreAnimation = true;
+        continue;
+      }
+      m_isAnimated = true;
+      m_decoder->setRepetitionCount((int)repetitionCount - 1);
+    } else if (isChunk(chunk, "fcTL")) {
+      if (m_ignoreAnimation)
+        continue;
+      if (length != 26 || !m_parsedIHDR ||
+          !checkCrc(reader, m_readOffset, 26)) {
+        m_ignoreAnimation = true;
+        continue;
+      }
+      chunk = readAsConstPngBytep(reader, m_readOffset + 8, length, readBuffer);
+      if (!parseFrameInfo(chunk) ||
+          m_newFrame.frameRect != IntRect(0, 0, m_width, m_height)) {
+        m_ignoreAnimation = true;

[Noel Gordon, 2017/03/06 05:21:48]

The logic of how m_ignoreAnimation is used is interesting, but if you return
false here, note that one of those invalid image tests (Default image's fcTL
size not matching IHDR) starts to pass.

Maybe with your failure strategy, which seems to be not to display the default
image, you could return false anytime you set m_ignoreAnimation to false. 
Something to think about.

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

But others will fail, whereas today we successfully show the default image (for
a failure prior to IDAT in an otherwise valid static PNG).
The strategy is described in the commit message, although maybe it should be
consolidated:

"If they contain an error before IDAT, the image
is treated as a static PNG, as recommended by the spec [1]. If they contain
an error after IDAT, this is a failure, since some frames may have been
decoded."

"Future work:
- Revert to showing the default image for failures past IDAT. This is tricky,
since the client may be holding on to previous frames, and we need to make
sure they switch to using the IDAT frame, even if it is not part of the
animation. For now, we mark the decoder as having failed."
Then we would call setFailed() and display nothing (unless this goes along with
more changes?), whereas currently this displays the default image, as intended.

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

IC, thanks for pointing at these sections of the ChangeLog, future work to do
...  think I mentioned awhile's back that I thought getting those test cases
working might be a time sink :)   Pushing them off into crbug as future work we
need to address is fine by me.
 
Displaying nothing (broken image) could be interpreted as "or show an error to
the user", which the spec might seem to allow as an option for the failing image
test cases (based on the notes I read in that Philip APNG test suite, and
assuming those notes are correct). 

Hard case is "error after IDAT", many frames already decoded / shown, and then
you hit an error, and show default image or broken image.  The visual effect of
that might be jarring to users, dunno ...

[scroggo_chromium, 2017/03/08 20:53:22]

I interpreted that to mean the three specific cases we were discussing in the
comment, which I filed as crbug.com/698808.

Other types of errors could show up after IDAT, e.g.
- more acTL chunks (I don't see that explicitly called out in the spec, but
https://philip.html5.org/tests/apng/tests.html considers it an error, at least
if there is more than one before IDAT)
- fcTL chunks with invalid data
- some internal libpng error

These we already detect as errors (unlike the ones in crbug.com/698808 which we
ignore), but we do not revert to showing the default image. (I have now filed
crbug.com/699675 to cover these cases.)
It more or less says that (though it seems to defer to the spec, and states
"This page is somewhat incomplete and quite possibly incorrect – use with
caution." at the top), but the spec (and the test suite) says "It is strongly
recommended that when any error is encountered decoders should discard all
subsequent frames, stop the animation, and revert to displaying the default
image. A decoder which detects an error before the animation has started should
display the default image. An error message may be displayed to the user if
appropriate."

I interpret that to prefer showing the default image. So I chose to show the
default image here, given that it's simple to handle. (We could take this a step
further and handle errors after IDAT but before we started the animation, though
I'm worried it would be ugly and it doesn't handle the larger issue - the
animation has already started.)
I think we already have everything necessary to show a broken image - call
setFailed(), which is what this patch does.


But I agree that handling these cases is harder, if we show the default image.
Perhaps the visual effect would be jarring, but that is not my primary concern.
My concern is that BitmapImage may have already decided how many frames there
are and what their durations are, some frames may have been cached etc, and I
suspect we may need to make changes upstack in order to correct that. (One hacky
solution might be to switch up decode(i) to always decode the default image, and
maybe max out the duration.) 

[Noel Gordon, 2017/03/13 12:16:17]

(Thanks for filing the bug).
  
(Don't like the sound of ugly :)  One possible interpretation is prefer to
display the default image, as you note.  But that last sentence "An error
message may be displayed to the user if appropriate" could also be interpreted
to mean do that in all cases.  The spec prose is not clear as to which, making
it harder for implementations to conform: I see no compat b/w implementing
browsers in the failure cases at this time ¯\(ツ)/¯.
Nod.
 
You have bug filed a bug re: the "harder, if we show the default image" route.
Addressing that bug might turn up the BitmapImage issues you mention.  We'll
see.

+        continue;
+      }
+      m_idatIsPartOfAnimation = true;
+    } else if (isChunk(chunk, "fdAT")) {
+      m_ignoreAnimation = true;
+    } else {
+      png_process_data(m_png, m_info, const_cast<png_byte*>(chunk), 8);
+      processData(reader, m_readOffset + 8, length + 4);
+      if (isChunk(chunk, "IHDR")) {
+        m_parsedIHDR = true;
+        m_width = png_get_image_width(m_png, m_info);
+        m_height = png_get_image_height(m_png, m_info);
+      }
+    }
+  }
+
+  // Not enough data to call headerAvailable.
+  return true;
+}
+
+bool PNGImageReader::firstFrameFullyReceived() const {
+  return !m_frameInfo.isEmpty() &&
+         m_frameInfo[0].byteLength != kFirstFrameIndicator;
+}
+
+void PNGImageReader::clearDecodeState(size_t frameIndex) {

[Noel Gordon, 2017/03/06 05:21:48]

frameIndex -> index.

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

Done.

+  if (frameIndex == 0) {

[Noel Gordon, 2017/03/06 05:21:47]

Comparison to 0.

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

Done.

+    png_destroy_read_struct(m_png ? &m_png : nullptr,
+                            m_info ? &m_info : nullptr, 0);
+    DCHECK(!m_png && !m_info);
+    m_progressiveDecodeOffset = 0;
+  }
+}
+
+size_t PNGImageReader::frameCount() const {
+  return m_frameInfo.size();
+}
+
+const PNGImageReader::FrameInfo& PNGImageReader::frameInfo(size_t index) const {
+  DCHECK(index < m_frameInfo.size());
+  return m_frameInfo[index];
+}
+
+// These constants map to the frame control parameters defined in fcTL chunks,
+// as specified at https://wiki.mozilla.org/APNG_Specification.
+#define kAPNG_DISPOSE_OP_NONE 0
+#define kAPNG_DISPOSE_OP_BACKGROUND 1
+#define kAPNG_DISPOSE_OP_PREVIOUS 2
+
+#define kAPNG_BLEND_OP_SOURCE 0
+#define kAPNG_BLEND_OP_OVER 1
+
+// Extract the frame control info and store it in m_newFrame. The length check
+// on the data chunk has been done by the calling code.
+bool PNGImageReader::parseFrameInfo(const png_byte* data) {
+  if (m_fctlNeedsDatChunk)
+    return false;
+
+  if (!checkSequenceNumber(data))

[Noel Gordon, 2017/03/06 05:21:47]

Move this check further down (see next comment).

+    return false;
+
+  png_uint_32 frameWidth = png_get_uint_32(data + 4);
+  png_uint_32 frameHeight = png_get_uint_32(data + 8);
+  png_uint_32 xOffset = png_get_uint_32(data + 12);
+  png_uint_32 yOffset = png_get_uint_32(data + 16);
+
+  if (!frameWidth || !frameHeight || xOffset + frameWidth > m_width ||

[Noel Gordon, 2017/03/06 05:21:48]

if (!checkSequenceNumber(data))
  return false;
if (!frameWidth || !frameHeight)
  return false;
if (xOffset + frameWidth > m_width || ...)
  return false;

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

Done.

+      yOffset + frameHeight > m_height)
+    return false;
+
+  png_uint_16 delayNumerator = png_get_uint_16(data + 20);
+  png_uint_16 delayDenominator = png_get_uint_16(data + 22);
+
+  m_newFrame.duration = (delayDenominator == 0)

[Noel Gordon, 2017/03/06 05:21:47]

  if (delayDenominator)
    m_newFrame.duration = delayNumerator * 1000 / delayDenominator;
  else
    m_newFrame.duration = delayNumerator * 10;

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

Done.

+                            ? delayNumerator * 10
+                            : delayNumerator * 1000 / delayDenominator;
+  m_newFrame.frameRect = IntRect(xOffset, yOffset, frameWidth, frameHeight);

[Noel Gordon, 2017/03/06 05:21:48]

Odd that m_newFrame.frameRect is set here.  Would it make sense to set it just
after you vetted it at line 667 above.

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

Done.

+  const uint8_t disposalMethod = data[24];

[Noel Gordon, 2017/03/06 05:21:47]

Space before this line, and make it

const png_byte& disposalMethod = data[24]; 

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

Done.

+  switch (disposalMethod) {
+    case kAPNG_DISPOSE_OP_NONE:
+      m_newFrame.disposalMethod = ImageFrame::DisposalMethod::DisposeKeep;
+      break;
+    case kAPNG_DISPOSE_OP_BACKGROUND:
+      m_newFrame.disposalMethod =
+          ImageFrame::DisposalMethod::DisposeOverwriteBgcolor;
+      break;
+    case kAPNG_DISPOSE_OP_PREVIOUS:
+      m_newFrame.disposalMethod =
+          ImageFrame::DisposalMethod::DisposeOverwritePrevious;
+      break;
+    default:
+      return false;
+  }
+
+  const uint8_t alphaBlend = data[25];

[Noel Gordon, 2017/03/06 05:21:47]

const png_byte& alphaBlend = data[25]; 

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

Done.

+  switch (alphaBlend) {
+    case kAPNG_BLEND_OP_SOURCE:
+      m_newFrame.alphaBlend = ImageFrame::AlphaBlendSource::BlendAtopBgcolor;
+      break;
+    case kAPNG_BLEND_OP_OVER:
+      m_newFrame.alphaBlend =
+          ImageFrame::AlphaBlendSource::BlendAtopPreviousFrame;
+      break;
+    default:
+      return false;
+  }
+  m_fctlNeedsDatChunk = true;

[Noel Gordon, 2017/03/06 05:21:48]

Space before this line.

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

Done.

+  return true;
 }
 
 }  // namespace blink