Add GLStreamTextureImage

Added GLStreamTextureImage : GLImage, which exposes
GetTextureMatrix.  This provides a texture matrix to
convert from hardware-specific texture coordinates.  Made
AVDACodecImage provide the current matrix from the SurfaceTexture.

Updates GLRenderer::DrawStreamVideoQuad to use the current texture
matrix from the GLImage if available, else fall back to the one
recorded quad.  Once stream_texture_android.cc returns the most
recent texture matrix, it may be possible to remove the entire
DidUpdateMatrix path in VideoFrameProvider.

Does not address CopyTextureCHROMIUM.

BUG=530681, 226218
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/df64f2cb11bb32d8522fa0ebeeed8653be158533
Cr-Commit-Position: refs/heads/master@{#377150}

[liberato (no reviews please), 2016-01-05 16:58:03]

Description was changed from

==========
Replace tex matrix uniform hack with glimage support.

BUG=
==========

to

==========
Replace tex matrix uniform hack with glimage support.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[liberato (no reviews please), 2016-01-13 01:58:09]

Description was changed from

==========
Replace tex matrix uniform hack with glimage support.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Replace tex matrix uniform hack with glimage support.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[liberato (no reviews please), 2016-01-13 01:58:09]

liberato@chromium.org changed reviewers:
+ piman@chromium.org, reveman@chromium.org, sievers@chromium.org

[liberato (no reviews please), 2016-01-13 02:09:58]

hi folks

this is a WIP.  I wanted to see if any of you have any strong opinions on the
general approach.  it's certainly not done yet.

the goal is to get android SurfaceTexture's texture matrix working properly when
copying (e.g. for posters or webgl video frames) and drawing stream texture
video quads.

this CL makes the texture matrix part of the GLImage, and teaches the
appropriate parts of the gles2 cmd decoder to use it.  the stream texture draw
quad is also modified to set the shader's texture matrix uniform properly.

the rationale is that the GLImage can provide a texture matrix to convert to
"default" texture coordinates.

as a follow-up to this, i'd like to get rid of the VideoFrameProvider's
DidUpdateMatrix that's used by stream_texture_android by using the same
mechanism there.  that would remove quite a bit of plumbing between the renderer
and gpu.

thanks
-fl

[reveman, 2016-01-13 14:21:27]

I'm not a fan of making the same mistake as SurfaceTexture and adding a matrix
to GLImage. Can we instead handle this as this patch does:
https://codereview.chromium.org/1536713002

[liberato (no reviews please), 2016-01-13 16:23:35]

On 2016/01/13 14:21:27, reveman wrote:
> I'm not a fan of making the same mistake as SurfaceTexture and adding a matrix
> to GLImage. Can we instead handle this as this patch does:
> https://codereview.chromium.org/1536713002

fair enough.

the copy texture changes are somewhat incidental; my main goal was to handle the
case where stream texture draw quad has to rely on getting the matrix from the
renderer.  it doesn't do a copy, but instead samples directly and has to know
the appropriate transform.  right now, in the shipping android video path,
there's an ipc from gpu to renderer that lets the renderer know the current
matrix, which is eventually sent to the browser compositor with the stream video
quad data.  the catch is that the renderer doesn't find out about it until a few
frames have gone by.  i didn't want a similar problem in the new Spitzer code.

one could work around this by doing the very first UpdateTexImage in the AVDA,
and sending it to the renderer before the first frame, then hoping that it stays
the same for subsequent frames.  that's not super elegant either, though.

as an aside, can one use CopyTextureCHROMIUMResourceManager directly in
CopyTexImage?

as a second aside, re "same mistake as SurfaceTexture", how would you structure
SurfaceTexture if you could start from scratch?

thanks
-fl

[reveman, 2016-01-13 19:06:44]

On 2016/01/13 at 16:23:35, liberato wrote:
> On 2016/01/13 14:21:27, reveman wrote:
> > I'm not a fan of making the same mistake as SurfaceTexture and adding a
matrix
> > to GLImage. Can we instead handle this as this patch does:
> > https://codereview.chromium.org/1536713002
> 
> fair enough.
> 
> the copy texture changes are somewhat incidental; my main goal was to handle
the case where stream texture draw quad has to rely on getting the matrix from
the renderer.  it doesn't do a copy, but instead samples directly and has to
know the appropriate transform.  right now, in the shipping android video path,
there's an ipc from gpu to renderer that lets the renderer know the current
matrix, which is eventually sent to the browser compositor with the stream video
quad data.  the catch is that the renderer doesn't find out about it until a few
frames have gone by.  i didn't want a similar problem in the new Spitzer code.
> 
> one could work around this by doing the very first UpdateTexImage in the AVDA,
and sending it to the renderer before the first frame, then hoping that it stays
the same for subsequent frames.  that's not super elegant either, though.
> 
> as an aside, can one use CopyTextureCHROMIUMResourceManager directly in
CopyTexImage?

That's typically a layering violation but it of course depends on where your
GLImage code lives. Ideally the GLImage code can live in ui/gl and not have any
dependencies that are not allowed there.

> 
> as a second aside, re "same mistake as SurfaceTexture", how would you
structure SurfaceTexture if you could start from scratch?

I'd keep the transform separate from the ST so it can be known without having to
call UpdateTexImage.

[liberato (no reviews please), 2016-01-19 19:49:36]

On 2016/01/13 19:06:44, reveman wrote:
> On 2016/01/13 at 16:23:35, liberato wrote:
> > On 2016/01/13 14:21:27, reveman wrote:
> > > I'm not a fan of making the same mistake as SurfaceTexture and adding a
> matrix
> > > to GLImage. Can we instead handle this as this patch does:
> > > https://codereview.chromium.org/1536713002
> > 
> > fair enough.
> > 
> > the copy texture changes are somewhat incidental; my main goal was to handle
> the case where stream texture draw quad has to rely on getting the matrix from
> the renderer.  it doesn't do a copy, but instead samples directly and has to
> know the appropriate transform.  right now, in the shipping android video
path,
> there's an ipc from gpu to renderer that lets the renderer know the current
> matrix, which is eventually sent to the browser compositor with the stream
video
> quad data.  the catch is that the renderer doesn't find out about it until a
few
> frames have gone by.  i didn't want a similar problem in the new Spitzer code.
> > 
> > one could work around this by doing the very first UpdateTexImage in the
AVDA,
> and sending it to the renderer before the first frame, then hoping that it
stays
> the same for subsequent frames.  that's not super elegant either, though.
> > 
> > as an aside, can one use CopyTextureCHROMIUMResourceManager directly in
> CopyTexImage?
> 
> That's typically a layering violation but it of course depends on where your
> GLImage code lives. Ideally the GLImage code can live in ui/gl and not have
any
> dependencies that are not allowed there.
> 
> > 
> > as a second aside, re "same mistake as SurfaceTexture", how would you
> structure SurfaceTexture if you could start from scratch?
> 
> I'd keep the transform separate from the ST so it can be known without having
to
> call UpdateTexImage.

thanks for the feedback.

i gave this a try this morning.  it was quite simple (thanks, tobiasjs@!) and i
was really excited until i ran it and nothing happened :)  the problem is that
not all paths to make a copy go through GLImage::Copy*Image.  For example, if a
webgl application configures gl to flip_y, then the copy is forced through
CopyTextureCHROMIUMResourceManager::DoCopyTextureWithTransform in the decoder.

other than GLImage::GetTransformMatrix, the only other way i can think of to
handle this is have the client send the transform matrix down with the copy. 
however, that's got some serious disadvantages also.  in particular, it sends
the matrix between renderer and gpu a lot.  it's also harder to keep it up to
date on the first few frames.

are you entirely against the GLImage::GetTransformMatrix idea?  if not, i think
it will contain the damage a bit more than exposing it to the renderer.  beyond
the ipc and frame-accurate stuff, hiding it from the renderer isn't all bad.

[reveman, 2016-01-20 04:15:14]

On 2016/01/19 at 19:49:36, liberato wrote:
> On 2016/01/13 19:06:44, reveman wrote:
> > On 2016/01/13 at 16:23:35, liberato wrote:
> > > On 2016/01/13 14:21:27, reveman wrote:
> > > > I'm not a fan of making the same mistake as SurfaceTexture and adding a
> > matrix
> > > > to GLImage. Can we instead handle this as this patch does:
> > > > https://codereview.chromium.org/1536713002
> > > 
> > > fair enough.
> > > 
> > > the copy texture changes are somewhat incidental; my main goal was to
handle
> > the case where stream texture draw quad has to rely on getting the matrix
from
> > the renderer.  it doesn't do a copy, but instead samples directly and has to
> > know the appropriate transform.  right now, in the shipping android video
path,
> > there's an ipc from gpu to renderer that lets the renderer know the current
> > matrix, which is eventually sent to the browser compositor with the stream
video
> > quad data.  the catch is that the renderer doesn't find out about it until a
few
> > frames have gone by.  i didn't want a similar problem in the new Spitzer
code.
> > > 
> > > one could work around this by doing the very first UpdateTexImage in the
AVDA,
> > and sending it to the renderer before the first frame, then hoping that it
stays
> > the same for subsequent frames.  that's not super elegant either, though.
> > > 
> > > as an aside, can one use CopyTextureCHROMIUMResourceManager directly in
> > CopyTexImage?
> > 
> > That's typically a layering violation but it of course depends on where your
> > GLImage code lives. Ideally the GLImage code can live in ui/gl and not have
any
> > dependencies that are not allowed there.
> > 
> > > 
> > > as a second aside, re "same mistake as SurfaceTexture", how would you
> > structure SurfaceTexture if you could start from scratch?
> > 
> > I'd keep the transform separate from the ST so it can be known without
having to
> > call UpdateTexImage.
> 
> thanks for the feedback.
> 
> i gave this a try this morning.  it was quite simple (thanks, tobiasjs@!) and
i was really excited until i ran it and nothing happened :)  the problem is that
not all paths to make a copy go through GLImage::Copy*Image.  For example, if a
webgl application configures gl to flip_y, then the copy is forced through
CopyTextureCHROMIUMResourceManager::DoCopyTextureWithTransform in the decoder.

Is that the only use of DoCopyTextureWithTransform? Maybe we should extend
GLImage::CopyTexImage with flip_y support and remove the *WithTransform version
in CopyTextureCHROMIUMResourceManager?

> 
> other than GLImage::GetTransformMatrix, the only other way i can think of to
handle this is have the client send the transform matrix down with the copy. 
however, that's got some serious disadvantages also.  in particular, it sends
the matrix between renderer and gpu a lot.  it's also harder to keep it up to
date on the first few frames.
> 
> are you entirely against the GLImage::GetTransformMatrix idea?  if not, i
think it will contain the damage a bit more than exposing it to the renderer. 
beyond the ipc and frame-accurate stuff, hiding it from the renderer isn't all
bad.

[liberato (no reviews please), 2016-02-16 18:37:23]

hi folks

after some offline chat, here's the latest attempt for getting the texture
matrix right.  i'm still doing testing on it, but feedback on the overall form
of the solution would be most welcome (and anything else!).  it does seem to
work fine so far.

@reveman: i think that i followed the spirit of our discussion in the
implementation, but had to improvise a bit.  i wasn't able to expose this on
Texture without also exposing it on GLImage.  to make it work, i don't see any
way except to make a call into a GLImage function at the time we need the
matrix, so that it can update the SurfaceTexture.  I apologize if i veered too
far towards the original idea.

- idea is to add the notion of setting uniforms to server-provided values,
specified by an enum.  in this case, it's just the texture matrix.

- added UniformMatrix4fvWithCustomMatrixCHROMIUM.  should have an enum like
'GL_STREAM_TEXTURE_MATRIX' but used '12345' until i figure out where the right
place to define the enum is.  it no longer takes a texture client id; for that
particular enum, the service gets it from the currently bound external texture
on the active texturing unit.  other enums might be handled differently.

- didn't move the copier into CopyTexImage, since it isn't always called.  the
alternative is the CopyOptions approach, which is more unwieldy.

- as an implementation detail, started checking the share group more often.  do
i need to do this?

- if we go this way, we can remove the entire
VideoFrameProvider::Client::DidUpdateMatrix and associated IPC once
StreamTexture starts exposing the texture matrix via GetCustomMatrix.  in the
interim, the new Uniform...CHROMIUM command takes a default matrix so that we
can support both cases.  the default matrix parameter can be removed too once
StreamTexture is updated, and the default can be 'identity'.

as always, comments are appreciated.

thanks
-fl

[piman, 2016-02-16 23:56:33]

https://codereview.chromium.org/1559203003/diff/80001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1559203003/diff/80001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:2210: 12345 /* TODO(liberato): enum */, false,
gl_matrix);
The enum will go to gpu/GLES2/gl2extchromium.h
See GL_SUBSCRIBED_VALUES_BUFFER_CHROMIUM and e.g. glBindValuebufferCHROMIUM as
an example to specify validation

https://codereview.chromium.org/1559203003/diff/80001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1559203003/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7461: // to work with the PUTn
autogen.
Use PUT instead of PUTn, and a static count in the py file - see e.g.
VertexAttrib4fv

https://codereview.chromium.org/1559203003/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7468: if (custom_matrix_id ==
12345) {  // TODO(liberato): enum
When adding the enum, make sure it is added to the validation, and replace this
by a DCHECK.

https://codereview.chromium.org/1559203003/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7472: if (gl::GLImage* image =
texture_ref->texture()->GetLevelImage(
I suggest raising errors (GL_INVALID_OPERATION) or provide a default value
(identity?) if there's no bound texture or no image.

[liberato (no reviews please), 2016-02-17 17:50:32]

thanks for the feedback.

-fl

https://codereview.chromium.org/1559203003/diff/80001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1559203003/diff/80001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:2210: 12345 /* TODO(liberato): enum */, false,
gl_matrix);
On 2016/02/16 23:56:33, piman wrote:
> The enum will go to gpu/GLES2/gl2extchromium.h
> See GL_SUBSCRIBED_VALUES_BUFFER_CHROMIUM and e.g. glBindValuebufferCHROMIUM as
> an example to specify validation

Done.

https://codereview.chromium.org/1559203003/diff/80001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1559203003/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7461: // to work with the PUTn
autogen.
On 2016/02/16 23:56:33, piman wrote:
> Use PUT instead of PUTn, and a static count in the py file - see e.g.
> VertexAttrib4fv

Done.

https://codereview.chromium.org/1559203003/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7468: if (custom_matrix_id ==
12345) {  // TODO(liberato): enum
On 2016/02/16 23:56:33, piman wrote:
> When adding the enum, make sure it is added to the validation, and replace
this
> by a DCHECK.

done, and checked that the validation is working.

https://codereview.chromium.org/1559203003/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7472: if (gl::GLImage* image =
texture_ref->texture()->GetLevelImage(
On 2016/02/16 23:56:33, piman wrote:
> I suggest raising errors (GL_INVALID_OPERATION) or provide a default value
> (identity?) if there's no bound texture or no image.

i was planning on making it identity once StreamTexture starts providing the
matrix also.  until then, this is to allow the gl_renderer to send in the matrix
that it got with the quad via the existing gpu => renderer IPC, so that it works
in that case too.

i plan on updating streamtexture for m51.

[piman, 2016-02-17 21:57:29]

Syntactically, the path taken looks reasonable to me. I'll let reveman/sievers
comment on whether they like the direction.

https://codereview.chromium.org/1559203003/diff/80001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1559203003/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7472: if (gl::GLImage* image =
texture_ref->texture()->GetLevelImage(
On 2016/02/17 17:50:32, liberato wrote:
> On 2016/02/16 23:56:33, piman wrote:
> > I suggest raising errors (GL_INVALID_OPERATION) or provide a default value
> > (identity?) if there's no bound texture or no image.
> 
> i was planning on making it identity once StreamTexture starts providing the
> matrix also.  until then, this is to allow the gl_renderer to send in the
matrix
> that it got with the quad via the existing gpu => renderer IPC, so that it
works
> in that case too.
> 
> i plan on updating streamtexture for m51.

Right, but what I meant is, this is always called (from the client side) with a
texture bound. It doesn't really make sense for this call to try to fallback if
a texture isn't bound (and in the end state, there will be no fallback). So
instead, raise an error.
Same with no image bound.
The only case where you want to fallback today is if GetCustomMatrix returns
false. The other cases are just invalid state from the client side.

[reveman, 2016-02-18 17:16:50]

https://codereview.chromium.org/1559203003/diff/100001/content/common/gpu/med...
File content/common/gpu/media/android_deferred_rendering_backing_strategy.cc
(right):

https://codereview.chromium.org/1559203003/diff/100001/content/common/gpu/med...
content/common/gpu/media/android_deferred_rendering_backing_strategy.cc:51:
shared_state_->set_share_group(gfx::GLContext::GetCurrent()->share_group());
is this related to this patch?

https://codereview.chromium.org/1559203003/diff/100001/gpu/GLES2/gl2extchromi...
File gpu/GLES2/gl2extchromium.h (right):

https://codereview.chromium.org/1559203003/diff/100001/gpu/GLES2/gl2extchromi...
gpu/GLES2/gl2extchromium.h:728: /* GL_CHROMIUM_custom_matrix */
Should we keep this more specific until we have a second use case?
CHROMIUM_stream_texture_matrix? That way we don't even need the enum value.

Also, can you add an extension spec file to gpu/GLES2/extensions/CHROMIUM for
this that describes what it does and how it should be used.

https://codereview.chromium.org/1559203003/diff/100001/gpu/command_buffer/cmd...
File gpu/command_buffer/cmd_buffer_functions.txt (right):

https://codereview.chromium.org/1559203003/diff/100001/gpu/command_buffer/cmd...
gpu/command_buffer/cmd_buffer_functions.txt:210: GL_APICALL void        
GL_APIENTRY glUniformMatrix4fvWithCustomMatrixCHROMIUM (GLintUniformLocation
location, GLenumCustomMatrixParameter custom_matrix, GLbooleanFalseOnly
transpose, const GLfloat* default_value);
do we need a default value?

https://codereview.chromium.org/1559203003/diff/100001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1559203003/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7462:
texture_ref->texture()->GetLevelImage(GL_TEXTURE_EXTERNAL_OES, 0)) {
What if the image here is not a stream texture image? We should fail or use the
default value in that case I think. Do we need a more explicit mechanism to keep
track of stream texture images?

Maybe add GetLevelStreamTextureImage that returns a pointer to a
GLImageStreamTexture instance. Where the GLImageStreamTexture class is derived
from GLImage but with the addition of a GetMatrix() function. That way we keep
all this matrix related code outside the general GLImage interface and specific
to stream textures. Wdyt?

https://codereview.chromium.org/1559203003/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13414: transform_matrix);
I don't think we should deal with the matrix here. Can we do CopyTexture support
as a separate patch?

[liberato (no reviews please), 2016-02-18 19:34:41]

thanks for the feedback.

i still need to add a test to TextureManager for the new setters, assuming that
we keep it this way.

thanks
-fl

https://codereview.chromium.org/1559203003/diff/100001/content/common/gpu/med...
File content/common/gpu/media/android_deferred_rendering_backing_strategy.cc
(right):

https://codereview.chromium.org/1559203003/diff/100001/content/common/gpu/med...
content/common/gpu/media/android_deferred_rendering_backing_strategy.cc:51:
shared_state_->set_share_group(gfx::GLContext::GetCurrent()->share_group());
On 2016/02/18 17:16:49, reveman wrote:
> is this related to this patch?

i'm not sure it's needed at all, to be honest.  if so, then i suppose it could
be separated out into a different CL.

it's here because the changes in avda_codec_image to support GetCustomMatrix
made it an obvious question to ask.

https://codereview.chromium.org/1559203003/diff/100001/gpu/GLES2/gl2extchromi...
File gpu/GLES2/gl2extchromium.h (right):

https://codereview.chromium.org/1559203003/diff/100001/gpu/GLES2/gl2extchromi...
gpu/GLES2/gl2extchromium.h:728: /* GL_CHROMIUM_custom_matrix */
On 2016/02/18 17:16:49, reveman wrote:
> Should we keep this more specific until we have a second use case?
> CHROMIUM_stream_texture_matrix? That way we don't even need the enum value.
> 
> Also, can you add an extension spec file to gpu/GLES2/extensions/CHROMIUM for
> this that describes what it does and how it should be used.

specific: sure, that's exactly how it was in PS4.  it thought you wanted the
enum + more general mechanism.  gladly removed.

extension spec: done.

https://codereview.chromium.org/1559203003/diff/100001/gpu/command_buffer/cmd...
File gpu/command_buffer/cmd_buffer_functions.txt (right):

https://codereview.chromium.org/1559203003/diff/100001/gpu/command_buffer/cmd...
gpu/command_buffer/cmd_buffer_functions.txt:210: GL_APICALL void        
GL_APIENTRY glUniformMatrix4fvWithCustomMatrixCHROMIUM (GLintUniformLocation
location, GLenumCustomMatrixParameter custom_matrix, GLbooleanFalseOnly
transpose, const GLfloat* default_value);
On 2016/02/18 17:16:49, reveman wrote:
> do we need a default value?

temporarily, yes.  that's what keeps stream_texture_android working until it's
updated to work similarly to AVDACodecImage.  i don't want to update it for m50.

after that, then the default matrix here + all IPC related to sending the matrix
via the renderer can be removed.

https://codereview.chromium.org/1559203003/diff/100001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1559203003/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7462:
texture_ref->texture()->GetLevelImage(GL_TEXTURE_EXTERNAL_OES, 0)) {
On 2016/02/18 17:16:50, reveman wrote:
> What if the image here is not a stream texture image? We should fail or use
the
> default value in that case I think. Do we need a more explicit mechanism to
keep
> track of stream texture images?
> 
> Maybe add GetLevelStreamTextureImage that returns a pointer to a
> GLImageStreamTexture instance. Where the GLImageStreamTexture class is derived
> from GLImage but with the addition of a GetMatrix() function. That way we keep
> all this matrix related code outside the general GLImage interface and
specific
> to stream textures. Wdyt?

sure, sounds great.  i'll add Texture::[GS]etLevelStreamTextureImage.

plus i don't need to update ~one million GLImage implementations to return
false.

https://codereview.chromium.org/1559203003/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13414: transform_matrix);
On 2016/02/18 17:16:49, reveman wrote:
> I don't think we should deal with the matrix here. Can we do CopyTexture
support
> as a separate patch?

'here' as in 'this CL' or 'here' as in 'DoCopyTextureCHROMIUM'?

i've left it as is for now.

[reveman, 2016-02-18 20:20:57]

https://codereview.chromium.org/1559203003/diff/100001/content/common/gpu/med...
File content/common/gpu/media/android_deferred_rendering_backing_strategy.cc
(right):

https://codereview.chromium.org/1559203003/diff/100001/content/common/gpu/med...
content/common/gpu/media/android_deferred_rendering_backing_strategy.cc:51:
shared_state_->set_share_group(gfx::GLContext::GetCurrent()->share_group());
On 2016/02/18 at 19:34:41, liberato wrote:
> On 2016/02/18 17:16:49, reveman wrote:
> > is this related to this patch?
> 
> i'm not sure it's needed at all, to be honest.  if so, then i suppose it could
be separated out into a different CL.
> 
> it's here because the changes in avda_codec_image to support GetCustomMatrix
made it an obvious question to ask.

Ok, can we do this in a separate patch?

https://codereview.chromium.org/1559203003/diff/100001/gpu/command_buffer/cmd...
File gpu/command_buffer/cmd_buffer_functions.txt (right):

https://codereview.chromium.org/1559203003/diff/100001/gpu/command_buffer/cmd...
gpu/command_buffer/cmd_buffer_functions.txt:210: GL_APICALL void        
GL_APIENTRY glUniformMatrix4fvWithCustomMatrixCHROMIUM (GLintUniformLocation
location, GLenumCustomMatrixParameter custom_matrix, GLbooleanFalseOnly
transpose, const GLfloat* default_value);
On 2016/02/18 at 19:34:41, liberato wrote:
> On 2016/02/18 17:16:49, reveman wrote:
> > do we need a default value?
> 
> temporarily, yes.  that's what keeps stream_texture_android working until it's
updated to work similarly to AVDACodecImage.  i don't want to update it for m50.
> 
> after that, then the default matrix here + all IPC related to sending the
matrix via the renderer can be removed.

Got it.

https://codereview.chromium.org/1559203003/diff/100001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1559203003/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13414: transform_matrix);
On 2016/02/18 at 19:34:41, liberato wrote:
> On 2016/02/18 17:16:49, reveman wrote:
> > I don't think we should deal with the matrix here. Can we do CopyTexture
support
> > as a separate patch?
> 
> 'here' as in 'this CL' or 'here' as in 'DoCopyTextureCHROMIUM'?
> 
> i've left it as is for now.

'here' as in 'this CL' as I have some questions regarding this and was thinking
it'd be nice to land the other parts of this patch first.

Question; when drawing (BindTexImageCHROMIUM path), the client is responsible
for having this matrix used and can make a decision. When CopyTextureCHROMIUM is
called then it's magically applied without the client being aware, that seems
inconsistent. Can we instead require the client to draw the stream texture to
another texture to apply this matrix instead of using CopyTexture?

https://codereview.chromium.org/1559203003/diff/120001/gpu/GLES2/extensions/C...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_stream_texture_matrix.txt (right):

https://codereview.chromium.org/1559203003/diff/120001/gpu/GLES2/extensions/C...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_stream_texture_matrix.txt:30: void
UniformMatrix4fvWithStreamTextureMatrixCHROMIUM(
Removing "With" would make the name a bit more GL-ish. Just FYI. No need to
change it unless if you prefer the current name.

https://codereview.chromium.org/1559203003/diff/120001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1559203003/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:1603: const GLfloat* value);
nit: s/value/default_value/

https://codereview.chromium.org/1559203003/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7453: bool copied_matrix =
false;
nit: move the memcpy here and remove this copied_matrix flag. I doubt copying 16
floats an extra time will be significantly slower than checking the
conditional..

https://codereview.chromium.org/1559203003/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7463: // TODO(liberato): verify
cl comments about !image.
not sure what this refers to

https://codereview.chromium.org/1559203003/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13410: bool got_matrix = false;
nit: same here. move memcpy here instead of this flag?

https://codereview.chromium.org/1559203003/diff/120001/gpu/command_buffer/ser...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/1559203003/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.cc:31: #include
"ui/gl/gl_stream_texture_image.h"
Did you forget to add this file? Btw, can this class live in gpu/command_buffer
instead of ui/gl? The stream texture concept is very command buffer specific and
not generic ui/gl code..

https://codereview.chromium.org/1559203003/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.cc:1391: return NULL;
nit: nullptr

https://codereview.chromium.org/1559203003/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.cc:1407: return NULL;
nit: nullptr

[liberato (no reviews please), 2016-02-19 18:35:55]

Description was changed from

==========
Replace tex matrix uniform hack with glimage support.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Added GLStreamTextureImage : GLImage, which exposes
GetTextureMatrix.  This provides a texture matrix to
convert from hardware-specific texture coordinates.  Made
AVDACodecImage provide the current matrix from the SurfaceTexture.

Updates GLRenderer::DrawStreamVideoQuad to use the current texture
matrix from the GLImage if available, else fall back to the one
recorded quad.  Once stream_texture_android.cc returns the most
recent texture matrix, it may be possible to remove the entire
DidUpdateMatrix path in VideoFrameProvider.

Does not address CopyTextureCHROMIUM.
    
BUG=530681,226218
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[liberato (no reviews please), 2016-02-19 18:39:56]

https://codereview.chromium.org/1559203003/diff/80001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1559203003/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7472: if (gl::GLImage* image =
texture_ref->texture()->GetLevelImage(
On 2016/02/17 21:57:29, piman wrote:
> On 2016/02/17 17:50:32, liberato wrote:
> > On 2016/02/16 23:56:33, piman wrote:
> > > I suggest raising errors (GL_INVALID_OPERATION) or provide a default value
> > > (identity?) if there's no bound texture or no image.
> > 
> > i was planning on making it identity once StreamTexture starts providing the
> > matrix also.  until then, this is to allow the gl_renderer to send in the
> matrix
> > that it got with the quad via the existing gpu => renderer IPC, so that it
> works
> > in that case too.
> > 
> > i plan on updating streamtexture for m51.
> 
> Right, but what I meant is, this is always called (from the client side) with
a
> texture bound. It doesn't really make sense for this call to try to fallback
if
> a texture isn't bound (and in the end state, there will be no fallback). So
> instead, raise an error.
> Same with no image bound.
> The only case where you want to fallback today is if GetCustomMatrix returns
> false. The other cases are just invalid state from the client side.

sorry, forgot to do this earlier.  done.

https://codereview.chromium.org/1559203003/diff/100001/content/common/gpu/med...
File content/common/gpu/media/android_deferred_rendering_backing_strategy.cc
(right):

https://codereview.chromium.org/1559203003/diff/100001/content/common/gpu/med...
content/common/gpu/media/android_deferred_rendering_backing_strategy.cc:51:
shared_state_->set_share_group(gfx::GLContext::GetCurrent()->share_group());
On 2016/02/18 20:20:56, reveman wrote:
> On 2016/02/18 at 19:34:41, liberato wrote:
> > On 2016/02/18 17:16:49, reveman wrote:
> > > is this related to this patch?
> > 
> > i'm not sure it's needed at all, to be honest.  if so, then i suppose it
could
> be separated out into a different CL.
> > 
> > it's here because the changes in avda_codec_image to support GetCustomMatrix
> made it an obvious question to ask.
> 
> Ok, can we do this in a separate patch?

Done.

https://codereview.chromium.org/1559203003/diff/100001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1559203003/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13414: transform_matrix);
On 2016/02/18 20:20:56, reveman wrote:
> On 2016/02/18 at 19:34:41, liberato wrote:
> > On 2016/02/18 17:16:49, reveman wrote:
> > > I don't think we should deal with the matrix here. Can we do CopyTexture
> support
> > > as a separate patch?
> > 
> > 'here' as in 'this CL' or 'here' as in 'DoCopyTextureCHROMIUM'?
> > 
> > i've left it as is for now.
> 
> 'here' as in 'this CL' as I have some questions regarding this and was
thinking
> it'd be nice to land the other parts of this patch first.
> 
> Question; when drawing (BindTexImageCHROMIUM path), the client is responsible
> for having this matrix used and can make a decision. When CopyTextureCHROMIUM
is
> called then it's magically applied without the client being aware, that seems
> inconsistent. Can we instead require the client to draw the stream texture to
> another texture to apply this matrix instead of using CopyTexture?

copy: sure, i'll split it.  my only concern is that it's the major reason of
doing any of this work, so they'd laugh at me at standup if i land this without
that part.  :)

inconsistent: i agree that it's inconsistent. the reason that i chose to do it
this way is that it prevents almost all consumers (e.g., webgl, canvas) from
needing to understand what a "stream texture".  it's a happy coincidence that
everybody except the gl_renderer just copies the thing.

it's somewhat safe, since the lifetime of video frames is tightly controlled.

there was a proposal to avoid the copy in webgl, among other advantages, six to
eight months ago.  i'm not sure where that ended up.  we would likely have to
copy the frame anyway in that case.

https://codereview.chromium.org/1559203003/diff/120001/gpu/GLES2/extensions/C...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_stream_texture_matrix.txt (right):

https://codereview.chromium.org/1559203003/diff/120001/gpu/GLES2/extensions/C...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_stream_texture_matrix.txt:30: void
UniformMatrix4fvWithStreamTextureMatrixCHROMIUM(
On 2016/02/18 20:20:56, reveman wrote:
> Removing "With" would make the name a bit more GL-ish. Just FYI. No need to
> change it unless if you prefer the current name.

Done.

https://codereview.chromium.org/1559203003/diff/120001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1559203003/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:1603: const GLfloat* value);
On 2016/02/18 20:20:56, reveman wrote:
> nit: s/value/default_value/

Done.

https://codereview.chromium.org/1559203003/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7453: bool copied_matrix =
false;
On 2016/02/18 20:20:56, reveman wrote:
> nit: move the memcpy here and remove this copied_matrix flag. I doubt copying
16
> floats an extra time will be significantly slower than checking the
> conditional..

Done.

https://codereview.chromium.org/1559203003/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7463: // TODO(liberato): verify
cl comments about !image.
On 2016/02/18 20:20:56, reveman wrote:
> not sure what this refers to

note to myself.  i forgot to remove it.

https://codereview.chromium.org/1559203003/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13410: bool got_matrix = false;
On 2016/02/18 20:20:56, reveman wrote:
> nit: same here. move memcpy here instead of this flag?

Done.

https://codereview.chromium.org/1559203003/diff/120001/gpu/command_buffer/ser...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/1559203003/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.cc:31: #include
"ui/gl/gl_stream_texture_image.h"
On 2016/02/18 20:20:57, reveman wrote:
> Did you forget to add this file? Btw, can this class live in
gpu/command_buffer
> instead of ui/gl? The stream texture concept is very command buffer specific
and
> not generic ui/gl code..

Done.

https://codereview.chromium.org/1559203003/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.cc:1391: return NULL;
On 2016/02/18 20:20:56, reveman wrote:
> nit: nullptr

Done.

https://codereview.chromium.org/1559203003/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.cc:1407: return NULL;
On 2016/02/18 20:20:57, reveman wrote:
> nit: nullptr

Done.

[reveman, 2016-02-19 19:15:40]

lgtm with nits

https://codereview.chromium.org/1559203003/diff/100001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1559203003/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13414: transform_matrix);
On 2016/02/19 at 18:39:55, liberato wrote:
> On 2016/02/18 20:20:56, reveman wrote:
> > On 2016/02/18 at 19:34:41, liberato wrote:
> > > On 2016/02/18 17:16:49, reveman wrote:
> > > > I don't think we should deal with the matrix here. Can we do CopyTexture
> > support
> > > > as a separate patch?
> > > 
> > > 'here' as in 'this CL' or 'here' as in 'DoCopyTextureCHROMIUM'?
> > > 
> > > i've left it as is for now.
> > 
> > 'here' as in 'this CL' as I have some questions regarding this and was
thinking
> > it'd be nice to land the other parts of this patch first.
> > 
> > Question; when drawing (BindTexImageCHROMIUM path), the client is
responsible
> > for having this matrix used and can make a decision. When
CopyTextureCHROMIUM is
> > called then it's magically applied without the client being aware, that
seems
> > inconsistent. Can we instead require the client to draw the stream texture
to
> > another texture to apply this matrix instead of using CopyTexture?
> 
> copy: sure, i'll split it.  my only concern is that it's the major reason of
doing any of this work, so they'd laugh at me at standup if i land this without
that part.  :)
> 
> inconsistent: i agree that it's inconsistent. the reason that i chose to do it
this way is that it prevents almost all consumers (e.g., webgl, canvas) from
needing to understand what a "stream texture".  it's a happy coincidence that
everybody except the gl_renderer just copies the thing.
> 
> it's somewhat safe, since the lifetime of video frames is tightly controlled.
> 
> there was a proposal to avoid the copy in webgl, among other advantages, six
to eight months ago.  i'm not sure where that ended up.  we would likely have to
copy the frame anyway in that case.

I'm OK landing something like this but prefer to review it as a separate patch
if possible. Long term it would be nice to move this copy logic to the client
instead of the GPU service as that means less code in the GPU process and more
code in the sandboxed renderer but I don't think that blocks us from making this
work properly using today's mechanism.

https://codereview.chromium.org/1559203003/diff/140001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1559203003/diff/140001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:2204: // start reporting the current matrix via
GLImage::GetCustomMatrix.  Until
nit: s/GLImage::GetCustomMatrix/GLStreamTextureImage::GetTextureMatrix/

https://codereview.chromium.org/1559203003/diff/140001/gpu/GLES2/extensions/C...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_stream_texture_matrix.txt (right):

https://codereview.chromium.org/1559203003/diff/140001/gpu/GLES2/extensions/C...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_stream_texture_matrix.txt:19: Allows
clients to set the value of a 4x4 unform to the current texture
nit: s/unform/uniform/

https://codereview.chromium.org/1559203003/diff/140001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gl_stream_texture_image.h (right):

https://codereview.chromium.org/1559203003/diff/140001/gpu/command_buffer/ser...
gpu/command_buffer/service/gl_stream_texture_image.h:1: // Copyright (c) 2016
The Chromium Authors. All rights reserved.
nit: Remove " (c)". Just "Copyright 2016" in new files.

https://codereview.chromium.org/1559203003/diff/140001/gpu/command_buffer/ser...
gpu/command_buffer/service/gl_stream_texture_image.h:17: GLStreamTextureImage()
{}
nit: don't think you need this if you make GetTextureMatrix pure virtual

https://codereview.chromium.org/1559203003/diff/140001/gpu/command_buffer/ser...
gpu/command_buffer/service/gl_stream_texture_image.h:19: // Get the matrix
nit: end with "." if this is the end of a comment or sentence. maybe the line
below is sufficient.

https://codereview.chromium.org/1559203003/diff/140001/gpu/command_buffer/ser...
gpu/command_buffer/service/gl_stream_texture_image.h:21: virtual void
GetTextureMatrix(float matrix[16]);
Pure virtual? ... = 0;

[liberato (no reviews please), 2016-02-19 19:46:17]

liberato@chromium.org changed reviewers:
+ yzshen@chromium.org

[liberato (no reviews please), 2016-02-19 19:48:11]

sievers, piman: PTAL.
reveman: thanks for the feedback.
yzshen: can you take a look at
mojo/public/c/gles2/gles2_call_visitor_chromium_extension_autogen.h

thanks
-fl

https://codereview.chromium.org/1559203003/diff/100001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1559203003/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13414: transform_matrix);
On 2016/02/19 19:15:39, reveman wrote:
> On 2016/02/19 at 18:39:55, liberato wrote:
> > On 2016/02/18 20:20:56, reveman wrote:
> > > On 2016/02/18 at 19:34:41, liberato wrote:
> > > > On 2016/02/18 17:16:49, reveman wrote:
> > > > > I don't think we should deal with the matrix here. Can we do
CopyTexture
> > > support
> > > > > as a separate patch?
> > > > 
> > > > 'here' as in 'this CL' or 'here' as in 'DoCopyTextureCHROMIUM'?
> > > > 
> > > > i've left it as is for now.
> > > 
> > > 'here' as in 'this CL' as I have some questions regarding this and was
> thinking
> > > it'd be nice to land the other parts of this patch first.
> > > 
> > > Question; when drawing (BindTexImageCHROMIUM path), the client is
> responsible
> > > for having this matrix used and can make a decision. When
> CopyTextureCHROMIUM is
> > > called then it's magically applied without the client being aware, that
> seems
> > > inconsistent. Can we instead require the client to draw the stream texture
> to
> > > another texture to apply this matrix instead of using CopyTexture?
> > 
> > copy: sure, i'll split it.  my only concern is that it's the major reason of
> doing any of this work, so they'd laugh at me at standup if i land this
without
> that part.  :)
> > 
> > inconsistent: i agree that it's inconsistent. the reason that i chose to do
it
> this way is that it prevents almost all consumers (e.g., webgl, canvas) from
> needing to understand what a "stream texture".  it's a happy coincidence that
> everybody except the gl_renderer just copies the thing.
> > 
> > it's somewhat safe, since the lifetime of video frames is tightly
controlled.
> > 
> > there was a proposal to avoid the copy in webgl, among other advantages, six
> to eight months ago.  i'm not sure where that ended up.  we would likely have
to
> copy the frame anyway in that case.
> 
> I'm OK landing something like this but prefer to review it as a separate patch
> if possible. Long term it would be nice to move this copy logic to the client
> instead of the GPU service as that means less code in the GPU process and more
> code in the sandboxed renderer but I don't think that blocks us from making
this
> work properly using today's mechanism.

Acknowledged.

https://codereview.chromium.org/1559203003/diff/140001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1559203003/diff/140001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:2204: // start reporting the current matrix via
GLImage::GetCustomMatrix.  Until
On 2016/02/19 19:15:39, reveman wrote:
> nit: s/GLImage::GetCustomMatrix/GLStreamTextureImage::GetTextureMatrix/

Done.

https://codereview.chromium.org/1559203003/diff/140001/gpu/GLES2/extensions/C...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_stream_texture_matrix.txt (right):

https://codereview.chromium.org/1559203003/diff/140001/gpu/GLES2/extensions/C...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_stream_texture_matrix.txt:19: Allows
clients to set the value of a 4x4 unform to the current texture
On 2016/02/19 19:15:39, reveman wrote:
> nit: s/unform/uniform/

Done.

https://codereview.chromium.org/1559203003/diff/140001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gl_stream_texture_image.h (right):

https://codereview.chromium.org/1559203003/diff/140001/gpu/command_buffer/ser...
gpu/command_buffer/service/gl_stream_texture_image.h:1: // Copyright (c) 2016
The Chromium Authors. All rights reserved.
On 2016/02/19 19:15:39, reveman wrote:
> nit: Remove " (c)". Just "Copyright 2016" in new files.

Done.

https://codereview.chromium.org/1559203003/diff/140001/gpu/command_buffer/ser...
gpu/command_buffer/service/gl_stream_texture_image.h:17: GLStreamTextureImage()
{}
On 2016/02/19 19:15:39, reveman wrote:
> nit: don't think you need this if you make GetTextureMatrix pure virtual

i thought so too, until it complained about lack of a default constructor in
AVDACodecImage.  not sure why, but i put it back anyway.

https://codereview.chromium.org/1559203003/diff/140001/gpu/command_buffer/ser...
gpu/command_buffer/service/gl_stream_texture_image.h:19: // Get the matrix
On 2016/02/19 19:15:39, reveman wrote:
> nit: end with "." if this is the end of a comment or sentence. maybe the line
> below is sufficient.

Done.

https://codereview.chromium.org/1559203003/diff/140001/gpu/command_buffer/ser...
gpu/command_buffer/service/gl_stream_texture_image.h:21: virtual void
GetTextureMatrix(float matrix[16]);
On 2016/02/19 19:15:39, reveman wrote:
> Pure virtual? ... = 0;

thanks, leftover from when i didn't want to do that on GLImage until the name
was finalized.

[no sievers, 2016-02-19 20:51:33]

Regarding CopyTextureCHROMIUM: Why can't we also just apply the correct matrix
there?

It's a bit inconsistent, but then again, those are very different APIs and it
doesn't feel all that terrible that 'CopyTexture' does the right thing
automatically.
The problem really comes from the SurfaceTexture API exposing this matrix. I'm
also fine if we let the client do the blit, but that still doesn't put us into a
great place, because someone might still add a new call site to CopyTexture in
the client somewhere without realizing that this doesn't apply the matrix (and
it might only be visible on some devices). Note that such a path was recently
added in VideoResourceProvider.

https://codereview.chromium.org/1559203003/diff/160001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1559203003/diff/160001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:2202: // TODO(liberato): stream_texture_android should
stop sending |gl_matrix| to
You can use crbug.com/371500.

I think you pretty much implemented what I was thinking back then :)

[reveman, 2016-02-19 21:26:47]

On 2016/02/19 at 20:51:33, sievers wrote:
> Regarding CopyTextureCHROMIUM: Why can't we also just apply the correct matrix
there?
> 
> It's a bit inconsistent, but then again, those are very different APIs and it
doesn't feel all that terrible that 'CopyTexture' does the right thing
automatically.
> The problem really comes from the SurfaceTexture API exposing this matrix. I'm
also fine if we let the client do the blit, but that still doesn't put us into a
great place, because someone might still add a new call site to CopyTexture in
the client somewhere without realizing that this doesn't apply the matrix (and
it might only be visible on some devices). Note that such a path was recently
added in VideoResourceProvider.

Like I mentioned in my previous comment. Applying the matrix in
CopyTextureCHROMIUM is fine with me but I think that's outside the scope of the
CHROMIUM_stream_texture_matrix extension being introduced by this patch.
CHROMIUM_stream_texture_matrix and this patch looks really solid and well
defined so I'd prefer to see that land on its own.

Making CopyTextureCHROMIUM automatically take the stream texture matrix into
account is a modification to the CHROMIUM_copy_texture extension and I'd prefer
if that was done separately.

In regards to having the client do the blit, I'd prefer that if possible as
special care needs to be taken by the client either way. To render a stream
texture correctly the client needs to either use CHROMIUM_stream_texture_matrix
or CHROMIUM_copy_texture, the first can be used to achieve the same result as
the second.

[yzshen1, 2016-02-22 17:16:16]

On 2016/02/19 19:48:11, liberato wrote:
> sievers, piman: PTAL.
> reveman: thanks for the feedback.
> yzshen: can you take a look at
> mojo/public/c/gles2/gles2_call_visitor_chromium_extension_autogen.h
> 
> thanks
> -fl
> 
>
https://codereview.chromium.org/1559203003/diff/100001/gpu/command_buffer/ser...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> 
>
https://codereview.chromium.org/1559203003/diff/100001/gpu/command_buffer/ser...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:13414: transform_matrix);
> On 2016/02/19 19:15:39, reveman wrote:
> > On 2016/02/19 at 18:39:55, liberato wrote:
> > > On 2016/02/18 20:20:56, reveman wrote:
> > > > On 2016/02/18 at 19:34:41, liberato wrote:
> > > > > On 2016/02/18 17:16:49, reveman wrote:
> > > > > > I don't think we should deal with the matrix here. Can we do
> CopyTexture
> > > > support
> > > > > > as a separate patch?
> > > > > 
> > > > > 'here' as in 'this CL' or 'here' as in 'DoCopyTextureCHROMIUM'?
> > > > > 
> > > > > i've left it as is for now.
> > > > 
> > > > 'here' as in 'this CL' as I have some questions regarding this and was
> > thinking
> > > > it'd be nice to land the other parts of this patch first.
> > > > 
> > > > Question; when drawing (BindTexImageCHROMIUM path), the client is
> > responsible
> > > > for having this matrix used and can make a decision. When
> > CopyTextureCHROMIUM is
> > > > called then it's magically applied without the client being aware, that
> > seems
> > > > inconsistent. Can we instead require the client to draw the stream
texture
> > to
> > > > another texture to apply this matrix instead of using CopyTexture?
> > > 
> > > copy: sure, i'll split it.  my only concern is that it's the major reason
of
> > doing any of this work, so they'd laugh at me at standup if i land this
> without
> > that part.  :)
> > > 
> > > inconsistent: i agree that it's inconsistent. the reason that i chose to
do
> it
> > this way is that it prevents almost all consumers (e.g., webgl, canvas) from
> > needing to understand what a "stream texture".  it's a happy coincidence
that
> > everybody except the gl_renderer just copies the thing.
> > > 
> > > it's somewhat safe, since the lifetime of video frames is tightly
> controlled.
> > > 
> > > there was a proposal to avoid the copy in webgl, among other advantages,
six
> > to eight months ago.  i'm not sure where that ended up.  we would likely
have
> to
> > copy the frame anyway in that case.
> > 
> > I'm OK landing something like this but prefer to review it as a separate
patch
> > if possible. Long term it would be nice to move this copy logic to the
client
> > instead of the GPU service as that means less code in the GPU process and
more
> > code in the sandboxed renderer but I don't think that blocks us from making
> this
> > work properly using today's mechanism.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/1559203003/diff/140001/cc/output/gl_renderer.cc
> File cc/output/gl_renderer.cc (right):
> 
>
https://codereview.chromium.org/1559203003/diff/140001/cc/output/gl_renderer....
> cc/output/gl_renderer.cc:2204: // start reporting the current matrix via
> GLImage::GetCustomMatrix.  Until
> On 2016/02/19 19:15:39, reveman wrote:
> > nit: s/GLImage::GetCustomMatrix/GLStreamTextureImage::GetTextureMatrix/
> 
> Done.
> 
>
https://codereview.chromium.org/1559203003/diff/140001/gpu/GLES2/extensions/C...
> File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_stream_texture_matrix.txt (right):
> 
>
https://codereview.chromium.org/1559203003/diff/140001/gpu/GLES2/extensions/C...
> gpu/GLES2/extensions/CHROMIUM/CHROMIUM_stream_texture_matrix.txt:19: Allows
> clients to set the value of a 4x4 unform to the current texture
> On 2016/02/19 19:15:39, reveman wrote:
> > nit: s/unform/uniform/
> 
> Done.
> 
>
https://codereview.chromium.org/1559203003/diff/140001/gpu/command_buffer/ser...
> File gpu/command_buffer/service/gl_stream_texture_image.h (right):
> 
>
https://codereview.chromium.org/1559203003/diff/140001/gpu/command_buffer/ser...
> gpu/command_buffer/service/gl_stream_texture_image.h:1: // Copyright (c) 2016
> The Chromium Authors. All rights reserved.
> On 2016/02/19 19:15:39, reveman wrote:
> > nit: Remove " (c)". Just "Copyright 2016" in new files.
> 
> Done.
> 
>
https://codereview.chromium.org/1559203003/diff/140001/gpu/command_buffer/ser...
> gpu/command_buffer/service/gl_stream_texture_image.h:17:
GLStreamTextureImage()
> {}
> On 2016/02/19 19:15:39, reveman wrote:
> > nit: don't think you need this if you make GetTextureMatrix pure virtual
> 
> i thought so too, until it complained about lack of a default constructor in
> AVDACodecImage.  not sure why, but i put it back anyway.
> 
>
https://codereview.chromium.org/1559203003/diff/140001/gpu/command_buffer/ser...
> gpu/command_buffer/service/gl_stream_texture_image.h:19: // Get the matrix
> On 2016/02/19 19:15:39, reveman wrote:
> > nit: end with "." if this is the end of a comment or sentence. maybe the
line
> > below is sufficient.
> 
> Done.
> 
>
https://codereview.chromium.org/1559203003/diff/140001/gpu/command_buffer/ser...
> gpu/command_buffer/service/gl_stream_texture_image.h:21: virtual void
> GetTextureMatrix(float matrix[16]);
> On 2016/02/19 19:15:39, reveman wrote:
> > Pure virtual? ... = 0;
> 
> thanks, leftover from when i didn't want to do that on GLImage until the name
> was finalized.

Rubber stamp LGTM for mojo/public/c/gles2/ changes.

[piman, 2016-02-22 19:17:15]

lgtm

[liberato (no reviews please), 2016-02-22 21:16:50]

thanks for the feedback.

mind taking a look at the new unit test in texture_manager_unittest.cc?  i said
that i'd add one, and almost forgot.

also fixed SetLevelInfo, which was clearing LevelInfo::image but not
LevelInfo::stream_texture_image.

thanks
-fl

[liberato (no reviews please), 2016-02-23 23:10:56]

The CQ bit was checked by liberato@chromium.org

[liberato (no reviews please), 2016-02-23 23:10:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, yzshen@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1559203003/#ps200001
(title: "s/GL_EXPORT/GPU_EXPORT for windows")

[commit-bot: I haz the power, 2016-02-23 23:16:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1559203003/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1559203003/200001

[commit-bot: I haz the power, 2016-02-24 00:25:04]

Description was changed from

==========
Added GLStreamTextureImage : GLImage, which exposes
GetTextureMatrix.  This provides a texture matrix to
convert from hardware-specific texture coordinates.  Made
AVDACodecImage provide the current matrix from the SurfaceTexture.

Updates GLRenderer::DrawStreamVideoQuad to use the current texture
matrix from the GLImage if available, else fall back to the one
recorded quad.  Once stream_texture_android.cc returns the most
recent texture matrix, it may be possible to remove the entire
DidUpdateMatrix path in VideoFrameProvider.

Does not address CopyTextureCHROMIUM.
    
BUG=530681,226218
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Added GLStreamTextureImage : GLImage, which exposes
GetTextureMatrix.  This provides a texture matrix to
convert from hardware-specific texture coordinates.  Made
AVDACodecImage provide the current matrix from the SurfaceTexture.

Updates GLRenderer::DrawStreamVideoQuad to use the current texture
matrix from the GLImage if available, else fall back to the one
recorded quad.  Once stream_texture_android.cc returns the most
recent texture matrix, it may be possible to remove the entire
DidUpdateMatrix path in VideoFrameProvider.

Does not address CopyTextureCHROMIUM.
    
BUG=530681,226218
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-02-24 00:25:05]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-02-24 00:26:06]

Description was changed from

==========
Added GLStreamTextureImage : GLImage, which exposes
GetTextureMatrix.  This provides a texture matrix to
convert from hardware-specific texture coordinates.  Made
AVDACodecImage provide the current matrix from the SurfaceTexture.

Updates GLRenderer::DrawStreamVideoQuad to use the current texture
matrix from the GLImage if available, else fall back to the one
recorded quad.  Once stream_texture_android.cc returns the most
recent texture matrix, it may be possible to remove the entire
DidUpdateMatrix path in VideoFrameProvider.

Does not address CopyTextureCHROMIUM.
    
BUG=530681,226218
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Added GLStreamTextureImage : GLImage, which exposes
GetTextureMatrix.  This provides a texture matrix to
convert from hardware-specific texture coordinates.  Made
AVDACodecImage provide the current matrix from the SurfaceTexture.

Updates GLRenderer::DrawStreamVideoQuad to use the current texture
matrix from the GLImage if available, else fall back to the one
recorded quad.  Once stream_texture_android.cc returns the most
recent texture matrix, it may be possible to remove the entire
DidUpdateMatrix path in VideoFrameProvider.

Does not address CopyTextureCHROMIUM.

BUG=530681,226218
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/df64f2cb11bb32d8522fa0ebeeed8653be158533
Cr-Commit-Position: refs/heads/master@{#377150}
==========

[commit-bot: I haz the power, 2016-02-24 00:26:08]

Patchset 11 (id:??) landed as
https://crrev.com/df64f2cb11bb32d8522fa0ebeeed8653be158533
Cr-Commit-Position: refs/heads/master@{#377150}