Refactoring source image usage in CanvasRenderingContext2D

Refactoring source image usage in CanvasRenderingContext2D

This change fixes a number of spec compliance and unsupported use case
issues with the drawImage and createPattern canvas API methods.

The CanvasImageSource abstration was added to greatly simplify
code in CanvasRenderingContext2D and make the code more
maintainable for future improvements to drawImage.

BUG=347265, 347229, 343916, 343662, 113592
R=senorblanco@chromium.org

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

[Justin Novosad, 2014-02-26 23:09:40]

PTAL

[Stephen White, 2014-02-27 18:53:41]

Great stuff! Nice to see so many tests fixed. The main concern I have at this
point is the added dependency on HTMLCanvasElement and CanvasRenderingContext in
all implementations of CanvasImageSource. Let's see if we can avoid adding that
dependency, and then we'll be good to go.

https://codereview.chromium.org/181693006/diff/1/LayoutTests/fast/canvas/canv...
File LayoutTests/fast/canvas/canvas-empty-image-pattern.html (left):

https://codereview.chromium.org/181693006/diff/1/LayoutTests/fast/canvas/canv...
LayoutTests/fast/canvas/canvas-empty-image-pattern.html:6:
testRunner.dumpAsTextWithPixelResults();
Bonus points for turning this into a text test.

https://codereview.chromium.org/181693006/diff/1/LayoutTests/fast/dom/gc-9.html
File LayoutTests/fast/dom/gc-9.html (right):

https://codereview.chromium.org/181693006/diff/1/LayoutTests/fast/dom/gc-9.ht...
LayoutTests/fast/dom/gc-9.html:124: [
"document.getElementsByTagName('canvas')[0].getContext('2d').createPattern(document.getElementsByTagName('canvas')[0],
'no-repeat')" ], // CanvasPattern
Tangentially: should we/do we have a test to check recursive canvas pattern
draws? (ie., creating a pattern from a canvas, and drawing that to the same
canvas). There's a special case for direct canvas-to-canvas drawing (makes a
copy); we should check that patterns work too.

https://codereview.chromium.org/181693006/diff/1/LayoutTests/media/video-canv...
File LayoutTests/media/video-canvas-draw.html (right):

https://codereview.chromium.org/181693006/diff/1/LayoutTests/media/video-canv...
LayoutTests/media/video-canvas-draw.html:22: ctx1.fillStyle =
ctx1.createPattern(video, 'repeat');
Should we be testing the other repetition modes as well (repeat-x, repeat-y,
no-repeat)?

https://codereview.chromium.org/181693006/diff/1/Source/core/html/HTMLCanvasE...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/181693006/diff/1/Source/core/html/HTMLCanvasE...
Source/core/html/HTMLCanvasElement.cpp:613: return copiedImage();
You might want to check the canvas-to-canvas self-copying for performance
regressions. I remember it was too easy to trigger a CPU readback in this case
inadvertently.

https://codereview.chromium.org/181693006/diff/1/Source/core/html/HTMLCanvasE...
Source/core/html/HTMLCanvasElement.cpp:617: *isVolatile = true;
Please add a comment here indicating that this only works because
canvas-to-canvas copies currently do a flush at the deferred canvas level. We
may have to revisit this if we later implement deferral for canvas-to-canvas
copies (or maybe not, but it seems good to document).

https://codereview.chromium.org/181693006/diff/1/Source/core/html/HTMLImageEl...
File Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/181693006/diff/1/Source/core/html/HTMLImageEl...
Source/core/html/HTMLImageElement.cpp:441: bool
HTMLImageElement::wouldTaintOrigin(CanvasRenderingContext* dst) const
It seems unfortunate to add a dependency on CanvasRenderingContext (and
HTMLCanvasElement) here. Could we just pass the security origin, and possibly an
interface for querying back the origin by URL instead? (or move the latter up to
a different common class, as you suggested.)

[Justin Novosad, 2014-03-04 20:10:14]

On 2014/02/27 18:53:41, Stephen White wrote:
> Great stuff! Nice to see so many tests fixed. The main concern I have at this
> point is the added dependency on HTMLCanvasElement and CanvasRenderingContext
in
> all implementations of CanvasImageSource. Let's see if we can avoid adding
that
> dependency, and then we'll be good to go.

Done. I removed both dependencies.

>
https://codereview.chromium.org/181693006/diff/1/LayoutTests/fast/dom/gc-9.html
> File LayoutTests/fast/dom/gc-9.html (right):
> 
>
https://codereview.chromium.org/181693006/diff/1/LayoutTests/fast/dom/gc-9.ht...
> LayoutTests/fast/dom/gc-9.html:124: [
>
"document.getElementsByTagName('canvas')[0].getContext('2d').createPattern(document.getElementsByTagName('canvas')[0],
> 'no-repeat')" ], // CanvasPattern
> Tangentially: should we/do we have a test to check recursive canvas pattern
> draws? (ie., creating a pattern from a canvas, and drawing that to the same
> canvas). There's a special case for direct canvas-to-canvas drawing (makes a
> copy); we should check that patterns work too.

Already covered by test fast/canvas/canvas-pattern-modify.html

> 
>
https://codereview.chromium.org/181693006/diff/1/LayoutTests/media/video-canv...
> File LayoutTests/media/video-canvas-draw.html (right):
> 
>
https://codereview.chromium.org/181693006/diff/1/LayoutTests/media/video-canv...
> LayoutTests/media/video-canvas-draw.html:22: ctx1.fillStyle =
> ctx1.createPattern(video, 'repeat');
> Should we be testing the other repetition modes as well (repeat-x, repeat-y,
> no-repeat)?

I don't think it is necessary to cover the entire source_type/repeat_mode
matrix.  The two are orthogonal in the code.

> 
>
https://codereview.chromium.org/181693006/diff/1/Source/core/html/HTMLCanvasE...
> File Source/core/html/HTMLCanvasElement.cpp (right):
> 
>
https://codereview.chromium.org/181693006/diff/1/Source/core/html/HTMLCanvasE...
> Source/core/html/HTMLCanvasElement.cpp:613: return copiedImage();
> You might want to check the canvas-to-canvas self-copying for performance
> regressions. I remember it was too easy to trigger a CPU readback in this case
> inadvertently.

Done.
> 
>
https://codereview.chromium.org/181693006/diff/1/Source/core/html/HTMLCanvasE...
> Source/core/html/HTMLCanvasElement.cpp:617: *isVolatile = true;
> Please add a comment here indicating that this only works because
> canvas-to-canvas copies currently do a flush at the deferred canvas level. We
> may have to revisit this if we later implement deferral for canvas-to-canvas
> copies (or maybe not, but it seems good to document).

Done.
> 
>
https://codereview.chromium.org/181693006/diff/1/Source/core/html/HTMLImageEl...
> File Source/core/html/HTMLImageElement.cpp (right):
> 
>
https://codereview.chromium.org/181693006/diff/1/Source/core/html/HTMLImageEl...
> Source/core/html/HTMLImageElement.cpp:441: bool
> HTMLImageElement::wouldTaintOrigin(CanvasRenderingContext* dst) const
> It seems unfortunate to add a dependency on CanvasRenderingContext (and
> HTMLCanvasElement) here. Could we just pass the security origin, and possibly
an
> interface for querying back the origin by URL instead? (or move the latter up
to
> a different common class, as you suggested.)

Done.

[Stephen White, 2014-03-06 16:06:16]

https://codereview.chromium.org/181693006/diff/20001/Source/core/frame/ImageB...
File Source/core/frame/ImageBitmap.h (right):

https://codereview.chromium.org/181693006/diff/20001/Source/core/frame/ImageB...
Source/core/frame/ImageBitmap.h:36: virtual IntPoint bitmapOffset() const {
return m_bitmapOffset; }
Is this no longer needed?

https://codereview.chromium.org/181693006/diff/60001/Source/core/frame/ImageB...
File Source/core/frame/ImageBitmap.h (right):

https://codereview.chromium.org/181693006/diff/60001/Source/core/frame/ImageB...
Source/core/frame/ImageBitmap.h:36: virtual IntPoint bitmapOffset() const {
return m_bitmapOffset; }
Nit: is this still necessary?

https://codereview.chromium.org/181693006/diff/60001/Source/core/html/HTMLCan...
File Source/core/html/HTMLCanvasElement.h (right):

https://codereview.chromium.org/181693006/diff/60001/Source/core/html/HTMLCan...
Source/core/html/HTMLCanvasElement.h:120: virtual bool originClean() const {
return m_originClean; }
Nit: is this still necessary?

https://codereview.chromium.org/181693006/diff/60001/Source/core/html/HTMLIma...
File Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/181693006/diff/60001/Source/core/html/HTMLIma...
Source/core/html/HTMLImageElement.cpp:429: return usage ==
PatternCanvasImageSourceUsage ? PassRefPtr<Image>() : Image::nullImage();
As discussed, let's see if we can get rid of the usage param, in order to make
the CanvasImageSource implementations independent of their usage. We could try
replacing it with a "copy" bool (for patterns and canvas-to-canvas copies), and
replace isVolatile with an enum for the image state.

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

https://codereview.chromium.org/181693006/diff/60001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1407:
GraphicsContextStateSaver stateSaver(*c);
Nit: might consider refactoring the video-specific stuff (clipping, scaling,
etc) into its own function.

https://codereview.chromium.org/181693006/diff/60001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1457:
c->setCompositeOperation(op, blink::WebBlendModeNormal);
To me, this would be cleaner if we had a drawImageInternal() which takes the
compositing op and blend mode, and call that here. The IDL-exposed drawImage()
could just call drawImageInternal() with the context's compositing params
(probably a one-liner).

[Justin Novosad, 2014-03-07 16:39:07]

New Patch

[Stephen White, 2014-03-10 15:27:02]

LGTM. Much cleaner, thank you!

Remainder of comments are at your discretion.

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

https://codereview.chromium.org/181693006/diff/100001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:293:
canvas()->setOriginTainted();
Do we have tests for pattern tainting (or tainting, generally?)

https://codereview.chromium.org/181693006/diff/100001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1384: SourceImageMode mode
= canvas()->asCanvasImageSource() == imageSource ? CopySourceImageIfVolatile :
DontCopySourceImage;
As discussed, this could just be canvas() == imageSource for clarity.

https://codereview.chromium.org/181693006/diff/100001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1436: if (sourceImageStatus
== ExternalSourceImageStatus && is2d() && isAccelerated() && canvas()->buffer())
<philosoraptor>I wonder if we'd ever get in here if is2d() was false.

[Justin Novosad, 2014-03-10 19:01:20]

On 2014/03/10 15:27:02, Stephen White wrote:
> LGTM. Much cleaner, thank you!
> 
> Remainder of comments are at your discretion.
> 
>
https://codereview.chromium.org/181693006/diff/100001/Source/core/html/canvas...
> File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):
> 
>
https://codereview.chromium.org/181693006/diff/100001/Source/core/html/canvas...
> Source/core/html/canvas/CanvasRenderingContext2D.cpp:293:
> canvas()->setOriginTainted();
> Do we have tests for pattern tainting (or tainting, generally?)

Yes. We're already well covered there.
> 
>
https://codereview.chromium.org/181693006/diff/100001/Source/core/html/canvas...
> Source/core/html/canvas/CanvasRenderingContext2D.cpp:1384: SourceImageMode
mode
> = canvas()->asCanvasImageSource() == imageSource ? CopySourceImageIfVolatile :
> DontCopySourceImage;
> As discussed, this could just be canvas() == imageSource for clarity.

Yep, will fix before committing
> 
>
https://codereview.chromium.org/181693006/diff/100001/Source/core/html/canvas...
> Source/core/html/canvas/CanvasRenderingContext2D.cpp:1436: if
(sourceImageStatus
> == ExternalSourceImageStatus && is2d() && isAccelerated() &&
canvas()->buffer())
> <philosoraptor>I wonder if we'd ever get in here if is2d() was false.

Good point, I just blindly copy-pasted that from the previous implementation,
but the is2d is totally unnecessary.

[Justin Novosad, 2014-03-10 19:27:21]

The CQ bit was checked by junov@chromium.org

[commit-bot: I haz the power, 2014-03-10 19:27:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/junov@chromium.org/181693006/120001

[commit-bot: I haz the power, 2014-03-10 22:10:26]

Change committed as 168864