media: SkCanvasVideoRenderer support for YUV native videoframes.

media: SkCanvasVideoRenderer support for YUV native videoframes.

Allows SkCanvasVideoRenderer to paint YUV native videoframes.

BUG=

Committed: https://crrev.com/31a65d43c6844bc5a92e873be0c13104f14c0a58
Cr-Commit-Position: refs/heads/master@{#333999}

[Daniele Castagna, 2015-06-09 23:23:43]

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

[Daniele Castagna, 2015-06-09 23:23:44]

Hi Dongseong!

Since crrev.com/1144323003 refactor is taking a little bit longer than I
expected, do you mind taking a look at this CL so I can move forward with YUV
native videoframes before crrev.com/1144323003 lands?

[dshwang, 2015-06-10 07:26:38]

overall direction looks good. How did you test this change?

https://codereview.chromium.org/1153623002/diff/40001/media/blink/skcanvas_vi...
File media/blink/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/1153623002/diff/40001/media/blink/skcanvas_vi...
media/blink/skcanvas_video_renderer.cc:110: mailbox_holder.texture_target ==
GL_TEXTURE_RECTANGLE_ARB);
TEXTURE_EXTERNAL_OES?

https://codereview.chromium.org/1153623002/diff/40001/media/blink/skcanvas_vi...
media/blink/skcanvas_video_renderer.cc:113: mailbox_holder.texture_target,
mailbox_holder.mailbox.name);
how these textures are deleted?

https://codereview.chromium.org/1153623002/diff/40001/media/blink/skcanvas_vi...
media/blink/skcanvas_video_renderer.cc:125: kTopLeft_GrSurfaceOrigin);
need to insert sync point here

[Daniele Castagna, 2015-06-10 14:22:54]

https://codereview.chromium.org/1153623002/diff/40001/media/blink/skcanvas_vi...
File media/blink/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/1153623002/diff/40001/media/blink/skcanvas_vi...
media/blink/skcanvas_video_renderer.cc:110: mailbox_holder.texture_target ==
GL_TEXTURE_RECTANGLE_ARB);
On 2015/06/10 at 07:26:38, dshwang wrote:
> TEXTURE_EXTERNAL_OES?

Done.

https://codereview.chromium.org/1153623002/diff/40001/media/blink/skcanvas_vi...
media/blink/skcanvas_video_renderer.cc:113: mailbox_holder.texture_target,
mailbox_holder.mailbox.name);
On 2015/06/10 at 07:26:38, dshwang wrote:
> how these textures are deleted?

Added glDeleteTextures.

https://codereview.chromium.org/1153623002/diff/40001/media/blink/skcanvas_vi...
media/blink/skcanvas_video_renderer.cc:125: kTopLeft_GrSurfaceOrigin);
On 2015/06/10 at 07:26:38, dshwang wrote:
> need to insert sync point here

Done.

[Daniele Castagna, 2015-06-10 14:25:32]

On 2015/06/10 at 07:26:38, dongseong.hwang wrote:
> overall direction looks good. How did you test this change?
> 

Patchset 2 on this cl tested enabling the native YUV videoframes after this
change and all the trybots are ok. Prior to this change there were a few trybots
failing when enabling that feature: crrev.com/1136383003
I also tested it locally with webgl animated textures just to make sure that
visually it's ok.

>
https://codereview.chromium.org/1153623002/diff/40001/media/blink/skcanvas_vi...
> File media/blink/skcanvas_video_renderer.cc (right):
> 
>
https://codereview.chromium.org/1153623002/diff/40001/media/blink/skcanvas_vi...
> media/blink/skcanvas_video_renderer.cc:110: mailbox_holder.texture_target ==
GL_TEXTURE_RECTANGLE_ARB);
> TEXTURE_EXTERNAL_OES?
> 
>
https://codereview.chromium.org/1153623002/diff/40001/media/blink/skcanvas_vi...
> media/blink/skcanvas_video_renderer.cc:113: mailbox_holder.texture_target,
mailbox_holder.mailbox.name);
> how these textures are deleted?
> 
>
https://codereview.chromium.org/1153623002/diff/40001/media/blink/skcanvas_vi...
> media/blink/skcanvas_video_renderer.cc:125: kTopLeft_GrSurfaceOrigin);
> need to insert sync point here

[dshwang, 2015-06-10 17:38:37]

non-owner lgtm with nits

https://codereview.chromium.org/1153623002/diff/60001/media/blink/skcanvas_vi...
File media/blink/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/1153623002/diff/60001/media/blink/skcanvas_vi...
media/blink/skcanvas_video_renderer.cc:355: accelerated_last_image_ =
CreateSkImageFromVideoFrameNativeTextures(
I'm not good at naming but I think the function name should include YUV. 
In addition, the term "Native Texture" is gone.
s/VideoFrameNativeTextures/VideoFrameTextures/

[Daniele Castagna, 2015-06-10 17:40:50]

dcastagna@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[Daniele Castagna, 2015-06-10 17:40:51]

+dalecurtis for owner review

[Daniele Castagna, 2015-06-10 17:42:34]

https://codereview.chromium.org/1153623002/diff/60001/media/blink/skcanvas_vi...
File media/blink/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/1153623002/diff/60001/media/blink/skcanvas_vi...
media/blink/skcanvas_video_renderer.cc:355: accelerated_last_image_ =
CreateSkImageFromVideoFrameNativeTextures(
On 2015/06/10 at 17:38:37, dshwang wrote:
> I'm not good at naming but I think the function name should include YUV. 
> In addition, the term "Native Texture" is gone.
> s/VideoFrameNativeTextures/VideoFrameTextures/

How do you like CreateSkImageFromVideoFrameYUVTextures?

[dshwang, 2015-06-10 17:45:05]

https://codereview.chromium.org/1153623002/diff/60001/media/blink/skcanvas_vi...
File media/blink/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/1153623002/diff/60001/media/blink/skcanvas_vi...
media/blink/skcanvas_video_renderer.cc:355: accelerated_last_image_ =
CreateSkImageFromVideoFrameNativeTextures(
On 2015/06/10 17:42:34, Daniele Castagna wrote:
> On 2015/06/10 at 17:38:37, dshwang wrote:
> > I'm not good at naming but I think the function name should include YUV. 
> > In addition, the term "Native Texture" is gone.
> > s/VideoFrameNativeTextures/VideoFrameTextures/
> 
> How do you like CreateSkImageFromVideoFrameYUVTextures?

I buy it :)

[Daniele Castagna, 2015-06-10 18:02:27]

Thank you for the review dongseong!

https://codereview.chromium.org/1153623002/diff/60001/media/blink/skcanvas_vi...
File media/blink/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/1153623002/diff/60001/media/blink/skcanvas_vi...
media/blink/skcanvas_video_renderer.cc:355: accelerated_last_image_ =
CreateSkImageFromVideoFrameNativeTextures(
On 2015/06/10 at 17:45:05, dshwang wrote:
> On 2015/06/10 17:42:34, Daniele Castagna wrote:
> > On 2015/06/10 at 17:38:37, dshwang wrote:
> > > I'm not good at naming but I think the function name should include YUV. 
> > > In addition, the term "Native Texture" is gone.
> > > s/VideoFrameNativeTextures/VideoFrameTextures/
> > 
> > How do you like CreateSkImageFromVideoFrameYUVTextures?
> 
> I buy it :)

Done.

[DaleCurtis, 2015-06-10 22:46:52]

One q, but otherwise looks good.

https://codereview.chromium.org/1153623002/diff/80001/media/blink/skcanvas_vi...
File media/blink/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/1153623002/diff/80001/media/blink/skcanvas_vi...
media/blink/skcanvas_video_renderer.cc:141: context_3d.gr_context,
kRec601_SkYUVColorSpace, handles, yuvSizes,
Is this the right color space? VideoFrame should have a color space metadata
value you can use.

[Daniele Castagna, 2015-06-11 15:45:03]

Patchset #6 (id:100001) has been deleted

[Daniele Castagna, 2015-06-11 15:45:36]

https://codereview.chromium.org/1153623002/diff/80001/media/blink/skcanvas_vi...
File media/blink/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/1153623002/diff/80001/media/blink/skcanvas_vi...
media/blink/skcanvas_video_renderer.cc:141: context_3d.gr_context,
kRec601_SkYUVColorSpace, handles, yuvSizes,
On 2015/06/10 at 22:46:51, DaleCurtis wrote:
> Is this the right color space? VideoFrame should have a color space metadata
value you can use.

We're using native YUV video frames only for I420, so I'd expect only REC601
color space.

Changed the code to get it from the metadata and added a DCHECK for the non
supported color space.

[DaleCurtis, 2015-06-11 16:28:18]

lgtm % checking that playing this file and copying it to canvas/webgl via this
code path doesn't doesn't trigger the dcheck:

https://code.google.com/p/chromium/codesearch#chromium/src/media/test/pipelin...

[Daniele Castagna, 2015-06-11 18:44:58]

On 2015/06/11 at 16:28:18, dalecurtis wrote:
> lgtm % checking that playing this file and copying it to canvas/webgl via this
code path doesn't doesn't trigger the dcheck:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/test/pipelin...

Tried with both WebGL and Canvas. The DCHECK doesn't trigger.

[Daniele Castagna, 2015-06-11 18:51:41]

The CQ bit was checked by dcastagna@chromium.org

[Daniele Castagna, 2015-06-11 18:51:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
dongseong.hwang@intel.com
Link to the patchset: https://codereview.chromium.org/1153623002/#ps120001
(title: "Set the color format.")

[commit-bot: I haz the power, 2015-06-11 18:53:08]

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

[commit-bot: I haz the power, 2015-06-11 19:01:10]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2015-06-11 19:02:30]

Patchset 6 (id:??) landed as
https://crrev.com/31a65d43c6844bc5a92e873be0c13104f14c0a58
Cr-Commit-Position: refs/heads/master@{#333999}