Make Android StreamTexture implement GLStreamTextureImage

Make Android StreamTexture implement GLStreamTextureImage

Then make CopySubTextureCHROMIUM use the transform matrix provided
by GLStreamTextureImage, and stop informing the compositor of the
transform matrix. Video quad drawing applies the matrix automatically
via the CHROMIUM_stream_texture_matrix extension.

This has has the advantage that we no longer need custom texture copying
code in StreamTextureAndroid and we also don't need the IPC that updates
the compositor view of the transform matrix (because it is applied
transparently when the texture is sampled from). This in turn means that
the transform matrix can't get out of sync with the texture data.

BUG=226218, 371500, 588837, 599848
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/8f2032aeb81c2a6a48ed2ed622859b59d84fbbaa
Cr-Commit-Position: refs/heads/master@{#384122}

Committed: https://crrev.com/9268401feeaaa16ffe7719d25e165108bddf9141
Cr-Commit-Position: refs/heads/master@{#385714}

[Tobias Sargeant, 2016-02-29 21:23:22]

Description was changed from

==========
Make Android StreamTexture implement GLStreamTextureImage

This has has the advantage that we no longer need custom texture copying
code in StreamTextureAndroid and we also don't need the IPC that updates
the compositor view of the transform matrix (because it is applied
transparently when the texture is sampled from).

BUG=371500,588837
==========

to

==========
Make Android StreamTexture implement GLStreamTextureImage

This has has the advantage that we no longer need custom texture copying
code in StreamTextureAndroid and we also don't need the IPC that updates
the compositor view of the transform matrix (because it is applied
transparently when the texture is sampled from).

BUG=371500,588837
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[Tobias Sargeant, 2016-02-29 21:26:23]

The CQ bit was checked by tobiasjs@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-29 21:26:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1746983002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1746983002/1

[Tobias Sargeant, 2016-02-29 21:39:05]

Description was changed from

==========
Make Android StreamTexture implement GLStreamTextureImage

This has has the advantage that we no longer need custom texture copying
code in StreamTextureAndroid and we also don't need the IPC that updates
the compositor view of the transform matrix (because it is applied
transparently when the texture is sampled from).

BUG=371500,588837
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Make Android StreamTexture implement GLStreamTextureImage

Make CopySubTextureCHROMIUM use the transform matrix provided by
GLStreamTextureImage, and then stop informing the compositor of the
transform matrix. Video quad drawing applies the matrix automatically
via the CHROMIUM_stream_texture_matrix extension.

This has has the advantage that we no longer need custom texture copying
code in StreamTextureAndroid and we also don't need the IPC that updates
the compositor view of the transform matrix (because it is applied
transparently when the texture is sampled from). This in turn means that
the transform matrix can't get out of sync with the texture data.

BUG=371500,588837
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[Tobias Sargeant, 2016-02-29 21:41:49]

Description was changed from

==========
Make Android StreamTexture implement GLStreamTextureImage

Make CopySubTextureCHROMIUM use the transform matrix provided by
GLStreamTextureImage, and then stop informing the compositor of the
transform matrix. Video quad drawing applies the matrix automatically
via the CHROMIUM_stream_texture_matrix extension.

This has has the advantage that we no longer need custom texture copying
code in StreamTextureAndroid and we also don't need the IPC that updates
the compositor view of the transform matrix (because it is applied
transparently when the texture is sampled from). This in turn means that
the transform matrix can't get out of sync with the texture data.

BUG=371500,588837
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Make Android StreamTexture implement GLStreamTextureImage

Then make CopySubTextureCHROMIUM use the transform matrix provided
by GLStreamTextureImage, and stop informing the compositor of the
transform matrix. Video quad drawing applies the matrix automatically
via the CHROMIUM_stream_texture_matrix extension.

This has has the advantage that we no longer need custom texture copying
code in StreamTextureAndroid and we also don't need the IPC that updates
the compositor view of the transform matrix (because it is applied
transparently when the texture is sampled from). This in turn means that
the transform matrix can't get out of sync with the texture data.

BUG=371500,588837
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[Tobias Sargeant, 2016-02-29 21:57:19]

tobiasjs@chromium.org changed reviewers:
+ dcheng@chromium.org, sievers@chromium.org

[liberato (no reviews please), 2016-02-29 22:33:31]

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

[liberato (no reviews please), 2016-02-29 22:33:32]

nice!

https://codereview.chromium.org/1746983002/diff/20001/content/common/gpu/stre...
File content/common/gpu/stream_texture_android.cc (right):

https://codereview.chromium.org/1746983002/diff/20001/content/common/gpu/stre...
content/common/gpu/stream_texture_android.cc:103: xform[13] += xform[5];
i'm not sure how this works for both AVDA and StreamTexture.  in particular,
AVDACodecImage doesn't do this.

the video stream draw quad shader
(https://code.google.com/p/chromium/codesearch#chromium/src/cc/output/shader.c...)
does something similar, which might explain why it's not broken there.

i think that you're applying the matrix in the right spot, and AVDA should be
similar.  however, at the moment, i don't see why nothing is flipped upside-down
in at least one of the cases.

in your testing, do you know if you were using the 'copy' or 'stream texture
draw quad' path?

https://codereview.chromium.org/1746983002/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (left):

https://codereview.chromium.org/1746983002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13753: // crbug.com/226218.
you don't have 226218 in the BUG= .

[dcheng, 2016-02-29 22:38:12]

rs lgtm for content/common/gpu/gpu_messages.h deletions.

[Tobias Sargeant, 2016-02-29 22:49:41]

Description was changed from

==========
Make Android StreamTexture implement GLStreamTextureImage

Then make CopySubTextureCHROMIUM use the transform matrix provided
by GLStreamTextureImage, and stop informing the compositor of the
transform matrix. Video quad drawing applies the matrix automatically
via the CHROMIUM_stream_texture_matrix extension.

This has has the advantage that we no longer need custom texture copying
code in StreamTextureAndroid and we also don't need the IPC that updates
the compositor view of the transform matrix (because it is applied
transparently when the texture is sampled from). This in turn means that
the transform matrix can't get out of sync with the texture data.

BUG=371500,588837
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Make Android StreamTexture implement GLStreamTextureImage

Then make CopySubTextureCHROMIUM use the transform matrix provided
by GLStreamTextureImage, and stop informing the compositor of the
transform matrix. Video quad drawing applies the matrix automatically
via the CHROMIUM_stream_texture_matrix extension.

This has has the advantage that we no longer need custom texture copying
code in StreamTextureAndroid and we also don't need the IPC that updates
the compositor view of the transform matrix (because it is applied
transparently when the texture is sampled from). This in turn means that
the transform matrix can't get out of sync with the texture data.

BUG=226218,371500,588837
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[Tobias Sargeant, 2016-02-29 23:57:50]

https://codereview.chromium.org/1746983002/diff/20001/content/common/gpu/stre...
File content/common/gpu/stream_texture_android.cc (right):

https://codereview.chromium.org/1746983002/diff/20001/content/common/gpu/stre...
content/common/gpu/stream_texture_android.cc:103: xform[13] += xform[5];
On 2016/02/29 22:33:32, liberato wrote:
> i'm not sure how this works for both AVDA and StreamTexture.  in particular,
> AVDACodecImage doesn't do this.
> 
> the video stream draw quad shader
>
(https://code.google.com/p/chromium/codesearch#chromium/src/cc/output/shader.c...)
> does something similar, which might explain why it's not broken there.
> 
> i think that you're applying the matrix in the right spot, and AVDA should be
> similar.  however, at the moment, i don't see why nothing is flipped
upside-down
> in at least one of the cases.
> 
> in your testing, do you know if you were using the 'copy' or 'stream texture
> draw quad' path?

I tested both paths, corresponding to producing an RGB texture quad and a stream
texture quad in video_layer_impl.cc, and both produce a correctly oriented set
of frames. SurfaceTexture is documented as providing a texture matrix that has
UV=(0,0) for the bottom left of the texture, whereas chromium expects (0,0) to
be the top left of the texture.

https://codereview.chromium.org/1746983002/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (left):

https://codereview.chromium.org/1746983002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13753: // crbug.com/226218.
On 2016/02/29 22:33:32, liberato wrote:
> you don't have 226218 in the BUG= .

Done.

[no sievers, 2016-03-01 00:50:28]

So to remove this entirely (from VideoResourceProvider) we also need to fix the
WebView in-process version. The GLImageImpl in
stream_texture_manager_in_process_android.cc would then also need to implement
GLStreamTextureImage, and you can remove the logic concerned with this in
stream_texture_factory_synchronous_impl.cc. But I'm guessing you wanted to do
that in a separate patch?

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

https://codereview.chromium.org/1746983002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13558: memcpy(transform_matrix,
kIdentityMatrix, sizeof(transform_matrix));
nit: Looks like we can get rid of |kIdentityMatrix| here since if there's no
stream image to override the matrix, we can just fall through below to
DoCopyTexture()?

https://codereview.chromium.org/1746983002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13754: // this correctly, but
others (e.g., stream_texture_android.cc) don't.
you are fixing this TODO right?

https://codereview.chromium.org/1746983002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13763:
copy_texture_CHROMIUM_->DoCopySubTextureWithTransform(
see above: if no matrix override we can fall through to else-...

https://codereview.chromium.org/1746983002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13946: source_height, false,
false, false, kIdentityMatrix);
So you removed the todo but didn't address it.

Is this an unecessary copy+paste in this routine? I mean we don't support stream
textures that are compressed, right?
So the if-.. can go?

[Tobias Sargeant, 2016-03-01 01:23:54]

On 2016/03/01 00:50:28, sievers wrote:
> So to remove this entirely (from VideoResourceProvider) we also need to fix
the
> WebView in-process version. The GLImageImpl in
> stream_texture_manager_in_process_android.cc would then also need to implement
> GLStreamTextureImage, and you can remove the logic concerned with this in
> stream_texture_factory_synchronous_impl.cc. But I'm guessing you wanted to do
> that in a separate patch?

Thanks for pointing this out; it's the missing piece of the y-flip puzzle above.
Although I tested the draw quad path, I didn't notice that the in-process
version reaches directly into the surface texture for the matrix, and passes
that to the compositor. So, there's a y-flip introduced there. There's also a
y-flip introduced really late by VertexShaderVideoTransform. So they cancel.
This explains why the ADVA path works right too.

The in process webview path is going to go soon (in fact it's now disabled
everywhere in favour of doing the texture copy).

What this means, however, is that the shaders in the video draw quad path are
explicitly out of sync (in terms of y orientation) with the shaders in the
texture copy path. I think what that is going to mean is that the most
straightforward thing to do is to add a y-flip to the copy sites, and return the
unmodified matrix from GetTextureMatrix.

[Tobias Sargeant, 2016-03-01 02:06:41]

The CQ bit was checked by tobiasjs@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-01 02:07:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1746983002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1746983002/60001

[commit-bot: I haz the power, 2016-03-01 03:34:06]

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

[commit-bot: I haz the power, 2016-03-01 03:34:07]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Tobias Sargeant, 2016-03-01 21:56:12]

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

https://codereview.chromium.org/1746983002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13558: memcpy(transform_matrix,
kIdentityMatrix, sizeof(transform_matrix));
On 2016/03/01 00:50:27, sievers wrote:
> nit: Looks like we can get rid of |kIdentityMatrix| here since if there's no
> stream image to override the matrix, we can just fall through below to
> DoCopyTexture()?

Done.

https://codereview.chromium.org/1746983002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13754: // this correctly, but
others (e.g., stream_texture_android.cc) don't.
On 2016/03/01 00:50:27, sievers wrote:
> you are fixing this TODO right?

Done.

https://codereview.chromium.org/1746983002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13763:
copy_texture_CHROMIUM_->DoCopySubTextureWithTransform(
On 2016/03/01 00:50:27, sievers wrote:
> see above: if no matrix override we can fall through to else-...

Done.

https://codereview.chromium.org/1746983002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13946: source_height, false,
false, false, kIdentityMatrix);
On 2016/03/01 00:50:27, sievers wrote:
> So you removed the todo but didn't address it.
> 
> Is this an unecessary copy+paste in this routine? I mean we don't support
stream
> textures that are compressed, right?
> So the if-.. can go?

I've removed the explicit support for GL_TEXTURE_EXTERNAL_OES. I don't think
compressed stream textures make any sense.

[Tobias Sargeant, 2016-03-01 21:57:46]

tobiasjs@chromium.org changed reviewers:
+ danakj@chromium.org

[Tobias Sargeant, 2016-03-01 22:00:05]

Dana: is it ok to remove DidUpdateMatrix from cc/layers now that we can get the
stream texture matrix either via the extension or at texture copy time?

[Tobias Sargeant, 2016-03-17 09:36:34]

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

[Tobias Sargeant, 2016-03-17 09:38:09]

ping

[liberato (no reviews please), 2016-03-17 14:38:01]

sorry for the delay.

thanks
-fl

https://codereview.chromium.org/1746983002/diff/120001/content/common/gpu/str...
File content/common/gpu/stream_texture_android.cc (right):

https://codereview.chromium.org/1746983002/diff/120001/content/common/gpu/str...
content/common/gpu/stream_texture_android.cc:94:
surface_texture_->GetTransformMatrix(xform);
i think that you need to call UpdateTexImage here.  for example, when
gl_renderer.cc gets the uniform, CopyTexImage might not have been called yet.

[Tobias Sargeant, 2016-03-17 15:17:58]

https://codereview.chromium.org/1746983002/diff/120001/content/common/gpu/str...
File content/common/gpu/stream_texture_android.cc (right):

https://codereview.chromium.org/1746983002/diff/120001/content/common/gpu/str...
content/common/gpu/stream_texture_android.cc:94:
surface_texture_->GetTransformMatrix(xform);
On 2016/03/17 14:38:01, liberato wrote:
> i think that you need to call UpdateTexImage here.  for example, when
> gl_renderer.cc gets the uniform, CopyTexImage might not have been called yet.

Done.

[liberato (no reviews please), 2016-03-17 15:23:36]

lgtm

[DaleCurtis, 2016-03-17 17:29:48]

media/ lgtm

[no sievers, 2016-03-18 01:34:12]

Is your patch missing the changes to StreamVideoDrawQuad and GLRenderer
(TODO(liberato) there)?

and thanks for addressing this longstanding issue!

https://codereview.chromium.org/1746983002/diff/140001/content/common/gpu/str...
File content/common/gpu/stream_texture_android.cc (right):

https://codereview.chromium.org/1746983002/diff/140001/content/common/gpu/str...
content/common/gpu/stream_texture_android.cc:94: UpdateTexImage();
Doesn't this blow up in Chrome without a |scoped_ptr<ui::ScopedMakeCurrent>
scoped_make_current(MakeStubCurrent())|? (And with virtual contexts disabled.)

SurfaceTexture doesn't allow calling UpdateTexImage(), see
GLConsumer::mEglContext checks.

So it would fail in the case where we call updateTexImage() from different
contexts which could be for example if we draw it in the browser vs. copying it
out in the renderer (webgl, canvas).

On a related note, it would be nice to avoid the context switch in the more
important case (browser context).

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

https://codereview.chromium.org/1746983002/diff/140001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13815: if (GLStreamTextureImage*
image =
nit: merge ifs

https://codereview.chromium.org/1746983002/diff/140001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:14009: if (GLStreamTextureImage*
image =
nit: merge ifs

[Tobias Sargeant, 2016-03-18 10:30:13]

https://codereview.chromium.org/1746983002/diff/140001/content/common/gpu/str...
File content/common/gpu/stream_texture_android.cc (right):

https://codereview.chromium.org/1746983002/diff/140001/content/common/gpu/str...
content/common/gpu/stream_texture_android.cc:94: UpdateTexImage();
On 2016/03/18 01:34:11, sievers wrote:
> Doesn't this blow up in Chrome without a |scoped_ptr<ui::ScopedMakeCurrent>
> scoped_make_current(MakeStubCurrent())|? (And with virtual contexts disabled.)
> 
> SurfaceTexture doesn't allow calling UpdateTexImage(), see
> GLConsumer::mEglContext checks.
> 
> So it would fail in the case where we call updateTexImage() from different
> contexts which could be for example if we draw it in the browser vs. copying
it
> out in the renderer (webgl, canvas).
> 
> On a related note, it would be nice to avoid the context switch in the more
> important case (browser context).

This is calling the UpdateTexImage below, which does the required context
switch, and also guards against calling surface_texture_->UpdateTexImage() more
than once per available frame.

I guess depending on where the surface texture is used from, we might end up
doing quite a bit of context switching. It seems we can't know ahead of time how
it will be used, but it could probably be optimized for the draw-only case.

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

https://codereview.chromium.org/1746983002/diff/140001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:14009: if (GLStreamTextureImage*
image =
On 2016/03/18 01:34:11, sievers wrote:
> nit: merge ifs

The inner if declares and assigns |image|, so it can't just be combined with the
outer if. It feels kind of odd to me to declare the variable outside the scope
of the if, especially as then the if would be a mix of equality test and
assignment. What would you prefer?

[Tobias Sargeant, 2016-03-18 12:00:35]

On 2016/03/18 01:34:12, sievers wrote:
> Is your patch missing the changes to StreamVideoDrawQuad and GLRenderer
> (TODO(liberato) there)?

Looking at this again, I think that the matrix in SVDQ is not actually
vestigial. Even in video_layer_impl.cc:322 we still pass a scaling matrix (which
had better be an identity scaling, presently). It seems like the right thing is
actually that the matrix passed to UniformMatrix4fvStreamTextureMatrixCHROMIUM
should not be a fallback; the uniform should be set to matrix * GLSTI matrix.
WDYT @liberato?

In looking at this I also noted that CAOverlayLayer takes a copy of the SVDQ
resource_id, which might indicate that there's another draw path that needs to
be made aware of the GLSTI matrix? cc'ing ccameron in case this is true.

[liberato (no reviews please), 2016-03-19 05:58:56]

On 2016/03/18 12:00:35, Tobias Sargeant wrote:
> On 2016/03/18 01:34:12, sievers wrote:
> > Is your patch missing the changes to StreamVideoDrawQuad and GLRenderer
> > (TODO(liberato) there)?
> 
> Looking at this again, I think that the matrix in SVDQ is not actually
> vestigial. Even in video_layer_impl.cc:322 we still pass a scaling matrix
(which
> had better be an identity scaling, presently). It seems like the right thing
is
> actually that the matrix passed to UniformMatrix4fvStreamTextureMatrixCHROMIUM
> should not be a fallback; the uniform should be set to matrix * GLSTI matrix.
> WDYT @liberato?
> 
> In looking at this I also noted that CAOverlayLayer takes a copy of the SVDQ
> resource_id, which might indicate that there's another draw path that needs to
> be made aware of the GLSTI matrix? cc'ing ccameron in case this is true.

multiplication: i agree.  that thought had also crossed my mind when i was doing
the original CL, but i missed that it was actually being used and just optimized
it out :)  good catch.

[Tobias Sargeant, 2016-03-21 15:29:20]

Patchset #9 (id:160001) has been deleted

[Tobias Sargeant, 2016-03-21 15:43:12]

> multiplication: i agree.  that thought had also crossed my mind when i was
doing
> the original CL, but i missed that it was actually being used and just
optimized
> it out :)  good catch.

Per this discussion, CL @ https://codereview.chromium.org/1818073002/

[no sievers, 2016-03-24 19:02:41]

lgtm

[Tobias Sargeant, 2016-03-29 09:50:03]

tobiasjs@chromium.org changed reviewers:
+ enne@chromium.org

[Tobias Sargeant, 2016-03-29 09:50:38]

Enne, could I please ask you to take a look at the cc/layers changes?

Thanks.

[danakj, 2016-03-30 18:29:49]

Oh, sorry I missed this. Deleting code LGTM

[Tobias Sargeant, 2016-03-30 19:41:43]

The CQ bit was checked by tobiasjs@chromium.org

[Tobias Sargeant, 2016-03-30 19:41:43]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1746983002/#ps140001
(title: "Call UpdateTexImage before fetching the surface texture matrix")

[commit-bot: I haz the power, 2016-03-30 19:42:07]

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

[commit-bot: I haz the power, 2016-03-30 19:44:39]

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

[commit-bot: I haz the power, 2016-03-30 19:44:41]

Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_optional_gpu_tests_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[Tobias Sargeant, 2016-03-30 21:16:27]

The CQ bit was checked by tobiasjs@chromium.org

[Tobias Sargeant, 2016-03-30 21:16:27]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org,
liberato@chromium.org, danakj@chromium.org, dalecurtis@chromium.org,
sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1746983002/#ps180001
(title: "Rebase")

[commit-bot: I haz the power, 2016-03-30 21:16:44]

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

[commit-bot: I haz the power, 2016-03-30 22:38:29]

Description was changed from

==========
Make Android StreamTexture implement GLStreamTextureImage

Then make CopySubTextureCHROMIUM use the transform matrix provided
by GLStreamTextureImage, and stop informing the compositor of the
transform matrix. Video quad drawing applies the matrix automatically
via the CHROMIUM_stream_texture_matrix extension.

This has has the advantage that we no longer need custom texture copying
code in StreamTextureAndroid and we also don't need the IPC that updates
the compositor view of the transform matrix (because it is applied
transparently when the texture is sampled from). This in turn means that
the transform matrix can't get out of sync with the texture data.

BUG=226218,371500,588837
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Make Android StreamTexture implement GLStreamTextureImage

Then make CopySubTextureCHROMIUM use the transform matrix provided
by GLStreamTextureImage, and stop informing the compositor of the
transform matrix. Video quad drawing applies the matrix automatically
via the CHROMIUM_stream_texture_matrix extension.

This has has the advantage that we no longer need custom texture copying
code in StreamTextureAndroid and we also don't need the IPC that updates
the compositor view of the transform matrix (because it is applied
transparently when the texture is sampled from). This in turn means that
the transform matrix can't get out of sync with the texture data.

BUG=226218,371500,588837
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-03-30 22:38:33]

Committed patchset #9 (id:180001)

[commit-bot: I haz the power, 2016-03-30 22:40:39]

Description was changed from

==========
Make Android StreamTexture implement GLStreamTextureImage

Then make CopySubTextureCHROMIUM use the transform matrix provided
by GLStreamTextureImage, and stop informing the compositor of the
transform matrix. Video quad drawing applies the matrix automatically
via the CHROMIUM_stream_texture_matrix extension.

This has has the advantage that we no longer need custom texture copying
code in StreamTextureAndroid and we also don't need the IPC that updates
the compositor view of the transform matrix (because it is applied
transparently when the texture is sampled from). This in turn means that
the transform matrix can't get out of sync with the texture data.

BUG=226218,371500,588837
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Make Android StreamTexture implement GLStreamTextureImage

Then make CopySubTextureCHROMIUM use the transform matrix provided
by GLStreamTextureImage, and stop informing the compositor of the
transform matrix. Video quad drawing applies the matrix automatically
via the CHROMIUM_stream_texture_matrix extension.

This has has the advantage that we no longer need custom texture copying
code in StreamTextureAndroid and we also don't need the IPC that updates
the compositor view of the transform matrix (because it is applied
transparently when the texture is sampled from). This in turn means that
the transform matrix can't get out of sync with the texture data.

BUG=226218,371500,588837
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/8f2032aeb81c2a6a48ed2ed622859b59d84fbbaa
Cr-Commit-Position: refs/heads/master@{#384122}
==========

[commit-bot: I haz the power, 2016-03-30 22:40:40]

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

[Daniel Kurtz, 2016-04-01 17:24:46]

A revert of this CL (patchset #9 id:180001) has been created in
https://codereview.chromium.org/1848923003/ by djkurtz@chromium.org.

The reason for reverting is: A git bisect suggests that this patch makes Youtube
and Hangouts appear upside-down on Chrome OS.

BUG=599848.

[Tobias Sargeant, 2016-04-06 12:36:27]

Description was changed from

==========
Make Android StreamTexture implement GLStreamTextureImage

Then make CopySubTextureCHROMIUM use the transform matrix provided
by GLStreamTextureImage, and stop informing the compositor of the
transform matrix. Video quad drawing applies the matrix automatically
via the CHROMIUM_stream_texture_matrix extension.

This has has the advantage that we no longer need custom texture copying
code in StreamTextureAndroid and we also don't need the IPC that updates
the compositor view of the transform matrix (because it is applied
transparently when the texture is sampled from). This in turn means that
the transform matrix can't get out of sync with the texture data.

BUG=226218,371500,588837
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/8f2032aeb81c2a6a48ed2ed622859b59d84fbbaa
Cr-Commit-Position: refs/heads/master@{#384122}
==========

to

==========
Make Android StreamTexture implement GLStreamTextureImage

Then make CopySubTextureCHROMIUM use the transform matrix provided
by GLStreamTextureImage, and stop informing the compositor of the
transform matrix. Video quad drawing applies the matrix automatically
via the CHROMIUM_stream_texture_matrix extension.

This has has the advantage that we no longer need custom texture copying
code in StreamTextureAndroid and we also don't need the IPC that updates
the compositor view of the transform matrix (because it is applied
transparently when the texture is sampled from). This in turn means that
the transform matrix can't get out of sync with the texture data.

BUG=226218,371500,588837,599848
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/8f2032aeb81c2a6a48ed2ed622859b59d84fbbaa
Cr-Commit-Position: refs/heads/master@{#384122}
==========

[Tobias Sargeant, 2016-04-06 13:08:38]

On 2016/04/01 17:24:46, Daniel Kurtz wrote:
> A revert of this CL (patchset #9 id:180001) has been created in
> https://codereview.chromium.org/1848923003/ by mailto:djkurtz@chromium.org.
> 
> The reason for reverting is: A git bisect suggests that this patch makes
Youtube
> and Hangouts appear upside-down on Chrome OS.
> 
> BUG=599848.

It turns out we can't remove the default (y-flip) stream_texture_matrix from
VideoFrameProviderClientImpl. Chrome OS provides GL_TEXTURE_EXTERNAL_OES
textures as video frames, and the y-flip needs to be there in order to undo the
y-flip in the StreamVideoDrawQuad shader. Thanksfully it's simple enough to
leave that code in (until the shader is fixed too) because the matrix held by
the SVDQ instance is (for the moment) ignored in all android paths. We can
safely keep the default (but can get rid of the updating code now). and then
remove it when we remove the y-flip from the shader.

I've manually tested:

Chrome on android +/- ADVA, both direct rendering of videos and copying to
canvas.
Chrome OS (youtube on samus)
WebView on android (multiprocess and singleprocess, rendering and copying)
Chrome on Linux (youtube)

So I think this resolves all the outstanding bugs.

djkurtz@, danakj@, sievers@, liberato@ Are you ok for me to reland?

[liberato (no reviews please), 2016-04-06 14:15:35]

lgtm

[no sievers, 2016-04-06 23:23:29]

On 2016/04/06 13:08:38, Tobias Sargeant wrote:
> On 2016/04/01 17:24:46, Daniel Kurtz wrote:
> > A revert of this CL (patchset #9 id:180001) has been created in
> > https://codereview.chromium.org/1848923003/ by mailto:djkurtz@chromium.org.
> > 
> > The reason for reverting is: A git bisect suggests that this patch makes
> Youtube
> > and Hangouts appear upside-down on Chrome OS.
> > 
> > BUG=599848.
> 
> It turns out we can't remove the default (y-flip) stream_texture_matrix from
> VideoFrameProviderClientImpl. Chrome OS provides GL_TEXTURE_EXTERNAL_OES
> textures as video frames, and the y-flip needs to be there in order to undo
the
> y-flip in the StreamVideoDrawQuad shader. Thanksfully it's simple enough to
> leave that code in (until the shader is fixed too) because the matrix held by
> the SVDQ instance is (for the moment) ignored in all android paths. We can
> safely keep the default (but can get rid of the updating code now). and then
> remove it when we remove the y-flip from the shader.
> 
> I've manually tested:
> 
> Chrome on android +/- ADVA, both direct rendering of videos and copying to
> canvas.
> Chrome OS (youtube on samus)
> WebView on android (multiprocess and singleprocess, rendering and copying)
> Chrome on Linux (youtube)
> 
> So I think this resolves all the outstanding bugs.
> 
> djkurtz@, danakj@, sievers@, liberato@ Are you ok for me to reland?

sounds good. thanks for figuring this all out!

[Tobias Sargeant, 2016-04-07 09:49:52]

The CQ bit was checked by tobiasjs@chromium.org

[Tobias Sargeant, 2016-04-07 09:49:53]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org,
danakj@chromium.org, dalecurtis@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1746983002/#ps200001
(title: "Don't remove the default stream texture matrix from cc/layers")

[commit-bot: I haz the power, 2016-04-07 09:50:08]

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

[commit-bot: I haz the power, 2016-04-07 11:49:08]

Description was changed from

==========
Make Android StreamTexture implement GLStreamTextureImage

Then make CopySubTextureCHROMIUM use the transform matrix provided
by GLStreamTextureImage, and stop informing the compositor of the
transform matrix. Video quad drawing applies the matrix automatically
via the CHROMIUM_stream_texture_matrix extension.

This has has the advantage that we no longer need custom texture copying
code in StreamTextureAndroid and we also don't need the IPC that updates
the compositor view of the transform matrix (because it is applied
transparently when the texture is sampled from). This in turn means that
the transform matrix can't get out of sync with the texture data.

BUG=226218,371500,588837,599848
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/8f2032aeb81c2a6a48ed2ed622859b59d84fbbaa
Cr-Commit-Position: refs/heads/master@{#384122}
==========

to

==========
Make Android StreamTexture implement GLStreamTextureImage

Then make CopySubTextureCHROMIUM use the transform matrix provided
by GLStreamTextureImage, and stop informing the compositor of the
transform matrix. Video quad drawing applies the matrix automatically
via the CHROMIUM_stream_texture_matrix extension.

This has has the advantage that we no longer need custom texture copying
code in StreamTextureAndroid and we also don't need the IPC that updates
the compositor view of the transform matrix (because it is applied
transparently when the texture is sampled from). This in turn means that
the transform matrix can't get out of sync with the texture data.

BUG=226218,371500,588837,599848
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/8f2032aeb81c2a6a48ed2ed622859b59d84fbbaa
Cr-Commit-Position: refs/heads/master@{#384122}
==========

[commit-bot: I haz the power, 2016-04-07 11:49:09]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2016-04-07 11:51:35]

Description was changed from

==========
Make Android StreamTexture implement GLStreamTextureImage

Then make CopySubTextureCHROMIUM use the transform matrix provided
by GLStreamTextureImage, and stop informing the compositor of the
transform matrix. Video quad drawing applies the matrix automatically
via the CHROMIUM_stream_texture_matrix extension.

This has has the advantage that we no longer need custom texture copying
code in StreamTextureAndroid and we also don't need the IPC that updates
the compositor view of the transform matrix (because it is applied
transparently when the texture is sampled from). This in turn means that
the transform matrix can't get out of sync with the texture data.

BUG=226218,371500,588837,599848
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/8f2032aeb81c2a6a48ed2ed622859b59d84fbbaa
Cr-Commit-Position: refs/heads/master@{#384122}
==========

to

==========
Make Android StreamTexture implement GLStreamTextureImage

Then make CopySubTextureCHROMIUM use the transform matrix provided
by GLStreamTextureImage, and stop informing the compositor of the
transform matrix. Video quad drawing applies the matrix automatically
via the CHROMIUM_stream_texture_matrix extension.

This has has the advantage that we no longer need custom texture copying
code in StreamTextureAndroid and we also don't need the IPC that updates
the compositor view of the transform matrix (because it is applied
transparently when the texture is sampled from). This in turn means that
the transform matrix can't get out of sync with the texture data.

BUG=226218,371500,588837,599848
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/8f2032aeb81c2a6a48ed2ed622859b59d84fbbaa
Cr-Commit-Position: refs/heads/master@{#384122}

Committed: https://crrev.com/9268401feeaaa16ffe7719d25e165108bddf9141
Cr-Commit-Position: refs/heads/master@{#385714}
==========

[commit-bot: I haz the power, 2016-04-07 11:51:37]

Patchset 10 (id:??) landed as
https://crrev.com/9268401feeaaa16ffe7719d25e165108bddf9141
Cr-Commit-Position: refs/heads/master@{#385714}