WIP: Implement APNG

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

Index: third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoderTest.cpp
diff --git a/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoderTest.cpp b/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoderTest.cpp
new file mode 100644
index 0000000000000000000000000000000000000000..74547734413207e3e6015b66fc97fc1a96359913
--- /dev/null
+++ b/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoderTest.cpp
@@ -0,0 +1,226 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "platform/image-decoders/png/PNGImageDecoder.h"
+
+#include "platform/image-decoders/ImageDecoderTestHelpers.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include <memory>
+
+namespace blink {
+
+namespace {
+
+std::unique_ptr<ImageDecoder> createDecoder(ImageDecoder::AlphaOption alphaOption)
+{
+    return wrapUnique(new PNGImageDecoder(alphaOption,
+                                          ImageDecoder::GammaAndColorProfileApplied,
+                                          ImageDecoder::noDecodedImageByteLimit));
+}
+
+std::unique_ptr<ImageDecoder> createDecoder()
+{
+    return createDecoder(ImageDecoder::AlphaNotPremultiplied);
+}
+
+std::unique_ptr<ImageDecoder> createDecoderWithPngData(const char* pngFile)
+{
+    auto decoder = createDecoder();
+    auto data = readFile(pngFile);
+    EXPECT_EQ(false, data->isEmpty());

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

This can be EXPECT_FALSE(data->isEmpty())

Or maybe it should be ASSERT_FALSE. EXPECT is an error when running the test,
but ASSERT is a fatal error, so the test will stop. I'm not sure what happens
when called from a helper function that is expected to return a value, though.

+    decoder->setData(data.get(), true);
+    return decoder;
+}
+
+void testSize(const char* pngFile, IntSize expectedSize)
+{
+    auto decoder = createDecoderWithPngData(pngFile);
+    EXPECT_EQ(true, decoder->isSizeAvailable());
+    EXPECT_EQ(expectedSize, decoder->size());
+}
+
+void testRepetitionCount(const char* pngFile, int expectedRepetitionCount)
+{
+    auto decoder = createDecoderWithPngData(pngFile);
+    // Decode frame count should see the number of repetitions as well.
+    decoder->frameCount();
+    EXPECT_EQ(false, decoder->failed());
+    EXPECT_EQ(expectedRepetitionCount, decoder->repetitionCount());
+}
+
+/*
+ * Test whether the querying for the size of the image works if we present the

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

nit: "Test whether querying..." ("the" is not needed here).

+ * data byte by byte.
+ */
+void testSizeByteByByte(const char *pngFile, size_t frameOffset, IntSize expectedSize)

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

Why is this variable named "frameOffset"? I think this is actually the minimum
bytes needed to decode the size?

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

I understand the confusion and will change the name of the variable, but for
some background information: the size only becomes available when the first
frame (i.e. IDAT) chunk is parsed by libpng, because at that point it will call
the headerAvailable callback.

+{
+    auto decoder = createDecoder();
+    auto data = readFile(pngFile);
+    EXPECT_EQ(false, data->isEmpty());

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

This should probably be ASSERT_FALSE

+    EXPECT_LT(frameOffset, data->size());
+
+    for (size_t length = 1; length <= frameOffset; length++) {
+        RefPtr<SharedBuffer> tempData = SharedBuffer::create(data->data(), length);
+        decoder->setData(tempData.get(), false);
+
+        if (length < frameOffset) {
+            EXPECT_FALSE(decoder->isSizeAvailable());
+            EXPECT_TRUE(decoder->size().isEmpty());
+            EXPECT_FALSE(decoder->failed());
+        } else {
+            EXPECT_TRUE(decoder->isSizeAvailable());
+            EXPECT_EQ(expectedSize, decoder->size());
+        }
+    }
+    EXPECT_FALSE(decoder->failed());
+}
+
+struct FrameInfo {

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

I find this confusing. I guess this is just the publicly interesting members of
PNGImageReader::FrameInfo?

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

Yes. I can rephrase it so that becomes clearer. I prefer this instead of using
the FrameInfo struct of PNGImageReader, since, like you said, I don't need to
add the extra overhead here for the non-interesting bits.

+    size_t duration;
+    IntPoint offset;
+    IntSize size;
+    ImageFrame::AlphaBlendSource alphaBlend;
+    ImageFrame::DisposalMethod disposalMethod;
+};
+
+/*
+ * This is the frame data for the following PNG image:
+ * /LayoutTests/fast/images/resources/png-animated-idat-part-of-animation.png
+ */
+static FrameInfo pngAnimatedFrameInfo[] = {
+    {75, IntPoint(0, 0),   IntSize(100, 100), ImageFrame::BlendAtopBgcolor, ImageFrame::DisposeOverwriteBgcolor},
+    {75, IntPoint(31, 36), IntSize(38, 63),   ImageFrame::BlendAtopBgcolor, ImageFrame::DisposeOverwriteBgcolor},
+    {75, IntPoint(31, 41), IntSize(38, 55),   ImageFrame::BlendAtopBgcolor, ImageFrame::DisposeKeep},
+    {75, IntPoint(31, 34), IntSize(38, 65),   ImageFrame::BlendAtopBgcolor, ImageFrame::DisposeKeep},
+    {75, IntPoint(31, 23), IntSize(38, 76),   ImageFrame::BlendAtopBgcolor, ImageFrame::DisposeKeep},
+    {75, IntPoint(31, 18), IntSize(38, 81),   ImageFrame::BlendAtopBgcolor, ImageFrame::DisposeKeep},
+    {75, IntPoint(31, 17), IntSize(38, 82),   ImageFrame::BlendAtopBgcolor, ImageFrame::DisposeKeep},
+    {75, IntPoint(31, 14), IntSize(38, 85),   ImageFrame::BlendAtopBgcolor, ImageFrame::DisposeKeep},
+    {75, IntPoint(31, 8),  IntSize(38, 91),   ImageFrame::BlendAtopBgcolor, ImageFrame::DisposeKeep},
+    {75, IntPoint(31, 3),  IntSize(38, 96),   ImageFrame::BlendAtopBgcolor, ImageFrame::DisposeOverwriteBgcolor},
+    {75, IntPoint(31, 3),  IntSize(38, 36),   ImageFrame::BlendAtopBgcolor, ImageFrame::DisposeOverwriteBgcolor},
+    {75, IntPoint(31, 3),  IntSize(38, 35),   ImageFrame::BlendAtopBgcolor, ImageFrame::DisposeKeep},
+    {75, IntPoint(31, 3),  IntSize(38, 36),   ImageFrame::BlendAtopBgcolor, ImageFrame::DisposeKeep},
+    {75, IntPoint(31, 3),  IntSize(38, 96),   ImageFrame::BlendAtopBgcolor, ImageFrame::DisposeOverwriteBgcolor},
+    {75, IntPoint(31, 5),  IntSize(38, 94),   ImageFrame::BlendAtopBgcolor, ImageFrame::DisposeOverwriteBgcolor},
+    {75, IntPoint(31, 8),  IntSize(38, 91),   ImageFrame::BlendAtopBgcolor, ImageFrame::DisposeOverwriteBgcolor},
+    {75, IntPoint(31, 11), IntSize(38, 88),   ImageFrame::BlendAtopBgcolor, ImageFrame::DisposeOverwriteBgcolor},
+    {75, IntPoint(31, 15), IntSize(38, 84),   ImageFrame::BlendAtopBgcolor, ImageFrame::DisposeOverwriteBgcolor},
+    {75, IntPoint(31, 20), IntSize(38, 79),   ImageFrame::BlendAtopBgcolor, ImageFrame::DisposeOverwriteBgcolor},
+    {75, IntPoint(31, 25), IntSize(38, 74),   ImageFrame::BlendAtopBgcolor, ImageFrame::DisposeKeep}
+};
+
+void compareFrameWithExpectation(FrameInfo& expected, ImageFrame* frame)

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

I think both of these parameters can be marked const?

+{
+    EXPECT_EQ(expected.duration, frame->duration());
+    EXPECT_EQ(expected.offset, frame->originalFrameRect().location());

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

If you combine offset and size into an IntRect, I think you can combine this
line and the next one into:

EXPECT_EQ(expected.rect, frame->originalFrameRect());

+    EXPECT_EQ(expected.size, frame->originalFrameRect().size());
+    EXPECT_EQ(expected.disposalMethod, frame->getDisposalMethod());
+    EXPECT_EQ(expected.alphaBlend, frame->getAlphaBlendSource());
+}
+
+} // Anonymous namespace
+
+// Animated PNG Tests
+
+TEST(AnimatedPNGTests, sizeTest)
+{
+    testSize("/LayoutTests/fast/images/resources/png-animated-idat-part-of-animation.png", IntSize(100, 100));
+    testSize("/LayoutTests/fast/images/resources/png-animated-idat-not-part-of-animation.png", IntSize(227, 35));
+}
+
+// TODO(joostouwerling): make animated PNG's with a non-infinite rep count.
+TEST(AnimatedPNGTests, repetitionCountTest)
+{
+    testRepetitionCount("/LayoutTests/fast/images/resources/png-animated-idat-part-of-animation.png", cAnimationLoopInfinite);
+    testRepetitionCount("/LayoutTests/fast/images/resources/png-animated-idat-not-part-of-animation.png", cAnimationLoopInfinite);
+}
+
+// Test if the decoded metdata corresponds to the defined expectations
+TEST(AnimatedPNGTests, MetaDataTest)
+{
+    const char* pngFile = "/LayoutTests/fast/images/resources/png-animated-idat-part-of-animation.png";
+    constexpr size_t expectedFrameCount = 20;
+
+    auto decoder = createDecoderWithPngData(pngFile);
+    EXPECT_EQ(expectedFrameCount, decoder->frameCount());

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

This should probably be ASSERT_EQ. If frameCount() returns less,
frameBufferAtIndex will start returning nullptr, resulting in some bad
dereferences.

+    for (size_t i = 0; i < expectedFrameCount; i++)
+        compareFrameWithExpectation(pngAnimatedFrameInfo[i],
+                                    decoder->frameBufferAtIndex(i));
+}
+
+TEST(AnimatedPNGTests, ByteByByteSizeAvailable)
+{
+    testSizeByteByByte("/LayoutTests/fast/images/resources/png-animated-idat-part-of-animation.png",
+                        100u, IntSize(100, 100));
+    testSizeByteByByte("/LayoutTests/fast/images/resources/png-animated-idat-not-part-of-animation.png",
+            80u, IntSize(227, 35));
+}
+
+/*
+ * Test whether the frame meta data decoding also works when we provide the data
+ * byte by byte. This should cover the case when the client does not provide
+ * all data at once. At given offsets, we expect frames to become available.
+ * This test checks whether that is the case, and if so, if the frame data is
+ * equal to what we expected.
+ */
+TEST(AnimatedPNGTests, ByteByByteMetaData)
+{
+    const char* pngFile = "/LayoutTests/fast/images/resources/png-animated-idat-part-of-animation.png";
+    constexpr size_t expectedFrameCount = 20;
+
+    /*
+     * These are the byte offsets where each frame should have been parsed.
+     * It boils down to the offset of the first fcTL / IEND after the last
+     * frame data chunk, plus 8 bytes for recognition.
+     */
+    size_t frameOffsets[expectedFrameCount] = { 4745,  9139, 13231, 17109,
+                                               20680, 23898, 26727, 29365,
+                                               32135, 34977, 37692, 40323,
+                                               43025, 45849, 48743, 51679,
+                                               54695, 57942, 61510, 65555};
+    auto decoder = createDecoder();
+    auto data = readFile(pngFile);
+    EXPECT_EQ(false, data->isEmpty());

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

ASSERT_FALSE

+    size_t framesParsed = 0;
+
+    for (size_t length = 1; length <= frameOffsets[expectedFrameCount - 1]; length++) {
+        RefPtr<SharedBuffer> tempData = SharedBuffer::create(data->data(), length);

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

We have some infrastructure to detect if your test takes longer than 50ms. If
so, it will file a bug and assign it to you. But you should go ahead and time
this (gtest will print out the time for each test). If it's too long,
crrev.com/1405053005/ shows how we shortened similar tests in the past.

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

Yep, my test runs in 800ms. I'll first work on a smaller image with fewer
frames, and see if that solves it for now. As far as I can see there is no
difference with testing a 3px by 3px image with 3 frames. 

+        decoder->setData(tempData.get(), false);
+        ASSERT(!decoder->failed());
+
+        if (length < frameOffsets[framesParsed]) {
+            EXPECT_EQ(framesParsed, decoder->frameCount());
+        } else {
+            EXPECT_EQ(framesParsed + 1, decoder->frameCount());

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

Why don't they match exactly?

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

When the code is at an offset where a new frame should appear, framesParsed has
not been incremented yet. That happens two lines later. This makes it easy to
access the frame info since the index is framecount minus 1.

+            compareFrameWithExpectation(pngAnimatedFrameInfo[framesParsed],
+                                        decoder->frameBufferAtIndex(framesParsed));
+            framesParsed++;
+        }
+    }
+    EXPECT_EQ(expectedFrameCount, decoder->frameCount());
+    EXPECT_FALSE(decoder->failed());
+}
+
+// Static PNG tests
+
+TEST(StaticPNGTests, repetitionCountTest)
+{
+    testRepetitionCount("/LayoutTests/fast/images/resources/png-simple.png", cAnimationNone);
+}
+
+TEST(StaticPNGTests, sizeTest)
+{
+    testSize("/LayoutTests/fast/images/resources/png-simple.png", IntSize(111, 29));
+}
+
+TEST(StaticPNGTests, MetaDataTest)
+{
+    size_t expectedFrameCount = 1;
+    size_t expectedDuration = 0;
+    auto decoder = createDecoderWithPngData("/LayoutTests/fast/images/resources/png-simple.png");
+    EXPECT_EQ(expectedFrameCount, decoder->frameCount());

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

nit: I would prefer to hardcode these numbers e.g.

EXPECT_EQ(1, decoder->frameCount());

I don't feel terribly strongly, though, and perhaps you have an argument for
doing it this way.

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

I prefer, but also not very strongly, this output:

Value of: decoder->frameCount()
  Actual: 1
Expected: expectedFrameCount
Which is: 10

to

Value of: decoder->frameCount()
  Actual: 1
Expected: 10u
Which is: 10

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

Wow, haha, I have not paid that much attention to the output. I agree that the
former is much more interesting!

+    EXPECT_EQ(expectedDuration, decoder->frameDurationAtIndex(0));
+}
+}; // namespace blink