Bug 113124 – Add animation support for WebP images

• Component: Images
• Status: RESOLVED FIXED

[Urvang Joshi]

Animation support is being added to WebP in v0.3.0.
This bug regarding adding the same support in WebKit.

[Urvang Joshi]

Created attachment 195692 [details]
Patch

[Urvang Joshi]

Here is a patch, with some non-pixel tests.

Note: Would love to add a pixel test too: any good ideas on how to do that for an animated image? (I had a look at the GIF tests, but most of them are bug-fixes).

[Urvang Joshi]

@Noel and others:
I believe that this patch is relevant to Chromium port only (looking at the TestExpectations etc); no other ports have 'USE(WEBP)' flag set. Is that correct?
If so, I'll revert this after migration.

[noel gordon]

(In reply to comment #3)
> @Noel and others:
> I believe that this patch is relevant to Chromium port only (looking at the TestExpectations etc); no other ports have 'USE(WEBP)' flag set. Is that correct?
> If so, I'll revert this after migration.

Other ports use webp and hence WEBPImageDecoder.{cpp,h}, but they won't get here (animation) until they upgrade their bots and builds to use libwebp 0.3.0.  Does that answer your question?

One thing I noticed with this patch: are you are creating m_demux once only from the input m_data->data()? That's gonna hurt if the demux maintains pointers into that m_data->data() with progressive decoding because the underlying data may be moved around (consolidated) on subsequent m_data->data() calls, bug 104302.  Any pointers to the original data may become invalid, meaning we'd crash on access.

[Urvang Joshi]

> Other ports use webp and hence WEBPImageDecoder.{cpp,h}, but they won't get 
> here (animation) until they upgrade their bots and builds to use libwebp 0.3.0.  
> Does that answer your question?

ok good, so this patch is relevant for Webkit too (and not just Blink). Just wanted to clarify that.

> One thing I noticed with this patch: are you are creating m_demux once only 
> from the input m_data->data()? That's gonna hurt if the demux maintains 
> pointers into that m_data->data() with progressive decoding because the
> underlying data may be moved around (consolidated) on subsequent m_data->data()
> calls, bug 104302.  Any pointers to the original data may become invalid, 
> meaning we'd crash on access.

I believe that any change to m_data->data() would occur through the setData() method only. And we are recreating the m_demux object whenever setData() is called. So, that should take care of any data being moved around, right?

[Note: One exception is that we don't touch the demux object once the demux state is WEBP_DEMUX_DONE. But we could change that to be safe.]

Thoughts?

[Urvang Joshi]

@Noel:
I'm working on this animation patch in Blink currently:
https://codereview.chromium.org/13980003/

Once that gets in, I'll migrate it here too.

For now, can u pls review the Blink patch?

[noel gordon]

(In reply to comment #5)
>
> I believe that any change to m_data->data() would occur through the setData() method only. And we are recreating the m_demux object whenever setData() is called. So, that should take care of any data being moved around, right?

Maybe. It'll require good testing and I don't think layout tests are the right was to test for this sort of image decoding data issue.

> [Note: One exception is that we don't touch the demux object once the demux state is WEBP_DEMUX_DONE. But we could change that to be safe.]
> 
> Thoughts?

Same as above.

[kej]

good!!

[Olivier Blin]

Created attachment 307748 [details]
Patch

[Olivier Blin]

Useful test resources:
https://storage.googleapis.com/downloads.webmproject.org/webp/images/dancing_banana2.lossless.webp
https://qual-e.appspot.com/webp.html
https://qual-e.appspot.com/awebp.html

[Carlos Alberto Lopez Perez]

Comment on attachment 307748 [details]
Patch

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

I was unable to test this because it currently fails to build (on WebKitGTK+ trunk of today=r216073).
Can you try to fix the comments below and upload a new version? Thanks

> Source/WebCore/ChangeLog:23
> +        - layout and pixel tests

I think at least one layout test is needed for the patch to land. So please, add some layout test on the next version of the patch.
I think there are some examples of how this can be done on the old patch attached on this bug.

Since the Mac port doesn't use this image decoders, I think any test added should be marked as skipped on the general TestExpectation file <LayoutTests/TestExpectations> and then marked to pass (overriding the general expectation) on the GTK test expectation file <LayoutTests/platform/gtk/TestExpectations>

> Source/WebCore/ChangeLog:24
> +        - remove support of old WebP versions in CMake files (all distros with WebKitGTK ship a recent enough WebP with demuxer enabled)

Our current minimum baseline supported distribution (Debian 8) ships with this. So I think is fine to assume demuxer should be available.

> Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:57
> +    , m_formatFlags(0)
> +    , m_frameBackgroundHasAlpha(false)
> +    , m_demux(0)
> +    , m_demuxState(WEBP_DEMUX_PARSING_HEADER)
> +    , m_haveAlreadyParsedThisData(false)
> +    , m_repetitionCount(WebCore::RepetitionCountOnce)
> +    , m_decodedHeight(0)

Where are this class variables defined?

This fails to build for me: http://sprunge.us/PMGC
The EWS was happy, so perhaps something changed since the patch was uploaded?

> Source/cmake/FindWebP.cmake:34
>  pkg_check_modules(PC_WEBP QUIET libwebp)
>  

Maybe this could be:
pkg_check_modules(PC_WEBP QUIET libwebp libwebpdemux)

So below you don't need to append to those arrays, neither change the variable names on find_path() and find_library()

This means we would require webp with demuxer support built-in or we will skip to build any webp support at all.

[Miguel Gomez]

Created attachment 322501 [details]
Patch

[Zan Dobersek]

Comment on attachment 322501 [details]
Patch

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

> Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:272
> +    if (decodedHeight <= 0)
> +        return;

Should the width be tested here as well?

> Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:276
> +    ASSERT_WITH_SECURITY_IMPLICATION(decodedWidth == frameRect.width());
> +    ASSERT_WITH_SECURITY_IMPLICATION(decodedHeight <= frameRect.height());

Is the ==/<= mismatch intentional?

[Miguel Gomez]

(In reply to Zan Dobersek from comment #13)
> Comment on attachment 322501 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322501&action=review
> 
> > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:272
> > +    if (decodedHeight <= 0)
> > +        return;
> 
> Should the width be tested here as well?

No need to. This checks whether there's at least a decoded row to render. If there isn't any row we won't render anything, so the returned width value is not relevant.
 
> > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:276
> > +    ASSERT_WITH_SECURITY_IMPLICATION(decodedWidth == frameRect.width());
> > +    ASSERT_WITH_SECURITY_IMPLICATION(decodedHeight <= frameRect.height());
> 
> Is the ==/<= mismatch intentional?

Yes. The decoded width must be the same we requested, but the height may be smaller if there isn't enough data to decode the whole frame.

[Zan Dobersek]

Comment on attachment 322501 [details]
Patch

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

> Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:268
> +    int decodedWidth;
> +    int decodedHeight;

Nit: initialize these to 0;

> Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:284
> +            RGBA32* address = buffer.backingStore()->pixelAt(canvasX, canvasY);

Does this backing store use something like tiles internally? Or is it just a single chunk of memory? I.e., can we iterate over the RGBA32 values in a single loop?

> Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.h:64
>      void applyColorProfile(const uint8_t*, size_t, ImageFrame&) { };

Can this function be dropped? I think it can be.

[Miguel Gomez]

(In reply to Zan Dobersek from comment #15)
> Comment on attachment 322501 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322501&action=review
> 
> > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:268
> > +    int decodedWidth;
> > +    int decodedHeight;
> 
> Nit: initialize these to 0;

Ok.

> > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:284
> > +            RGBA32* address = buffer.backingStore()->pixelAt(canvasX, canvasY);
> 
> Does this backing store use something like tiles internally? Or is it just a
> single chunk of memory? I.e., can we iterate over the RGBA32 values in a
> single loop?

Internally it's a single memory chunk. We could iterate it with a single loop, yes, but this way the implementation is isolated from the internal storage of the backingStore (in case any day we want to change it), and also it's the same pattern that is followed by the other decoders.

> > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.h:64
> >      void applyColorProfile(const uint8_t*, size_t, ImageFrame&) { };
> 
> Can this function be dropped? I think it can be.

Indeed.

[Miguel Gomez]

Created attachment 322643 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 322643 [details]
Patch

Clearing flags on attachment: 322643

Committed r222841: <http://trac.webkit.org/changeset/222841>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34810198>

[Konstantin Tokarev]

Comment on attachment 322643 [details]
Patch

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

> Source/cmake/FindWebP.cmake:-36
> -find_path(WEBP_INCLUDE_DIRS

Modern convention in CMake find modules is to use package_INCLUDE_DIRS and package_LIBRARIES variables

[Miguel Gomez]

(In reply to Konstantin Tokarev from comment #21)
> Comment on attachment 322643 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322643&action=review
> 
> > Source/cmake/FindWebP.cmake:-36
> > -find_path(WEBP_INCLUDE_DIRS
> 
> Modern convention in CMake find modules is to use package_INCLUDE_DIRS and
> package_LIBRARIES variables

I'm using WEBP_INCLUDE_DIR, WEBP_LIBRARY, WEBP_DEMUX_INCLUDE_DIR and WEBP_DEMUX_LIBRARY as temporal placeholders.
Their content is appended to the WEBP_INCUDE_DIRS and WEBP_LIBRARIES variables afterwards.

[Konstantin Tokarev]

Comment on attachment 322643 [details]
Patch

Sorry, my bad

[Said Abou-Hallawa]

Comment on attachment 322643 [details]
Patch

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

> Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:101
> +    // The first frame doesn't depend on any other.
> +    if (!frameIndex)
> +        return 0;
> +
> +    // Check the most probable scenario first: the previous frame is complete, so we can decode the requested one.
> +    if (m_frameBufferCache[frameIndex - 1].isComplete())
> +        return frameIndex;
> +
> +    // Check if the requested frame can be rendered without dependencies. This happens if the frame
> +    // fills the whole area and doesn't have alpha.
> +    WebPIterator webpFrame;
> +    if (WebPDemuxGetFrame(demuxer, frameIndex + 1, &webpFrame)) {
> +        IntRect frameRect(webpFrame.x_offset, webpFrame.y_offset, webpFrame.width, webpFrame.height);
> +        if (frameRect.contains(IntRect(IntPoint(), size())) && !webpFrame.has_alpha)
> +            return frameIndex;
> +    }

I think these special cases can fit nicely in the loop below. The first if-statment is already handled by the for-loop start setting and the termination condition. The second if-statment is handled by the first if-statment inside the for-loop below.

> Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:110
> +        if (WebPDemuxGetFrame(demuxer, i + 1, &webpFrame)) {

Because the argument 'frame_number' of WebPDemuxGetFrame() is 1-based, this is very hard to read. I would suggest adding a helper function to make the "+1" hidden in one place:

bool webpFrameAtIndex(WebPDemuxer* demuxer, size_t index, WebPIterator* webpFrame) { return WebPDemuxGetFrame(demuxer, index + 1, webpFrame); }

> Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:148
> +        setFailed();

Don't we need to call WebPDemuxDelete(demuxer) before returning?

> Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:189
> +    decoderBuffer.u.RGBA.rgba = reinterpret_cast<uint8_t*>(fastMalloc(decoderBuffer.u.RGBA.size));

Another way to allocate this memory would be 

std::unique_ptr<unsigned char[]> p(new uint8_t[ecoderBuffer.u.RGBA.size]());
decoderBuffer.u.RGBA.rgba = p.get();

> Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:235
> +    // Make sure the frameRect doesn't extend outside the buffer.
> +    if (frameRect.maxX() > size().width())
> +        frameRect.setWidth(size().width() - webpFrame->x_offset);
> +    if (frameRect.maxY() > size().height())
> +        frameRect.setHeight(size().height() - webpFrame->y_offset);

Another way to write these statements is:

frameRect.intersect({ { }, size() });

> Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:259
> +    buffer.setDecodingStatus(DecodingStatus::Partial);

What does Partial means here?

Does it mean, the decoding of this frame is partial? Or in other words the frame is being decoded?
Or does it mean the encoded data of the frame is not complete and it the frame is displayed it won't show the whole frame or it will show a lower resolution of it?

> Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:361
> +        if (buffer.isComplete() || buffer.isInvalid())

Why do we keep the "partially" decoded frames here?

> LayoutTests/fast/images/animated-webp.html:15
> +        window.setTimeout(function() {
> +            if (window.testRunner)
> +                testRunner.notifyDone();
> +        }, 500);

500ms is way too long for a layout test. How many frames in these images? You can use the HTMLImageElement.decode() API to better control this test. You can have a look at LayoutTests/fast/images/decode-animated-image.html.

[Miguel Gomez]

(In reply to Said Abou-Hallawa from comment #24)
> Comment on attachment 322643 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322643&action=review
> 
> > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:101
> > +    // The first frame doesn't depend on any other.
> > +    if (!frameIndex)
> > +        return 0;
> > +
> > +    // Check the most probable scenario first: the previous frame is complete, so we can decode the requested one.
> > +    if (m_frameBufferCache[frameIndex - 1].isComplete())
> > +        return frameIndex;
> > +
> > +    // Check if the requested frame can be rendered without dependencies. This happens if the frame
> > +    // fills the whole area and doesn't have alpha.
> > +    WebPIterator webpFrame;
> > +    if (WebPDemuxGetFrame(demuxer, frameIndex + 1, &webpFrame)) {
> > +        IntRect frameRect(webpFrame.x_offset, webpFrame.y_offset, webpFrame.width, webpFrame.height);
> > +        if (frameRect.contains(IntRect(IntPoint(), size())) && !webpFrame.has_alpha)
> > +            return frameIndex;
> > +    }
> 
> I think these special cases can fit nicely in the loop below. The first
> if-statment is already handled by the for-loop start setting and the
> termination condition. The second if-statment is handled by the first
> if-statment inside the for-loop below.

Yes, that can be reworked a bit to make it more readable.

> > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:110
> > +        if (WebPDemuxGetFrame(demuxer, i + 1, &webpFrame)) {
> 
> Because the argument 'frame_number' of WebPDemuxGetFrame() is 1-based, this
> is very hard to read. I would suggest adding a helper function to make the
> "+1" hidden in one place:

Ditto.

> bool webpFrameAtIndex(WebPDemuxer* demuxer, size_t index, WebPIterator*
> webpFrame) { return WebPDemuxGetFrame(demuxer, index + 1, webpFrame); }
> 
> > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:148
> > +        setFailed();
> 
> Don't we need to call WebPDemuxDelete(demuxer) before returning?

Indeed!!
 
> > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:259
> > +    buffer.setDecodingStatus(DecodingStatus::Partial);
> 
> What does Partial means here?
> 
> Does it mean, the decoding of this frame is partial? Or in other words the
> frame is being decoded?
> Or does it mean the encoded data of the frame is not complete and it the
> frame is displayed it won't show the whole frame or it will show a lower
> resolution of it?

At that point it means that the frame is being decoded. But I realized I made a mistake and it should be DecodingStatus::Decoding instead,
leaving DecodingStatus::Partial for frames with incomplete data.

> > Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:361
> > +        if (buffer.isComplete() || buffer.isInvalid())
> 
> Why do we keep the "partially" decoded frames here?

When I use DecodingStatus::Partial only for incomplete frames I can remove them here as well, keeping only the decoding ones.

> > LayoutTests/fast/images/animated-webp.html:15
> > +        window.setTimeout(function() {
> > +            if (window.testRunner)
> > +                testRunner.notifyDone();
> > +        }, 500);
> 
> 500ms is way too long for a layout test. How many frames in these images?
> You can use the HTMLImageElement.decode() API to better control this test.
> You can have a look at LayoutTests/fast/images/decode-animated-image.html.

Ah, I should have reduced that to 300ms, which is the time used by fast/images/animated-png.html that I used as reference.

I'll fix those issues and file a new bug to commit them.