WIP: Implement APNG

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

Index: third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp
diff --git a/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp b/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp
index dff41e392ebff0c2226f28f98a7fb5d92c734c52..234cce7bd3e4a44df2defd3ab8d1b4bf64a4bdf0 100644
--- a/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp
@@ -38,10 +38,11 @@
 
 #include "platform/image-decoders/png/PNGImageDecoder.h"
 
+#include "platform/image-decoders/png/PNGImageReader.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
@@ -55,123 +56,80 @@
 #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));
-}
+namespace blink {
 
-void PNGAPI pngHeaderAvailable(png_structp png, png_infop)
+PNGImageDecoder::PNGImageDecoder(AlphaOption alphaOption, GammaAndColorProfileOption colorOptions, size_t maxDecodedBytes, size_t offset)
+    : ImageDecoder(alphaOption, colorOptions, maxDecodedBytes)
+    , m_offset(offset)
+    , m_metaDataDecoded(false)
+    , m_frameCount(0)
+    , m_repetitionCount(cAnimationNone)
 {
-    imageDecoder(png)->headerAvailable();
 }
 
-void PNGAPI pngRowAvailable(png_structp png, png_bytep row, png_uint_32 rowIndex, int state)
+PNGImageDecoder::~PNGImageDecoder()
 {
-    imageDecoder(png)->rowAvailable(row, rowIndex, state);
 }
 
-void PNGAPI pngComplete(png_structp png, png_infop)
+size_t PNGImageDecoder::decodeFrameCount()
 {
-    imageDecoder(png)->complete();
+    if (m_metaDataDecoded)
+        return m_frameCount;

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

If we failed, does this get set to m_frame_bufferCache.size()? If not, I think
this is incorrect. But if so, you can simplify the later return statement to
just say "return m_frameCount".

In fact, this could perhaps be

if (!m_metaDataDecoded) {
  parse(...);
}

return m_frameCount;

You can override setFailed() to update m_frameCount, and that might be the
easiest way to make sure this happens.

(It would be good to add a test that verifies this is correct.)

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

I'm not sure this answers completely what you're getting at, but: if
m_metaDataDecoded is set to true, it means m_reader was able to parse the frame
count up to the IEND chunk and has returned true. Since it has returned true,
the parse() method of the decoder will update m_frameCount to the number of
frames the reader has found.

But your proposal looks cleaner. I'll see if I can nicely implement it.

[joostouwerling, 2016/10/13 18:50:22]

Now that I've been thinking about his, I don't think there is a need for
changing |m_frameCount| when decoding fails, at all. When the first query for
the frame count succeeds (up to a certain point in the stream), it will set
|m_frameCount| to the number of frames it has seen so far, at the end of
parse(PNGParseQuery). This propagates back to ImageDecoder::frameCount() where
it will resize |m_frameBufferCache|. 

Now, if we change the following:

--------------
if (query == PNGParseQuery::PNGMetaDataQuery)
    m_frameCount = m_reader->frameCount();

===>

if (!failed() && query == PNGParseQuery::PNGMetaDataQuery)
    m_frameCount = m_reader->frameCount();
--------------

When the decoding fails on the next decodeFrameCount call, or through any other
call for that matter, |m_frameCount| won't be changed and will still be equal to
|m_frameBufferCache|.size(), which is the desired behavior, if I'm correct. 

That is, what is the desired behavior when, on the first decodeFrameCount call,
we've parsed the metadata of the first three frames, but it fails on the fourth
frame. Do we want to report a complete failure or do we want to report the image
having three frames?

+
+    parse(PNGParseQuery::PNGMetaDataQuery);
+    // If decoding fails, we don't return 0 but we return the existing number
+    // of frames. This prevents us from reporting zero frames when decoding
+    // fails halfway through the image.
+    return failed() ? m_frameBufferCache.size() : m_frameCount;
 }
 
-void PNGAPI pngFailed(png_structp png, png_const_charp)
+void PNGImageDecoder::decode(size_t index)
 {
-    longjmp(JMPBUF(png), 1);
+    m_reader->decode(*m_data, index);

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

Are we guaranteed to have an m_reader here?

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

m_reader is not deleted by the decoder anymore.

The prerequisite for this method is that decodeFrameCount() is called, since
that method will create m_reader and populates the frame info. I assume that all
clients use the ImageDecoders in this way. I can add a check if the requested
index is within the number of frames we found, but the result won't change so
much with it.

Note that most work on this method is done in the next iteration, where frames
are decoded.

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

I believe that is correct, but it is fragile. Note that the GIF override of this
method [1] calls parse, in case the client did not call decodeFrameCount. parse
should be fairly cheap if we have already parsed.

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image...
That doesn't seem necessary. I presume m_reader->decode() will do that check.

 }
 
-} // namespace
-
-namespace blink {
-
-class PNGImageReader final {
-    USING_FAST_MALLOC(PNGImageReader);
-    WTF_MAKE_NONCOPYABLE(PNGImageReader);
-public:
-    PNGImageReader(PNGImageDecoder* decoder, size_t readOffset)
-        : m_decoder(decoder)
-        , m_readOffset(readOffset)
-        , m_currentBufferSize(0)
-        , m_decodingSizeOnly(false)
-        , m_hasAlpha(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);
-    }
-
-    ~PNGImageReader()
-    {
-        png_destroy_read_struct(m_png ? &m_png : 0, m_info ? &m_info : 0, 0);
-        ASSERT(!m_png && !m_info);
+void PNGImageDecoder::parse(PNGParseQuery query)
+{
+    if (failed())
+        return;
 
-        m_readOffset = 0;
-    }
+    if (!m_reader)
+        m_reader = wrapUnique(new PNGImageReader(this, m_offset));
 
-    bool decode(const SegmentReader& data, bool sizeOnly)
+    if (!m_reader->parse(*m_data, query) && isAllDataReceived())
     {

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

Curly braces go on their own line for methods, but I think for if statements
they go on the same line, e.g.

if (condition) {

(That said, once you remove the print statement, there should be no braces,
according to the style guide.)

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

Ack, that was due to debugging. 

-        m_decodingSizeOnly = sizeOnly;
-
-        // We need to do the setjmp here. Otherwise bad things will happen.
-        if (setjmp(JMPBUF(m_png)))
-            return m_decoder->setFailed();
-
-        const char* segment;
-        while (size_t segmentLength = data.getSomeData(segment, m_readOffset)) {
-            m_readOffset += segmentLength;
-            m_currentBufferSize = m_readOffset;
-            png_process_data(m_png, m_info, reinterpret_cast<png_bytep>(const_cast<char*>(segment)), segmentLength);
-            if (sizeOnly ? m_decoder->isDecodedSizeAvailable() : m_decoder->frameIsCompleteAtIndex(0))
-                return true;
-        }
-
-        return false;
+        SkDebugf("Setting failed in PNGImageDecoder::parse");

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

You'll need to remove this before submitting.

+        setFailed();
     }
 
-    png_structp pngPtr() const { return m_png; }
-    png_infop infoPtr() const { return m_info; }
-
-    size_t getReadOffset() const { return m_readOffset; }
-    void setReadOffset(size_t offset) { m_readOffset = offset; }
-    size_t currentBufferSize() const { return m_currentBufferSize; }
-    bool decodingSizeOnly() const { return m_decodingSizeOnly; }
-    void setHasAlpha(bool hasAlpha) { m_hasAlpha = hasAlpha; }
-    bool hasAlpha() const { return m_hasAlpha; }
-
-    png_bytep interlaceBuffer() const { return m_interlaceBuffer.get(); }
-    void createInterlaceBuffer(int size) { m_interlaceBuffer = wrapArrayUnique(new png_byte[size]); }
-#if USE(QCMSLIB)
-    png_bytep rowBuffer() const { return m_rowBuffer.get(); }
-    void createRowBuffer(int size) { m_rowBuffer = wrapArrayUnique(new png_byte[size]); }
-#endif
+    if (query == PNGParseQuery::PNGMetaDataQuery)
+        m_frameCount = m_reader->frameCount();
+}
 
-private:
-    png_structp m_png;
-    png_infop m_info;
-    PNGImageDecoder* m_decoder;
-    size_t m_readOffset;
-    size_t m_currentBufferSize;
-    bool m_decodingSizeOnly;
-    bool m_hasAlpha;
-    std::unique_ptr<png_byte[]> m_interlaceBuffer;
-#if USE(QCMSLIB)
-    std::unique_ptr<png_byte[]> m_rowBuffer;
-#endif
-};
+void PNGImageDecoder::setRepetitionCount(size_t repetitionCount)
+{
+    m_repetitionCount = (repetitionCount == 0) ? cAnimationLoopInfinite
+                            : repetitionCount;
+}
 
-PNGImageDecoder::PNGImageDecoder(AlphaOption alphaOption, GammaAndColorProfileOption colorOptions, size_t maxDecodedBytes, size_t offset)
-    : ImageDecoder(alphaOption, colorOptions, maxDecodedBytes)
-    , m_offset(offset)
+// FIXME(joostouwerling): crbug.com/267883

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

It is good to have the bug as a reference, but it would also be helpful to say a
little something here like

This matches the existing behavior to loop once if decoding fails, but this
should be changed to stick with m_repititionCount to match other browsers. See
crbug.com/267883

+int PNGImageDecoder::repetitionCount() const
 {
+    return failed() ? cAnimationLoopOnce : m_repetitionCount;
 }
 
-PNGImageDecoder::~PNGImageDecoder()
+void PNGImageDecoder::initializeNewFrame(size_t index)
 {
+    const auto& frameInfo = m_reader->frameInfo(index);

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

I'm not convinced using "auto" here helps readability (see
https://google.github.io/styleguide/cppguide.html#auto)

+    auto buffer = &m_frameBufferCache[index];

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

If we use "auto" here, I think it should be "auto*", but I think it would
clearer to specify the type.

+
+    IntRect frameRect(frameInfo.offset, frameInfo.size);
+    IntRect frameRectWithinSize = intersection(frameRect, {IntPoint(), size()});
+    buffer->setOriginalFrameRect(frameRectWithinSize);
+    buffer->setDuration(frameInfo.duration);
+    buffer->setDisposalMethod(frameInfo.disposalMethod);
+    buffer->setAlphaBlendSource(frameInfo.alphaBlend);
 }
 
 void PNGImageDecoder::headerAvailable()
@@ -270,21 +228,11 @@ void PNGImageDecoder::headerAvailable()
     ASSERT(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)
-        // '0' argument to png_process_data_pause means: Do not 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
-    }
 }
 
 void PNGImageDecoder::rowAvailable(unsigned char* rowBuffer, unsigned rowIndex, int)
 {
+
     if (m_frameBufferCache.isEmpty())
         return;
 
@@ -413,31 +361,6 @@ void PNGImageDecoder::complete()
 {
     if (m_frameBufferCache.isEmpty())
         return;
-
-    m_frameBufferCache[0].setStatus(ImageFrame::FrameComplete);
-}
-
-inline bool isComplete(const PNGImageDecoder* decoder)
-{
-    return decoder->frameIsCompleteAtIndex(0);
-}
-
-void PNGImageDecoder::decode(bool onlySize)
-{
-    if (failed())
-        return;
-
-    if (!m_reader)
-        m_reader = wrapUnique(new 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();
 }
 
 } // namespace blink