[Webkit-unassigned] [Bug 17022] Add APNG support
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Mar 5 01:52:51 PST 2015
https://bugs.webkit.org/show_bug.cgi?id=17022
--- Comment #35 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 236444
--> https://bugs.webkit.org/attachment.cgi?id=236444
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.
--
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150305/87299b09/attachment-0002.html>
More information about the webkit-unassigned
mailing list