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..52c9d7552743f5c8a921917711d90f914d719255 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,63 @@
 #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_frameCountDecoded(false)
+    , m_frameCount(0)
 {
-    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_frameCountDecoded)
+        return m_frameCount;
+
+    parse(PNGParseQuery::PNGFrameCountQuery);
+    // Similar reasoning to GIFImageDecodeer. If decoding fails, we don't

[cblume, 2016/10/01 12:01:14]

Probably don't need to mention the GIF decoder.
The description itself was clear.

[joostouwerling, 2016/10/01 17:51:38]

Acknowledged.

+    // 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)
+inline bool isComplete(const PNGImageDecoder* decoder)
 {
-    longjmp(JMPBUF(png), 1);
+    return decoder->frameIsCompleteAtIndex(0);

[cblume, 2016/10/01 12:01:14]

I think this shouldn't be index 0. Maybe?

The old decode() had a comment explaining that once the decoding is done or we
have failed, we no longer need the PNGImageReader.

decode() became parse() and the comment was removed. But it seems to be doing
the same thing.

I think we only really want to return true from isComplete once the final frame
has been decoded, right?


As a side note, how does that work when the frames are purged?

[joostouwerling, 2016/10/01 17:51:38]

You're correct. This slipped through since it was not relevant for decoding the
frame count in the simple approach of this CL. I'll correct this in a later CL. 
Hmm... would it be considered complete if the client purges all frames expect
the final frame? We arguably do not need the reader if we have all frames
cached, so in that sense, isComplete should return if all frames are complete.
One way could be, and I think this is how it works for GIF, is to go over the
data with a FastSharedBufferReader when collecting metadata and keep track of
where frame chunks start and end. Especially if we will collect all metadata
upfront (see the comment below) this seems the most logical approach to me. 

[cblume, 2016/10/02 19:55:43]

I took a quick look at the GIF decoder and it doesn't release the reader.

I think the reasoning is:
- If we have a still image, we can decode it and destroy the reader. If that
image itself is ever purged, a new ImageDecoder will be created, which creates a
new reader.
- If we have an animated image, the animation frames are purged constantly and
we need to reread them. So we never get to destroy the reader.

If my understanding is correct, we should change the parse() function below to
not m_reader.reset().


In addition to that, the GIF decoder doesn't seem to have an isComplete(). It
has one for frame index. Maybe there is no real concept of being complete since
frames are purged.

[joostouwerling, 2016/10/03 14:39:10]

Ack.
Note that isComplete is just an inline function (not a method of
PNGImageDecoder) that is *probably* only used for naming clarity reasons in this
context. With your reasoning above it makes sense that GIFImageDecoder does not
have a similar function.

Though, for static PNG there was a reason to delete the reader, apparently. My
guess would be to free up memory. If we are dealing with static PNG's, we can
still release the reader when the frame is loaded.

[scroggo_chromium, 2016/10/04 14:50:16]

I would remove the method isComplete entirely. I don't think it provides any
clarity.

BTW, frameIsCompleteAtIndex is a very confusing method. It was introduced for
animated images, but it was really intended to mean that we've received (and
parsed - only way for us to know it was received) all the data for an image
frame. But for single frame images we use to mean FrameStatus::FrameComplete
(FWIW, in that case we don't know whether we've received "enough" data for fully
frame until we decode). There was an effort to make this more clear (see
crrev.com/1962563002), but the person working on it has dropped it.

 }
 
-} // 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);
-
-        m_readOffset = 0;
-    }
-
-    bool decode(const SegmentReader& data, bool sizeOnly)
-    {
-        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;
-    }
+void PNGImageDecoder::parse(PNGParseQuery query)
+{
+    if (failed())
+        return;
 
-    png_structp pngPtr() const { return m_png; }
-    png_infop infoPtr() const { return m_info; }
+    if (!m_reader)
+        m_reader = wrapUnique(new PNGImageReader(this, m_offset));
 
-    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; }
+    if (!m_reader->parse(*m_data, query))
+        setFailed();
 
-    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 (isComplete(this) || failed())
+        m_reader.reset();
 
-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
-};
+}
 
-PNGImageDecoder::PNGImageDecoder(AlphaOption alphaOption, GammaAndColorProfileOption colorOptions, size_t maxDecodedBytes, size_t offset)
-    : ImageDecoder(alphaOption, colorOptions, maxDecodedBytes)
-    , m_offset(offset)
+bool PNGImageDecoder::isDecodedFrameCountAvailable() const
 {
+    return !failed() && m_frameCountDecoded;
 }
 
-PNGImageDecoder::~PNGImageDecoder()
+void PNGImageDecoder::animationControlAvailable(size_t numFrames, size_t numRepetitions)
 {
+    m_frameCount = numFrames;
+    m_frameCountDecoded = true;
 }
 
 void PNGImageDecoder::headerAvailable()
@@ -285,6 +226,13 @@ void PNGImageDecoder::headerAvailable()
 
 void PNGImageDecoder::rowAvailable(unsigned char* rowBuffer, unsigned rowIndex, int)
 {
+    // If we see a frame, but the frame count has not yet been decoded, we are

[scroggo_chromium, 2016/10/04 14:50:16]

Nit: I would lean towards using the word "parsed" here. I don't think "decoded"
is wrong, per se, but elsewhere (e.g. GIFImageDecoder) we use "parse" to mean
reading through the stream and storing the metadata, and "decode" to mean
decoding the pixels.

+    // dealing with a non animated PNG.

[cblume, 2016/10/01 12:01:14]

When a GIF says it has 3 frames but it actually has 5, I think we go with the 5
frames. I could be wrong about that.

But that said, we may want to have a same behavior, which ever it is. There may
not be a chunk telling us how many frames there are. Despite that, if we find
multiple frames, maybe we want to use them.

[joostouwerling, 2016/10/01 17:51:39]

This is similar to the discussion in issue 2045293002. The consensus seems that
fetching all the meta data, as far as we can, is the approach we want to take.
In that case, it makes sense to take the same approach as the GIF image decoder,
indeed.

For APNG, this means we need to decide how to handle some forms of wrongly
formatted files. Most importantly, each frame starts with one frame control
chunk and is followed by one or more frame data chunks. All these chunks have a
sequence number, which starts at 0 for the first frame control chunk, and must
increment by 1 for each next chunk. See
https://wiki.mozilla.org/APNG_Specification#Chunk_Sequence_Numbers for more
details.

How do we deal with a) chunk sequence numbers that are out of order, and b) gaps
in the sequence numbers? The spec page linked to above suggests treating these
cases as erroneous images, but we can try to repair them by, for a), rearranging
the chunks and for b), try to ignore the gaps and see if the chunks still makes
sense, or maybe ignoring frames with gaps and only take well sequenced frames
into account?

That being said, afaik there is no data available on how many images out there
on the web are wrongly formatted. If, say, 99.9% is well formatted, it may not
be worth the extra effort to fetch all metadata upfront. But that's another
discussion. 

[cblume, 2016/10/02 19:55:43]

On 2016/10/01 17:51:39, joostouwerling wrote: 
I'm not very familiar with how we handle errors in files like gif / webp. I
think we ignore the error and assume it was a mistake.

So for example, if the indices are wrong we might just ignore the index and
don't rearrange or anything. Just assume the next frame is indeed the next
frame.


I'm not too worried about this, however. Like you said, there might not be too
many apngs out there with errors in them. So long as there isn't already a
history for bad files, I'm okay simply not displaying them the way an author may
have intended. It should be their responsibility to format the image correctly.
So whatever you think is best is fine by me.

[scroggo_chromium, 2016/10/04 14:50:16]

GIF does not have any notion of number of frames (you just keep decoding until
there are no more frames) or sequence numbers.

There are some errors that we fix (e.g. first frame is outside the overall
bounds [1]) and others we ignore (e.g. second frame is outside the overall
bounds - we just clip).

And yet other errors we call setFailed(), which deletes the reader [2]. I think
really we just want to stop trying to parse after that, but it also has the
effect that we cannot redecode old frames. Currently, if an image has an error i
frames in, we stop looping, even if we're supposed to loop [3]. I think we have
some inconsistent code around failed() - in some cases we try to report valid
values even though we've already failed (e.g. in
GIFImageDecoder::decodeFrameCount [4], which you've mimicked here), even though
we've lost some functionality (e.g. decoding old frames).

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image...
[2]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image...
[3] crbug.com/267883
[4]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image...
That sounds like the right approach to me. If we get some reports/complaints
that we did the wrong thing, we can revisit.
Back to the specific (potential) error here - we must be reading an IDAT chunk
without having seen either an acTL or an fcTL. The missing acTL seems less
important to me - it tells how many frames there will be (which we may not
respect) and how many times to loop (we can come up with a sensible default).
But the fcTL chunk tells us

- how long to display this frame
- the rectangle of this frame
- how to dispose of this frame
- how to blend this frame

Without all that information, I don't know how we can sensibly guess what the
author intended and try to animate (assuming there's a frame that follows this
one). So I think it's correct to interpret this as meaning that it is a single
frame image.

All that said, perhaps we can know this before we get here? The way I imagine
parse() working (haven't gotten there yet), it will try to parse the whole
stream and then we can come back and decode individual frames. As soon as parse
sees the IDAT chunk, we know there's only one frame, so by the time we get to
this method we do not need to check.

[joostouwerling, 2016/10/04 17:48:31]

If there is no fcTL before the IDAT chunk, it only serves as a "default" image,
and is not part of the animation. So in that case, we only need to look at the
subsequent fcTL and fdAT chunks. But there may be some UX improvements possible,
like showing the default image until we've decoded enough to have a somewhat
smooth animation.

+    if (!m_frameCountDecoded) {
+        m_frameCountDecoded = true;
+        m_frameCount = 1;
+    }
+
     if (m_frameBufferCache.isEmpty())
         return;
 
@@ -417,27 +365,4 @@ void PNGImageDecoder::complete()
     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