WIP: Implement APNG

third_party/WebKit/Source/platform/image-decoders/png/PNGImageReader.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
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ * Alternatively, the contents of this file may be used under the terms
+ * of either the Mozilla Public License Version 1.1, found at
+ * http://www.mozilla.org/MPL/ (the "MPL") or the GNU General Public
+ * License Version 2.0, found at http://www.fsf.org/copyleft/gpl.html
+ * (the "GPL"), in which case the provisions of the MPL or the GPL are
+ * applicable instead of those above.  If you wish to allow use of your
+ * version of this file only under the terms of one of those two
+ * 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/PNGImageReader.h"
+
+#include "platform/image-decoders/png/PNGImageDecoder.h"
+#include "platform/image-decoders/FastSharedBufferReader.h"
+#include "png.h"
+#include "wtf/PtrUtil.h"
+#include <memory>
+
+#if !defined(PNG_LIBPNG_VER_MAJOR) || !defined(PNG_LIBPNG_VER_MINOR)
+#error version error: compile against a versioned libpng.
+#endif
+#if USE(QCMSLIB)
+#include "qcms.h"
+#endif
+
+#if PNG_LIBPNG_VER_MAJOR > 1 || (PNG_LIBPNG_VER_MAJOR == 1 && PNG_LIBPNG_VER_MINOR >= 4)
+#define JMPBUF(png_ptr) png_jmpbuf(png_ptr)
+#else
+#define JMPBUF(png_ptr) png_ptr->jmpbuf
+#endif
+
+namespace {
+
+inline blink::PNGImageDecoder* imageDecoder(png_structp png)
+{
+    return static_cast<blink::PNGImageDecoder*>(png_get_progressive_ptr(png));
+}
+
+void PNGAPI pngFrameHeaderAvailable(png_structp png, png_infop)
+{
+    imageDecoder(png)->frameHeaderAvailable();
+}
+
+void PNGAPI pngHeaderAvailable(png_structp png, png_infop)
+{
+    imageDecoder(png)->headerAvailable();
+}
+
+void PNGAPI pngRowAvailable(png_structp png, png_bytep row, png_uint_32 rowIndex, int state)
+{
+    imageDecoder(png)->rowAvailable(row, rowIndex, state);
+}
+
+void PNGAPI pngComplete(png_structp png, png_infop)
+{
+    imageDecoder(png)->complete();
+}
+
+void PNGAPI pngFailed(png_structp png, png_const_charp err)
+{
+    SkDebugf("In pngFailed with err %s.", err);

[scroggo_chromium, 2016/10/11 20:13:10]

You'll need to remove this before submitting.

+    longjmp(JMPBUF(png), 1);
+}
+
+} // namespace
+
+namespace blink {
+
+/*

[scroggo_chromium, 2016/10/11 20:13:11]

I think most of the comments in ImageDecoder use //. According to
https://google.github.io/styleguide/cppguide.html#Comments, we should be
consistent (and // is more common).

+ * This is the callback function for unknown PNG chunks, which is used to
+ * extract the animation chunks.
+ */
+static int readAnimationChunk(png_structp png_ptr, png_unknown_chunkp chunk)
+{
+    PNGImageReader* reader = (PNGImageReader*) png_get_user_chunk_ptr(png_ptr);
+    reader->parseAnimationChunk((const char*) chunk->name, chunk->data, chunk->size);
+    return 1;
+}
+
+PNGImageReader::PNGImageReader(PNGImageDecoder* decoder, size_t readOffset)
+    : m_decoder(decoder)
+    , m_readOffset(readOffset)
+    , m_hasAlpha(false)
+    , m_idatPartOfAnimation(false)
+    , m_parsedSignature(false)
+#if USE(QCMSLIB)
+    , m_rowBuffer()
+#endif
+{
+    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);
+
+    // Keep the chunks which are of interest for APNG.
+    png_byte apngChunks[] = {"acTL\0fcTL\0fdAT\0"};
+    png_set_keep_unknown_chunks(m_png, PNG_HANDLE_CHUNK_NEVER, apngChunks, 3);
+    png_set_read_user_chunk_fn(m_png, (png_voidp) this, readAnimationChunk);
+}
+
+PNGImageReader::~PNGImageReader()
+{
+    png_destroy_read_struct(m_png ? &m_png : 0, m_info ? &m_info : 0, 0);
+    ASSERT(!m_png && !m_info);
+}
+
+/*
+ * @TODO(joostouwerling) implement this method
+ * Decode the frame at `index`

[scroggo_chromium, 2016/10/11 20:13:11]

Lots of places in the code would refer to this as |index|, although I have not
found that in the style guide, and I think someone has recommended against it in
a code review...

[joostouwerling, 2016/10/12 20:49:47]

The |var| notation confuses me because my mind interprets it as the length of
something. But I'll modify it for consistency.

[scroggo_chromium, 2016/10/13 13:49:32]

Yay, math!

+ */
+void PNGImageReader::decode(SegmentReader& data, size_t index)
+{
+
+}
+
+/*
+ * This method reads from the FastSharedBufferReader, starting at offset,
+ * and returns |length| bytes in the form of a pointer to a png_byte*. This
+ * function is used to make it easy to pass data from the stream to 
+ * png_process_data.
+ *
+ * Pre-conditions before using this:
+ * - reader.size() >= offset + length

[scroggo_chromium, 2016/10/11 20:13:11]

nit: readOffset

+ * - |m_readBuffer| = 26 >= length

[scroggo_chromium, 2016/10/11 20:13:10]

Does m_readBuffer need to be a member variable?

I think this would be easier to follow if you just put a char buffer on the
stack in the method that calls this, and pass it as a parameter. You can also
set the size to a constant and compare:

constexpr size_t kBufferSize = 26;

static png_byte* readAsPngBytep(..., length, buffer)
{
  ASSERT(length <= kBufferSize)
  return ...
}

(It doesn't guarantee someone won't call your method with a smaller buffer, but
it should be more obvious not to do that.)

Then at the call site:

{
  ...
  char buffer[kBufferSize];
  ...
  var = readAsPngBytep(..., length, buffer);
  ...
}

Now that we don't use m_readBuffer, this can be a static function in this file
instead of a member function.

[joostouwerling, 2016/10/12 20:49:47]

The reason I created this function is so I don't need to do two casts every time
I want to get data processed by libpng, since libpng requires non-const
png_byte* as the data stream. Now that I'm thinking about this, I would say that
it is better to wait with the const_cast until the png_process_data call. With
that in mind, it may not be beneficial to have a separate function for this at
all.

The advantage of my approach is that you need less code to read the data, and
can make a very natural call to readAsPngBytep, by providing just the reader,
offset and length. But I agree that it is more prone to misuse and pollutes the
class a little. 

+ *
+ * The reason for the last precondition is that at this point, the fcTL
+ * chunk (26B) is the largest chunk that is read using this method. If the data
+ * is not consecutive, it is stored in m_readBuffer, which has a size of 26B.
+ */
+png_byte* PNGImageReader::readAsPngBytep(const FastSharedBufferReader &reader,

[scroggo_chromium, 2016/10/11 20:13:10]

nit: & should go next to the type:

FastSharedBufferReader& reader

+                                         size_t readOffset, size_t length)
+{
+    return const_cast<png_byte*>(reinterpret_cast<const png_byte*>(

[scroggo_chromium, 2016/10/11 20:13:11]

Why the const_cast? I suppose the methods you'll call do not specify const? I
hope nothing is modifying the returned bytes!

[joostouwerling, 2016/10/12 20:49:47]

See comments above.

+                reader.getConsecutiveData(readOffset, length, m_readBuffer)));
+}
+
+bool PNGImageReader::parse(SegmentReader& data, PNGImageDecoder::PNGParseQuery query)
+{
+    if (setjmp(JMPBUF(m_png)))
+        return m_decoder->setFailed();
+
+    /*
+     * If the size has not been parsed, do that first, since it's necessary
+     * for both the Size and MetaData query. If parseSize returns false,
+     * it failed because of a lack of data so we can return false at this point.
+     */
+    if (!m_decoder->isDecodedSizeAvailable()) {
+        if (!parseSize(data))

[scroggo_chromium, 2016/10/11 20:13:10]

nit: This could be one if statement:

if (!m_decoder->isDecodedSizeAvailable() && !parseSize(data))

+            return false;
+    }
+    
+    if (query == PNGImageDecoder::PNGParseQuery::PNGSizeQuery)
+        return m_decoder->isDecodedSizeAvailable();
+    
+    FastSharedBufferReader reader(&data);
+
+    /*
+     * At this point, the query is FrameMetaDataQuery. Let's go ahead and loop

[scroggo_chromium, 2016/10/11 20:13:11]

nit: "Let's go ahead and" is informal, and probably not necessary. How about:

Loop over the data ...

+     * over the data and register all frames we can find. A frame is registered
+     * on the next fcTL chunk or when the IEND chunk is found. This ensures
+     * that all frame data is available, and that the frame data chunks have the

[scroggo_chromium, 2016/10/11 20:13:11]

This is true, but I think it could be more explicit. The interesting point is
that we try not to report information about partial frames. How about saying
something like:

This ensures that only complete frames are reported (unless there is an error in
the stream).

+     * length they said they have.
+     */
+    while (reader.size() >= m_readOffset + 8) {
+        png_byte* chunk = readAsPngBytep(reader, m_readOffset, 8);
+        size_t length = png_get_uint_32(chunk);

[scroggo_chromium, 2016/10/11 20:13:11]

nit: I think this can be const. Same for the bools down below.

+        
+        /*
+         * When we find an IDAT chunk (when the IDAT is part of the animation),
+         * or an fdAT chunk, and the readOffset field of the newFrame is 0,
+         * we have found the beginning of a new block of frame data.
+         */
+        bool isFrameData = (memcmp(chunk + 4, "IDAT", 4) == 0 && m_idatPartOfAnimation)
+                           || memcmp(chunk + 4, "fdAT", 4) == 0;
+        if (m_newFrame.readOffset == 0 && isFrameData)
+            m_newFrame.readOffset = m_readOffset;
+
+        /*
+         * An fcTL or IEND marks the end of the previous frame. Thus, the
+         * FrameInfo data in m_newFrame is submitted to the m_frameInfo vector.
+         *
+         * Furthermore, an fcTL chunk indicates a new frame is coming,
+         * so the m_newFrame variable is prepared accordingly by setting the 
+         * readOffset field to 0, which indicates that the frame control info
+         * is available but that we haven't seen any frame data yet.
+         */ 
+        bool isFCTL = memcmp(chunk + 4, "fcTL", 4) == 0;
+        bool isIEND = memcmp(chunk + 4, "IEND", 4) == 0;
+        if (isFCTL || isIEND) {
+            if (m_newFrame.readOffset != 0) {
+                m_newFrame.byteLength = (m_readOffset - 1) - m_newFrame.readOffset;
+                m_frameInfo.append(m_newFrame);
+                m_newFrame.readOffset = 0;
+            }
+
+            if (reader.size() < m_readOffset + 12 + length)
+                return false;

[scroggo_chromium, 2016/10/11 20:13:11]

If you return false here, and then receive more data, does this work the way you
expect it to? It looks to me like the next time you try to parse you will add an
extra frameInfo to m_frameInfo.

First time in the loop:

- m_newFrame.readOffset == 0 && isFrameData
  => m_newFrame.readOffset is set to m_readOffset
- isFCTL or isIEND
  && m_newFrame.readOffset != 0 (based on the step above)
  => append(m_newFrame)
  => m_newFrame.readOffset is set to 0

Next time in the loop looks to be the same (we haven't changed m_readOffset, so
we're reading the same data, and m_newFrame.readOffset is back to zero), so we
add another frame to m_frameInfo.

[joostouwerling, 2016/10/12 20:49:47]

Afaik it should be fine since isFrameData and (isFCTL || isIEND) are mutually
exclusive, so both will never be called in the same loop.

[scroggo_chromium, 2016/10/13 13:49:33]

Ah yes, those are mutually exclusive, since |chunk| cannot say both e.g. "IDAT"
and "IEND". So we don't actually need to check for fcTL or IEND if isFrameData
was true. What do you think about rearranging the code to something like:

if (isFrameData) {
  ....
} else {
  bool isFCTL = ...
  ....
}

+            
+            if (isIEND) {
+                // Let the decoder know we've parsed all data, so it does not 
+                // need to query again.
+                m_decoder->setMetaDataDecoded();
+                return true;
+            }
+
+            // Prepare the new frame info and read the frame control data.
+            chunk = readAsPngBytep(reader, m_readOffset + 8, length);
+            parseFrameInfo(chunk); 
+        }
+        m_readOffset += 12 + length;
+    }
+    return false; 
+}
+
+bool PNGImageReader::processData(SegmentReader& data, size_t offset, size_t length)
+{
+    const char* segment;
+    size_t decodedLength = 0;

[scroggo_chromium, 2016/10/11 20:13:10]

nit: I think if you put the word "total" in this name it will make it clearer
what it's for. totalDecodedLength? totalDecoded? totalProcessed?

+    while (size_t segmentLength = data.getSomeData(segment, offset)) {

[scroggo_chromium, 2016/10/11 20:13:11]

I think you need to add decodedLength to offset, or else you'll just be reading
the same bit of information forever.

+        if (segmentLength > length)
+            segmentLength = length;
+        png_process_data(m_png, m_info,

[scroggo_chromium, 2016/10/11 20:13:10]

Do you need to call setjmp in this method in case there is an error? Or is the
intent that a longjmp will unwind back to the caller?

[joostouwerling, 2016/10/12 20:49:46]

Unwinding to the caller is sufficient, for now. I'll revisit this in further
iterations.

+                         reinterpret_cast<png_byte*>(const_cast<char*>(segment)),
+                         segmentLength);
+       decodedLength += segmentLength;
+       if (decodedLength == length)
+           return true;
+    }
+    return false;
+}
+
+/*
+ * This methods reads through the stream until it has parsed the image size.
+ * It returns true when it succeeds. If there is not enough data or when the

[scroggo_chromium, 2016/10/11 20:13:10]

Again, I think some formatting can make this clearer.

The following sentence is just a fragment:

"If there is not enough data or when the decoding by libpng fails." I think you
mean to say that it returns false.

+ * decoding by libpng fails. In the latter case, it will also call setFailed
+ * on m_decoder. 
+ */
+bool PNGImageReader::parseSize(SegmentReader &data)
+{
+    FastSharedBufferReader reader(&data);
+    
+    /*
+     * Process the PNG signature and the IHDR with libpng, such that this code 
+     * does not need to be bothered with parsing the contents. This also enables
+     * the reader to use the existing headerAvailable callback in the decoder.
+     *
+     * When we already have decoded the signature, we don't need to do it again.
+     * By setting a flag for this we allow for byte by byte parsing.
+     */
+    png_byte* chunk;
+    if (!m_parsedSignature) {
+        if (reader.size() < m_readOffset + 8)

[scroggo_chromium, 2016/10/11 20:13:10]

I'm assuming m_readOffset will be zero at this point? I think other
ImageDecoders use this as a signal that m_parsedSignature is false, so you could
drop the extra boolean.

[joostouwerling, 2016/10/12 20:49:46]

I (wrongly?) assumed that PNGImageReader was constructed with a readOffset value
so it could pass in byte streams where the PNG image was not starting at byte 0
(for whatever reason). If that's not the case, then yes, checking if the
m_readOffset is zero would suffice.

[scroggo_chromium, 2016/10/13 13:49:32]

Oops - no, you are correct. In particular, I think the offset is used when the
PNG is inside an ICO.

+            return false;
+        chunk = readAsPngBytep(reader, m_readOffset, 8);
+        png_process_data(m_png, m_info, chunk, 8);
+        m_readOffset += 8;
+        m_parsedSignature = true;
+        // Initialize the newFrame by setting the readOffset to 0.
+        m_newFrame.readOffset = 0;
+    }
+
+    /*
+     * This loop peeks at the chunk tag until the IDAT chunk is found. When
+     * a different tag is encountered, pass it on to libpng for general parsing.
+     * We can peek at chunks by looking at the first 8 bytes, which contain the
+     * length and the chunk tag.
+     *
+     * When an fcTL (frame control) is encountered before the IDAT, the frame
+     * data in the IDAT chunk is part of the animation. This case is flagged
+     * and the frame info is stored by parsing the fcTL chunk.
+     */
+    while (reader.size() > m_readOffset + 8) {
+        chunk = readAsPngBytep(reader, m_readOffset, 8);

[scroggo_chromium, 2016/10/11 20:13:10]

I think you should separately declare png_byte* chunk each place it's used. You
don't actually take advantage of the fact that you're using the same variable
(and you don't want to).

+        png_uint_32 length = png_get_uint_32(chunk);

[scroggo_chromium, 2016/10/11 20:13:10]

nit: I think this can be const.

+
+        /*
+         * If we encounter the IDAT chunk, we're done with the header. 
+         * Indicate this to libpng by sending the beginning of the IDAT chunk

[scroggo_chromium, 2016/10/11 20:13:11]

We're telling libpng that we're done with the header? What does libpng do with
that information?

+         * The size is encoded in the header, so it can return true.
+         */
+        if (memcmp(chunk + 4, "IDAT", 4) == 0) {
+            m_newFrame.readOffset = m_readOffset;
+            png_process_data(m_png, m_info, chunk, 8);
+            return true;
+        };

[scroggo_chromium, 2016/10/11 20:13:11]

No need for a ";" here.

+
+        if (memcmp(chunk + 4, "fcTL", 4) == 0)
+            m_idatPartOfAnimation = true;
+        
+        // 12 is the length, tag and crc part of the chunk, which are all 4B.
+        if (reader.size() < m_readOffset + length + 12)
+            break;  
+
+        png_process_data(m_png, m_info, chunk, 8);
+        processData(data, m_readOffset + 8, length + 4);
+        m_readOffset += length + 12;
+    }
+
+    // If we end up here, not enough data was available for the IDAT chunk
+    // So libpng would not have called headerAvailable yet.
+    return false;
+} 
+
+
+void PNGImageReader::parseAnimationChunk(const char tag[], const void* data_chunk, size_t length)
+{
+    const png_byte* data = static_cast<const png_byte*>(data_chunk);
+
+    if (strcmp(tag, "acTL") == 0 && length == 8)
+        parseAnimationControl(data);
+    else if (strcmp(tag, "fcTL") == 0 && length == 26)
+        parseFrameInfo(data);
+
+}
+
+size_t PNGImageReader::frameCount() const
+{
+    return m_frameInfo.size();
+}
+
+const PNGImageReader::FrameInfo& PNGImageReader::frameInfo(size_t index) const
+{
+    ASSERT(index < m_frameInfo.size());
+    return m_frameInfo[index];
+}
+
+/*
+ * The number of frames as indicated in the animation control chunk is ignored,
+ * and the number of frames that are actually present is used. For now, when the
+ * number of indicated frames is different from the number of supplied frames, 
+ * the number of supplied frames is what is provided to the decoder. Therefore,
+ * it does not add any benefit of looking at the value of the indicated
+ * framecount. A note here is that there may be optimisations available by, for 
+ * example, prescaling vectors.
+ */
+void PNGImageReader::parseAnimationControl(const png_byte* data)

[scroggo_chromium, 2016/10/11 20:13:10]

nit: It seems like you could put this method's contents directly in the one
place it's used, since it's so small. It would also make me feel better about
you reading a seemingly arbitrary amount of data from the pointer. (The
call-site checks to make sure those numbers are safe.)

+{
+    png_uint_32 repetitionCount = png_get_uint_32(data + 4);
+    m_decoder->setRepetitionCount(repetitionCount);
+}
+
+/*
+ * These are mapped according to:
+ * https://wiki.mozilla.org/APNG_Specification#.60fcTL.60:_The_Frame_Control_Chunk
+ */
+inline ImageFrame::DisposalMethod getDisposalMethod(uint8_t disposalMethod)
+{
+    switch (disposalMethod) {
+        case 0:
+            return ImageFrame::DisposalMethod::DisposeKeep;
+        case 1:
+            return ImageFrame::DisposalMethod::DisposeOverwriteBgcolor;
+        case 2:
+            return ImageFrame::DisposalMethod::DisposeOverwritePrevious;
+    }
+    return ImageFrame::DisposalMethod::DisposeNotSpecified;
+}
+
+/*
+ * These are mapped according to:
+ * https://wiki.mozilla.org/APNG_Specification#.60fcTL.60:_The_Frame_Control_Chunk
+ */
+inline ImageFrame::AlphaBlendSource getAlphaBlend(uint8_t alphaBlend)
+{
+    if (alphaBlend == 1)
+        return ImageFrame::AlphaBlendSource::BlendAtopPreviousFrame;
+    return ImageFrame::AlphaBlendSource::BlendAtopBgcolor;
+}
+
+/*
+ * Extract the frame control info and store it in m_newFrame. Use the fcTL
+ * specification which can be found at:
+ * https://wiki.mozilla.org/APNG_Specification#.60fcTL.60:_The_Frame_Control_Chunk
+ */
+void PNGImageReader::parseFrameInfo(const png_byte* data)
+{
+    png_uint_32 width, height, xOffset, yOffset;
+    png_uint_16 delayNumerator, delayDenominator;
+    png_byte disposalMethod, alphaBlend;    
+    width               = png_get_uint_32(data + 4);
+    height              = png_get_uint_32(data + 8);
+    xOffset             = png_get_uint_32(data + 12);
+    yOffset             = png_get_uint_32(data + 16);
+    delayNumerator      = png_get_uint_16(data + 20);
+    delayDenominator    = png_get_uint_16(data + 22);
+    disposalMethod      = data[24];
+    alphaBlend          = data[25]; 
+
+    m_newFrame.duration         = (delayDenominator == 0) ? delayNumerator * 10
+                                    : delayNumerator * 1000 / delayDenominator;
+    m_newFrame.offset           = IntPoint(xOffset, yOffset);
+    m_newFrame.size             = IntSize(width, height);
+    m_newFrame.disposalMethod   = getDisposalMethod(disposalMethod);
+    m_newFrame.alphaBlend       = getAlphaBlend(alphaBlend);
+    
+}
+
+}; // namespace blink