vaapi plumbing to allow hardware video overlays

vaapi plumbing to allow hardware video overlays

Uses ozone GLImage instead of GLImageLinuxDMABuffer and binds it in the
TextureManager, this lets it be scheduled as an overlay

BUG=370522
TEST=loaded on device (with 812913003, 806413004, 855403002, 810343004), play h264 video, verify that overlays are used and no artifacts are produced)

Committed: https://crrev.com/79812c1a67c5cbd7b60ecc117c3d1d6492c8db38
Cr-Commit-Position: refs/heads/master@{#314376}

[achaulk, 2015-01-19 18:41:17]

achaulk@chromium.org changed reviewers:
+ spang@chromium.org, vrk@chromium.org

[spang, 2015-01-19 18:46:18]

spang@chromium.org changed reviewers:
+ alexst@chromium.org

[alexst (slow to review), 2015-01-19 19:02:17]

https://codereview.chromium.org/858653002/diff/20001/content/common/gpu/media...
File content/common/gpu/media/vaapi_video_decode_accelerator.h (right):

https://codereview.chromium.org/858653002/diff/20001/content/common/gpu/media...
content/common/gpu/media/vaapi_video_decode_accelerator.h:274:
gpu::gles2::TextureManager* texture_manager_;
Is it safe to cache this here from a lifetime perspective?

[achaulk, 2015-01-19 19:05:49]

https://codereview.chromium.org/858653002/diff/20001/content/common/gpu/media...
File content/common/gpu/media/vaapi_video_decode_accelerator.h (right):

https://codereview.chromium.org/858653002/diff/20001/content/common/gpu/media...
content/common/gpu/media/vaapi_video_decode_accelerator.h:274:
gpu::gles2::TextureManager* texture_manager_;
On 2015/01/19 19:02:17, alexst wrote:
> Is it safe to cache this here from a lifetime perspective?

It should be. This object is owned by the GpuVideoDecodeAccelerator which in
turn has its lifetime bounded by GpuCommandBufferStub, so when the command
buffer is destroyed this object should also be torn down

[alexst (slow to review), 2015-01-19 20:02:36]

alexst@chromium.org changed reviewers:
+ posciak@chromium.org

[alexst (slow to review), 2015-01-19 20:03:25]

lgtm

and +posciak for chromeos video TL

[DaleCurtis, 2015-01-20 18:33:23]

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

[DaleCurtis, 2015-01-20 18:33:24]

-vrk who no longer works on the project.

[Pawel Osciak, 2015-01-21 13:07:07]

posciak@chromium.org changed reviewers:
+ danakj@chromium.org, piman@chromium.org

[Pawel Osciak, 2015-01-21 13:07:08]

Please provide TEST= line and explain how/on what this was tested.

Also, would it be possible to have this working in
video_decode_accelerator_unittest?

PictureBuffer is used by PepperVideoDecoderHost, after the change in
PictureBuffer interface, I don't think it compiles anymore?

video_decode_accelerator_unittest will also not compile after this change I
think?

https://chromiumcodereview.appspot.com/858653002/diff/20001/content/common/gp...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/858653002/diff/20001/content/common/gp...
content/common/gpu/media/gpu_video_decode_accelerator.cc:483:
service_texture_id, texture_ids[i]));
Could you please explain the difference between service_texture_id and
texture_id ? Why do we need texture_id vs service_texture_id?

https://chromiumcodereview.appspot.com/858653002/diff/20001/content/common/gp...
File content/common/gpu/media/vaapi_drm_picture.cc (right):

https://chromiumcodereview.appspot.com/858653002/diff/20001/content/common/gp...
content/common/gpu/media/vaapi_drm_picture.cc:119:
texture_manager->SetLevelImage(texture_ref, GL_TEXTURE_EXTERNAL_OES, 0,
I think we can get texture_ref from texture_manager by passing texture_id? 
Could we just pass texture_manager only to this method to save on arguments?

https://chromiumcodereview.appspot.com/858653002/diff/20001/content/common/gp...
File content/common/gpu/media/vaapi_drm_picture.h (right):

https://chromiumcodereview.appspot.com/858653002/diff/20001/content/common/gp...
content/common/gpu/media/vaapi_drm_picture.h:52: // EGLImage bound to the GL
textures used by the VDA client.
Please update documentation.

https://chromiumcodereview.appspot.com/858653002/diff/20001/content/common/gp...
File content/common/gpu/media/vaapi_picture.h (right):

https://chromiumcodereview.appspot.com/858653002/diff/20001/content/common/gp...
content/common/gpu/media/vaapi_picture.h:37: virtual bool
Initialize(gpu::gles2::TextureManager* texture_manager,
Please add documentation for changed methods in this file.

https://chromiumcodereview.appspot.com/858653002/diff/20001/content/common/gp...
content/common/gpu/media/vaapi_picture.h:43: bool allow_overlay() const { return
allow_overlay_; }
Could this instead be a virtual method returning false, and overriden to return
true in DrmPicture?

https://chromiumcodereview.appspot.com/858653002/diff/20001/content/common/gp...
File content/common/gpu/media/vaapi_tfp_picture.cc (right):

https://chromiumcodereview.appspot.com/858653002/diff/20001/content/common/gp...
content/common/gpu/media/vaapi_tfp_picture.cc:39: bool
VaapiTFPPicture::Initialize(gpu::gles2::TextureManager* texture_manager,
Unused parameters should be commented out per coding style.

[achaulk, 2015-01-21 17:10:03]

Everything builds fine - I added a new constructor instead of changing the
existing one. I'll look at adding something to the test file

https://chromiumcodereview.appspot.com/858653002/diff/20001/content/common/gp...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/858653002/diff/20001/content/common/gp...
content/common/gpu/media/gpu_video_decode_accelerator.cc:483:
service_texture_id, texture_ids[i]));
On 2015/01/21 13:07:07, Pawel Osciak wrote:
> Could you please explain the difference between service_texture_id and
> texture_id ? Why do we need texture_id vs service_texture_id?

texture_id[] is the client-side (browser) texture ID, while service_texture_id
is the real one that GL actually uses. We need texture_id[] to look up the
TextureRef later on, and service_texture_id to pass to GL to actually do the
binding, etc

https://chromiumcodereview.appspot.com/858653002/diff/20001/content/common/gp...
File content/common/gpu/media/vaapi_drm_picture.cc (right):

https://chromiumcodereview.appspot.com/858653002/diff/20001/content/common/gp...
content/common/gpu/media/vaapi_drm_picture.cc:119:
texture_manager->SetLevelImage(texture_ref, GL_TEXTURE_EXTERNAL_OES, 0,
On 2015/01/21 13:07:08, Pawel Osciak wrote:
> I think we can get texture_ref from texture_manager by passing texture_id? 
> Could we just pass texture_manager only to this method to save on arguments?

Unfortunately, it's the wrong texture_id, we need the client ID to get the
TextureRef, but have the service ID. We would still need to pass the extra arg
either way, all that would change is the lookup location

https://chromiumcodereview.appspot.com/858653002/diff/20001/content/common/gp...
File content/common/gpu/media/vaapi_drm_picture.h (right):

https://chromiumcodereview.appspot.com/858653002/diff/20001/content/common/gp...
content/common/gpu/media/vaapi_drm_picture.h:52: // EGLImage bound to the GL
textures used by the VDA client.
On 2015/01/21 13:07:08, Pawel Osciak wrote:
> Please update documentation.

This is still right, it is still an EGLImage, just through a different subclass

https://chromiumcodereview.appspot.com/858653002/diff/20001/content/common/gp...
File content/common/gpu/media/vaapi_picture.h (right):

https://chromiumcodereview.appspot.com/858653002/diff/20001/content/common/gp...
content/common/gpu/media/vaapi_picture.h:43: bool allow_overlay() const { return
allow_overlay_; }
On 2015/01/21 13:07:08, Pawel Osciak wrote:
> Could this instead be a virtual method returning false, and overriden to
return
> true in DrmPicture?

It could be, sure

[achaulk, 2015-01-21 19:14:57]

Oh, yeah the tests are broken in one particular configuration. Hmm

[piman, 2015-01-21 23:58:22]

LGTM overall... It'd be nice if we didn't have to expose the gles2 classes all
the way down to VAAPI stuff (feels a bit of a layering inversion), but I don't
have a great idea that doesn't introduce more complexity.

I don't think you need 2 ids in PictureBuffer. If you have the client id, it's
easy to get to the service id.

https://chromiumcodereview.appspot.com/858653002/diff/20001/content/common/gp...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/858653002/diff/20001/content/common/gp...
content/common/gpu/media/gpu_video_decode_accelerator.cc:476: if
(!command_decoder->GetServiceTextureId(
Unrelated to your patch, but this is not needed any more, since you already have
the TextureRef (the Texture, even). This would save a hash lookup.
GetServiceTextureId was introduced only for GVDA, it can be ripped out
completely.

https://chromiumcodereview.appspot.com/858653002/diff/20001/content/common/gp...
content/common/gpu/media/gpu_video_decode_accelerator.cc:483:
service_texture_id, texture_ids[i]));
On 2015/01/21 17:10:03, achaulk wrote:
> On 2015/01/21 13:07:07, Pawel Osciak wrote:
> > Could you please explain the difference between service_texture_id and
> > texture_id ? Why do we need texture_id vs service_texture_id?
> 
> texture_id[] is the client-side (browser) texture ID, while service_texture_id
> is the real one that GL actually uses. We need texture_id[] to look up the
> TextureRef later on, and service_texture_id to pass to GL to actually do the
> binding, etc

If you have the TextureRef, you can get the service_id() directly
(texture_ref->service_id()), so maybe you don't need to keep both ids?

https://chromiumcodereview.appspot.com/858653002/diff/20001/content/common/gp...
File content/common/gpu/media/vaapi_picture.h (right):

https://chromiumcodereview.appspot.com/858653002/diff/20001/content/common/gp...
content/common/gpu/media/vaapi_picture.h:56: gpu::gles2::TextureRef*
texture_ref,
You may want to document that the texture_ref can't be kept around, or pass in a
scoped_refptr.
Otherwise the client could delete its texture id, causing the TextureRef to be
destroyed. The TextureRef is only valid from the time of the lookup and the next
command execution (in practice the next task).

[achaulk, 2015-01-22 18:13:47]

Is is ugly, yes. I've uploaded another approach which doesn't import gles into
vaapi, it seems a bit cleaner

https://chromiumcodereview.appspot.com/858653002/diff/20001/content/common/gp...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/858653002/diff/20001/content/common/gp...
content/common/gpu/media/gpu_video_decode_accelerator.cc:476: if
(!command_decoder->GetServiceTextureId(
On 2015/01/21 23:58:22, piman (Very slow to review) wrote:
> Unrelated to your patch, but this is not needed any more, since you already
have
> the TextureRef (the Texture, even). This would save a hash lookup.
> GetServiceTextureId was introduced only for GVDA, it can be ripped out
> completely.

Done.

https://chromiumcodereview.appspot.com/858653002/diff/20001/content/common/gp...
content/common/gpu/media/gpu_video_decode_accelerator.cc:483:
service_texture_id, texture_ids[i]));
On 2015/01/21 23:58:22, piman (Very slow to review) wrote:
> On 2015/01/21 17:10:03, achaulk wrote:
> > On 2015/01/21 13:07:07, Pawel Osciak wrote:
> > > Could you please explain the difference between service_texture_id and
> > > texture_id ? Why do we need texture_id vs service_texture_id?
> > 
> > texture_id[] is the client-side (browser) texture ID, while
service_texture_id
> > is the real one that GL actually uses. We need texture_id[] to look up the
> > TextureRef later on, and service_texture_id to pass to GL to actually do the
> > binding, etc
> 
> If you have the TextureRef, you can get the service_id() directly
> (texture_ref->service_id()), so maybe you don't need to keep both ids?

Yes, but if I pass only the client ID instead, all VDA implementations would
need to be changed and also have to know about gles stuff

[piman, 2015-01-22 19:18:32]

OK, I like this. LGTM.

[Pawel Osciak, 2015-01-24 00:58:55]

https://chromiumcodereview.appspot.com/858653002/diff/60001/content/common/gp...
File content/common/gpu/media/vaapi_picture.h (right):

https://chromiumcodereview.appspot.com/858653002/diff/60001/content/common/gp...
content/common/gpu/media/vaapi_picture.h:67: bool allow_overlay)
Could we please get rid of this in favor of a virtual method returning false and
overriden to true in DrmPicture as discussed previously?

https://chromiumcodereview.appspot.com/858653002/diff/60001/content/common/gp...
File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/858653002/diff/60001/content/common/gp...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:78: bind_image)
Is this correct indentation?

https://chromiumcodereview.appspot.com/858653002/diff/60001/content/common/gp...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:536:
bind_image_.Run(buffers[i].internal_texture_id(),
Sorry, I'm confused, could you please explain why is this better done here than
in VaapiDrmPicture?
Also, is there a way we could have a version of Bind for VaapiTfpPicture as
well?

https://chromiumcodereview.appspot.com/858653002/diff/60001/content/common/gp...
File content/common/gpu/media/vaapi_video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/858653002/diff/60001/content/common/gp...
content/common/gpu/media/vaapi_video_decode_accelerator.h:53: bind_image);
Is this a correct indentation?

https://chromiumcodereview.appspot.com/858653002/diff/60001/content/common/gp...
content/common/gpu/media/vaapi_video_decode_accelerator.h:273:
base::Callback<void(uint32, uint32, scoped_refptr<gfx::GLImage>)> bind_image_;
Please document.

[Pawel Osciak, 2015-01-24 01:32:38]

How could we have overlays working in video_decode_accelerator_unittest?

[achaulk, 2015-01-27 18:36:48]

https://codereview.chromium.org/858653002/diff/60001/content/common/gpu/media...
File content/common/gpu/media/vaapi_picture.h (right):

https://codereview.chromium.org/858653002/diff/60001/content/common/gpu/media...
content/common/gpu/media/vaapi_picture.h:67: bool allow_overlay)
On 2015/01/24 00:58:54, Pawel Osciak wrote:
> Could we please get rid of this in favor of a virtual method returning false
and
> overriden to true in DrmPicture as discussed previously?

Done.

https://codereview.chromium.org/858653002/diff/60001/content/common/gpu/media...
File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right):

https://codereview.chromium.org/858653002/diff/60001/content/common/gpu/media...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:536:
bind_image_.Run(buffers[i].internal_texture_id(),
On 2015/01/24 00:58:54, Pawel Osciak wrote:
> Sorry, I'm confused, could you please explain why is this better done here
than
> in VaapiDrmPicture?
> Also, is there a way we could have a version of Bind for VaapiTfpPicture as
> well?

It's roughly equivalent, but in the case where we want a non-drm version to do
this as well this is easier. I'm not sure if that will ever be the case though

https://codereview.chromium.org/858653002/diff/60001/content/common/gpu/media...
File content/common/gpu/media/vaapi_video_decode_accelerator.h (right):

https://codereview.chromium.org/858653002/diff/60001/content/common/gpu/media...
content/common/gpu/media/vaapi_video_decode_accelerator.h:53: bind_image);
On 2015/01/24 00:58:54, Pawel Osciak wrote:
> Is this a correct indentation?

That's what git cl format produces

https://codereview.chromium.org/858653002/diff/60001/content/common/gpu/media...
content/common/gpu/media/vaapi_video_decode_accelerator.h:273:
base::Callback<void(uint32, uint32, scoped_refptr<gfx::GLImage>)> bind_image_;
On 2015/01/24 00:58:54, Pawel Osciak wrote:
> Please document.

Done.

[DaleCurtis, 2015-01-28 23:38:41]

Media/ lgtm if posciak is happy

[Pawel Osciak, 2015-01-28 23:52:02]

I would kindly request *before* this goes in to:

- please add an about:flags switch to turn overlays on/off so that we have a
kill switch in case something goes wrong

- update video tests to work with this so that we don't lose test coverage

Also, how does this work with tab casting? Did you test casting a tab while
playing back a video with this?

Thank you.

https://codereview.chromium.org/858653002/diff/60001/content/common/gpu/media...
File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right):

https://codereview.chromium.org/858653002/diff/60001/content/common/gpu/media...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:536:
bind_image_.Run(buffers[i].internal_texture_id(),
On 2015/01/27 18:36:48, achaulk wrote:
> On 2015/01/24 00:58:54, Pawel Osciak wrote:
> > Sorry, I'm confused, could you please explain why is this better done here
> than
> > in VaapiDrmPicture?
> > Also, is there a way we could have a version of Bind for VaapiTfpPicture as
> > well?
> 
> It's roughly equivalent, but in the case where we want a non-drm version to do
> this as well this is easier. I'm not sure if that will ever be the case though

I think we can't create a GLImage from XPixmap?

https://codereview.chromium.org/858653002/diff/60001/content/common/gpu/media...
File content/common/gpu/media/vaapi_video_decode_accelerator.h (right):

https://codereview.chromium.org/858653002/diff/60001/content/common/gpu/media...
content/common/gpu/media/vaapi_video_decode_accelerator.h:273:
base::Callback<void(uint32, uint32, scoped_refptr<gfx::GLImage>)> bind_image_;
On 2015/01/27 18:36:48, achaulk wrote:
> On 2015/01/24 00:58:54, Pawel Osciak wrote:
> > Please document.
> 
> Done.

Sorry, where is that documentation?

https://codereview.chromium.org/858653002/diff/80001/content/common/gpu/media...
File content/common/gpu/media/vaapi_picture.h (right):

https://codereview.chromium.org/858653002/diff/80001/content/common/gpu/media...
content/common/gpu/media/vaapi_picture.h:40: virtual bool AllowOverlay() const;
Please document.

https://codereview.chromium.org/858653002/diff/80001/content/common/gpu/media...
File content/common/gpu/media/vaapi_video_decode_accelerator.h (right):

https://codereview.chromium.org/858653002/diff/80001/content/common/gpu/media...
content/common/gpu/media/vaapi_video_decode_accelerator.h:52: const
base::Callback<void(uint32, uint32, scoped_refptr<gfx::GLImage>)>&
Please document.

[achaulk, 2015-01-28 23:54:14]

On 2015/01/28 23:52:02, Pawel Osciak wrote:
> I would kindly request *before* this goes in to:
> 
> - please add an about:flags switch to turn overlays on/off so that we have a
> kill switch in case something goes wrong
> 
> - update video tests to work with this so that we don't lose test coverage
> 
> Also, how does this work with tab casting? Did you test casting a tab while
> playing back a video with this?
> 
> Thank you.
> 
>
https://codereview.chromium.org/858653002/diff/60001/content/common/gpu/media...
> File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right):
> 
>
https://codereview.chromium.org/858653002/diff/60001/content/common/gpu/media...
> content/common/gpu/media/vaapi_video_decode_accelerator.cc:536:
> bind_image_.Run(buffers[i].internal_texture_id(),
> On 2015/01/27 18:36:48, achaulk wrote:
> > On 2015/01/24 00:58:54, Pawel Osciak wrote:
> > > Sorry, I'm confused, could you please explain why is this better done here
> > than
> > > in VaapiDrmPicture?
> > > Also, is there a way we could have a version of Bind for VaapiTfpPicture
as
> > > well?
> > 
> > It's roughly equivalent, but in the case where we want a non-drm version to
do
> > this as well this is easier. I'm not sure if that will ever be the case
though
> 
> I think we can't create a GLImage from XPixmap?
> 
>
https://codereview.chromium.org/858653002/diff/60001/content/common/gpu/media...
> File content/common/gpu/media/vaapi_video_decode_accelerator.h (right):
> 
>
https://codereview.chromium.org/858653002/diff/60001/content/common/gpu/media...
> content/common/gpu/media/vaapi_video_decode_accelerator.h:273:
> base::Callback<void(uint32, uint32, scoped_refptr<gfx::GLImage>)> bind_image_;
> On 2015/01/27 18:36:48, achaulk wrote:
> > On 2015/01/24 00:58:54, Pawel Osciak wrote:
> > > Please document.
> > 
> > Done.
> 
> Sorry, where is that documentation?
> 
>
https://codereview.chromium.org/858653002/diff/80001/content/common/gpu/media...
> File content/common/gpu/media/vaapi_picture.h (right):
> 
>
https://codereview.chromium.org/858653002/diff/80001/content/common/gpu/media...
> content/common/gpu/media/vaapi_picture.h:40: virtual bool AllowOverlay()
const;
> Please document.
> 
>
https://codereview.chromium.org/858653002/diff/80001/content/common/gpu/media...
> File content/common/gpu/media/vaapi_video_decode_accelerator.h (right):
> 
>
https://codereview.chromium.org/858653002/diff/80001/content/common/gpu/media...
> content/common/gpu/media/vaapi_video_decode_accelerator.h:52: const
> base::Callback<void(uint32, uint32, scoped_refptr<gfx::GLImage>)>&
> Please document.

There are already switches for cc and ozone to control overall overlay
behaviour. The overlay flag is only a hint in any case, if it's used on a path
that doesn't support overlaying nothing different happens

[Pawel Osciak, 2015-01-28 23:55:00]

On 2015/01/28 23:54:14, achaulk wrote:
> On 2015/01/28 23:52:02, Pawel Osciak wrote:
> > I would kindly request *before* this goes in to:
> > 
> > - please add an about:flags switch to turn overlays on/off so that we have a
> > kill switch in case something goes wrong
> > 
> > - update video tests to work with this so that we don't lose test coverage
> > 
> > Also, how does this work with tab casting? Did you test casting a tab while
> > playing back a video with this?
> > 
> > Thank you.
> > 
> >
>
https://codereview.chromium.org/858653002/diff/60001/content/common/gpu/media...
> > File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right):
> > 
> >
>
https://codereview.chromium.org/858653002/diff/60001/content/common/gpu/media...
> > content/common/gpu/media/vaapi_video_decode_accelerator.cc:536:
> > bind_image_.Run(buffers[i].internal_texture_id(),
> > On 2015/01/27 18:36:48, achaulk wrote:
> > > On 2015/01/24 00:58:54, Pawel Osciak wrote:
> > > > Sorry, I'm confused, could you please explain why is this better done
here
> > > than
> > > > in VaapiDrmPicture?
> > > > Also, is there a way we could have a version of Bind for VaapiTfpPicture
> as
> > > > well?
> > > 
> > > It's roughly equivalent, but in the case where we want a non-drm version
to
> do
> > > this as well this is easier. I'm not sure if that will ever be the case
> though
> > 
> > I think we can't create a GLImage from XPixmap?
> > 
> >
>
https://codereview.chromium.org/858653002/diff/60001/content/common/gpu/media...
> > File content/common/gpu/media/vaapi_video_decode_accelerator.h (right):
> > 
> >
>
https://codereview.chromium.org/858653002/diff/60001/content/common/gpu/media...
> > content/common/gpu/media/vaapi_video_decode_accelerator.h:273:
> > base::Callback<void(uint32, uint32, scoped_refptr<gfx::GLImage>)>
bind_image_;
> > On 2015/01/27 18:36:48, achaulk wrote:
> > > On 2015/01/24 00:58:54, Pawel Osciak wrote:
> > > > Please document.
> > > 
> > > Done.
> > 
> > Sorry, where is that documentation?
> > 
> >
>
https://codereview.chromium.org/858653002/diff/80001/content/common/gpu/media...
> > File content/common/gpu/media/vaapi_picture.h (right):
> > 
> >
>
https://codereview.chromium.org/858653002/diff/80001/content/common/gpu/media...
> > content/common/gpu/media/vaapi_picture.h:40: virtual bool AllowOverlay()
> const;
> > Please document.
> > 
> >
>
https://codereview.chromium.org/858653002/diff/80001/content/common/gpu/media...
> > File content/common/gpu/media/vaapi_video_decode_accelerator.h (right):
> > 
> >
>
https://codereview.chromium.org/858653002/diff/80001/content/common/gpu/media...
> > content/common/gpu/media/vaapi_video_decode_accelerator.h:52: const
> > base::Callback<void(uint32, uint32, scoped_refptr<gfx::GLImage>)>&
> > Please document.
> 
> There are already switches for cc and ozone to control overall overlay
> behaviour. The overlay flag is only a hint in any case, if it's used on a path
> that doesn't support overlaying nothing different happens

Even if overlay is supported we need a way to disable it. Could you point me to
the correct flag name? Thanks.

[achaulk, 2015-01-28 23:57:35]

--enable-hardware-overlays is the flag for now to turn it on, it's defaulted to
off

[Pawel Osciak, 2015-01-28 23:58:30]

On 2015/01/28 23:57:35, achaulk wrote:
> --enable-hardware-overlays is the flag for now to turn it on, it's defaulted
to
> off

Perfect, thank you.

[Pawel Osciak, 2015-01-29 00:13:35]

Per offline discussion this is going in disabled. The next step before enabling
is to add support for overlays to the test and we will make sure that the test
gets passed an --enable-hardware-overlays on platforms that support it. We also
need to make sure we handle tab casting before this is enabled.

With the above and previous nits lgtm.
Thank you!

[piman, 2015-01-29 00:44:37]

On Wed, Jan 28, 2015 at 4:13 PM, <posciak@chromium.org> wrote:

> Per offline discussion this is going in disabled. The next step before
> enabling
> is to add support for overlays to the test and we will make sure that the
> test
> gets passed an --enable-hardware-overlays on platforms that support it. We
> also
> need to make sure we handle tab casting before this is enabled.
>

It should be verified of course, but there's no fundamental reason why tab
casting shouldn't be supported. Overlay textures need to have the property
that they can be used as GL textures for compositing fallback. That ensures
that we can do the partial composite with GL for the tab capture as normal.

A potential optimization is to avoid that partial composite if the only
thing to capture is the single video overlay (e.g. full screen), and feed
that texture back to the encoder. But that's out-of-scope at this point.


> With the above and previous nits lgtm.
> Thank you!
>
> https://codereview.chromium.org/858653002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[achaulk, 2015-02-03 18:53:37]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2015-02-03 18:54:12]

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

[commit-bot: I haz the power, 2015-02-03 18:58:41]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-02-03 18:59:25]

Patchset 9 (id:??) landed as
https://crrev.com/79812c1a67c5cbd7b60ecc117c3d1d6492c8db38
Cr-Commit-Position: refs/heads/master@{#314376}