2D Canvas doesn't blend video with the destination buffer.

2D Canvas doesn't blend video with the destination buffer.

r283338 causes this bug because the fix overwrites the buffer no matter what.
The compositor needs to completely overwrite, while Blink needs to blend. It's
because the compositor reuses the buffer resource and Blink draws the video
frame on the existing GraphicsContext.

BUG=401027
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291070

[dshwang, 2014-08-06 11:10:30]

Hi, this CL fixes canvas bug which https://codereview.chromium.org/388363002/
made.

scherkus@, reed1@, could you review media/ ?
danakj@, could you review cc/ ?

[dshwang, 2014-08-06 11:11:12]

https://codereview.chromium.org/441303002/diff/1/media/filters/skcanvas_video...
File media/filters/skcanvas_video_renderer.cc (left):

https://codereview.chromium.org/441303002/diff/1/media/filters/skcanvas_video...
media/filters/skcanvas_video_renderer.cc:221:
paint.setFilterLevel(SkPaint::kLow_FilterLevel);
Is Low filter really needed?

[danakj, 2014-08-06 13:39:15]

https://codereview.chromium.org/441303002/diff/20001/media/filters/skcanvas_v...
File media/filters/skcanvas_video_renderer_unittest.cc (right):

https://codereview.chromium.org/441303002/diff/20001/media/filters/skcanvas_v...
media/filters/skcanvas_video_renderer_unittest.cc:218: // Test that we don't
blend with existing canvas contents.
This comment is wrong then. But why does this change anything? We go from, for
eg. just writing alpha 0.5 before, to clearing to 1 and blending with 0.5 which
gives us 1*0.5 = 0.5 again.

[Justin Novosad, 2014-08-06 14:29:16]

Using the src compositing mode and skipping the initial clear is a generally
useful optimization.  Could we make this fix context sensitive so that we can
continue to use the optimization in cases where it is safe?

[dshwang, 2014-08-06 14:35:53]

On 2014/08/06 14:29:16, junov wrote:
> Using the src compositing mode and skipping the initial clear is a generally
> useful optimization.  Could we make this fix context sensitive so that we can
> continue to use the optimization in cases where it is safe?

That's good point. Even though this CL fixes alpha issue, Canvas has still
issue; composite and blend mode.
I'll fix it by passing SkXfermode from Blink canvas to SkCanvasVideoRenderer
soon. At that time, compositor can pass Src mode also.
In a while, software compositor lives with this slightly inefficient code.

[dshwang, 2014-08-06 14:47:51]

https://codereview.chromium.org/441303002/diff/20001/media/filters/skcanvas_v...
File media/filters/skcanvas_video_renderer_unittest.cc (right):

https://codereview.chromium.org/441303002/diff/20001/media/filters/skcanvas_v...
media/filters/skcanvas_video_renderer_unittest.cc:218: // Test that we don't
blend with existing canvas contents.
Ah, I've missed. thx. I removed.
This test
1. fill red on canvas
2. blend transparent (0x00000000) video frame on canvas
so color on canvas is still red.

Previously, SkCanvasVideoRenderer::Paint blended by Src mode. so result was
0x00000000

[danakj, 2014-08-06 15:56:40]

On 2014/08/06 14:35:53, dshwang wrote:
> On 2014/08/06 14:29:16, junov wrote:
> > Using the src compositing mode and skipping the initial clear is a generally
> > useful optimization.  Could we make this fix context sensitive so that we
can
> > continue to use the optimization in cases where it is safe?
> 
> That's good point. Even though this CL fixes alpha issue, Canvas has still
> issue; composite and blend mode.
> I'll fix it by passing SkXfermode from Blink canvas to SkCanvasVideoRenderer
> soon. At that time, compositor can pass Src mode also.
> In a while, software compositor lives with this slightly inefficient code.

Can you pass the blend mode from somewhere on the chromium side for now and then
hook it up from blink later (ie pass Src from compositor now tho)?

[dshwang, 2014-08-06 17:41:30]

On 2014/08/06 15:56:40, danakj wrote:
> Can you pass the blend mode from somewhere on the chromium side for now and
then
> hook it up from blink later (ie pass Src from compositor now tho)?

danakj@, That's very good advice. Done.
junov@, Now the optimization is preserved!

[danakj, 2014-08-06 18:07:16]

https://codereview.chromium.org/441303002/diff/100001/cc/resources/video_reso...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/441303002/diff/100001/cc/resources/video_reso...
cc/resources/video_resource_updater.cc:252: // Use SRC mode so we completely
overwrite the buffer because reusing the
It's more like Use Src mode to avoid clearing the buffer first?

https://codereview.chromium.org/441303002/diff/100001/cc/resources/video_reso...
cc/resources/video_resource_updater.cc:254:
lock.sk_canvas()->clear(SK_ColorTRANSPARENT);
Clear isn't needed then anymore right?

[dshwang, 2014-08-06 18:12:40]

https://codereview.chromium.org/441303002/diff/100001/cc/resources/video_reso...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/441303002/diff/100001/cc/resources/video_reso...
cc/resources/video_resource_updater.cc:252: // Use SRC mode so we completely
overwrite the buffer because reusing the
Done.

https://codereview.chromium.org/441303002/diff/100001/cc/resources/video_reso...
cc/resources/video_resource_updater.cc:254:
lock.sk_canvas()->clear(SK_ColorTRANSPARENT);
Oops, you're right! Done.

[danakj, 2014-08-06 18:18:48]

cc LGTM

[Justin Novosad, 2014-08-06 18:30:29]

lgtm (non-owner)

https://codereview.chromium.org/441303002/diff/120001/cc/resources/video_reso...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/441303002/diff/120001/cc/resources/video_reso...
cc/resources/video_resource_updater.cc:257: SkXfermode::kSrc_Mode);
Excellent!

[danakj, 2014-08-06 18:31:48]

https://codereview.chromium.org/441303002/diff/120001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_browsertest.cc
(right):

https://codereview.chromium.org/441303002/diff/120001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:494:
bitmap.eraseColor(SK_ColorTRANSPARENT);
btw can we remove this then?

[dshwang, 2014-08-06 19:22:01]

danakj, junov, Thank you for quick review.

scherkus@, could you review in media/ and content/renderer/media ?
aelias@, could you review
content/browser/renderer_host/render_widget_host_view_browsertest.cc ?

https://codereview.chromium.org/441303002/diff/120001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_browsertest.cc
(right):

https://codereview.chromium.org/441303002/diff/120001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:494:
bitmap.eraseColor(SK_ColorTRANSPARENT);
we can remove this if we use kSrc_Mode, instead of kSrcOver_Mode.
But IMO current code is better, because it's easier to understand.

The logic flow is
1. create and clear SkBitmap
2. create SkCanvas based on SkBitmap
3. draw video_frame on SkCanvas <- 2d graphics folks usually expects video_frame
would be composited by SrcOver.

[danakj, 2014-08-06 19:23:41]

https://codereview.chromium.org/441303002/diff/120001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_browsertest.cc
(right):

https://codereview.chromium.org/441303002/diff/120001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:494:
bitmap.eraseColor(SK_ColorTRANSPARENT);
On 2014/08/06 19:22:01, dshwang wrote:
> we can remove this if we use kSrc_Mode, instead of kSrcOver_Mode.
> But IMO current code is better, because it's easier to understand.
> 
> The logic flow is
> 1. create and clear SkBitmap
> 2. create SkCanvas based on SkBitmap
> 3. draw video_frame on SkCanvas <- 2d graphics folks usually expects
video_frame
> would be composited by SrcOver.

It's a waste of time to touch all the pixels twice tho, just put a comment like
you did in cc?

Maybe a test doesn't matter but I think test speed should matter a bit. Up to
you/aelias though.

[dshwang, 2014-08-07 12:47:53]

scherkus@, could you review in media/ and content/renderer/media ?
aelias@, could you review
content/browser/renderer_host/render_widget_host_view_browsertest.cc ?

https://codereview.chromium.org/441303002/diff/120001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_browsertest.cc
(right):

https://codereview.chromium.org/441303002/diff/120001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:494:
bitmap.eraseColor(SK_ColorTRANSPARENT);
I apply your optimization to new patch set. Thank you for discussing.

https://codereview.chromium.org/441303002/diff/140001/cc/resources/video_reso...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/441303002/diff/140001/cc/resources/video_reso...
cc/resources/video_resource_updater.cc:252: // Use Src mode to completely
overwrite the reusing buffer.
I change this comment from "Use Src mode to avoid clearing the buffer"
Previous comment is technically wrong. SrcOver mode also doesn't clear the
buffer. The benefit of Src mode is that it doesn't blend src with dest buffer.
It just copy&paste src to dest buffer. compositor needs to use Src mode because
of reducing clearing operation for performance.

[aelias_OOO_until_Jul13, 2014-08-07 18:38:50]

render_widget_host_view_browsertest.cc lgtm

[scherkus (not reviewing), 2014-08-08 00:03:25]

rileya: can you look at this for me? you're more up to date on graphics stuff

[rileya (GONE FROM CHROMIUM), 2014-08-08 21:03:24]

On 2014/08/08 00:03:25, scherkus wrote:
> rileya: can you look at this for me? you're more up to date on graphics stuff

This solution looks fine to me (not an OWNER though), but I have a thought:

I find it kinda silly that we're using "Paint()" for what is ultimately just a
copy in most cases. Both cc/ and copying to WebGL want a straight copy, and
painting it through Skia seems like a waste there.

If we're changing interfaces anyways, could we just add a "Copy()" method to
SkCanvasVideoRenderer and WebMediaPlayer? Then Paint() can just always blend,
which seems more consistent with how the rest of Blink uses "Paint".

[aelias_OOO_until_Jul13, 2014-08-08 21:09:39]

On 2014/08/08 at 21:03:24, rileya wrote:
> On 2014/08/08 00:03:25, scherkus wrote:
> > rileya: can you look at this for me? you're more up to date on graphics
stuff
> 
> This solution looks fine to me (not an OWNER though), but I have a thought:
> 
> I find it kinda silly that we're using "Paint()" for what is ultimately just a
copy in most cases. Both cc/ and copying to WebGL want a straight copy, and
painting it through Skia seems like a waste there.
> 
> If we're changing interfaces anyways, could we just add a "Copy()" method to
SkCanvasVideoRenderer and WebMediaPlayer? Then Paint() can just always blend,
which seems more consistent with how the rest of Blink uses "Paint".

Personally, I'd advise against that.  In my experience pixel copy methods that
bypass Skia often have bugs involving pixel format conversions, that sometimes
only reproduce on particular platforms or configurations.  Skia automatically
does the right thing in those cases.  And if you use the right mode (which this
CL appears to), Skia should also go through a fast path that doesn't do
unnecessary work.

[rileya1, 2014-08-08 21:14:32]

Even if it still goes through Skia, I think calling it a copy rather than a
paint is more apt.

[aelias_OOO_until_Jul13, 2014-08-08 21:18:17]

On 2014/08/08 at 21:14:32, rileya wrote:
> Even if it still goes through Skia, I think calling it a copy rather than a
paint is more apt.

OK, I have no strong opinion on the naming.

[dshwang, 2014-08-11 08:53:10]

On 2014/08/08 21:18:17, aelias wrote:
> On 2014/08/08 at 21:14:32, rileya wrote:
> > Even if it still goes through Skia, I think calling it a copy rather than a
> paint is more apt.
> 
> OK, I have no strong opinion on the naming.

rlleya1@, Thank you for good opinion. Do you prefer SkCanvasVideoRenderer::Copy
to SkCanvasVideoRenderer::Paint ?
However, Copy with alpha and SkXFermode arguments is a bit weird..
void Copy(media::VideoFrame* video_frame,
              SkCanvas* canvas,
              const gfx::RectF& dest_rect,
              uint8 alpha,
              SkXfermode::Mode mode);
If we decide to change the name, do I need to change both
SkCanvasVideoRenderer::Paint and WebMediaPlayer::Paint?

[rileya (GONE FROM CHROMIUM), 2014-08-11 20:25:53]

What I meant was keeping Paint() around as-is (without xfermode, just always
blend), and having a separate new Copy() method that overwrites the destination
buffer (for use with cc/ and WebGL).

Down the road I'd like to see about optimizing this path a bit more (in the
WebGL case, it seems silly that we YUV convert in software and then do an
additional copy of the frame here, before uploading to the GPU...).

[dshwang, 2014-08-12 08:31:26]

On 2014/08/11 20:25:53, rileya wrote:
> What I meant was keeping Paint() around as-is (without xfermode, just always
> blend), and having a separate new Copy() method that overwrites the
destination
> buffer (for use with cc/ and WebGL).

Yep, I add Copy() and make cc/ use it. But WebGL cannot use it because we don't
know which WebGL or Canvas or Blink renderer call WebMediaPlayer::Paint().
And I remained xfermode argument also to fix Canvas blend bug in the next CL.
Could you review again?

> Down the road I'd like to see about optimizing this path a bit more (in the
> WebGL case, it seems silly that we YUV convert in software and then do an
> additional copy of the frame here, before uploading to the GPU...).

That's cool plan. I hope to involve it.

[dshwang, 2014-08-14 12:01:55]

rileya@, scherkus@, reed1@, could you review media/ and content/renderer/media ?

[reed1, 2014-08-14 13:47:40]

skia usage looks good

[dshwang, 2014-08-18 13:24:51]

On 2014/08/14 13:47:40, reed1 wrote:
> skia usage looks good

Thank you.
scherkus@, reed1@, could you review media/ and content/renderer/media ?

[dshwang, 2014-08-20 16:50:40]

rileya@, scherkus@, could you review media/ and content/renderer/media again?
Thanks.

[rileya (GONE FROM CHROMIUM), 2014-08-21 00:14:11]

Sorry for the delay, was out last week.

media/ looks good to me, looks like once things are in line on the Blink side
we'll be able to clean this up a bit more.

I'm not an OWNER though, so scherkus@ will need to take a look.

[scherkus (not reviewing), 2014-08-21 01:09:01]

nits on comments, but lgtm otherwise

https://codereview.chromium.org/441303002/diff/160001/content/renderer/media/...
File content/renderer/media/android/webmediaplayer_android.h (right):

https://codereview.chromium.org/441303002/diff/160001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.h:117: // TODO(dshwang):
remove it.
it's a bit silly... but I really like having TODOs be self-explanatory at all
times -- even if you plan on fixing it soon

if you go on vacation for a month after this lands (or get busy working on
something else, etc...) the next person who sees this code will have to figure
out what your original intent was

here we should at least have a few words saying "why" and/or link to a bug that
contains more details

https://codereview.chromium.org/441303002/diff/160001/content/renderer/media/...
File content/renderer/media/webmediaplayer_impl.h (right):

https://codereview.chromium.org/441303002/diff/160001/content/renderer/media/...
content/renderer/media/webmediaplayer_impl.h:96: // TODO(dshwang): remove it.
ditto

https://codereview.chromium.org/441303002/diff/160001/content/renderer/media/...
File content/renderer/media/webmediaplayer_ms.h (right):

https://codereview.chromium.org/441303002/diff/160001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms.h:84: // TODO(dshwang): remove it.
ditto

[dshwang, 2014-08-21 12:43:17]

On 2014/08/21 01:09:01, scherkus wrote:
> nits on comments, but lgtm otherwise
> 
>
https://codereview.chromium.org/441303002/diff/160001/content/renderer/media/...
> File content/renderer/media/android/webmediaplayer_android.h (right):
> 
>
https://codereview.chromium.org/441303002/diff/160001/content/renderer/media/...
> content/renderer/media/android/webmediaplayer_android.h:117: // TODO(dshwang):
> remove it.
> it's a bit silly... but I really like having TODOs be self-explanatory at all
> times -- even if you plan on fixing it soon
> 
> if you go on vacation for a month after this lands (or get busy working on
> something else, etc...) the next person who sees this code will have to figure
> out what your original intent was
> 
> here we should at least have a few words saying "why" and/or link to a bug
that
> contains more details

Thank you for review. DONE.

[dshwang, 2014-08-21 12:43:22]

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

[commit-bot: I haz the power, 2014-08-21 12:44:39]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/441303002/18...

[commit-bot: I haz the power, 2014-08-21 15:00:40]

Committed patchset #10 (180001) as 291070