Add OES_texture_half_float extension support in Chromium for desktop GL implementation.

Add OES_texture_half_float extension support in Chromium for desktop GL implementation.

BUG=235031

[Jun Jiang, 2013-04-22 09:25:17]

This patch is to work together with https://codereview.chromium.org/13842017/ to
enable OES_texture_half_float extension support in Chromium for desktop GL
implementation.

[greggman, 2013-04-22 18:37:01]

On 2013/04/22 09:25:17, Jun Jiang wrote:
> This patch is to work together with https://codereview.chromium.org/13842017/
to
> enable OES_texture_half_float extension support in Chromium for desktop GL
> implementation.

Does 'type' in DoTexSubImage2D also need to call this function?

[Jun Jiang, 2013-04-23 01:52:44]

On 2013/04/22 18:37:01, greggman wrote:
> On 2013/04/22 09:25:17, Jun Jiang wrote:
> > This patch is to work together with
https://codereview.chromium.org/13842017/
> to
> > enable OES_texture_half_float extension support in Chromium for desktop GL
> > implementation.
> 
> Does 'type' in DoTexSubImage2D also need to call this function?

Hi, Gregg. Thanks for your comments.
Yes, DoTexSubImage2D also needs to call this function. Currently,
teximage2d_faster_than_texsubimage2d_ is true for Non-Angle port, and
DoTexSubImage2D will be routed to WrappedTexImage2D which calls GetTexType(). So
the tests can pass in my Linux environment. For Angle port or those where
teximage2d_faster_than_texsubimage2d_ is false, GetTexType() should be called in
the subImage2D path. I will change it accordingly.

[Jun Jiang, 2013-04-23 04:27:06]

On 2013/04/23 01:52:44, Jun Jiang wrote:
> On 2013/04/22 18:37:01, greggman wrote:
> > On 2013/04/22 09:25:17, Jun Jiang wrote:
> > > This patch is to work together with
> https://codereview.chromium.org/13842017/
> > to
> > > enable OES_texture_half_float extension support in Chromium for desktop GL
> > > implementation.
> > 
> > Does 'type' in DoTexSubImage2D also need to call this function?
> 
> Hi, Gregg. Thanks for your comments.
> Yes, DoTexSubImage2D also needs to call this function. Currently,
> teximage2d_faster_than_texsubimage2d_ is true for Non-Angle port, and
> DoTexSubImage2D will be routed to WrappedTexImage2D which calls GetTexType().
So
> the tests can pass in my Linux environment. For Angle port or those where
> teximage2d_faster_than_texsubimage2d_ is false, GetTexType() should be called
in
> the subImage2D path. I will change it accordingly.

Done.

[Jun Jiang, 2013-04-23 06:03:48]

On 2013/04/23 04:27:06, Jun Jiang wrote:
> On 2013/04/23 01:52:44, Jun Jiang wrote:
> > On 2013/04/22 18:37:01, greggman wrote:
> > > On 2013/04/22 09:25:17, Jun Jiang wrote:
> > > > This patch is to work together with
> > https://codereview.chromium.org/13842017/
> > > to
> > > > enable OES_texture_half_float extension support in Chromium for desktop
GL
> > > > implementation.
> > > 
> > > Does 'type' in DoTexSubImage2D also need to call this function?
> > 
> > Hi, Gregg. Thanks for your comments.
> > Yes, DoTexSubImage2D also needs to call this function. Currently,
> > teximage2d_faster_than_texsubimage2d_ is true for Non-Angle port, and
> > DoTexSubImage2D will be routed to WrappedTexImage2D which calls
GetTexType().
> So
> > the tests can pass in my Linux environment. For Angle port or those where
> > teximage2d_faster_than_texsubimage2d_ is false, GetTexType() should be
called
> in
> > the subImage2D path. I will change it accordingly.
> 
> Done.

It seems the Trybot is still not available to non-committers.

[greggman, 2013-04-23 17:57:53]

On 2013/04/23 06:03:48, Jun Jiang wrote:
> On 2013/04/23 04:27:06, Jun Jiang wrote:
> > On 2013/04/23 01:52:44, Jun Jiang wrote:
> > > On 2013/04/22 18:37:01, greggman wrote:
> > > > On 2013/04/22 09:25:17, Jun Jiang wrote:
> > > > > This patch is to work together with
> > > https://codereview.chromium.org/13842017/
> > > > to
> > > > > enable OES_texture_half_float extension support in Chromium for
desktop
> GL
> > > > > implementation.
> > > > 
> > > > Does 'type' in DoTexSubImage2D also need to call this function?
> > > 
> > > Hi, Gregg. Thanks for your comments.
> > > Yes, DoTexSubImage2D also needs to call this function. Currently,
> > > teximage2d_faster_than_texsubimage2d_ is true for Non-Angle port, and
> > > DoTexSubImage2D will be routed to WrappedTexImage2D which calls
> GetTexType().
> > So
> > > the tests can pass in my Linux environment. For Angle port or those where
> > > teximage2d_faster_than_texsubimage2d_ is false, GetTexType() should be
> called
> > in
> > > the subImage2D path. I will change it accordingly.
> > 
> > Done.
> 
> It seems the Trybot is still not available to non-committers.

One more thing. Do you mind adding a test for this?  Either a test in
gles2_cmd_decoder_unittest.cc or a test in gpu/command_buffer/tests so we don't
regress?

[greggman, 2013-04-23 18:01:13]

On 2013/04/23 17:57:53, greggman wrote:
> On 2013/04/23 06:03:48, Jun Jiang wrote:
> > On 2013/04/23 04:27:06, Jun Jiang wrote:
> > > On 2013/04/23 01:52:44, Jun Jiang wrote:
> > > > On 2013/04/22 18:37:01, greggman wrote:
> > > > > On 2013/04/22 09:25:17, Jun Jiang wrote:
> > > > > > This patch is to work together with
> > > > https://codereview.chromium.org/13842017/
> > > > > to
> > > > > > enable OES_texture_half_float extension support in Chromium for
> desktop
> > GL
> > > > > > implementation.
> > > > > 
> > > > > Does 'type' in DoTexSubImage2D also need to call this function?
> > > > 
> > > > Hi, Gregg. Thanks for your comments.
> > > > Yes, DoTexSubImage2D also needs to call this function. Currently,
> > > > teximage2d_faster_than_texsubimage2d_ is true for Non-Angle port, and
> > > > DoTexSubImage2D will be routed to WrappedTexImage2D which calls
> > GetTexType().
> > > So
> > > > the tests can pass in my Linux environment. For Angle port or those
where
> > > > teximage2d_faster_than_texsubimage2d_ is false, GetTexType() should be
> > called
> > > in
> > > > the subImage2D path. I will change it accordingly.
> > > 
> > > Done.
> > 
> > It seems the Trybot is still not available to non-committers.
> 
> One more thing. Do you mind adding a test for this?  Either a test in
> gles2_cmd_decoder_unittest.cc or a test in gpu/command_buffer/tests so we
don't
> regress?

Oh, I also just remembered you'll need to add similar calls to
HandleAsyncTexImage2DCHROMIUM and HandleAsyncTexSubImage2DCHROMIUM

[Ken Russell (switch to Gerrit), 2013-04-24 02:31:45]

Drive-by comment: please file bugs for these CLs and reference them. Otherwise
the CLs can't be found easily later.

Looking forward to seeing this patch land.

[Jun Jiang, 2013-04-24 02:44:08]

On 2013/04/24 02:31:45, kbr wrote:
> Drive-by comment: please file bugs for these CLs and reference them. Otherwise
> the CLs can't be found easily later.
> 
> Looking forward to seeing this patch land.

Hi, Kenneth.
Thanks for your comments. I will open a bug for this to track the CL.

[Jun Jiang, 2013-04-24 15:48:54]

update the BUG= to BUG=235035.

[Jun Jiang, 2013-04-24 15:50:09]

On 2013/04/23 18:01:13, greggman wrote:
> On 2013/04/23 17:57:53, greggman wrote:
> > On 2013/04/23 06:03:48, Jun Jiang wrote:
> > > On 2013/04/23 04:27:06, Jun Jiang wrote:
> > > > On 2013/04/23 01:52:44, Jun Jiang wrote:
> > > > > On 2013/04/22 18:37:01, greggman wrote:
> > > > > > On 2013/04/22 09:25:17, Jun Jiang wrote:
> > > > > > > This patch is to work together with
> > > > > https://codereview.chromium.org/13842017/
> > > > > > to
> > > > > > > enable OES_texture_half_float extension support in Chromium for
> > desktop
> > > GL
> > > > > > > implementation.
> > > > > > 
> > > > > > Does 'type' in DoTexSubImage2D also need to call this function?
> > > > > 
> > > > > Hi, Gregg. Thanks for your comments.
> > > > > Yes, DoTexSubImage2D also needs to call this function. Currently,
> > > > > teximage2d_faster_than_texsubimage2d_ is true for Non-Angle port, and
> > > > > DoTexSubImage2D will be routed to WrappedTexImage2D which calls
> > > GetTexType().
> > > > So
> > > > > the tests can pass in my Linux environment. For Angle port or those
> where
> > > > > teximage2d_faster_than_texsubimage2d_ is false, GetTexType() should be
> > > called
> > > > in
> > > > > the subImage2D path. I will change it accordingly.
> > > > 
> > > > Done.
> > > 
> > > It seems the Trybot is still not available to non-committers.
> > 
> > One more thing. Do you mind adding a test for this?  Either a test in
> > gles2_cmd_decoder_unittest.cc or a test in gpu/command_buffer/tests so we
> don't
> > regress?
> 
> Oh, I also just remembered you'll need to add similar calls to
> HandleAsyncTexImage2DCHROMIUM and HandleAsyncTexSubImage2DCHROMIUM

Hi, Gregg. Thanks for your comments. The changes are done.

[greggman, 2013-04-24 16:00:57]

On 2013/04/24 15:50:09, Jun Jiang wrote:
> On 2013/04/23 18:01:13, greggman wrote:
> > On 2013/04/23 17:57:53, greggman wrote:
> > > On 2013/04/23 06:03:48, Jun Jiang wrote:
> > > > On 2013/04/23 04:27:06, Jun Jiang wrote:
> > > > > On 2013/04/23 01:52:44, Jun Jiang wrote:
> > > > > > On 2013/04/22 18:37:01, greggman wrote:
> > > > > > > On 2013/04/22 09:25:17, Jun Jiang wrote:
> > > > > > > > This patch is to work together with
> > > > > > https://codereview.chromium.org/13842017/
> > > > > > > to
> > > > > > > > enable OES_texture_half_float extension support in Chromium for
> > > desktop
> > > > GL
> > > > > > > > implementation.
> > > > > > > 
> > > > > > > Does 'type' in DoTexSubImage2D also need to call this function?
> > > > > > 
> > > > > > Hi, Gregg. Thanks for your comments.
> > > > > > Yes, DoTexSubImage2D also needs to call this function. Currently,
> > > > > > teximage2d_faster_than_texsubimage2d_ is true for Non-Angle port,
and
> > > > > > DoTexSubImage2D will be routed to WrappedTexImage2D which calls
> > > > GetTexType().
> > > > > So
> > > > > > the tests can pass in my Linux environment. For Angle port or those
> > where
> > > > > > teximage2d_faster_than_texsubimage2d_ is false, GetTexType() should
be
> > > > called
> > > > > in
> > > > > > the subImage2D path. I will change it accordingly.
> > > > > 
> > > > > Done.
> > > > 
> > > > It seems the Trybot is still not available to non-committers.
> > > 
> > > One more thing. Do you mind adding a test for this?  Either a test in
> > > gles2_cmd_decoder_unittest.cc or a test in gpu/command_buffer/tests so we
> > don't
> > > regress?
> > 
> > Oh, I also just remembered you'll need to add similar calls to
> > HandleAsyncTexImage2DCHROMIUM and HandleAsyncTexSubImage2DCHROMIUM
> 
> Hi, Gregg. Thanks for your comments. The changes are done.

lgtm

Wouldn't it be nice if the tests actually rendered something and checked the
results such that it would fail if HALF_FLOAT really isn't working. For example.
Make a 1 pixel texture with 0.25, 0.5, 2.0, 4.0 then render with

   gl_FragColor = texture2D(u_texture, vec2(0.5, 0.5)) * 
                  vec4(4.0, 2.0, 0.5, 0.25);

And expect reading back you get 255, 255, 255, 255 within some tolerance

[Jun Jiang, 2013-04-25 01:09:06]

On 2013/04/24 16:00:57, greggman wrote:
> On 2013/04/24 15:50:09, Jun Jiang wrote:
> > On 2013/04/23 18:01:13, greggman wrote:
> > > On 2013/04/23 17:57:53, greggman wrote:
> > > > On 2013/04/23 06:03:48, Jun Jiang wrote:
> > > > > On 2013/04/23 04:27:06, Jun Jiang wrote:
> > > > > > On 2013/04/23 01:52:44, Jun Jiang wrote:
> > > > > > > On 2013/04/22 18:37:01, greggman wrote:
> > > > > > > > On 2013/04/22 09:25:17, Jun Jiang wrote:
> > > > > > > > > This patch is to work together with
> > > > > > > https://codereview.chromium.org/13842017/
> > > > > > > > to
> > > > > > > > > enable OES_texture_half_float extension support in Chromium
for
> > > > desktop
> > > > > GL
> > > > > > > > > implementation.
> > > > > > > > 
> > > > > > > > Does 'type' in DoTexSubImage2D also need to call this function?
> > > > > > > 
> > > > > > > Hi, Gregg. Thanks for your comments.
> > > > > > > Yes, DoTexSubImage2D also needs to call this function. Currently,
> > > > > > > teximage2d_faster_than_texsubimage2d_ is true for Non-Angle port,
> and
> > > > > > > DoTexSubImage2D will be routed to WrappedTexImage2D which calls
> > > > > GetTexType().
> > > > > > So
> > > > > > > the tests can pass in my Linux environment. For Angle port or
those
> > > where
> > > > > > > teximage2d_faster_than_texsubimage2d_ is false, GetTexType()
should
> be
> > > > > called
> > > > > > in
> > > > > > > the subImage2D path. I will change it accordingly.
> > > > > > 
> > > > > > Done.
> > > > > 
> > > > > It seems the Trybot is still not available to non-committers.
> > > > 
> > > > One more thing. Do you mind adding a test for this?  Either a test in
> > > > gles2_cmd_decoder_unittest.cc or a test in gpu/command_buffer/tests so
we
> > > don't
> > > > regress?
> > > 
> > > Oh, I also just remembered you'll need to add similar calls to
> > > HandleAsyncTexImage2DCHROMIUM and HandleAsyncTexSubImage2DCHROMIUM
> > 
> > Hi, Gregg. Thanks for your comments. The changes are done.
> 
> lgtm
> 
> Wouldn't it be nice if the tests actually rendered something and checked the
> results such that it would fail if HALF_FLOAT really isn't working. For
example.
> Make a 1 pixel texture with 0.25, 0.5, 2.0, 4.0 then render with
> 
>    gl_FragColor = texture2D(u_texture, vec2(0.5, 0.5)) * 
>                   vec4(4.0, 2.0, 0.5, 0.25);
> 
> And expect reading back you get 255, 255, 255, 255 within some tolerance

Yes, it is better to do a true positive test to make sure HALF_FLOAT really
works. Will be updated.

[Jun Jiang, 2013-04-25 13:06:48]

On 2013/04/25 01:09:06, Jun Jiang wrote:
> On 2013/04/24 16:00:57, greggman wrote:
> > On 2013/04/24 15:50:09, Jun Jiang wrote:
> > > On 2013/04/23 18:01:13, greggman wrote:
> > > > On 2013/04/23 17:57:53, greggman wrote:
> > > > > On 2013/04/23 06:03:48, Jun Jiang wrote:
> > > > > > On 2013/04/23 04:27:06, Jun Jiang wrote:
> > > > > > > On 2013/04/23 01:52:44, Jun Jiang wrote:
> > > > > > > > On 2013/04/22 18:37:01, greggman wrote:
> > > > > > > > > On 2013/04/22 09:25:17, Jun Jiang wrote:
> > > > > > > > > > This patch is to work together with
> > > > > > > > https://codereview.chromium.org/13842017/
> > > > > > > > > to
> > > > > > > > > > enable OES_texture_half_float extension support in Chromium
> for
> > > > > desktop
> > > > > > GL
> > > > > > > > > > implementation.
> > > > > > > > > 
> > > > > > > > > Does 'type' in DoTexSubImage2D also need to call this
function?
> > > > > > > > 
> > > > > > > > Hi, Gregg. Thanks for your comments.
> > > > > > > > Yes, DoTexSubImage2D also needs to call this function.
Currently,
> > > > > > > > teximage2d_faster_than_texsubimage2d_ is true for Non-Angle
port,
> > and
> > > > > > > > DoTexSubImage2D will be routed to WrappedTexImage2D which calls
> > > > > > GetTexType().
> > > > > > > So
> > > > > > > > the tests can pass in my Linux environment. For Angle port or
> those
> > > > where
> > > > > > > > teximage2d_faster_than_texsubimage2d_ is false, GetTexType()
> should
> > be
> > > > > > called
> > > > > > > in
> > > > > > > > the subImage2D path. I will change it accordingly.
> > > > > > > 
> > > > > > > Done.
> > > > > > 
> > > > > > It seems the Trybot is still not available to non-committers.
> > > > > 
> > > > > One more thing. Do you mind adding a test for this?  Either a test in
> > > > > gles2_cmd_decoder_unittest.cc or a test in gpu/command_buffer/tests so
> we
> > > > don't
> > > > > regress?
> > > > 
> > > > Oh, I also just remembered you'll need to add similar calls to
> > > > HandleAsyncTexImage2DCHROMIUM and HandleAsyncTexSubImage2DCHROMIUM
> > > 
> > > Hi, Gregg. Thanks for your comments. The changes are done.
> > 
> > lgtm
> > 
> > Wouldn't it be nice if the tests actually rendered something and checked the
> > results such that it would fail if HALF_FLOAT really isn't working. For
> example.
> > Make a 1 pixel texture with 0.25, 0.5, 2.0, 4.0 then render with
> > 
> >    gl_FragColor = texture2D(u_texture, vec2(0.5, 0.5)) * 
> >                   vec4(4.0, 2.0, 0.5, 0.25);
> > 
> > And expect reading back you get 255, 255, 255, 255 within some tolerance
> 
> Yes, it is better to do a true positive test to make sure HALF_FLOAT really
> works. Will be updated.

Read the pixel value(RGBA, unsigned byte) out of the uploaded pixels(RGBA, half
float) and do a comparison to check if OES_HALF_FLOAT extension works.

[Jun Jiang, 2013-04-29 06:39:01]

On 2013/04/25 13:06:48, Jun Jiang wrote:
> On 2013/04/25 01:09:06, Jun Jiang wrote:
> > On 2013/04/24 16:00:57, greggman wrote:
> > > On 2013/04/24 15:50:09, Jun Jiang wrote:
> > > > On 2013/04/23 18:01:13, greggman wrote:
> > > > > On 2013/04/23 17:57:53, greggman wrote:
> > > > > > On 2013/04/23 06:03:48, Jun Jiang wrote:
> > > > > > > On 2013/04/23 04:27:06, Jun Jiang wrote:
> > > > > > > > On 2013/04/23 01:52:44, Jun Jiang wrote:
> > > > > > > > > On 2013/04/22 18:37:01, greggman wrote:
> > > > > > > > > > On 2013/04/22 09:25:17, Jun Jiang wrote:
> > > > > > > > > > > This patch is to work together with
> > > > > > > > > https://codereview.chromium.org/13842017/
> > > > > > > > > > to
> > > > > > > > > > > enable OES_texture_half_float extension support in
Chromium
> > for
> > > > > > desktop
> > > > > > > GL
> > > > > > > > > > > implementation.
> > > > > > > > > > 
> > > > > > > > > > Does 'type' in DoTexSubImage2D also need to call this
> function?
> > > > > > > > > 
> > > > > > > > > Hi, Gregg. Thanks for your comments.
> > > > > > > > > Yes, DoTexSubImage2D also needs to call this function.
> Currently,
> > > > > > > > > teximage2d_faster_than_texsubimage2d_ is true for Non-Angle
> port,
> > > and
> > > > > > > > > DoTexSubImage2D will be routed to WrappedTexImage2D which
calls
> > > > > > > GetTexType().
> > > > > > > > So
> > > > > > > > > the tests can pass in my Linux environment. For Angle port or
> > those
> > > > > where
> > > > > > > > > teximage2d_faster_than_texsubimage2d_ is false, GetTexType()
> > should
> > > be
> > > > > > > called
> > > > > > > > in
> > > > > > > > > the subImage2D path. I will change it accordingly.
> > > > > > > > 
> > > > > > > > Done.
> > > > > > > 
> > > > > > > It seems the Trybot is still not available to non-committers.
> > > > > > 
> > > > > > One more thing. Do you mind adding a test for this?  Either a test
in
> > > > > > gles2_cmd_decoder_unittest.cc or a test in gpu/command_buffer/tests
so
> > we
> > > > > don't
> > > > > > regress?
> > > > > 
> > > > > Oh, I also just remembered you'll need to add similar calls to
> > > > > HandleAsyncTexImage2DCHROMIUM and HandleAsyncTexSubImage2DCHROMIUM
> > > > 
> > > > Hi, Gregg. Thanks for your comments. The changes are done.
> > > 
> > > lgtm
> > > 
> > > Wouldn't it be nice if the tests actually rendered something and checked
the
> > > results such that it would fail if HALF_FLOAT really isn't working. For
> > example.
> > > Make a 1 pixel texture with 0.25, 0.5, 2.0, 4.0 then render with
> > > 
> > >    gl_FragColor = texture2D(u_texture, vec2(0.5, 0.5)) * 
> > >                   vec4(4.0, 2.0, 0.5, 0.25);
> > > 
> > > And expect reading back you get 255, 255, 255, 255 within some tolerance
> > 
> > Yes, it is better to do a true positive test to make sure HALF_FLOAT really
> > works. Will be updated.
> 
> Read the pixel value(RGBA, unsigned byte) out of the uploaded pixels(RGBA,
half
> float) and do a comparison to check if OES_HALF_FLOAT extension works.

It seems my added tests in gl_tests fail for win_gpu build. And failures for
other trybots seem not related with my patch. Will check the failure of added
tests for win_gpu.

[Jun Jiang, 2013-05-02 10:36:18]

On 2013/04/29 06:39:01, Jun Jiang wrote:
> On 2013/04/25 13:06:48, Jun Jiang wrote:
> > On 2013/04/25 01:09:06, Jun Jiang wrote:
> > > On 2013/04/24 16:00:57, greggman wrote:
> > > > On 2013/04/24 15:50:09, Jun Jiang wrote:
> > > > > On 2013/04/23 18:01:13, greggman wrote:
> > > > > > On 2013/04/23 17:57:53, greggman wrote:
> > > > > > > On 2013/04/23 06:03:48, Jun Jiang wrote:
> > > > > > > > On 2013/04/23 04:27:06, Jun Jiang wrote:
> > > > > > > > > On 2013/04/23 01:52:44, Jun Jiang wrote:
> > > > > > > > > > On 2013/04/22 18:37:01, greggman wrote:
> > > > > > > > > > > On 2013/04/22 09:25:17, Jun Jiang wrote:
> > > > > > > > > > > > This patch is to work together with
> > > > > > > > > > https://codereview.chromium.org/13842017/
> > > > > > > > > > > to
> > > > > > > > > > > > enable OES_texture_half_float extension support in
> Chromium
> > > for
> > > > > > > desktop
> > > > > > > > GL
> > > > > > > > > > > > implementation.
> > > > > > > > > > > 
> > > > > > > > > > > Does 'type' in DoTexSubImage2D also need to call this
> > function?
> > > > > > > > > > 
> > > > > > > > > > Hi, Gregg. Thanks for your comments.
> > > > > > > > > > Yes, DoTexSubImage2D also needs to call this function.
> > Currently,
> > > > > > > > > > teximage2d_faster_than_texsubimage2d_ is true for Non-Angle
> > port,
> > > > and
> > > > > > > > > > DoTexSubImage2D will be routed to WrappedTexImage2D which
> calls
> > > > > > > > GetTexType().
> > > > > > > > > So
> > > > > > > > > > the tests can pass in my Linux environment. For Angle port
or
> > > those
> > > > > > where
> > > > > > > > > > teximage2d_faster_than_texsubimage2d_ is false, GetTexType()
> > > should
> > > > be
> > > > > > > > called
> > > > > > > > > in
> > > > > > > > > > the subImage2D path. I will change it accordingly.
> > > > > > > > > 
> > > > > > > > > Done.
> > > > > > > > 
> > > > > > > > It seems the Trybot is still not available to non-committers.
> > > > > > > 
> > > > > > > One more thing. Do you mind adding a test for this?  Either a test
> in
> > > > > > > gles2_cmd_decoder_unittest.cc or a test in
gpu/command_buffer/tests
> so
> > > we
> > > > > > don't
> > > > > > > regress?
> > > > > > 
> > > > > > Oh, I also just remembered you'll need to add similar calls to
> > > > > > HandleAsyncTexImage2DCHROMIUM and HandleAsyncTexSubImage2DCHROMIUM
> > > > > 
> > > > > Hi, Gregg. Thanks for your comments. The changes are done.
> > > > 
> > > > lgtm
> > > > 
> > > > Wouldn't it be nice if the tests actually rendered something and checked
> the
> > > > results such that it would fail if HALF_FLOAT really isn't working. For
> > > example.
> > > > Make a 1 pixel texture with 0.25, 0.5, 2.0, 4.0 then render with
> > > > 
> > > >    gl_FragColor = texture2D(u_texture, vec2(0.5, 0.5)) * 
> > > >                   vec4(4.0, 2.0, 0.5, 0.25);
> > > > 
> > > > And expect reading back you get 255, 255, 255, 255 within some tolerance
> > > 
> > > Yes, it is better to do a true positive test to make sure HALF_FLOAT
really
> > > works. Will be updated.
> > 
> > Read the pixel value(RGBA, unsigned byte) out of the uploaded pixels(RGBA,
> half
> > float) and do a comparison to check if OES_HALF_FLOAT extension works.
> 
> It seems my added tests in gl_tests fail for win_gpu build. And failures for
> other trybots seem not related with my patch. Will check the failure of added
> tests for win_gpu.

Hi, Gregg. I had add supports for glReadPixels and updated the test cases. And
the test cases have been verified on Mac, Linux and Win. Please help to have a
review. Thanks.

[greggman, 2013-05-02 16:37:25]

re: my comment about ReadPixels not support HALF_FLOAT

There are some bugs we need to fix

#1) feature_info.cc should not be adding HALF_FLOAT to the ReadPixelType nor to
FLOAT to ReadPixelType

#2) build_gles2_cmd_buffer.py should not have GL_UNSIGNED_SHORT_5_6_5, 5_5_5_1
and 4_4_4_4 in ReadPixelType

Explanation: The way it works in GL ES is you query,
GL_IMPLEMENTATION_COLOR_READ_TYPE , and GL_IMPLEMENTATION_COLOR_READ_FORMAT to
find out what format the driver supports for the current FB for ReadPixels

If you look in GLES2DecoderImpl::GetHelper you see we only support
GL_RGBA/GL_UNSIGNED_BYTE  (we make an assumption that all drivers we run on
support this)

I suppose the long term version would have to query which formats are actually
supported for which FB formats but that's too much work for now so rather let's
just keep it GL_RGBA/GL_UNSIGNED_BYTE only

https://codereview.chromium.org/14402003/diff/32001/gpu/command_buffer/tests/...
File gpu/command_buffer/tests/gl_oes_texture_half_float_unittest.cc (right):

https://codereview.chromium.org/14402003/diff/32001/gpu/command_buffer/tests/...
gpu/command_buffer/tests/gl_oes_texture_half_float_unittest.cc:22: attribute
vec4 v_position;
nit: a_ for attribute, v_ for varying?

https://codereview.chromium.org/14402003/diff/32001/gpu/command_buffer/tests/...
gpu/command_buffer/tests/gl_oes_texture_half_float_unittest.cc:52: textures[1],
0);
nit: indenting

https://codereview.chromium.org/14402003/diff/32001/gpu/command_buffer/tests/...
gpu/command_buffer/tests/gl_oes_texture_half_float_unittest.cc:68: GLuint
GLOesTextureHalfFloatTest::SetupUnitQuad(GLint position_location) {
Can you just use GLTestHelper::SetupUnitQuad?

https://codereview.chromium.org/14402003/diff/32001/gpu/command_buffer/tests/...
gpu/command_buffer/tests/gl_oes_texture_half_float_unittest.cc:93: int16
pixels[1 * 4] = { 0x3400, 0x3800, 0x4000, 0x4400 };
static?

https://codereview.chromium.org/14402003/diff/32001/gpu/command_buffer/tests/...
gpu/command_buffer/tests/gl_oes_texture_half_float_unittest.cc:141: 0, 0, 1, 1,
GL_RGBA, GL_HALF_FLOAT_OES,
AFAIK in GL ES 2.0 ReadPixels does not have to support GL_HALF_FLOAT_OES. In
fact I believe we don't allow anything but GL_RGBA, GL_UNSIGNED_BYTE so the
final result needs to be GL_RGBA, GL_UNSIGNED_BYTE in which case you can just
use GLTestHelper::CheckRect

https://codereview.chromium.org/14402003/diff/32001/gpu/command_buffer/tests/...
gpu/command_buffer/tests/gl_oes_texture_half_float_unittest.cc:174: int16
pixels[1 * 4] = { 0x3400, 0x3800, 0x4000, 0x4400 };
static?

https://codereview.chromium.org/14402003/diff/32001/gpu/command_buffer/tests/...
gpu/command_buffer/tests/gl_oes_texture_half_float_unittest.cc:181: uint8
actual_pixels[1 * 4] = { 0 };
use GLTestHelper::CheckRect?

https://codereview.chromium.org/14402003/diff/32001/gpu/command_buffer/tests/...
gpu/command_buffer/tests/gl_oes_texture_half_float_unittest.cc:200: for
(unsigned int i = 0; i < 4; i++) {
use GLTestHelper::CheckRect?

[Jun Jiang, 2013-05-03 02:15:16]

On 2013/05/02 16:37:25, greggman wrote:
> re: my comment about ReadPixels not support HALF_FLOAT
> 
> There are some bugs we need to fix
> 
> #1) feature_info.cc should not be adding HALF_FLOAT to the ReadPixelType nor
to
> FLOAT to ReadPixelType
> 
> #2) build_gles2_cmd_buffer.py should not have GL_UNSIGNED_SHORT_5_6_5, 5_5_5_1
> and 4_4_4_4 in ReadPixelType
> 
> Explanation: The way it works in GL ES is you query,
> GL_IMPLEMENTATION_COLOR_READ_TYPE , and GL_IMPLEMENTATION_COLOR_READ_FORMAT to
> find out what format the driver supports for the current FB for ReadPixels
> 
> If you look in GLES2DecoderImpl::GetHelper you see we only support
> GL_RGBA/GL_UNSIGNED_BYTE  (we make an assumption that all drivers we run on
> support this)
> 
> I suppose the long term version would have to query which formats are actually
> supported for which FB formats but that's too much work for now so rather
let's
> just keep it GL_RGBA/GL_UNSIGNED_BYTE only
> 
>
https://codereview.chromium.org/14402003/diff/32001/gpu/command_buffer/tests/...
> File gpu/command_buffer/tests/gl_oes_texture_half_float_unittest.cc (right):
> 
>
https://codereview.chromium.org/14402003/diff/32001/gpu/command_buffer/tests/...
> gpu/command_buffer/tests/gl_oes_texture_half_float_unittest.cc:22: attribute
> vec4 v_position;
> nit: a_ for attribute, v_ for varying?
> 
>
https://codereview.chromium.org/14402003/diff/32001/gpu/command_buffer/tests/...
> gpu/command_buffer/tests/gl_oes_texture_half_float_unittest.cc:52:
textures[1],
> 0);
> nit: indenting
> 
>
https://codereview.chromium.org/14402003/diff/32001/gpu/command_buffer/tests/...
> gpu/command_buffer/tests/gl_oes_texture_half_float_unittest.cc:68: GLuint
> GLOesTextureHalfFloatTest::SetupUnitQuad(GLint position_location) {
> Can you just use GLTestHelper::SetupUnitQuad?
> 
>
https://codereview.chromium.org/14402003/diff/32001/gpu/command_buffer/tests/...
> gpu/command_buffer/tests/gl_oes_texture_half_float_unittest.cc:93: int16
> pixels[1 * 4] = { 0x3400, 0x3800, 0x4000, 0x4400 };
> static?
> 
>
https://codereview.chromium.org/14402003/diff/32001/gpu/command_buffer/tests/...
> gpu/command_buffer/tests/gl_oes_texture_half_float_unittest.cc:141: 0, 0, 1,
1,
> GL_RGBA, GL_HALF_FLOAT_OES,
> AFAIK in GL ES 2.0 ReadPixels does not have to support GL_HALF_FLOAT_OES. In
> fact I believe we don't allow anything but GL_RGBA, GL_UNSIGNED_BYTE so the
> final result needs to be GL_RGBA, GL_UNSIGNED_BYTE in which case you can just
> use GLTestHelper::CheckRect
> 
>
https://codereview.chromium.org/14402003/diff/32001/gpu/command_buffer/tests/...
> gpu/command_buffer/tests/gl_oes_texture_half_float_unittest.cc:174: int16
> pixels[1 * 4] = { 0x3400, 0x3800, 0x4000, 0x4400 };
> static?
> 
>
https://codereview.chromium.org/14402003/diff/32001/gpu/command_buffer/tests/...
> gpu/command_buffer/tests/gl_oes_texture_half_float_unittest.cc:181: uint8
> actual_pixels[1 * 4] = { 0 };
> use GLTestHelper::CheckRect?
> 
>
https://codereview.chromium.org/14402003/diff/32001/gpu/command_buffer/tests/...
> gpu/command_buffer/tests/gl_oes_texture_half_float_unittest.cc:200: for
> (unsigned int i = 0; i < 4; i++) {
> use GLTestHelper::CheckRect?

Hi, Gregg. Thanks for your details on not to support HALF_FLOAT for
glReadPixels. I agree with you to use GL_IMPLEMENTATION_COLOR_READ_TYPE and
GL_IMPLEMENTATION_COLOR_READ_FORMAT to
find out what format the driver supports for the current FB for ReadPixels in
long term and at present keep to restrict to GL_RGBA/GL_UNSIGNED_BYTE only. I
will update the code and test cases accordingly.

[Jun Jiang, 2013-05-03 04:55:16]

On 2013/05/03 02:15:16, Jun Jiang wrote:
> On 2013/05/02 16:37:25, greggman wrote:
> > re: my comment about ReadPixels not support HALF_FLOAT
> > 
> > There are some bugs we need to fix
> > 
> > #1) feature_info.cc should not be adding HALF_FLOAT to the ReadPixelType nor
> to
> > FLOAT to ReadPixelType
> > 
> > #2) build_gles2_cmd_buffer.py should not have GL_UNSIGNED_SHORT_5_6_5,
5_5_5_1
> > and 4_4_4_4 in ReadPixelType
> > 
> > Explanation: The way it works in GL ES is you query,
> > GL_IMPLEMENTATION_COLOR_READ_TYPE , and GL_IMPLEMENTATION_COLOR_READ_FORMAT
to
> > find out what format the driver supports for the current FB for ReadPixels
> > 
> > If you look in GLES2DecoderImpl::GetHelper you see we only support
> > GL_RGBA/GL_UNSIGNED_BYTE  (we make an assumption that all drivers we run on
> > support this)
> > 
> > I suppose the long term version would have to query which formats are
actually
> > supported for which FB formats but that's too much work for now so rather
> let's
> > just keep it GL_RGBA/GL_UNSIGNED_BYTE only
> > 
> >
>
https://codereview.chromium.org/14402003/diff/32001/gpu/command_buffer/tests/...
> > File gpu/command_buffer/tests/gl_oes_texture_half_float_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/14402003/diff/32001/gpu/command_buffer/tests/...
> > gpu/command_buffer/tests/gl_oes_texture_half_float_unittest.cc:22: attribute
> > vec4 v_position;
> > nit: a_ for attribute, v_ for varying?
> > 
> >
>
https://codereview.chromium.org/14402003/diff/32001/gpu/command_buffer/tests/...
> > gpu/command_buffer/tests/gl_oes_texture_half_float_unittest.cc:52:
> textures[1],
> > 0);
> > nit: indenting
> > 
> >
>
https://codereview.chromium.org/14402003/diff/32001/gpu/command_buffer/tests/...
> > gpu/command_buffer/tests/gl_oes_texture_half_float_unittest.cc:68: GLuint
> > GLOesTextureHalfFloatTest::SetupUnitQuad(GLint position_location) {
> > Can you just use GLTestHelper::SetupUnitQuad?
> > 
> >
>
https://codereview.chromium.org/14402003/diff/32001/gpu/command_buffer/tests/...
> > gpu/command_buffer/tests/gl_oes_texture_half_float_unittest.cc:93: int16
> > pixels[1 * 4] = { 0x3400, 0x3800, 0x4000, 0x4400 };
> > static?
> > 
> >
>
https://codereview.chromium.org/14402003/diff/32001/gpu/command_buffer/tests/...
> > gpu/command_buffer/tests/gl_oes_texture_half_float_unittest.cc:141: 0, 0, 1,
> 1,
> > GL_RGBA, GL_HALF_FLOAT_OES,
> > AFAIK in GL ES 2.0 ReadPixels does not have to support GL_HALF_FLOAT_OES. In
> > fact I believe we don't allow anything but GL_RGBA, GL_UNSIGNED_BYTE so the
> > final result needs to be GL_RGBA, GL_UNSIGNED_BYTE in which case you can
just
> > use GLTestHelper::CheckRect
> > 
> >
>
https://codereview.chromium.org/14402003/diff/32001/gpu/command_buffer/tests/...
> > gpu/command_buffer/tests/gl_oes_texture_half_float_unittest.cc:174: int16
> > pixels[1 * 4] = { 0x3400, 0x3800, 0x4000, 0x4400 };
> > static?
> > 
> >
>
https://codereview.chromium.org/14402003/diff/32001/gpu/command_buffer/tests/...
> > gpu/command_buffer/tests/gl_oes_texture_half_float_unittest.cc:181: uint8
> > actual_pixels[1 * 4] = { 0 };
> > use GLTestHelper::CheckRect?
> > 
> >
>
https://codereview.chromium.org/14402003/diff/32001/gpu/command_buffer/tests/...
> > gpu/command_buffer/tests/gl_oes_texture_half_float_unittest.cc:200: for
> > (unsigned int i = 0; i < 4; i++) {
> > use GLTestHelper::CheckRect?
> 
> Hi, Gregg. Thanks for your details on not to support HALF_FLOAT for
> glReadPixels. I agree with you to use GL_IMPLEMENTATION_COLOR_READ_TYPE and
> GL_IMPLEMENTATION_COLOR_READ_FORMAT to
> find out what format the driver supports for the current FB for ReadPixels in
> long term and at present keep to restrict to GL_RGBA/GL_UNSIGNED_BYTE only. I
> will update the code and test cases accordingly.

Done.

[greggman, 2013-05-03 06:26:21]

https://codereview.chromium.org/14402003/diff/41001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/14402003/diff/41001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10297: GLenum gl_type =
GetTexType(type);
Hey Jun, Sorry about this but I just remembered that converting gl_type here
won't work because it's used later to call texture->SetLevelInfo and that wants
the original value.

As I was trying to think of solutions I realized the best way would be to get
rid of WrappedTexImage2D and WrappeTexSubImage2D and move that stuff to ui/gl

I was about to explain it but I realized it was quicker to just do it so... that
CL is here

https://codereview.chromium.org/14902003

Once that's in you can check in the tests from this CL. The changes in this file
are not needed I think.

[Jun Jiang, 2013-05-03 06:46:50]

On 2013/05/03 06:26:21, greggman wrote:
>
https://codereview.chromium.org/14402003/diff/41001/gpu/command_buffer/servic...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> 
>
https://codereview.chromium.org/14402003/diff/41001/gpu/command_buffer/servic...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:10297: GLenum gl_type =
> GetTexType(type);
> Hey Jun, Sorry about this but I just remembered that converting gl_type here
> won't work because it's used later to call texture->SetLevelInfo and that
wants
> the original value.
> 
> As I was trying to think of solutions I realized the best way would be to get
> rid of WrappedTexImage2D and WrappeTexSubImage2D and move that stuff to ui/gl
> 
> I was about to explain it but I realized it was quicker to just do it so...
that
> CL is here
> 
> https://codereview.chromium.org/14902003
> 
> Once that's in you can check in the tests from this CL. The changes in this
file
> are not needed I think.

Hi, Gregg. I agree with you and it would be better to change it in ui/gl. Once
the patch on 14902003 landed, I will upload the test in this CL.

[Jun Jiang, 2013-05-15 07:40:57]

On 2013/05/03 06:46:50, Jun Jiang wrote:
> On 2013/05/03 06:26:21, greggman wrote:
> >
>
https://codereview.chromium.org/14402003/diff/41001/gpu/command_buffer/servic...
> > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> > 
> >
>
https://codereview.chromium.org/14402003/diff/41001/gpu/command_buffer/servic...
> > gpu/command_buffer/service/gles2_cmd_decoder.cc:10297: GLenum gl_type =
> > GetTexType(type);
> > Hey Jun, Sorry about this but I just remembered that converting gl_type here
> > won't work because it's used later to call texture->SetLevelInfo and that
> wants
> > the original value.
> > 
> > As I was trying to think of solutions I realized the best way would be to
get
> > rid of WrappedTexImage2D and WrappeTexSubImage2D and move that stuff to
ui/gl
> > 
> > I was about to explain it but I realized it was quicker to just do it so...
> that
> > CL is here
> > 
> > https://codereview.chromium.org/14902003
> > 
> > Once that's in you can check in the tests from this CL. The changes in this
> file
> > are not needed I think.
> 
> Hi, Gregg. I agree with you and it would be better to change it in ui/gl. Once
> the patch on 14902003 landed, I will upload the test in this CL.

The patch to add support for OES_HALF_FLOAT extension in ui/gl by Gregg has been
landed. The test for OES_HALF_FLOAT extension support is uploaded for review at
https://codereview.chromium.org/14770011/.
This issue can be closed now.