Drawing an animated image to a canvas now behave as expected

Drawing an animated image to a canvas now behave as expected

According to the spec, drawing an animated image to a canvas should only
draw the first frame.

BUG=286673
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178959

[Rémi Piotaix, 2014-07-14 20:20:54]

PTAL

[Justin Novosad, 2014-07-16 19:36:02]

https://codereview.chromium.org/388253004/diff/1/Source/core/html/canvas/Canv...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/388253004/diff/1/Source/core/html/canvas/Canv...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1513: image =
BitmapImage::create(static_cast<BitmapImage*>(image.get())->frameAtIndex(0));
Would be cleaner to put this logic inside BitmapImage.  You could add to
WebCore::Image a virtual method called imageForDefaultFrame. The default
implementation would just return 'this' and the override in BitmapImage would do
what you have here.

[Rémi Piotaix, 2014-07-17 17:38:20]

https://codereview.chromium.org/388253004/diff/1/Source/core/html/canvas/Canv...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/388253004/diff/1/Source/core/html/canvas/Canv...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1513: image =
BitmapImage::create(static_cast<BitmapImage*>(image.get())->frameAtIndex(0));
On 2014/07/16 19:36:02, junov wrote:
> Would be cleaner to put this logic inside BitmapImage.  You could add to
> WebCore::Image a virtual method called imageForDefaultFrame. The default
> implementation would just return 'this' and the override in BitmapImage would
do
> what you have here.

Done.

[Justin Novosad, 2014-07-17 17:45:25]

Code looks good. All you are missing now is a layout test.

https://codereview.chromium.org/388253004/diff/20001/Source/platform/graphics...
File Source/platform/graphics/Image.cpp (right):

https://codereview.chromium.org/388253004/diff/20001/Source/platform/graphics...
Source/platform/graphics/Image.cpp:242: RefPtr<Image> img(this);
Nit: img -> image (we don't usually abbreviate names in Blink)

[Rémi Piotaix, 2014-07-17 17:56:45]

Test is on his way...

https://codereview.chromium.org/388253004/diff/20001/Source/platform/graphics...
File Source/platform/graphics/Image.cpp (right):

https://codereview.chromium.org/388253004/diff/20001/Source/platform/graphics...
Source/platform/graphics/Image.cpp:242: RefPtr<Image> img(this);
On 2014/07/17 17:45:25, junov wrote:
> Nit: img -> image (we don't usually abbreviate names in Blink)

Done.

[pdr., 2014-07-18 14:42:09]

On 2014/07/17 17:56:45, Rémi Piotaix wrote:
> Test is on his way...
> 
>
https://codereview.chromium.org/388253004/diff/20001/Source/platform/graphics...
> File Source/platform/graphics/Image.cpp (right):
> 
>
https://codereview.chromium.org/388253004/diff/20001/Source/platform/graphics...
> Source/platform/graphics/Image.cpp:242: RefPtr<Image> img(this);
> On 2014/07/17 17:45:25, junov wrote:
> > Nit: img -> image (we don't usually abbreviate names in Blink)
> 
> Done.

Do we have a teat for the animayed SVG case? My recollection is that we only
draw the first frame, but I'd like to be sure.

[Justin Novosad, 2014-07-18 15:22:32]

https://codereview.chromium.org/388253004/diff/60001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/388253004/diff/60001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1512: image =
image->imageForDefaultFrame();
Wait a minute.. according to the spec, the behavior of fetching the default
frame is specific to image elements. No need to call it for all types of image
sources. This call should go in HTMLImageElement::getSourceImageForCanvas.

[Justin Novosad, 2014-07-18 15:24:33]

On 2014/07/18 14:42:09, pdr wrote:
> On 2014/07/17 17:56:45, Rémi Piotaix wrote:
> > Test is on his way...
> > 
> >
>
https://codereview.chromium.org/388253004/diff/20001/Source/platform/graphics...
> > File Source/platform/graphics/Image.cpp (right):
> > 
> >
>
https://codereview.chromium.org/388253004/diff/20001/Source/platform/graphics...
> > Source/platform/graphics/Image.cpp:242: RefPtr<Image> img(this);
> > On 2014/07/17 17:45:25, junov wrote:
> > > Nit: img -> image (we don't usually abbreviate names in Blink)
> > 
> > Done.
> 
> Do we have a teat for the animayed SVG case? My recollection is that we only
> draw the first frame, but I'd like to be sure.

That is correct. The spec says: "Specifically, when a CanvasImageSource object
represents an animated image in an HTMLImageElement, the user agent must use the
default image of the animation (the one that the format defines is to be used
when animation is not supported or is disabled), or, if there is no such image,
the first frame of the animation, when rendering the image for
CanvasRenderingContext2D APIs."

So yes it should apply to SVG as well.  That can be fixed in a separate CL
though. Rémi: please file a separate bug for the SVG case?

[Rémi Piotaix, 2014-07-18 16:04:35]

https://codereview.chromium.org/388253004/diff/60001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/388253004/diff/60001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1512: image =
image->imageForDefaultFrame();
On 2014/07/18 15:22:32, junov wrote:
> Wait a minute.. according to the spec, the behavior of fetching the default
> frame is specific to image elements. No need to call it for all types of image
> sources. This call should go in HTMLImageElement::getSourceImageForCanvas.

Done.

[Justin Novosad, 2014-07-24 17:49:22]

https://codereview.chromium.org/388253004/diff/80001/LayoutTests/fast/canvas/...
File LayoutTests/fast/canvas/canvas-drawImage-animated.html (right):

https://codereview.chromium.org/388253004/diff/80001/LayoutTests/fast/canvas/...
LayoutTests/fast/canvas/canvas-drawImage-animated.html:7: <img id="image"
src="data:image/gif;base64,R0lGODlhAQABAIACAEAEHgqZHiH+EUNyZWF0ZWQgd2l0aCBHSU1QACH5BAkAAAEALAAAAAABAAEAAAICRAEAIfkECAAA/wAsAAAAAAEAAQAAAgJMAQA7"
alt="Animated Image" />
Add a comment to explain what is in this image

https://codereview.chromium.org/388253004/diff/80001/LayoutTests/fast/canvas/...
LayoutTests/fast/canvas/canvas-drawImage-animated.html:19: //           
requestAnimationFrame(truc);
Don't submit commented code.

https://codereview.chromium.org/388253004/diff/80001/LayoutTests/fast/canvas/...
LayoutTests/fast/canvas/canvas-drawImage-animated.html:23: imageData =
canvasContext.getImageData(0, 0, 1, 1);
Did you verify that this test fails without your fix? You are not letting any
time pass so even with the bug present, this may be getting the first frame.

https://codereview.chromium.org/388253004/diff/80001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/388253004/diff/80001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:64: #include
"platform/graphics/BitmapImage.h"
No longer needed

[Rémi Piotaix, 2014-07-25 18:21:00]

https://codereview.chromium.org/388253004/diff/80001/LayoutTests/fast/canvas/...
File LayoutTests/fast/canvas/canvas-drawImage-animated.html (right):

https://codereview.chromium.org/388253004/diff/80001/LayoutTests/fast/canvas/...
LayoutTests/fast/canvas/canvas-drawImage-animated.html:7: <img id="image"
src="data:image/gif;base64,R0lGODlhAQABAIACAEAEHgqZHiH+EUNyZWF0ZWQgd2l0aCBHSU1QACH5BAkAAAEALAAAAAABAAEAAAICRAEAIfkECAAA/wAsAAAAAAEAAQAAAgJMAQA7"
alt="Animated Image" />
On 2014/07/24 17:49:22, junov wrote:
> Add a comment to explain what is in this image

Done.

https://codereview.chromium.org/388253004/diff/80001/LayoutTests/fast/canvas/...
LayoutTests/fast/canvas/canvas-drawImage-animated.html:19: //           
requestAnimationFrame(truc);
On 2014/07/24 17:49:22, junov wrote:
> Don't submit commented code.

Done.

https://codereview.chromium.org/388253004/diff/80001/LayoutTests/fast/canvas/...
LayoutTests/fast/canvas/canvas-drawImage-animated.html:23: imageData =
canvasContext.getImageData(0, 0, 1, 1);
On 2014/07/24 17:49:22, junov wrote:
> Did you verify that this test fails without your fix? You are not letting any
> time pass so even with the bug present, this may be getting the first frame.

The test fails on M35/36.

https://codereview.chromium.org/388253004/diff/80001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/388253004/diff/80001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:64: #include
"platform/graphics/BitmapImage.h"
On 2014/07/24 17:49:22, junov wrote:
> No longer needed

Done.

[Justin Novosad, 2014-07-25 18:26:14]

On 2014/07/25 18:21:00, Rémi Piotaix wrote:
>
https://codereview.chromium.org/388253004/diff/80001/LayoutTests/fast/canvas/...
> File LayoutTests/fast/canvas/canvas-drawImage-animated.html (right):
> 
>
https://codereview.chromium.org/388253004/diff/80001/LayoutTests/fast/canvas/...
> LayoutTests/fast/canvas/canvas-drawImage-animated.html:7: <img id="image"
>
src="data:image/gif;base64,R0lGODlhAQABAIACAEAEHgqZHiH+EUNyZWF0ZWQgd2l0aCBHSU1QACH5BAkAAAEALAAAAAABAAEAAAICRAEAIfkECAAA/wAsAAAAAAEAAQAAAgJMAQA7"
> alt="Animated Image" />
> On 2014/07/24 17:49:22, junov wrote:
> > Add a comment to explain what is in this image
> 
> Done.
> 
>
https://codereview.chromium.org/388253004/diff/80001/LayoutTests/fast/canvas/...
> LayoutTests/fast/canvas/canvas-drawImage-animated.html:19: //           
> requestAnimationFrame(truc);
> On 2014/07/24 17:49:22, junov wrote:
> > Don't submit commented code.
> 
> Done.
> 
>
https://codereview.chromium.org/388253004/diff/80001/LayoutTests/fast/canvas/...
> LayoutTests/fast/canvas/canvas-drawImage-animated.html:23: imageData =
> canvasContext.getImageData(0, 0, 1, 1);
> On 2014/07/24 17:49:22, junov wrote:
> > Did you verify that this test fails without your fix? You are not letting
any
> > time pass so even with the bug present, this may be getting the first frame.
> 
> The test fails on M35/36.
> 
>
https://codereview.chromium.org/388253004/diff/80001/Source/core/html/canvas/...
> File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):
> 
>
https://codereview.chromium.org/388253004/diff/80001/Source/core/html/canvas/...
> Source/core/html/canvas/CanvasRenderingContext2D.cpp:64: #include
> "platform/graphics/BitmapImage.h"
> On 2014/07/24 17:49:22, junov wrote:
> > No longer needed
> 
> Done.

lgtm

[Rémi Piotaix, 2014-07-25 20:10:01]

The CQ bit was checked by piotaixr@chromium.org

[commit-bot: I haz the power, 2014-07-25 20:11:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/piotaixr@chromium.org/388253004/100001

[commit-bot: I haz the power, 2014-07-25 21:21:09]

Change committed as 178959