Please add APNG support to WebKit. Firefox and Opera already supports this.
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.
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.
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/
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+
Bug 43828 tracks APNG support in Safari (as mentioned above, that's blocked on ImageIO).
want!
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.
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.
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.
(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.
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
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.
(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.
(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.
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.
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.
OK. I was looking at LayoutTests/fast/images/png-suite/test.html as an example. But I can convert it into a reftest.
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.
Created attachment 236373 [details] v05_apng_webkit.patch Previous patch was formatted incorrectly. Fixed now.
Created attachment 236375 [details] v06_apng_webkit.patch Really fixed now. Sorry, I'm new to this.
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
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
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
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
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
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
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
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
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.
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.
+1 Add APNG support. Max's patch works. And he's willing to maintain. Takes away all excuses not to.
(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
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.
(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.
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.
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.
(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.
(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.
Created attachment 248547 [details] v09_apng_webkit.patch OK, moved m_currentFrame outside of the #ifdef, and simplified the code in a few places. .
Comment on attachment 248547 [details] v09_apng_webkit.patch Looks good to me, the test pass and demos work great. Thanks!
Comment on attachment 248547 [details] v09_apng_webkit.patch Clearing flags on attachment: 248547 Committed r181553: <http://trac.webkit.org/changeset/181553>
All reviewed patches have been landed. Closing bug.
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?
(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.
(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.
(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>.
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)
(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.
(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?
> 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
(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.
Removing Mavericks-specific lines from platform/mac/TestExpecations due to no longer supporting Mavericks.
Committed r193663: <http://trac.webkit.org/changeset/193663>