cc: Add support for non-2D texture targets to YUVVideoQuad.

cc: Add support for non-2D texture targets to YUVVideoQuad.

This removes the TEXTURE_2D target and normalized coordinates
assumption from the YUVVideoQuad code. This is a prerequisite
to using IOSurface/SurfaceTexture backed GpuMemoryBuffers for
zero-copy transfer of software decoded video frames to the
graphics hardware.

BUG=485859
TEST=cc_unittests --gtest_filter=VideoGLRendererPixelTest.*

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

[reveman, 2015-05-09 00:16:36]

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

[Daniele Castagna, 2015-05-09 00:43:45]

Two nits.
LGTM % danakj's review.

https://codereview.chromium.org/1131253003/diff/1/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1131253003/diff/1/cc/output/gl_renderer.cc#ne...
cc/output/gl_renderer.cc:1979: sampler == SAMPLER_TYPE_2D_RECT ? gfx::Size(1, 1)
: quad->ya_tex_size;
nit: You can compute the inverse here and multiply everywhere.

https://codereview.chromium.org/1131253003/diff/1/cc/output/shader.cc
File cc/output/shader.cc (right):

https://codereview.chromium.org/1131253003/diff/1/cc/output/shader.cc#newcode...
cc/output/shader.cc:2076: max(ya_clamp_rect.xy, min(ya_clamp_rect.zw,
v_yaTexCoord));
nit: clamp?

[reveman, 2015-05-09 00:56:38]

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

[reveman, 2015-05-09 00:56:39]

+danakj

https://codereview.chromium.org/1131253003/diff/1/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1131253003/diff/1/cc/output/gl_renderer.cc#ne...
cc/output/gl_renderer.cc:1979: sampler == SAMPLER_TYPE_2D_RECT ? gfx::Size(1, 1)
: quad->ya_tex_size;
On 2015/05/09 at 00:43:44, Daniele Castagna wrote:
> nit: You can compute the inverse here and multiply everywhere.

Done.

https://codereview.chromium.org/1131253003/diff/1/cc/output/shader.cc
File cc/output/shader.cc (right):

https://codereview.chromium.org/1131253003/diff/1/cc/output/shader.cc#newcode...
cc/output/shader.cc:2076: max(ya_clamp_rect.xy, min(ya_clamp_rect.zw,
v_yaTexCoord));
On 2015/05/09 at 00:43:44, Daniele Castagna wrote:
> nit: clamp?

In follow up patch, maybe.

[danakj, 2015-05-11 17:03:50]

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

[danakj, 2015-05-11 17:03:51]

+hendrikw

[hendrikw, 2015-05-11 23:22:21]

https://codereview.chromium.org/1131253003/diff/20001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1131253003/diff/20001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:1948: a_texture_location =
program->fragment_shader().a_texture_location();
This if/else is getting long, and only 2 lines differ, should we consider
handling the program and a_texture_location separately?  We wouldn't need all of
the -1's above either then.

[reveman, 2015-05-11 23:35:01]

https://codereview.chromium.org/1131253003/diff/20001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1131253003/diff/20001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:1948: a_texture_location =
program->fragment_shader().a_texture_location();
On 2015/05/11 23:22:20, hendrikw wrote:
> This if/else is getting long, and only 2 lines differ, should we consider
> handling the program and a_texture_location separately?  We wouldn't need all
of
> the -1's above either then.

The program types are different so we'd need add some template function function
to change this. That might be a good idea but I'd rather do that in a separate
patch.

[hendrikw, 2015-05-11 23:37:29]

On 2015/05/11 23:35:01, reveman wrote:
> https://codereview.chromium.org/1131253003/diff/20001/cc/output/gl_renderer.cc
> File cc/output/gl_renderer.cc (right):
> 
>
https://codereview.chromium.org/1131253003/diff/20001/cc/output/gl_renderer.c...
> cc/output/gl_renderer.cc:1948: a_texture_location =
> program->fragment_shader().a_texture_location();
> On 2015/05/11 23:22:20, hendrikw wrote:
> > This if/else is getting long, and only 2 lines differ, should we consider
> > handling the program and a_texture_location separately?  We wouldn't need
all
> of
> > the -1's above either then.
> 
> The program types are different so we'd need add some template function
function
> to change this. That might be a good idea but I'd rather do that in a separate
> patch.

ok, non-owner LGTM

[danakj, 2015-05-12 16:32:12]

https://codereview.chromium.org/1131253003/diff/20001/cc/layers/video_layer_i...
File cc/layers/video_layer_impl.cc (right):

https://codereview.chromium.org/1131253003/diff/20001/cc/layers/video_layer_i...
cc/layers/video_layer_impl.cc:247: gfx::RectF ya_tex_coord_rect(visible_rect);
if these are in pixels not in [0,1] they aren't really tex coord rects anymore?
our texel space is [0,1]

i think you should do the math you have in GLRenderer over here to get this
right, eg:

ya_vertex_tex_translate_x = quad->ya_tex_coord_rect.x() * ya_tex_scale.width()

Instead you should divide the ya_tex_coord_rect.x here by the ya_tex_size.width

Is there a reason you can't do that math here so you have [0,1] coords?

https://codereview.chromium.org/1131253003/diff/20001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1131253003/diff/20001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:1983: ya_tex_scale.Scale(1.0f /
quad->ya_tex_size.width(),
you're scaling 1 by this, you could just say = gfx::SizeF(1/ya_tex_size,...)
etc?

[danakj, 2015-05-12 16:47:41]

https://codereview.chromium.org/1131253003/diff/20001/cc/test/render_pass_tes...
File cc/test/render_pass_test_common.cc (right):

https://codereview.chromium.org/1131253003/diff/20001/cc/test/render_pass_tes...
cc/test/render_pass_test_common.cc:248: gfx::RectF(0, 0, 100, 100),
gfx::RectF(0, 0, 50, 50),
float literals should be written with f suffix

[reveman, 2015-05-12 17:09:36]

PTAL

https://codereview.chromium.org/1131253003/diff/20001/cc/layers/video_layer_i...
File cc/layers/video_layer_impl.cc (right):

https://codereview.chromium.org/1131253003/diff/20001/cc/layers/video_layer_i...
cc/layers/video_layer_impl.cc:247: gfx::RectF ya_tex_coord_rect(visible_rect);
On 2015/05/12 at 16:32:12, danakj wrote:
> if these are in pixels not in [0,1] they aren't really tex coord rects
anymore? our texel space is [0,1]
> 
> i think you should do the math you have in GLRenderer over here to get this
right, eg:
> 
> ya_vertex_tex_translate_x = quad->ya_tex_coord_rect.x() * ya_tex_scale.width()
> 
> Instead you should divide the ya_tex_coord_rect.x here by the
ya_tex_size.width
> 
> Is there a reason you can't do that math here so you have [0,1] coords?

If tex coords are normalized or not depend on the target. tex_coord_rect for
quads that support targets with non-normalized coordinates (e.g. TileDrawQuad)
currently use a non-normalized tex_coord_rect and convert to normalized
coordinates in GLRenderer when needed. I prefer not doing things differently in
the VideoDrawQuad case but I guess we can change it everywhere in a follow up
patch if you think it's better to always pass normalized coords to the quads and
convert to non-normalized in the Renderer impl.

https://codereview.chromium.org/1131253003/diff/20001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1131253003/diff/20001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:1983: ya_tex_scale.Scale(1.0f /
quad->ya_tex_size.width(),
On 2015/05/12 at 16:32:12, danakj wrote:
> you're scaling 1 by this, you could just say = gfx::SizeF(1/ya_tex_size,...)
etc?


Done.

https://codereview.chromium.org/1131253003/diff/20001/cc/test/render_pass_tes...
File cc/test/render_pass_test_common.cc (right):

https://codereview.chromium.org/1131253003/diff/20001/cc/test/render_pass_tes...
cc/test/render_pass_test_common.cc:248: gfx::RectF(0, 0, 100, 100),
gfx::RectF(0, 0, 50, 50),
On 2015/05/12 at 16:47:41, danakj wrote:
> float literals should be written with f suffix

Done.

fyi, this file is not very consistent in this regard.

[danakj, 2015-05-12 17:22:11]

https://codereview.chromium.org/1131253003/diff/20001/cc/layers/video_layer_i...
File cc/layers/video_layer_impl.cc (right):

https://codereview.chromium.org/1131253003/diff/20001/cc/layers/video_layer_i...
cc/layers/video_layer_impl.cc:247: gfx::RectF ya_tex_coord_rect(visible_rect);
On 2015/05/12 17:09:35, reveman wrote:
> On 2015/05/12 at 16:32:12, danakj wrote:
> > if these are in pixels not in [0,1] they aren't really tex coord rects
> anymore? our texel space is [0,1]
> > 
> > i think you should do the math you have in GLRenderer over here to get this
> right, eg:
> > 
> > ya_vertex_tex_translate_x = quad->ya_tex_coord_rect.x() *
ya_tex_scale.width()
> > 
> > Instead you should divide the ya_tex_coord_rect.x here by the
> ya_tex_size.width
> > 
> > Is there a reason you can't do that math here so you have [0,1] coords?
> 
> If tex coords are normalized or not depend on the target. tex_coord_rect for
> quads that support targets with non-normalized coordinates (e.g. TileDrawQuad)
> currently use a non-normalized tex_coord_rect and convert to normalized
> coordinates in GLRenderer when needed. I prefer not doing things differently
in
> the VideoDrawQuad case but I guess we can change it everywhere in a follow up
> patch if you think it's better to always pass normalized coords to the quads
and
> convert to non-normalized in the Renderer impl. 

Maybe we're a bit inconsistent but this used to be normalized and most places
are using normalized tex coords, I think Tiles may be the exception.

TextureLayerImpl:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/texture_...

RenderSurfaceImpl:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/render_s...

Video RGB:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/video_la...

For PictureLayerImpl it calls it texture_rect (not tex_coord) fwiw, it's
explicitly non-normalized. But I think this should be consistent with everything
else?

[reveman, 2015-05-12 17:40:35]

https://codereview.chromium.org/1131253003/diff/20001/cc/layers/video_layer_i...
File cc/layers/video_layer_impl.cc (right):

https://codereview.chromium.org/1131253003/diff/20001/cc/layers/video_layer_i...
cc/layers/video_layer_impl.cc:247: gfx::RectF ya_tex_coord_rect(visible_rect);
On 2015/05/12 17:22:11, danakj wrote:
> On 2015/05/12 17:09:35, reveman wrote:
> > On 2015/05/12 at 16:32:12, danakj wrote:
> > > if these are in pixels not in [0,1] they aren't really tex coord rects
> > anymore? our texel space is [0,1]
> > > 
> > > i think you should do the math you have in GLRenderer over here to get
this
> > right, eg:
> > > 
> > > ya_vertex_tex_translate_x = quad->ya_tex_coord_rect.x() *
> ya_tex_scale.width()
> > > 
> > > Instead you should divide the ya_tex_coord_rect.x here by the
> > ya_tex_size.width
> > > 
> > > Is there a reason you can't do that math here so you have [0,1] coords?
> > 
> > If tex coords are normalized or not depend on the target. tex_coord_rect for
> > quads that support targets with non-normalized coordinates (e.g.
TileDrawQuad)
> > currently use a non-normalized tex_coord_rect and convert to normalized
> > coordinates in GLRenderer when needed. I prefer not doing things differently
> in
> > the VideoDrawQuad case but I guess we can change it everywhere in a follow
up
> > patch if you think it's better to always pass normalized coords to the quads
> and
> > convert to non-normalized in the Renderer impl. 
> 
> Maybe we're a bit inconsistent but this used to be normalized and most places
> are using normalized tex coords, I think Tiles may be the exception.
> 
> TextureLayerImpl:
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/texture_...
> 
> RenderSurfaceImpl:
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/render_s...
> 
> Video RGB:
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/video_la...
> 
> For PictureLayerImpl it calls it texture_rect (not tex_coord) fwiw, it's
> explicitly non-normalized. But I think this should be consistent with
everything
> else?

It's "tex_coord_rect" here:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/quads/tile_draw...
it just happens that texture_rect is what we should use in the case of
PictureLayerImpl.

I'd really prefer if we didn't handle TEXTURE_RECTANGLE differently in
GLRenderer for VideoDrawQuad and TileDrawQuad.

I can add TODOs here and follow up with a patch that tries to make this
consistent across all quad types. Not sure normalized is preferred over
non-normalized though. I'd probably go with the latter and avoid the conversion
to normalized coords unless necessary.

[danakj, 2015-05-12 17:45:28]

On Tue, May 12, 2015 at 10:40 AM, <reveman@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1131253003/diff/20001/cc/layers/video_layer_i...
> File cc/layers/video_layer_impl.cc (right):
>
>
>
https://codereview.chromium.org/1131253003/diff/20001/cc/layers/video_layer_i...
> cc/layers/video_layer_impl.cc:247: gfx::RectF
> ya_tex_coord_rect(visible_rect);
> On 2015/05/12 17:22:11, danakj wrote:
>
>> On 2015/05/12 17:09:35, reveman wrote:
>> > On 2015/05/12 at 16:32:12, danakj wrote:
>> > > if these are in pixels not in [0,1] they aren't really tex coord
>>
> rects
>
>> > anymore? our texel space is [0,1]
>> > >
>> > > i think you should do the math you have in GLRenderer over here to
>>
> get this
>
>> > right, eg:
>> > >
>> > > ya_vertex_tex_translate_x = quad->ya_tex_coord_rect.x() *
>> ya_tex_scale.width()
>> > >
>> > > Instead you should divide the ya_tex_coord_rect.x here by the
>> > ya_tex_size.width
>> > >
>> > > Is there a reason you can't do that math here so you have [0,1]
>>
> coords?
>
>> >
>> > If tex coords are normalized or not depend on the target.
>>
> tex_coord_rect for
>
>> > quads that support targets with non-normalized coordinates (e.g.
>>
> TileDrawQuad)
>
>> > currently use a non-normalized tex_coord_rect and convert to
>>
> normalized
>
>> > coordinates in GLRenderer when needed. I prefer not doing things
>>
> differently
>
>> in
>> > the VideoDrawQuad case but I guess we can change it everywhere in a
>>
> follow up
>
>> > patch if you think it's better to always pass normalized coords to
>>
> the quads
>
>> and
>> > convert to non-normalized in the Renderer impl.
>>
>
>  Maybe we're a bit inconsistent but this used to be normalized and most
>>
> places
>
>> are using normalized tex coords, I think Tiles may be the exception.
>>
>
>  TextureLayerImpl:
>>
>
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/texture_...
>
>  RenderSurfaceImpl:
>>
>
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/render_s...
>
>  Video RGB:
>>
>
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/video_la...
>
>  For PictureLayerImpl it calls it texture_rect (not tex_coord) fwiw,
>>
> it's
>
>> explicitly non-normalized. But I think this should be consistent with
>>
> everything
>
>> else?
>>
>
> It's "tex_coord_rect" here:
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/quads/tile_draw...
> it just happens that texture_rect is what we should use in the case of
> PictureLayerImpl.
>
> I'd really prefer if we didn't handle TEXTURE_RECTANGLE differently in
> GLRenderer for VideoDrawQuad and TileDrawQuad.
>

But now it's being inconsistent even within VideoLayerImpl with YUV vs RGB.


> I can add TODOs here and follow up with a patch that tries to make this
> consistent across all quad types. Not sure normalized is preferred over
> non-normalized though. I'd probably go with the latter and avoid the
> conversion to normalized coords unless necessary.
>

OK this discussion has gone beyond productive. Please add a TODO and file a
bug and point the TODO at it.

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

[danakj, 2015-05-12 17:46:40]

https://codereview.chromium.org/1131253003/diff/60001/mojo/converters/surface...
File mojo/converters/surfaces/surfaces_type_converters.cc (right):

https://codereview.chromium.org/1131253003/diff/60001/mojo/converters/surface...
mojo/converters/surfaces/surfaces_type_converters.cc:176:
yuv_state->tex_coord_rect.To<gfx::RectF>(),
You need to serialize both rects no? This will not do the right thing.

[reveman, 2015-05-12 21:06:33]

reveman@chromium.org changed reviewers:
+ jamesr@chromium.org, wfh@chromium.org

[reveman, 2015-05-12 21:06:34]

Added TODOs. PTAL.

+wfh for content/common/cc_messages.h
+jamesr for mojo/converters/surfaces/surfaces_type_converters.cc

[danakj, 2015-05-12 21:13:17]

LGTM

[Will Harris, 2015-05-12 21:18:03]

content/common/cc_messages.h lgtm

[reveman, 2015-05-12 21:42:01]

reveman@chromium.org changed reviewers:
+ sky@chromium.org

[reveman, 2015-05-12 21:42:02]

+sky for mojo/converters/surfaces/surfaces_type_converters.cc

[sky, 2015-05-12 21:44:06]

I have no idea what that is, but I'll try and figure it out. LGTM

[reveman, 2015-05-12 21:54:50]

On 2015/05/12 at 21:44:06, sky wrote:
> I have no idea what that is, but I'll try and figure it out. LGTM

Thanks. I think all that is needed is to update this code once
third_party/mojo_services has been updated to reflect the IPC change.

Fyi, the video quad IPC will likely change again soon so I wouldn't rush to get
it fixed.

[reveman, 2015-05-12 21:54:59]

The CQ bit was checked by reveman@chromium.org

[reveman, 2015-05-12 21:55:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
dcastagna@chromium.org, hendrikw@chromium.org, danakj@chromium.org,
wfh@chromium.org
Link to the patchset: https://codereview.chromium.org/1131253003/#ps100001
(title: "s/jamesr/sky/")

[commit-bot: I haz the power, 2015-05-12 21:56:08]

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

[commit-bot: I haz the power, 2015-05-12 23:02:01]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-05-12 23:03:01]

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