canvas2d.drawImage(video) doesn't composite correctly.

canvas2d.drawImage(video) doesn't composite correctly.

Following composite operation should update whole canvas area; SourceIn,
SourceOut, DestinationIn, DestinationAtop, Copy. However, drawImage(video)
doesn't do that unlike drawImage(image) and drawImage(canvas)

In addition, drawImage(video) invalidated a bigger region that needed. This
CL fixes it and add a relevant test.

TEST=fast/canvas/canvas-composite-video.html,
fast/canvas/canvas-composite-repaint-by-all-imagesource.html
BUG=407079
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183541

[dshwang, 2014-09-01 10:58:38]

dongseong.hwang@intel.com changed reviewers:
+ junov@chromium.org

[dshwang, 2014-09-01 10:59:18]

Hi, could you review? The video png is very same to -canvas and -image png

[Justin Novosad, 2014-09-02 16:42:56]

On 2014/09/01 10:59:18, dshwang(Vacation) wrote:
> Hi, could you review? The video png is very same to -canvas and -image png

The code change looks good, but could you add one more test that checks repaint
regions?
see fast/repaint/canvas-putImageData.html for an example of a canvas repaint
test.

It would be nice to have throrough coverage of repaint bounds od drawImage calls
for all permutations CanvasImageSource type, CompositeOperation, and geometric
configuration
By geometric configuration, I mean:
a) destination rect contains the clip
b) clip contains destination rect
c) clip intersect destination rect
d) destination rect is outside of clip
e) two non-intersecting rects that are inside the clip.

[Justin Novosad, 2014-09-02 16:45:03]

https://codereview.chromium.org/530723003/diff/20001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (left):

https://codereview.chromium.org/530723003/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1540: FloatRect dirtyRect;
I think there was a bug here in the old code here. Because we were re-decalring
dirtyRect in this local scope, we probably ended up invalidating the entire clip
bounds instead of just the updated region. Having a test that checks repaint
bounds would have caught this.

[dshwang, 2014-10-03 17:08:15]

On 2014/09/02 16:45:03, junov wrote:
>
https://codereview.chromium.org/530723003/diff/20001/Source/core/html/canvas/...
> File Source/core/html/canvas/CanvasRenderingContext2D.cpp (left):
> 
>
https://codereview.chromium.org/530723003/diff/20001/Source/core/html/canvas/...
> Source/core/html/canvas/CanvasRenderingContext2D.cpp:1540: FloatRect
dirtyRect;
> I think there was a bug here in the old code here. Because we were
re-decalring
> dirtyRect in this local scope, we probably ended up invalidating the entire
clip
> bounds instead of just the updated region. Having a test that checks repaint
> bounds would have caught this.

Sorry for delay, I'm back from long vacation. I'll do it soon.

[dshwang, 2014-10-03 19:13:38]

I add canvas-composite-repaint-by-all-imagesource.html which check repaint
regions on all combinations of fillRect() and drawImage(3 kinds source) with all
composite modes.
Could you review again?

[Justin Novosad, 2014-10-06 17:47:32]

On 2014/10/03 19:13:38, dshwang wrote:
> I add canvas-composite-repaint-by-all-imagesource.html which check repaint
> regions on all combinations of fillRect() and drawImage(3 kinds source) with
all
> composite modes.
> Could you review again?

Very nice test.
The failures on the bots are because the results of
canvas-composite-repaint-by-all-imagesource are different when GPU acceleration
is enabled.  This is normal. It is because each canvas renders to a separate
compositing layer when gpu acceleration is enabled.  Just add a new baseline for
the accelerated case as
virtual/gpu/fast/canvas/canvas-composite-repaint-by-all-imagesource-expected.txt,
and this change will be good to go.

lgtm

[dshwang, 2014-10-09 17:54:40]

On 2014/10/06 17:47:32, junov wrote:
> On 2014/10/03 19:13:38, dshwang wrote:
> > I add canvas-composite-repaint-by-all-imagesource.html which check repaint
> > regions on all combinations of fillRect() and drawImage(3 kinds source) with
> all
> > composite modes.
> > Could you review again?
> 
> Very nice test.
> The failures on the bots are because the results of
> canvas-composite-repaint-by-all-imagesource are different when GPU
acceleration
> is enabled.  This is normal. It is because each canvas renders to a separate
> compositing layer when gpu acceleration is enabled.  Just add a new baseline
for
> the accelerated case as
>
virtual/gpu/fast/canvas/canvas-composite-repaint-by-all-imagesource-expected.txt,
> and this change will be good to go.
> 
> lgtm


Thank you for review and good explanation why trybot failed. I marked
virtual/gpu/fast/canvas/canvas-composite-repaint-by-all-imagesource.html to
NeedsRebaseline.

[dshwang, 2014-10-09 17:54:44]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2014-10-09 17:56:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/530723003/60001

[commit-bot: I haz the power, 2014-10-10 05:42:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-10 05:42:16]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/26579)

[dshwang, 2014-10-10 11:47:46]

The CQ bit was checked by dongseong.hwang@intel.com

[dshwang, 2014-10-10 11:48:22]

On 2014/10/10 05:42:16, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   mac_blink_rel on tryserver.blink
>
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/26579)

repaint test might depends on platform also, so mark
fast/canvas/canvas-composite-repaint-by-all-imagesource.html to NeedsRebaseline

[commit-bot: I haz the power, 2014-10-10 11:48:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/530723003/490001

[commit-bot: I haz the power, 2014-10-10 13:05:52]

Committed patchset #5 (id:490001) as 183541