gpu video: optimize HW video to SW canvas and implement it for WebRTC.

gpu video: optimize HW video to SW canvas and implement it for WebRTC.

Currently, very complicated callback mechanism is used to copy HW video to SW
canvas in SkCanvasVideoRenderer. When Blink thread needs to readback HW video,
the readback callback of VideoFrame is called, and then readback is executed in the
media thread using gl context belonging to gpu decoder.

It has two bad points:
1. Blink thread is blocked until the media thread gets readback done.
2. It's not implemented yet for WebRTC HW Video to be drawn on SW Canvas.
In detail for #2, VideoCaptureImpl creates a texture VideoFrame using the
mailbox made in browser process. VideoCaptureImpl doesn't use any gl context so
the readback callback is not implemented. Now the callback is removed.

This CL implements drawing HW Video on SW SkCanvas in SkCanvasVideoRenderer, so
two bad points are resolved.
1. SkCanvasVideoRenderer draws HW Video on SW SkCanvas in Blink thread without
interaction with the media thread.
2. WebRTC impl (i.e. VideoCaptureImpl) doesn't need to do anything because
SkCanvasVideoRenderer has all logic to draw HW Video on SW SkCanvas.

TEST=blink layout test virtual/gpu/fast/canvas/yuv-video-on-accelerated-canvas.html
BUG=401058
Committed: https://crrev.com/16e17206be4635966d3ee5a4bf28bcce9cde5d3f
Cr-Commit-Position: refs/heads/master@{#315819}

[dshwang, 2015-01-14 20:32:12]

dongseong.hwang@intel.com changed reviewers:
+ dalecurtis@chromium.org, scherkus@chromium.org, sievers@chromium.org

[dshwang, 2015-01-14 20:32:14]

Could you review this a bit huge refactory?
This CL removes lots of code.

https://codereview.chromium.org/850993002/diff/1/content/renderer/media/rtc_v...
File content/renderer/media/rtc_video_decoder.cc (left):

https://codereview.chromium.org/850993002/diff/1/content/renderer/media/rtc_v...
content/renderer/media/rtc_video_decoder.cc:426: base::WaitableEvent event(true,
false);
similar impl to GpuVideoDecoder.

https://codereview.chromium.org/850993002/diff/1/media/filters/gpu_video_deco...
File media/filters/gpu_video_decoder.cc (left):

https://codereview.chromium.org/850993002/diff/1/media/filters/gpu_video_deco...
media/filters/gpu_video_decoder.cc:394: factories->ReadPixels(texture_id,
visible_rect, pixels);
it's executed in media thread.

https://codereview.chromium.org/850993002/diff/1/media/filters/gpu_video_deco...
media/filters/gpu_video_decoder.cc:403: base::WaitableEvent event(true, false);
wait Blink thread until the job getting done in media thread

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

https://codereview.chromium.org/850993002/diff/1/media/filters/skcanvas_video...
media/filters/skcanvas_video_renderer.cc:213:
video_frame->ReadPixelsFromNativeTexture(tmp);
Remove this complicated ReadPixel callback implementation in several files.

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

https://codereview.chromium.org/850993002/diff/1/media/filters/skcanvas_video...
media/filters/skcanvas_video_renderer.cc:424: // In SW Canvas case, rely on skia
drawing Ganesh SkBitmap on SW SkCanvas.
As comment mentions, drawing ganesh SkBitmap on Sw SkCanvas is executed here,
which is in Blink thread.

[dshwang, 2015-01-14 20:41:19]

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

[dshwang, 2015-01-14 20:41:20]

https://codereview.chromium.org/850993002/diff/1/content/renderer/media/webme...
File content/renderer/media/webmediaplayer_ms.cc (right):

https://codereview.chromium.org/850993002/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_ms.cc:75:
bitmap.eraseColor(SK_ColorTRANSPARENT);
This CL removes VideoFrame::ReadPixelsFromNativeTexture.
In addition, reusing SkCanvasVideoRenderer::Copy can be faster because
SkCanvasVideoRenderer might cache the texture of video frame.

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

https://codereview.chromium.org/850993002/diff/20001/media/filters/skcanvas_v...
media/filters/skcanvas_video_renderer.cc:271: bitmap->notifyPixelsChanged();
After SkBitmap is changed, it's needed for SW SkCanvas.
For some reason, ganesh SkCanvas is fine without this line.
@junov, do you know the reason?

https://codereview.chromium.org/850993002/diff/20001/media/filters/skcanvas_v...
media/filters/skcanvas_video_renderer.cc:424: // In SW Canvas case, rely on skia
drawing Ganesh SkBitmap on SW SkCanvas.
@junov, Is it safe to draw Ganesh SkBitmap on SW SkCanvas?

[DaleCurtis, 2015-01-14 21:14:28]

I haven't reviewed yet, but your description only lists the bad points and not
what you're doing to address them. Can you add a synopsis to the description?

[dshwang, 2015-01-14 21:46:38]

On 2015/01/14 21:14:28, DaleCurtis wrote:
> I haven't reviewed yet, but your description only lists the bad points and not
> what you're doing to address them. Can you add a synopsis to the description?

Thank you for pointing out. I add what this CL changes in the description.

[Justin Novosad, 2015-01-14 22:06:00]

I am not an OWNER in these areas, but the interactions with SkCanvas look fine
to me.

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

https://codereview.chromium.org/850993002/diff/20001/media/filters/skcanvas_v...
media/filters/skcanvas_video_renderer.cc:271: bitmap->notifyPixelsChanged();
On 2015/01/14 20:41:20, dshwang wrote:
> After SkBitmap is changed, it's needed for SW SkCanvas.
> For some reason, ganesh SkCanvas is fine without this line.
> @junov, do you know the reason?

This call is used for bumping the bitmap's generation ID, which is used as a
cache key in many code paths.  If the rendering code path that is used does not
involve any caching, it will work without this. But it is best to always do it
when you change the bitmap. It is not an expensive call.

https://codereview.chromium.org/850993002/diff/20001/media/filters/skcanvas_v...
media/filters/skcanvas_video_renderer.cc:424: // In SW Canvas case, rely on skia
drawing Ganesh SkBitmap on SW SkCanvas.
On 2015/01/14 20:41:20, dshwang wrote:
> @junov, Is it safe to draw Ganesh SkBitmap on SW SkCanvas?

Yes, it is safe. The use of SkAutoLockPixels inside SkDraw::drawBitmap ensures
that the pixels are accessible in RAM. It will perform a GPU readback if
necessary. It works, but it's slow.

[DaleCurtis, 2015-01-14 22:44:01]

This all looks great to me, deleted code and a simplified process == \o/ 

I'm not very familiar with these paths though, so I'll hold my approval until
the cc and skia folk approve.

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

https://codereview.chromium.org/850993002/diff/20001/media/filters/skcanvas_v...
media/filters/skcanvas_video_renderer.cc:452: {
Extra {} seems unnecessary I don't see any scoped values.

https://codereview.chromium.org/850993002/diff/20001/media/filters/skcanvas_v...
File media/filters/skcanvas_video_renderer.h (right):

https://codereview.chromium.org/850993002/diff/20001/media/filters/skcanvas_v...
media/filters/skcanvas_video_renderer.h:49: void Copy(const
scoped_refptr<VideoFrame>&,
add video_frame variable name.

[dshwang, 2015-01-15 13:44:16]

@junov, @DaleCurtis, thx for reviewing!

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

Skia owner @junov checked skia code is sane.

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

https://codereview.chromium.org/850993002/diff/20001/media/filters/skcanvas_v...
media/filters/skcanvas_video_renderer.cc:452: {
On 2015/01/14 22:44:01, DaleCurtis wrote:
> Extra {} seems unnecessary I don't see any scoped values.

Oops, done :)

https://codereview.chromium.org/850993002/diff/20001/media/filters/skcanvas_v...
File media/filters/skcanvas_video_renderer.h (right):

https://codereview.chromium.org/850993002/diff/20001/media/filters/skcanvas_v...
media/filters/skcanvas_video_renderer.h:49: void Copy(const
scoped_refptr<VideoFrame>&,
On 2015/01/14 22:44:01, DaleCurtis wrote:
> add video_frame variable name.

Done.

[scherkus (not reviewing), 2015-01-15 19:19:29]

lgtm

[dshwang, 2015-01-16 16:15:41]

On 2015/01/15 19:19:29, scherkus wrote:
> lgtm

thx for reviewing!

@danakj, could you review cc/ ?

[dshwang, 2015-01-19 14:43:49]

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

[dshwang, 2015-01-19 14:44:28]

On 2015/01/16 16:15:41, dshwang wrote:
> On 2015/01/15 19:19:29, scherkus wrote:
> > lgtm
> 
> thx for reviewing!
> 
> @danakj, could you review cc/ ?

@reveman, could you review cc/? @danakj might be ooo

[DaleCurtis, 2015-01-20 18:49:28]

dalecurtis@chromium.org changed reviewers:
- dalecurtis@chromium.org

[DaleCurtis, 2015-01-20 18:49:29]

(moving myself to cc since scherkus gave approval!)

[dshwang, 2015-01-21 13:40:32]

On 2015/01/20 18:49:29, DaleCurtis wrote:
> (moving myself to cc since scherkus gave approval!)

thx for being reviewing cc/. wdyt?

https://codereview.chromium.org/850993002/diff/40001/cc/resources/video_resou...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/850993002/diff/40001/cc/resources/video_resou...
cc/resources/video_resource_updater.cc:297: video_renderer_->Copy(video_frame,
&canvas, media::Context3D());
this is software path, so canvas and video_frame are always backed by software

https://codereview.chromium.org/850993002/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_context.cc (right):

https://codereview.chromium.org/850993002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_context.cc:1073: gfx::Rect(0, 0, 3, 2),
gfx::Size(4, 4), base::TimeDelta());
just remove VideoFrame::ReadPixelsCB() but gitclformat reformatted whole lines.

[dshwang, 2015-02-10 08:36:47]

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

[dshwang, 2015-02-10 08:38:45]

@danakj, could you review cc/ and content/browser/renderer_host/ ?

[dshwang, 2015-02-10 08:47:45]

On 2015/02/10 08:38:45, dshwang wrote:
> @danakj, could you review cc/ and content/browser/renderer_host/ ?

blink thread can stop and wait for impl thread.
previously, blink thread can stop and wait for media thread also. when media
thread provides video frame, blink and impl thread use same lock. it looks
dangerous.
this CL removes the code making blink thread wait for media thread.

[danakj, 2015-02-10 17:48:06]

On 2015/02/10 08:38:45, dshwang wrote:
> @danakj, could you review cc/ and content/browser/renderer_host/ ?

LGTM

[danakj, 2015-02-10 17:48:13]

https://codereview.chromium.org/850993002/diff/40001/cc/resources/video_resou...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/850993002/diff/40001/cc/resources/video_resou...
cc/resources/video_resource_updater.cc:297: video_renderer_->Copy(video_frame,
&canvas, media::Context3D());
On 2015/01/21 13:40:32, dshwang wrote:
> this is software path, so canvas and video_frame are always backed by software


how about making this a comment in the code?

[dshwang, 2015-02-10 19:09:13]

@sievers, could you review
content/browser/renderer_host/render_widget_host_view_browsertest.cc ?

On 2015/02/10 17:48:13, danakj wrote:
>
https://codereview.chromium.org/850993002/diff/40001/cc/resources/video_resou...
> File cc/resources/video_resource_updater.cc (right):
> 
>
https://codereview.chromium.org/850993002/diff/40001/cc/resources/video_resou...
> cc/resources/video_resource_updater.cc:297: video_renderer_->Copy(video_frame,
> &canvas, media::Context3D());
> On 2015/01/21 13:40:32, dshwang wrote:
> > this is software path, so canvas and video_frame are always backed by
software
> 
> 
> how about making this a comment in the code?

Thx for reviewing cc/. I'll update

[no sievers, 2015-02-11 01:02:53]

On 2015/02/10 19:09:13, dshwang wrote:
> @sievers, could you review
> content/browser/renderer_host/render_widget_host_view_browsertest.cc ?


lgtm

[dshwang, 2015-02-11 10:14:55]

New patchsets have been uploaded after l-g-t-m from
danakj@chromium.org,sievers@chromium.org

[dshwang, 2015-02-11 13:57:43]

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

[commit-bot: I haz the power, 2015-02-11 13:58:39]

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

[commit-bot: I haz the power, 2015-02-11 14:06:51]

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

[commit-bot: I haz the power, 2015-02-11 14:06:53]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[dshwang, 2015-02-11 18:51:46]

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

[commit-bot: I haz the power, 2015-02-11 18:52:37]

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

[commit-bot: I haz the power, 2015-02-11 20:08:15]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-02-11 20:09:03]

Patchset 5 (id:??) landed as
https://crrev.com/16e17206be4635966d3ee5a4bf28bcce9cde5d3f
Cr-Commit-Position: refs/heads/master@{#315819}