gpu: fix CopyTextureCHROMIUM for immutable texture.

gpu: fix CopyTextureCHROMIUM for immutable texture.

We cannot use glCopyTexImage2D on the texture which is allocated by glTexStorage2D [1]
[1] https://www.opengl.org/registry/specs/ARB/texture_storage.txt
The fast path uses glCopyTexSubImage2D instead of glCopyTexImage2D.

In addition, optimize CopyTextureCHROMIUM for GL_TEXTURE_RECTANGLE_ARB,
which is often used by the compositor, so make CopyTextureCHROMIUM use
the fast path.

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

[dshwang, 2014-11-20 14:59:01]

dongseong.hwang@intel.com changed reviewers:
+ sievers@chromium.org

[dshwang, 2014-11-20 14:59:40]

could you review?

one-copy uses CopyTextureCHROMIUM in ResourceProvider::CopyResource and one-copy
uses GL_TEXTURE_RECTANGLE_ARB. Refer to
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...

https://codereview.chromium.org/744713002/diff/1/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right):

https://codereview.chromium.org/744713002/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:226: source_target
== GL_TEXTURE_RECTANGLE_ARB);
already check GL_TEXTURE_RECTANGLE_ARB

[dshwang, 2014-11-20 15:23:07]

https://codereview.chromium.org/744713002/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (left):

https://codereview.chromium.org/744713002/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:336: // so
restrict this to GL_TEXTURE_2D.
ARB_texture_rectangle spec[1] don't mention about FBO.
I tests it using mesa egl driver, and work well on GLES3.

[1] https://www.opengl.org/registry/specs/ARB/texture_rectangle.txt

[no sievers, 2014-11-20 20:47:21]

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

[no sievers, 2014-11-20 20:49:42]

Good catch.

+reveman to double-check but both changes seem ok to me.

Mind adding a test to gl_copy_texture_CHROMIUM_unittest.cc for at least the
first issue (texture allocated with glTexStorage2D())?

[dshwang, 2014-11-20 21:43:38]

On 2014/11/20 20:49:42, sievers wrote:
> Good catch.
> 
> +reveman to double-check but both changes seem ok to me.

Yes, thank you.

> Mind adding a test to gl_copy_texture_CHROMIUM_unittest.cc for at least the
> first issue (texture allocated with glTexStorage2D())?

I tried to add new test for glTexStorage2D but I failed because we cannot use gl
extentions in unittests.
To use extentions in unittests requires huge code change. e.g. GLManager should
call FeatureInfo::InitializeFeatures() which requires real gl context.

[no sievers, 2014-11-20 21:45:22]

On 2014/11/20 21:43:38, dshwang wrote:
> On 2014/11/20 20:49:42, sievers wrote:
> > Good catch.
> > 
> > +reveman to double-check but both changes seem ok to me.
> 
> Yes, thank you.
> 
> > Mind adding a test to gl_copy_texture_CHROMIUM_unittest.cc for at least the
> > first issue (texture allocated with glTexStorage2D())?
> 
> I tried to add new test for glTexStorage2D but I failed because we cannot use
gl
> extentions in unittests.
> To use extentions in unittests requires huge code change. e.g. GLManager
should
> call FeatureInfo::InitializeFeatures() which requires real gl context.

gl_tests uses real GL. And there are existing tests that use extensions, for
example:

TEST_F(GLChromiumFramebufferMultisampleTest, DrawAndResolve) {
  if (!GLTestHelper::HasExtension("GL_CHROMIUM_framebuffer_multisample")) {
    return;
  }

[reveman, 2014-11-20 22:16:11]

lgtm % test and sievers review

[dshwang, 2014-11-21 10:26:28]

On 2014/11/20 22:16:11, reveman wrote:
> lgtm % test and sievers review

Thank you for reviewing.

On 2014/11/20 21:45:22, sievers wrote:
> gl_tests uses real GL. And there are existing tests that use extensions, for
> example:
> 
> TEST_F(GLChromiumFramebufferMultisampleTest, DrawAndResolve) {
>   if (!GLTestHelper::HasExtension("GL_CHROMIUM_framebuffer_multisample")) {
>     return;
>   }

Thank you for explaining. I add new unittests in Patch Set 3.
Could you review again?

[no sievers, 2014-11-21 18:39:49]

lgtm

https://codereview.chromium.org/744713002/diff/40001/gpu/command_buffer/tests...
File gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc (right):

https://codereview.chromium.org/744713002/diff/40001/gpu/command_buffer/tests...
gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:80: if
(!GLTestHelper::HasExtension("GL_EXT_texture_storage")) {
nit: can you put LOG(INFO) << "GL_EXT_texture_storage not supported. Skipping
test..."?

[dshwang, 2014-11-21 18:42:48]

On 2014/11/21 18:39:49, sievers wrote:
> lgtm

Thank you for reviewing!

> 
>
https://codereview.chromium.org/744713002/diff/40001/gpu/command_buffer/tests...
> File gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc (right):
> 
>
https://codereview.chromium.org/744713002/diff/40001/gpu/command_buffer/tests...
> gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:80: if
> (!GLTestHelper::HasExtension("GL_EXT_texture_storage")) {
> nit: can you put LOG(INFO) << "GL_EXT_texture_storage not supported. Skipping
> test..."?

Yes, Done.

[dshwang, 2014-11-21 18:42:53]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2014-11-21 18:44:12]

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

[commit-bot: I haz the power, 2014-11-21 19:54:48]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-11-21 19:55:34]

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

[Ken Russell (switch to Gerrit), 2014-11-27 00:48:14]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/760173002/ by kbr@chromium.org.

The reason for reverting is: Broke video uploads to WebGL on official builds
(taking the hardware accelerated video decode path). See Issue 436538.
.