Bug 17022 – Add APNG support

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[José Jeria]

Please add APNG support to WebKit. Firefox and Opera already supports this.

[Mark Rowe (bdash)]

WebKit uses the platform's underlying image libraries for format support, so APNG would need to be implemented there for WebKit to decode it.  This would require ImageIO on Mac/Windows to support it (<rdar://problem/5618737>), and QImage for Qt.  Only the Gtk and wx ports make use of the bare basic image decoders in the WebKit source tree.

[Mark Rowe (bdash)]

Closer inspection shows that our in-tree PNG decoder is just a wrapper around libpng.  For the Gtk and wx ports to support APNG we would need libpng to support it.

[Christian Dywan]

Incidentally there are patches available for libpng to support apng, but they are still waiting for approval. Time will tell whether WebKit needs to ship a fork of libpng or upstream supports this in the future.

http://littlesvr.ca/apng/

[Mark Steele]

Created attachment 30764 [details]
Add APNG support, re-bundle libpng with APNG patch

Rough patch to PNGImageDecoder to support APNG.
Bundle libpng with APNG patch.
A few things are missing/broken, but basic support works on Webkit/GTK+

[Alexey Proskuryakov]

Bug 43828 tracks APNG support in Safari (as mentioned above, that's blocked on ImageIO).

[Jared Grippe]

want!

[Max Stepin]

Created attachment 192943 [details]
v01_apng_webkit.patch

If external libpng supports APNG extension, ask libpng to decode animation frames. 

Some Linux distros (Arch, Gentoo, Mageia, etc) ship with apng-patched libpng as a system library. With this patch WebKit would take advantage of that and request libpng to read APNG frames.

[Adam Barth]

Comment on attachment 192943 [details]
v01_apng_webkit.patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192943&action=review

We shouldn't land this patch unless one or more WebKit work wishes to enable this code.  If we're going to accept this patch, we'll also need a number of tests.

> Source/WebCore/ChangeLog:3
> +        [GTK] [wx] Add APNG support

This issue is not specific to the GTK and wx ports, so it shouldn't have these prefixes.

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:108
> +#ifdef PNG_APNG_SUPPORTED

This should read:

#if ENABLE(APNG)

Also, all the code introduced to support APNG should be behind this compile flag.

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:241
> +    , m_frameIsHidden(false)
> +    , m_curFrame(0)
> +    , m_loopsCount(0)
> +    , m_delayNum(1)
> +    , m_delayDen(1)
> +    , m_dispose(0)
> +    , m_blend(0)

All this state as well.

[ManDay]

Please do NOT add "apng" support to Webkit. "gif" was a first mistake in design of the web - these days luckily being replaced by proper video one after another. Adding apng works against the combined efforts to build a clean (flash free, GIF free) web where images are images (displayed by webkit itself) and animations are animations (displayed by gstreamer or a similar library which specializes on rendering animations).

Seperation of responsibilities and specialization is the foundation of open source and provides clean, stream-lined concepts. The PNG-developers do not disapprove of apng without a reason.

[Christian Dywan]

(In reply to comment #8)
> We shouldn't land this patch unless one or more WebKit work wishes to enable this code.  If we're going to accept this patch, we'll also need a number of tests.

From Midori's perspective, I'd encourage enabling APNG - if WebKitGTK/ Epiphany team agree. In Archlinux it could work out of the box.

[Max Stepin]

Created attachment 196541 [details]
v02_apng_webkit.patch

New code is now in 

#if ENABLE(APNG) && defined(PNG_APNG_SUPPORTED)
#endif

Also added this ENABLE_APNG flag into Source/WTF/wtf/FeatureDefines.h

[Benjamin Poulain]

Comment on attachment 196541 [details]
v02_apng_webkit.patch

No tests?

Did any port actually agree to maintain this? If not, there is no point.

[Allan Sandfeld Jensen]

(In reply to comment #11)
> Created an attachment (id=196541) [details]
> v02_apng_webkit.patch
> 
> New code is now in 
> 
> #if ENABLE(APNG) && defined(PNG_APNG_SUPPORTED)
> #endif
> 
> Also added this ENABLE_APNG flag into Source/WTF/wtf/FeatureDefines.h

I would suggest setting ENABLE_APNG based on defined(PNG_APNG_SUPPORTED), that way you don't have to check for both, and ENABLE_APNG will only be set when APNG is supported.

[Allan Sandfeld Jensen]

(In reply to comment #12)
> (From update of attachment 196541 [details])
> No tests?
> 
> Did any port actually agree to maintain this? If not, there is no point.

Hopefully it will not require much maintaince. Though it will need tests, and someone should run a bot with it enabled. I guess for GTK and Qt it will depend on the libpng available on the system.

[Max Stepin]

Created attachment 236137 [details]
 v03_apng_webkit.patch

I rewrote the APNG support code, please take a look.
     
This version decodes APNG using only standard libpng (turns out it's possible).
     
I also added test files: a dozen small animations that would play once, and finish asap. After a short delay a pixel-test can be compared against the expected result. It covers most of the apng features at different png colortypes.
     
I'm going to ask GTK port people, and will try to contact other ports too.

     
P.S. Safari's own implementation of APNG might benefit from the test suite too.

[Benjamin Poulain]

I think you could convert the test to a ref-test instead of a pixel test. Tests should have a description and explanations of what is tested and how.

[Max Stepin]

OK. I was looking at LayoutTests/fast/images/png-suite/test.html 
as an example. But I can convert it into a reftest.

[Max Stepin]

Created attachment 236335 [details]
v04_apng_webkit.patch

Now with reftest. All animations included in the reftest should play once and stop at the last frame. After that, compare the result against reference static PNGs.

[Max Stepin]

Created attachment 236373 [details]
v05_apng_webkit.patch

Previous patch was formatted incorrectly. Fixed now.

[Max Stepin]

Created attachment 236375 [details]
v06_apng_webkit.patch

Really fixed now. Sorry, I'm new to this.

[Build Bot]

Comment on attachment 236375 [details]
v06_apng_webkit.patch

Attachment 236375 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6305820962717696

New failing tests:
fast/images/animated-png.html

[Build Bot]

Created attachment 236396 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 236375 [details]
v06_apng_webkit.patch

Attachment 236375 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4795727104442368

New failing tests:
fast/images/animated-png.html

[Build Bot]

Created attachment 236402 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 236375 [details]
v06_apng_webkit.patch

Attachment 236375 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5712234470703104

New failing tests:
fast/images/animated-png.html

[Build Bot]

Created attachment 236406 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 236375 [details]
v06_apng_webkit.patch

Attachment 236375 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5681974278619136

New failing tests:
fast/images/animated-png.html

[Build Bot]

Created attachment 236412 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Max Stepin]

Created attachment 236444 [details]
v07_apng_webkit.patch

It appears that mac target doesn't use WebKit's internal image decoders, so I'll going to skip that test, just for mac.

[Max Stepin]

Comment on attachment 236444 [details]
v07_apng_webkit.patch

I'm adding "review?" flag, since EFL is ok with enabling it as experimental feature. See here:
https://lists.webkit.org/pipermail/webkit-efl/2014-August/thread.html

I'm willing to help with the maintenance, the code is well tested and clean, so I don't expect any issues.

[OC2PS]

+1

Add APNG support. 

Max's patch works. And he's willing to maintain. 

Takes away all excuses not to.

[Benjamin Poulain]

(In reply to comment #31)
> +1
> 
> Add APNG support. 
> 
> Max's patch works. And he's willing to maintain. 
> 
> Takes away all excuses not to.

This is for GTK right?

Has anyone contacted the GTK folks to get a review on this patch? WebKit-GTK has a mailing list: https://lists.webkit.org/mailman/listinfo/webkit-gtk

[Max Stepin]

I asked them twice back at August, but couldn't get any straight answer:
https://lists.webkit.org/pipermail/webkit-gtk/2014-August/thread.html

ELF folks seems to be a little bit interested, and willing to try it as experimental feature, but I'll understand if you think one small port is not enough.

I really wish GTK folks would make their position clear.

[Benjamin Poulain]

(In reply to comment #33)
> I asked them twice back at August, but couldn't get any straight answer:
> https://lists.webkit.org/pipermail/webkit-gtk/2014-August/thread.html
> 
> ELF folks seems to be a little bit interested, and willing to try it as
> experimental feature, but I'll understand if you think one small port is not
> enough.
> 
> I really wish GTK folks would make their position clear.

The GTK port is more active than EFL, they also have more users. I am concerned about landing this if they do not approve.

[Carlos Garcia Campos]

Comment on attachment 236444 [details]
v07_apng_webkit.patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236444&action=review

I'm fine with adding APNG support to WebKitGTK+. I don't know the APNG details, so I can't review this from that point of view (fortunately it includes layout tests), but I can comment about coding style and other general nits. Thanks for the patch!

> Source/WTF/ChangeLog:3
> +        Added ENABLE(APNG)

Use the bug title here

> Source/WebCore/platform/image-decoders/ImageDecoder.h:169
> +        inline unsigned divide255(unsigned a)

This could probably be static

> Source/WebCore/platform/image-decoders/ImageDecoder.h:177
> +            if (!a)
> +                return;

Shouldn't we initialize dest in this case? *dest = 0;

> Source/WebCore/platform/image-decoders/ImageDecoder.h:201
> +                } else
> +                if (m_premultiplyAlpha) {

This should be a single line.

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:244
> +    , m_png(0)

Use nullptr instead of 0 to initialize all pointer members.

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:259
> +    , m_width(0)
> +    , m_height(0)

Could this be an IntSize?

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:261
> +    , m_xOffset(0)
> +    , m_yOffset(0)

And this IntPoint?

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:295
> +        return 0;

nullptr.

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:433
> +            } else
> +            if (colorType == PNG_COLOR_TYPE_GRAY) {

This should be a single line.

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:478
> +        m_gamma = (int)(gamma*100000);

Use C++ style cast here and leave space around the *

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:568
> +#if ENABLE(APNG)
> +        if (!m_curFrame)
> +#endif

So, it seems many things depend on m_curFrame, I wonder if we could move m_curFrame out of the #ifdef, but it will always be 0 for non APNG. This way we can avoid a lof of #ifdefs in the code I think.

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:725
> +            return 1;

So, what's this function supposed to return? It seems we are always retuning 1. If that's the case, I would make this function void and return 1 unconditionally in the static readChunks()

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:742
> +        for (size_t i = 0; i < m_frameCount; ++i)
> +            m_frameBufferCache[i].setPremultiplyAlpha(m_premultiplyAlpha);

You could use a modern loop here 

for (auto& imageFrame : m_frameBufferCache)
    imageFrame.setPremultiplyAlpha(m_premultiplyAlpha);

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:744
> +    } else
> +    if (!memcmp(chunk->name, "fcTL", 4) && chunk->size == 26) {

This should be a single line.

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:785
> +    } else
> +    if (!memcmp(chunk->name, "fdAT", 4) && chunk->size >= 4) {

This should be a single line

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:808
> +    double gamma;

Move this where it's first needed.

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:809
> +    int bitDepth = png_get_bit_depth(m_png, m_info);

Ditto, this could be moved after the first if

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:849
> +        return; // Nothing to do.

I don't think the comment is needed.

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:880
> +    if (frameIndex >= frameCount())
> +        return;

Could we check this before? I guess the frameRect computation doesn't affect this.

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:882
> +    ImageFrame* const buffer = &m_frameBufferCache[frameIndex];

Could this be const ImageFrame& instead of using a pointer?

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:935
> +    else
> +    if (!m_delayDen)

This should be a single line

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:943
> +    else
> +    if (m_dispose == 1)

Ditto.

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:1022
> +    png_byte dataPNG[8] = {137, 80, 78, 71, 13, 10, 26, 10};
> +    png_byte datagAMA[16] = {0, 0, 0, 4, 103, 65, 77, 65};

Could this be static?

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:1054
> +    png_byte dataIEND[12] = {0, 0, 0, 0, 73, 69, 78, 68, 174, 66, 96, 130};

Ditto.

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:1078
> +    return 1;

Why does this method returns int if it always returns 1?

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.h:31
> +#include "png.h"

I guess this should be <png.h>

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.h:49
> +        virtual size_t frameCount() { return m_frameCount; }
> +        virtual int repetitionCount() const { return m_playCount-1; }

These should be marked as override

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.h:112
> +        unsigned m_curFrame;

m_currentFrame

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.h:116
> +        unsigned m_seqNum;

m_sequenceNumber

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.h:122
> +        unsigned m_delayNum;
> +        unsigned m_delayDen;

What do Num and Den stand for here? Do not use abbreviations.

> LayoutTests/ChangeLog:4
> +        Added APNG reftest.
> +        https://bugs.webkit.org/show_bug.cgi?id=17022

Keep the bug title as the title of all changelogs of the same commit.

[Max Stepin]

Created attachment 248452 [details]
v08_apng_webkit.patch

OK, most of the suggestions implemented.

Few other cases:

> Shouldn't we initialize dest in this case? *dest = 0;

No, this is by design. Applying top layer with alpha = 0 over the background layer means nothing will change, and we can bailout early.

> Could this be an IntSize?
> And this IntPoint?

I would really like to keep them unsigned, makes it easier to work with libpng.

> I wonder if we could move m_curFrame out of the #ifdef, but it will always be 0 for non APNG. 
> This way we can avoid a lof of #ifdefs in the code I think.

Yes, it's always 0 for non APNG, but I would like to keep all new code inside #if ENABLE(APNG) as requested in comment 8.

[Carlos Garcia Campos]

(In reply to comment #36)
> Created attachment 248452 [details]
> v08_apng_webkit.patch
> 
> OK, most of the suggestions implemented.

Thanks!

> Few other cases:
> 
> > Shouldn't we initialize dest in this case? *dest = 0;
> 
> No, this is by design. Applying top layer with alpha = 0 over the background
> layer means nothing will change, and we can bailout early.

Ok.

> > Could this be an IntSize?
> > And this IntPoint?
> 
> I would really like to keep them unsigned, makes it easier to work with
> libpng.

Ah, I see.
 
> > I wonder if we could move m_curFrame out of the #ifdef, but it will always be 0 for non APNG. 
> > This way we can avoid a lof of #ifdefs in the code I think.
> 
> Yes, it's always 0 for non APNG, but I would like to keep all new code
> inside #if ENABLE(APNG) as requested in comment 8.

Well, I disagree, as long as it builds with and without the APNG enabled, and the code not protected by the ifdefs doesn't affect the performance, I think it's better to reduce the amount of ifdefs to improve the code readability and maintainability.

[Max Stepin]

(In reply to comment #37)
> Well, I disagree, as long as it builds with and without the APNG enabled,
> and the code not protected by the ifdefs doesn't affect the performance, I
> think it's better to reduce the amount of ifdefs to improve the code
> readability and maintainability.

All right, I'll fix that.

[Max Stepin]

Created attachment 248547 [details]
v09_apng_webkit.patch

OK, moved m_currentFrame outside of the #ifdef, and simplified the code in a few places.
.

[Carlos Garcia Campos]

Comment on attachment 248547 [details]
v09_apng_webkit.patch

Looks good to me, the test pass and demos work great. Thanks!

[WebKit Commit Bot]

Comment on attachment 248547 [details]
v09_apng_webkit.patch

Clearing flags on attachment: 248547

Committed r181553: <http://trac.webkit.org/changeset/181553>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brent Fulgham]

This change introduced a test failure on Windows. I'm working on getting tests running on Windows EWS again to avoid this problem in the future.

Any ideas why this might be failing?

[Brent Fulgham]

(In reply to comment #43)
> This change introduced a test failure on Windows. I'm working on getting
> tests running on Windows EWS again to avoid this problem in the future.
> 
> Any ideas why this might be failing?

I'm guessing ImageIO on Windows doesn't support APNG. Since WinCairo uses the same image decoders as gtk+, it probably works there.

[Carlos Garcia Campos]

(In reply to comment #44)
> (In reply to comment #43)
> > This change introduced a test failure on Windows. I'm working on getting
> > tests running on Windows EWS again to avoid this problem in the future.
> > 
> > Any ideas why this might be failing?
> 
> I'm guessing ImageIO on Windows doesn't support APNG. Since WinCairo uses
> the same image decoders as gtk+, it probably works there.

oh, right, I'm sorry, I guess the test should be skipped in win mac, but should probably work just fine in win cairo.

[Brent Fulgham]

(In reply to comment #45)
> (In reply to comment #44)
> > (In reply to comment #43)
> > > This change introduced a test failure on Windows. I'm working on getting
> > > tests running on Windows EWS again to avoid this problem in the future.
> > > 
> > > Any ideas why this might be failing?
> > 
> > I'm guessing ImageIO on Windows doesn't support APNG. Since WinCairo uses
> > the same image decoders as gtk+, it probably works there.
> 
> oh, right, I'm sorry, I guess the test should be skipped in win mac, but
> should probably work just fine in win cairo.

I skipped the test on Apple Windows <https://trac.webkit.org/changeset/181564>.

[Max Stepin]

Thanks everybody!

The same APNG Test Suite, online:
http://www.littlesvr.ca/apng/test.html

Now we have 3 implementations:

1. Firefox - open source (looks good)
2. WebKit - open source (looks good)
3. Safari - closed source (a little bit buggy)

[Jon Lee]

(In reply to comment #47)
> Thanks everybody!
> 
> The same APNG Test Suite, online:
> http://www.littlesvr.ca/apng/test.html
> 
> Now we have 3 implementations:
> 
> 1. Firefox - open source (looks good)
> 2. WebKit - open source (looks good)
> 3. Safari - closed source (a little bit buggy)

Filed bug 142741 to cover this.

[ChangSeok Oh]

(In reply to comment #48)
> (In reply to comment #47)
> > Thanks everybody!
> > 
> > The same APNG Test Suite, online:
> > http://www.littlesvr.ca/apng/test.html
> > 
> > Now we have 3 implementations:
> > 
> > 1. Firefox - open source (looks good)
> > 2. WebKit - open source (looks good)
> > 3. Safari - closed source (a little bit buggy)
> 
> Filed bug 142741 to cover this.

Out of interest, why apng, not webp nor jxr? If the compatibility among vendors is important to you, then is webp a better candidate for an alternative image format?

[OC2PS]

> why apng, not webp nor jxr? 
JPEG XR is a *still-image* compression standard and file format.
APNG is an *animation* format.
 
>  If the compatibility among vendors is important to you, then is webp a better candidate for an alternative image format?
No. 
Only Google supports webp.

In any case this ticket is about getting APNG which is already supported by a few browsers to be supported by Webkit, not about which is the best image file format.

If you feel strongly about webp, please contribute at https://bugs.webkit.org/show_bug.cgi?id=105915

[ChangSeok Oh]

(In reply to comment #50)
> > why apng, not webp nor jxr? 
> JPEG XR is a *still-image* compression standard and file format.
> APNG is an *animation* format.
>  
> >  If the compatibility among vendors is important to you, then is webp a better candidate for an alternative image format?
> No. 
> Only Google supports web.
And Opera does. =)

> In any case this ticket is about getting APNG which is already supported by
> a few browsers to be supported by Webkit, not about which is the best image
> file format.
Agree.

> If you feel strongly about webp, please contribute at
> https://bugs.webkit.org/show_bug.cgi?id=105915
Oh. I didn't know that webp was already there! Thanks for pointing the ticket.

[Myles C. Maxfield]

Removing Mavericks-specific lines from platform/mac/TestExpecations due to no longer supporting Mavericks.

[Myles C. Maxfield]

Committed r193663: <http://trac.webkit.org/changeset/193663>