Fixing src rectangle clipping with drawImage

Fixing src rectangle clipping with drawImage

BUG=260292
TEST=LayoutTests/fast/canvas/canvas-drawImage-out-of-bounds-src.html

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

[Justin Novosad, 2013-07-22 20:43:49]

PTAL

https://codereview.chromium.org/19267027/diff/11001/LayoutTests/canvas/philip...
File LayoutTests/canvas/philip/tests/2d.drawImage.outsidesource-expected.txt
(right):

https://codereview.chromium.org/19267027/diff/11001/LayoutTests/canvas/philip...
LayoutTests/canvas/philip/tests/2d.drawImage.outsidesource-expected.txt:9:
Failed assertion: got pixel [255,0,0,255] at (50,25), expected [0,255,0,255] +/-
2
This test is obsolete w.r.t. current version of the spec. Not adjusting test
because it is from upstream.

[Stephen White, 2013-07-22 22:02:05]

https://codereview.chromium.org/19267027/diff/26001/LayoutTests/fast/canvas/c...
File LayoutTests/fast/canvas/canvas-drawImage-out-of-bounds-src.html (right):

https://codereview.chromium.org/19267027/diff/26001/LayoutTests/fast/canvas/c...
LayoutTests/fast/canvas/canvas-drawImage-out-of-bounds-src.html:15: img.src =
bgcanvas.toDataURL();
Looks like you create this image, but don't do anything with it. Am I missing
something?

Shouldn't we also have tests for video?

https://codereview.chromium.org/19267027/diff/26001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/19267027/diff/26001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1194: static inline void
clipRectsToImageRect(const FloatRect& imageRect, FloatRect& srcRect, FloatRect&
dstRect)
Just personal style, but when it comes to arguments, I prefer non-const pointers
over non-const references. It makes it easier to see what may be modified at the
call site. (Code style doesn't say either way, though, so I won't stop you.)

[aandrey, 2013-07-23 09:25:39]

FYI

https://codereview.chromium.org/19267027/diff/26001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/19267027/diff/26001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1520: if
(!videoRect.intersects(normalizeRect(normalizedSrcRect)) ||
!normalizedDstRect.width() || !normalizedDstRect.height())
remove redundant normalizeRect() call?

[Justin Novosad, 2013-07-23 17:38:34]

New Patch.

https://codereview.chromium.org/19267027/diff/26001/LayoutTests/fast/canvas/c...
File LayoutTests/fast/canvas/canvas-drawImage-out-of-bounds-src.html (right):

https://codereview.chromium.org/19267027/diff/26001/LayoutTests/fast/canvas/c...
LayoutTests/fast/canvas/canvas-drawImage-out-of-bounds-src.html:15: img.src =
bgcanvas.toDataURL();
On 2013/07/22 22:02:06, Stephen White wrote:
> Looks like you create this image, but don't do anything with it. Am I missing
> something?
> 
> Shouldn't we also have tests for video?

Done. 'img' was for the second block of tests (copy/paste whoops).  Added test
for video. Also added test for ImageBitmap

https://codereview.chromium.org/19267027/diff/26001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/19267027/diff/26001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1194: static inline void
clipRectsToImageRect(const FloatRect& imageRect, FloatRect& srcRect, FloatRect&
dstRect)
On 2013/07/22 22:02:06, Stephen White wrote:
> Just personal style, but when it comes to arguments, I prefer non-const
pointers
> over non-const references. It makes it easier to see what may be modified at
the
> call site. (Code style doesn't say either way, though, so I won't stop you.)

Done.

https://codereview.chromium.org/19267027/diff/26001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1520: if
(!videoRect.intersects(normalizeRect(normalizedSrcRect)) ||
!normalizedDstRect.width() || !normalizedDstRect.height())
On 2013/07/23 09:25:40, aandrey wrote:
> remove redundant normalizeRect() call?

Done.

[Justin Novosad, 2013-07-23 18:39:05]

In the latest patch I modified the test so that it passes with gpu-acceleration
enabled.  The filtering that gets applied when painting from an accelerated
video decoder was resulting in sub-pixels discrepancies that were preventing the
test from working as a ref test. To fix the issue, an new video resource was
added. The video is uniform green, so it is not affected by color interpolation
filters.

[Stephen White, 2013-07-23 19:18:04]

On 2013/07/23 18:39:05, junov wrote:
> In the latest patch I modified the test so that it passes with
gpu-acceleration
> enabled.  The filtering that gets applied when painting from an accelerated
> video decoder was resulting in sub-pixels discrepancies that were preventing
the
> test from working as a ref test. To fix the issue, an new video resource was
> added. The video is uniform green, so it is not affected by color
interpolation
> filters.

I'm seeing timeouts on the new test on both raster and GPU:

Regressions: Unexpected timeouts (2)
  fast/canvas/canvas-drawImage-out-of-bounds-src.html [ Timeout ]
  virtual/gpu/fast/canvas/canvas-drawImage-out-of-bounds-src.html [ Timeout ]

Is that the most recent patch?

[Stephen White, 2013-07-23 19:20:16]

On 2013/07/23 19:18:04, Stephen White wrote:
> On 2013/07/23 18:39:05, junov wrote:
> > In the latest patch I modified the test so that it passes with
> gpu-acceleration
> > enabled.  The filtering that gets applied when painting from an accelerated
> > video decoder was resulting in sub-pixels discrepancies that were preventing
> the
> > test from working as a ref test. To fix the issue, an new video resource was
> > added. The video is uniform green, so it is not affected by color
> interpolation
> > filters.
> 
> I'm seeing timeouts on the new test on both raster and GPU:
> 
> Regressions: Unexpected timeouts (2)
>   fast/canvas/canvas-drawImage-out-of-bounds-src.html [ Timeout ]
>   virtual/gpu/fast/canvas/canvas-drawImage-out-of-bounds-src.html [ Timeout ]
> 
> Is that the most recent patch?

Hmm, maybe that's because of the video: trybots don't like binary patches.

At a minimum, you should mark the new test as failing until it lands (with the
video).

Alternatively, you could land the video first, then run this patch by the
trybots.

[Justin Novosad, 2013-07-23 19:55:09]

I went for the second proposal. Video file was pre-submitted separately.  Now we
can use try+CQ for the good stuff

[Stephen White, 2013-07-23 20:39:34]

LGTM

[commit-bot: I haz the power, 2013-07-23 22:23:29]

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

[commit-bot: I haz the power, 2013-07-24 00:58:31]

Change committed as 154798