Bug 74779 – Drawing an animated image to a canvas via drawImage should draw the first frame

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Boris Zbarsky]

The spec at http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#dom-context-2d-drawimage is very clear:

  When the drawImage() method is passed an animated image as its image argument, the user
  agent must use the poster frame of the animation, or, if there is no poster frame, the
  first frame of the animation.

Presto, Trident, and Gecko get this right.

[Alexey Proskuryakov]

Which part of this does WebKit get wrong? Do you have a test case that you could attach?

I'm not an expert on this code, just trying to have all the necessary information captured in the bug.

[Boris Zbarsky]

http://jnglike.site40.net/rgba/basic.html seems to be an example where the current frame, not the first frame, is painted.

[Lu Guanqun]

(In reply to comment #2)
> http://jnglike.site40.net/rgba/basic.html seems to be an example where the current frame, not the first frame, is painted.

The above link is down, I get 404 error. Could you post another test example?

[Boris Zbarsky]

I gave you what I had.  That link was from someone complaining about an animation that worked in WebKit but not other browsers.

I guess if I come across something else I'll let you know.

[Boris Zbarsky]

OK, trivial testcase coming up.

[Boris Zbarsky]

Created attachment 130001 [details]
Image for testcase

[Boris Zbarsky]

Created attachment 130002 [details]
Testcase

[Abhijeet Kandalkar]

In above mentioned test case Gecko's drawImage always paints the first frame of animated images.(Gecko mentioned that it is as per spec.)

While in case webkit,drawImage paints currentFrame.

There is bug related to same discussion in Mozilla bugzilla,
https://bugzilla.mozilla.org/show_bug.cgi?id=578514(Drawing an animated image into canvas does not work)

Mozilla is following spec here.

If Webkit also wants same behavior,I would like to fix this bug :)

[Dirk Schulze]

(In reply to comment #8)
> In above mentioned test case Gecko's drawImage always paints the first frame of animated images.(Gecko mentioned that it is as per spec.)
> 
> While in case webkit,drawImage paints currentFrame.
> 
> There is bug related to same discussion in Mozilla bugzilla,
> https://bugzilla.mozilla.org/show_bug.cgi?id=578514(Drawing an animated image into canvas does not work)
> 
> Mozilla is following spec here.
> 
> If Webkit also wants same behavior,I would like to fix this bug :)

Since you can not control the time or the frame you want to use, I agree that we should just paint the first frame (or poster image).  Abhijeet, do you want to take a look?

[Radar WebKit Bug Importer]

<rdar://problem/42282454>

[Carlos Alberto Lopez Perez]

Here is the WPT test-case: http://w3c-test.org/2dcontext/drawing-images-to-the-canvas/2d.drawImage.animated.gif.html

[Carlos Alberto Lopez Perez]

Created attachment 375622 [details]
Patch

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #12)
> Created attachment 375622 [details]
> Patch

This is going to break a number of tests because it turns out that over time we have relied on this "feature" [1] to create tests with animated images and check the contents of current frame that is being drawn.

Let's see how many tests broke in the EWS.

[1] "feature" = drawing an animated image to a canvas before this patch draws the current frame being played.

[EWS Watchlist]

Comment on attachment 375622 [details]
Patch

Attachment 375622 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12869276

New failing tests:
fast/canvas/drawImage-animated-gif-draws-first-frame-and-no-reset-image.html
fast/images/decode-animated-image.html
fast/images/decode-render-animated-image.html
fast/images/animated-image-mp4.html
fast/images/ordered-animated-image-frames.html
fast/images/animated-gif-restored-from-bfcache.html
fast/images/slower-decoding-than-animation-image.html

[EWS Watchlist]

Created attachment 375627 [details]
Archive of layout-test-results from ews103 for mac-highsierra

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

[EWS Watchlist]

Comment on attachment 375622 [details]
Patch

Attachment 375622 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12869348

New failing tests:
fast/images/decode-animated-image.html
fast/images/animated-image-loop-count.html
fast/images/slower-animation-than-decoding-image.html
fast/images/decode-render-animated-image.html
fast/images/reset-image-animation.html
fast/images/ordered-animated-image-frames.html
fast/images/animated-gif-restored-from-bfcache.html
fast/images/animated-image-different-dest-size.html
fast/images/slower-decoding-than-animation-image.html

[EWS Watchlist]

Created attachment 375628 [details]
Archive of layout-test-results from ews212 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews212  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[EWS Watchlist]

Comment on attachment 375622 [details]
Patch

Attachment 375622 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12869346

New failing tests:
fast/canvas/drawImage-animated-gif-draws-first-frame-and-no-reset-image.html
fast/images/decode-animated-image.html
fast/images/decode-render-animated-image.html
fast/images/animated-image-mp4.html
fast/images/ordered-animated-image-frames.html
fast/images/animated-gif-restored-from-bfcache.html
fast/images/slower-decoding-than-animation-image.html

[EWS Watchlist]

Created attachment 375629 [details]
Archive of layout-test-results from ews115 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Carlos Alberto Lopez Perez]

I have checked the tests that fail now with the new behaviour of drawImage().


(In reply to Build Bot from comment #14)
> Comment on attachment 375622 [details]
> Patch
> 
> Attachment 375622 [details] did not pass mac-ews (mac):
> Output: https://webkit-queues.webkit.org/results/12869276
> 
> New failing tests:
> fast/canvas/drawImage-animated-gif-draws-first-frame-and-no-reset-image.html

The above test checks that WebKit doesn't crash (according to the changelog),so is easy to fix for the new behaviour of drawImage().

> fast/images/decode-animated-image.html
> fast/images/decode-render-animated-image.html

The above test cases, test that calling HTMLImageElement.decode() more than once for an animated image
actually decode the Nth frame. To do this check the decoded frame is drawn into a canvas
with drawImage() and the pixel output compared.

> fast/images/animated-image-mp4.html

This one draws the frames of a video like if it was an image and uses drawImage()
to compare the pixel output of the frames.

> fast/images/ordered-animated-image-frames.html

The above one was added in r207386 and its original description reads:

    This animated gif has one red frame, one green frame and two red frames.
    The test page renders only two frames from this this image on a canvas.
    The test passes if the second frame (the green one) is rendered on the 
    canvas even if drawImage() is called after the duration of the first frame.


> fast/images/animated-gif-restored-from-bfcache.html

The above one tests that animated GIFs resume animating after restoring a page from the back forward cache. 
And, of course, for checking that the animation restores as expected it uses a canvas to compare pixels with drawImage().

> fast/images/slower-decoding-than-animation-image.html

Added in r208511 and its description reads:

    In this test, CanvasRenderingContext2D.drawImage() is used to better
    control advancing the animation of an animated image. A setTimeout() is
    used instead of the frame duration to schedule when the drawing happens.
    The test ensures that faster decoding does not overrule the frame
    duration; the setTimeout interval in this case.


Maybe this one is wrong, as far as I understand "CanvasRenderingContext2D.drawImage()"
should not advance the animation, right? Said?

[Carlos Alberto Lopez Perez]

In any case it seems that is desirable to be able to get the pixel output of the current frame of an animated image that is playing, because most of this tests seem useful to me.

The issue is that I don't know how to fix them without a way of achieving that without canvas drawImage().

Anyone knows how to get the pixel bytes of an image, (or of a coordinate on the screen) without using Canvas?

[Said Abou-Hallawa]

> Anyone knows how to get the pixel bytes of an image, (or of a coordinate on
> the screen) without using Canvas?

Maybe you can add an internal setting that allows drawing the current frame on the canvas and you call this API from the failed tests.

if (window.internals)
    internals.settings.setAnimatedImageDebugCanvasDrawingEnabled(true);

In the canvas drawing code, you check for this setting and make a decision: to draw the first frame or to draw the current frame.

[Carlos Alberto Lopez Perez]

(In reply to Said Abou-Hallawa from comment #22)
> > Anyone knows how to get the pixel bytes of an image, (or of a coordinate on
> > the screen) without using Canvas?
> 
> Maybe you can add an internal setting that allows drawing the current frame
> on the canvas and you call this API from the failed tests.
> 
> if (window.internals)
>     internals.settings.setAnimatedImageDebugCanvasDrawingEnabled(true);
> 
> In the canvas drawing code, you check for this setting and make a decision:
> to draw the first frame or to draw the current frame.

Good idea!

Will do that :)

Thanks

[Carlos Alberto Lopez Perez]

Created attachment 375915 [details]
Patch

[EWS Watchlist]

Comment on attachment 375915 [details]
Patch

Attachment 375915 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12885208

New failing tests:
fast/canvas/drawImage-animated-gif.html

[EWS Watchlist]

Created attachment 375923 [details]
Archive of layout-test-results from ews102 for mac-highsierra

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

[Darin Adler]

Comment on attachment 375915 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        It also adds an internal setting to keep the previous behaviour
> +        (draw the current frame playing) because there are several layout
> +        tests that rely on canvas.drawImage() to check that animated images
> +        play properly.

I’m not sure that’s a good rationale for keeping this internal setting. Maybe we can figure out a different testing strategy, perhaps involving the "internals" object?

[Darin Adler]

Comment on attachment 375915 [details]
Patch

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

>> Source/WebCore/ChangeLog:15
>> +        play properly.
> 
> I’m not sure that’s a good rationale for keeping this internal setting. Maybe we can figure out a different testing strategy, perhaps involving the "internals" object?

Oh, I see Said suggested this and I suppose I don’t have a specific better idea.

[EWS Watchlist]

Comment on attachment 375915 [details]
Patch

Attachment 375915 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12885237

New failing tests:
fast/canvas/drawImage-animated-gif.html

[EWS Watchlist]

Created attachment 375928 [details]
Archive of layout-test-results from ews115 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Carlos Alberto Lopez Perez]

(In reply to Build Bot from comment #26)
> Created attachment 375923 [details]
> Archive of layout-test-results from ews102 for mac-highsierra
> 
> The attached test failures were seen while running run-webkit-tests on the
> mac-ews.
> Bot: ews102  Port: mac-highsierra  Platform: Mac OS X 10.13.6

I have been having issues writing a test that works on Mac high-sierra (that is what the EWS for mac and mac-debug runs).

My tests work on all platforms I tested (WebKitGTK and Mac Mojave) and also work on all the other EWS here, but mac high-sierra.

First I started with a more complex test case for this feature that not only tested if the first frame of the image was drawn on the canvas, but also tested that the playing animated image didn't got reset to the first frame when the call to canvas drawImage() was done.

After the failures I encountered with Mac high-sierra I tried to simplify the test case, but not even with that it works.

Now I believe the issue is an unrelated bug to this one.

I reported this on bug 200577

So, I will go back to the more complex test case and mark the tests added on this bug as failing on mac high-sierra (pointing to bug 200577).

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #31)
> (In reply to Build Bot from comment #26)
> > Created attachment 375923 [details]
> > Archive of layout-test-results from ews102 for mac-highsierra
> > 
> > The attached test failures were seen while running run-webkit-tests on the
> > mac-ews.
> > Bot: ews102  Port: mac-highsierra  Platform: Mac OS X 10.13.6
> 
> I have been having issues writing a test that works on Mac high-sierra (that
> is what the EWS for mac and mac-debug runs).
> 
> My tests work on all platforms I tested (WebKitGTK and Mac Mojave) and also
> work on all the other EWS here, but mac high-sierra.
> 
> First I started with a more complex test case for this feature that not only
> tested if the first frame of the image was drawn on the canvas, but also
> tested that the playing animated image didn't got reset to the first frame
> when the call to canvas drawImage() was done.
> 
> After the failures I encountered with Mac high-sierra I tried to simplify
> the test case, but not even with that it works.
> 
> Now I believe the issue is an unrelated bug to this one.
> 
> I reported this on bug 200577
> 
> So, I will go back to the more complex test case and mark the tests added on
> this bug as failing on mac high-sierra (pointing to bug 200577).

Seems this issue had nothing to do with mac high-sierra, but with WebKit1 (DumpRenderTree) that defaults to [window setAutodisplay:NO]. It can be worked around by adding a composited element on the tests to turn autodisplay on.

[Carlos Alberto Lopez Perez]

Created attachment 376066 [details]
Patch

[Carlos Alberto Lopez Perez]

Ping reviewers?

[Said Abou-Hallawa]

Comment on attachment 376066 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        This patch passes as image to be drawn into the canvas context a
> +        new image with the contents of the first frame of the animated image.

This statement is a little bit hard to read.

> LayoutTests/fast/canvas/drawImage-animated-gif-draws-first-frame-and-no-reset-image.html:11
> +        description("This tests that when drawing an animated image to a canvas it draws the first frame and that the playing image doesn't get reseted.");

Testing the loop count is covered by other tests.

> LayoutTests/fast/canvas/drawImage-animated-gif-draws-first-frame-and-no-reset-image.html:17
> +        var canvas = document.getElementsByTagName("canvas")[0];
> +        var previousFrameIndex = 0;
> +        var currentFrameIndex = 0;
> +        var canvasAlreadyDrawed = false;
> +        var imgData = [];

These variables can be just local variables or just not needed.

> LayoutTests/fast/canvas/drawImage-animated-gif-draws-first-frame-and-no-reset-image.html:41
> +            canvasAlreadyDrawed = true;

No need to track whether an image is to drawn to the canvas. The caller should call this function only once: when the current frame is the last frame.

> LayoutTests/fast/canvas/drawImage-animated-gif-draws-first-frame-and-no-reset-image.html:46
> +            shouldBe("internals.imageFrameIndex(image)", "3");

What is "3"? Did you mean to use imageLastFrameIndex instead?

> LayoutTests/fast/canvas/drawImage-animated-gif-draws-first-frame-and-no-reset-image.html:51
> +        function imageCheckDrawFrames()
> +        {

The curly brace should be moved to the previous line.

> LayoutTests/fast/canvas/drawImage-animated-gif-draws-first-frame-and-no-reset-image.html:54
> +            if (currentFrameIndex == 3) {

What does 3 mean here? Did you mean to use imageLastFrameIndex instead?

> LayoutTests/fast/canvas/drawImage-animated-gif-draws-first-frame-and-no-reset-image.html:61
> +            if (currentFrameIndex < previousFrameIndex) {
> +                testFailed("The GIF animation should not be resetted.");
> +                finishJSTest();
> +            }

No need to test the loop count here.

> LayoutTests/fast/canvas/drawImage-animated-gif-draws-first-frame-and-no-reset-image.html:64
> +                    testPassed("The GIF animation has completed playing and the Canvas has been drawn.");
> +                    finishJSTest();

Wrong indentation.

> LayoutTests/fast/canvas/drawImage-animated-gif-draws-first-frame-and-no-reset-image.html:66
> +            previousFrameIndex = currentFrameIndex;

I am a little puzzled by the logic in this function. Can't the logic be simplified like this:

image.addEventListener("webkitImageFrameReady", () => {
    if (internals.imageFrameIndex(image) == internals.imageFrameCount(image) - 1) {
        drawAndCheckCanvas();
        finishJSTest();
    }
}

> LayoutTests/fast/canvas/drawImage-animated-gif.html:2
> +<title>This tests that when drawing an animated image to a canvas it draws the first frame.</title>

There is no much difference between this test and drawImage-animated-gif-draws-first-frame-and-no-reset-image.html. In fact the other test is timed much better than this one. You can also convert the other test to ref-test if this is the goal of a second test.

> LayoutTests/fast/canvas/drawImage-animated-gif.html:19
> +    function runTest() {
> +        // wait for the gif loop to be completed
> +        setTimeout(drawCanvas, 500);
> +    }

This is bad timing technique. The other test is timed much better than this which relies on setTimeout(). If the EWS bots are overloaded, drawing the frames of the animated image might take longer than 500ms. In this case, the test will be flaky.

[Carlos Alberto Lopez Perez]

Comment on attachment 376066 [details]
Patch

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

>> Source/WebCore/ChangeLog:10
>> +        new image with the contents of the first frame of the animated image.
> 
> This statement is a little bit hard to read.

I'll reword it

>> LayoutTests/fast/canvas/drawImage-animated-gif-draws-first-frame-and-no-reset-image.html:11
>> +        description("This tests that when drawing an animated image to a canvas it draws the first frame and that the playing image doesn't get reseted.");
> 
> Testing the loop count is covered by other tests.

Its not testing the loop count. The image used in this test doesn't loop. It only plays once.

What it tries to test is that when canvas.drawImage(image) is called, the animation itself doesn't reset or pause.
It happened to me on a previous impl that I tested locally, that the animated image was resetting when drawn into the canvas (it started to play back from frame zero).

>> LayoutTests/fast/canvas/drawImage-animated-gif-draws-first-frame-and-no-reset-image.html:41
>> +            canvasAlreadyDrawed = true;
> 
> No need to track whether an image is to drawn to the canvas. The caller should call this function only once: when the current frame is the last frame.

The test doesn't try to draw to the canvas when the current frame is the last one, but when the frame is the 4th.
The idea is that it can test after drawing to the canvas that the image will play the 5th and 6th frames without going back.

>> LayoutTests/fast/canvas/drawImage-animated-gif-draws-first-frame-and-no-reset-image.html:46
>> +            shouldBe("internals.imageFrameIndex(image)", "3");
> 
> What is "3"? Did you mean to use imageLastFrameIndex instead?

"3" is just the 4th frame (counting from zero).
It checks that when the image is drawn into the canvas the animated image is playing on the 4th frame (index=3)

>> LayoutTests/fast/canvas/drawImage-animated-gif-draws-first-frame-and-no-reset-image.html:51
>> +        {
> 
> The curly brace should be moved to the previous line.

ok

>> LayoutTests/fast/canvas/drawImage-animated-gif-draws-first-frame-and-no-reset-image.html:54
>> +            if (currentFrameIndex == 3) {
> 
> What does 3 mean here? Did you mean to use imageLastFrameIndex instead?

Same than above, "3" means just the 4th frame.
I don't mean to test with the last frame (as this image has 6 frames)

>> LayoutTests/fast/canvas/drawImage-animated-gif-draws-first-frame-and-no-reset-image.html:66
>> +            previousFrameIndex = currentFrameIndex;
> 
> I am a little puzzled by the logic in this function. Can't the logic be simplified like this:
> 
> image.addEventListener("webkitImageFrameReady", () => {
>     if (internals.imageFrameIndex(image) == internals.imageFrameCount(image) - 1) {
>         drawAndCheckCanvas();
>         finishJSTest();
>     }
> }

That would make this a different test.
The intention is not to only draw to a canvas and check that the drawed frame is the first one.
It also tries check that the image continues playing as expected after calling canvas.drawImage(). For example, when calling it on the 4th frame the next playing frame of the image should be the 5th.

I will rework the test a bit and try to clarify the intention.

>> LayoutTests/fast/canvas/drawImage-animated-gif.html:2
>> +<title>This tests that when drawing an animated image to a canvas it draws the first frame.</title>
> 
> There is no much difference between this test and drawImage-animated-gif-draws-first-frame-and-no-reset-image.html. In fact the other test is timed much better than this one. You can also convert the other test to ref-test if this is the goal of a second test.

Ok. I will drop this test. I agree the other test is better.

[Carlos Alberto Lopez Perez]

Created attachment 377344 [details]
Patch

patch for landing

[WebKit Commit Bot]

Comment on attachment 377344 [details]
Patch

Clearing flags on attachment: 377344

Committed r249162: <https://trac.webkit.org/changeset/249162>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.