Reland of Revert of gpu: introduce glCopySubTextureCHROMIUM (patchset #1 id:1 of https://codereview…

Reland of Revert of gpu: introduce glCopySubTextureCHROMIUM (patchset #1 id:1 of https://codereview.chromium.org/972833004/)

Reason for revert:
Fix OSX failure. glValidateProgram() in OSX has very strange behavior. It validates FBO unexpectedly.
So glValidateProgram() falsely fails if FBO is not bound. Move the validation code after FBO binding.
In addition, previous patch hits one-copy performance badly because CopySubTextureCHROMIUM
didn't use image copy optimization like CopyTextureCHROMIUM. This CL applied it.

Original issue's description:
> Revert of gpu: introduce glCopySubTextureCHROMIUM (patchset #4 id:80001 of https://codereview.chromium.org/864513004/)
>
> Reason for revert:
> This broke telemetry_unittests and content_browsertests on Mac (see issue 463439)
>
> Original issue's description:
> > gpu: introduce glCopySubTextureCHROMIUM
> >
> > Introduce glCopySubTextureCHROMIUM to support immutable texture as well as to
> > optimize WebGL texSubImage2D(video | canvas).
> >
> > This CL changes gl_renderer to use glCopySubTextureCHROMIUM because the
> > destination texture is immutable.
> >
> > TEST=GLCopyTextureCHROMIUMTest
> > BUG=443151
> >
> > Committed: https://crrev.com/fac6a2d44323ad51429ee728e241fe22242f5ebd
> > Cr-Commit-Position: refs/heads/master@{#318855}
>
> TBR=sievers@chromium.org,piman@chromium.org,kbr@chromium.org,zmo@chromium.org,scherkus@chromium.org,dongseong.hwang@intel.com
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=443151
>
> Review URL: https://codereview.chromium.org/972833004
>
> Cr-Commit-Position: refs/heads/master@{#318858}

BUG=443151, 463439
Committed: https://crrev.com/46305b130beb809b98d02830c857dc3dfaf5b98b
Cr-Commit-Position: refs/heads/master@{#319288}

[dshwang, 2015-03-04 09:09:36]

Patchset #2 (id:20001) has been deleted

[dshwang, 2015-03-04 09:17:01]

dongseong.hwang@intel.com changed reviewers:
+ piman@chromium.org, scherkus@chromium.org, ulan@chromium.org

[dshwang, 2015-03-04 09:25:36]

Patchset #2 (id:40001) has been deleted

[dshwang, 2015-03-04 09:35:42]

@piman, could you review again?

https://codereview.chromium.org/977853002/diff/60001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right):

https://codereview.chromium.org/977853002/diff/60001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:559: //
glValidateProgram must be called after FBO binding. crbug.com/463439
Fix OSX failure. glValidateProgram() in OSX has very strange behavior. It
validates FBO unexpectedly.
So glValidateProgram() falsely fails if FBO is not bound. Move the validation
code after FBO binding.
This hidden bug is revealed by reverted patch. It means OSX never use this code
before the patch.
glCopyTextureCHROMIUM doesn't reach here because of image optimization. see
comment in gles2_cmd_decoder

https://codereview.chromium.org/977853002/diff/60001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/977853002/diff/60001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder.cc:11133: if
(image->CopyTexSubImage(GL_TEXTURE_2D, 0, 0))
we can use CopyTexSubImage in CopyTextureCHROMIUM because this method defines
dest texture if needed 20 lines before

https://codereview.chromium.org/977853002/diff/60001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder.cc:11272: if
(image->CopyTexSubImage(GL_TEXTURE_2D, xoffset, yoffset))
reverted patch hits one-copy performance badly because CopySubTextureCHROMIUM
didn't use image copy optimization like CopyTextureCHROMIUM. This CL applied it.

[dshwang, 2015-03-04 09:36:43]

@scherkus could you review media/ ?

The patch set 1 is same to reverted patch
And patch set 2 shows what I changed to reland.

[dshwang, 2015-03-04 09:39:36]

https://codereview.chromium.org/977853002/diff/60001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right):

https://codereview.chromium.org/977853002/diff/60001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:559: //
glValidateProgram must be called after FBO binding. crbug.com/463439
On 2015/03/04 09:35:42, dshwang wrote:
> Fix OSX failure. glValidateProgram() in OSX has very strange behavior. It
> validates FBO unexpectedly.
> So glValidateProgram() falsely fails if FBO is not bound. Move the validation
> code after FBO binding.
> This hidden bug is revealed by reverted patch. It means OSX never use this
code
> before the patch.
> glCopyTextureCHROMIUM doesn't reach here because of image optimization. see
> comment in gles2_cmd_decoder

many osx developers are confused,
http://stackoverflow.com/questions/15335510/opengl-glvalidateprogram-error-on...

[dshwang, 2015-03-04 11:19:39]

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

[scherkus (not reviewing), 2015-03-04 18:00:23]

lgtm (doesn't look like media/ changed at all)

[dshwang, 2015-03-04 18:09:35]

On 2015/03/04 18:00:23, scherkus wrote:
> lgtm (doesn't look like media/ changed at all)

Thx! yes, media/ is same to previous.

[piman, 2015-03-04 20:38:39]

LGTM+nit, but make sure you have the ok from reveman@ before landing.

https://codereview.chromium.org/977853002/diff/60001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right):

https://codereview.chromium.org/977853002/diff/60001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:560:
glValidateProgram(info->program);
nit: can you fix indentation on the moved code?

[dshwang, 2015-03-04 20:51:45]

On 2015/03/04 20:38:39, piman (Very slow to review) wrote:
> LGTM+nit, but make sure you have the ok from reveman@ before landing.

thx for reviewing!

reveman@, could you review how CopySubTextureCHROMIUM to use gl_image for
zero/one-copy?


>
https://codereview.chromium.org/977853002/diff/60001/gpu/command_buffer/servi...
> File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right):
> 
>
https://codereview.chromium.org/977853002/diff/60001/gpu/command_buffer/servi...
> gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:560:
> glValidateProgram(info->program);
> nit: can you fix indentation on the moved code?

oops, I'll update.

[reveman, 2015-03-04 20:56:46]

https://codereview.chromium.org/977853002/diff/60001/ui/gl/gl_image.h
File ui/gl/gl_image.h (right):

https://codereview.chromium.org/977853002/diff/60001/ui/gl/gl_image.h#newcode37
ui/gl/gl_image.h:37: virtual bool CopyTexSubImage(unsigned target, int xoffset,
int yoffset) = 0;
We don't really need xoffset and yoffset for this optimization. Can we just keep
this as it is and not call this function if offsets are non-zero?

[dshwang, 2015-03-04 21:02:15]

https://codereview.chromium.org/977853002/diff/60001/ui/gl/gl_image.h
File ui/gl/gl_image.h (right):

https://codereview.chromium.org/977853002/diff/60001/ui/gl/gl_image.h#newcode37
ui/gl/gl_image.h:37: virtual bool CopyTexSubImage(unsigned target, int xoffset,
int yoffset) = 0;
On 2015/03/04 20:56:46, reveman wrote:
> We don't really need xoffset and yoffset for this optimization. Can we just
keep
> this as it is and not call this function if offsets are non-zero?

Indeed. However, implementation can handle offsets.
Anyway as you want, I'll rollback this change to as-is

[dshwang, 2015-03-05 06:59:42]

On 2015/03/04 20:56:46, reveman wrote:
> https://codereview.chromium.org/977853002/diff/60001/ui/gl/gl_image.h
> File ui/gl/gl_image.h (right):
> 
>
https://codereview.chromium.org/977853002/diff/60001/ui/gl/gl_image.h#newcode37
> ui/gl/gl_image.h:37: virtual bool CopyTexSubImage(unsigned target, int
xoffset,
> int yoffset) = 0;
> We don't really need xoffset and yoffset for this optimization. Can we just
keep
> this as it is and not call this function if offsets are non-zero?

I changed the code as you suggested. could you review again?

[reveman, 2015-03-05 13:46:23]

lgtm. just one question

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

https://codereview.chromium.org/977853002/diff/80001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder.cc:11145: if
(image->CopyTexImage(GL_TEXTURE_2D))
Are we still allowed to use CopyTexImage in the DoCopyTextureCHROMIUM case?

[dshwang, 2015-03-05 14:55:59]

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

https://codereview.chromium.org/977853002/diff/80001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder.cc:11145: if
(image->CopyTexImage(GL_TEXTURE_2D))
On 2015/03/05 13:46:23, reveman wrote:
> Are we still allowed to use CopyTexImage in the DoCopyTextureCHROMIUM case?

It's not needed for zero/one-copy. currently no client uses this code. Do you
want to remove this code?
As I understand, you want to support this optimization only for exact
zero/one-copy code.

[reveman, 2015-03-05 18:20:01]

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

https://codereview.chromium.org/977853002/diff/80001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder.cc:11145: if
(image->CopyTexImage(GL_TEXTURE_2D))
On 2015/03/05 14:55:59, dshwang wrote:
> On 2015/03/05 13:46:23, reveman wrote:
> > Are we still allowed to use CopyTexImage in the DoCopyTextureCHROMIUM case?
> 
> It's not needed for zero/one-copy. currently no client uses this code. Do you
> want to remove this code?
> As I understand, you want to support this optimization only for exact
> zero/one-copy code.

You can keep it if you like. I just wanted to make sure it was not incorrect
behavior to call this from here.

[dshwang, 2015-03-05 18:22:46]

On 2015/03/05 18:20:01, reveman wrote:
>
https://codereview.chromium.org/977853002/diff/80001/gpu/command_buffer/servi...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> 
>
https://codereview.chromium.org/977853002/diff/80001/gpu/command_buffer/servi...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:11145: if
> (image->CopyTexImage(GL_TEXTURE_2D))
> On 2015/03/05 14:55:59, dshwang wrote:
> > On 2015/03/05 13:46:23, reveman wrote:
> > > Are we still allowed to use CopyTexImage in the DoCopyTextureCHROMIUM
case?
> > 
> > It's not needed for zero/one-copy. currently no client uses this code. Do
you
> > want to remove this code?
> > As I understand, you want to support this optimization only for exact
> > zero/one-copy code.
> 
> You can keep it if you like. I just wanted to make sure it was not incorrect
> behavior to call this from here.

Ok, I keep it like as-is. The code is correct. Thanks for reviewing.

[dshwang, 2015-03-05 18:22:52]

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

[dshwang, 2015-03-05 18:22:53]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
scherkus@chromium.org
Link to the patchset: https://codereview.chromium.org/977853002/#ps80001 (title:
"Apply image opt when offsets are zero")

[commit-bot: I haz the power, 2015-03-05 18:24:10]

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

[commit-bot: I haz the power, 2015-03-05 18:28:16]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2015-03-05 18:30:22]

Patchset 3 (id:??) landed as
https://crrev.com/46305b130beb809b98d02830c857dc3dfaf5b98b
Cr-Commit-Position: refs/heads/master@{#319288}