Reland of "webgl: optimize webgl.texSubImage2D(video) path."

Reland of "webgl: optimize webgl.texSubImage2D(video) path."

Original CL: https://codereview.chromium.org/1315323006/
Revert: https://codereview.chromium.org/1418513016

Reason for revert:
Reverting to address webgl_conformance_tests break in:
https://build.chromium.org/p/chromium.gpu/builders/Android%20Debug%20%28Nexus%209%29/builds/3534

Reason for reland:
There is a bug in glCopySubTextureCHROMIUM extension. GLES2DecoderImpl::DoCopySubTextureCHROMIUM/DoCompressedCopySubTextureCHROMIUM
uses ValidForTexture to check dimensions of source texture. If the source
texture is backed by GL Image, the decoder can report false positive error.
This CL doesn't use Texture::ValidForTexture() for GL Image.

TEST=WebglConformance.conformance_textures_video_tex_image_and_sub_image_2d_with_video_*
BUG=349871, 504773, 549531
Committed: https://crrev.com/7c26f04cf12c2c72fdab8e4764faf9305724c7ee
Cr-Commit-Position: refs/heads/master@{#357100}

[dshwang, 2015-10-28 19:38:35]

Description was changed from

==========
Reland of "webgl: optimize webgl.texSubImage2D(video) path."

Original CL: https://codereview.chromium.org/1315323006/
Revert: https://codereview.chromium.org/1418513016

Reason for revert:
Reverting to address webgl_conformance_tests break in:
https://build.chromium.org/p/chromium.gpu/builders/Android%20Debug%20%28Nexus...

Reason for reland:
There is a bug in glCopySubTextureCHROMIUM extension. As
https://codereview.chromium.org/1417913004/ resolves the bug, reland it.

TEST=WebglConformance.conformance_textures_video_tex_image_and_sub_image_2d_with_video_*
BUG=349871, 504773
==========

to

==========
Reland of "webgl: optimize webgl.texSubImage2D(video) path."

Original CL: https://codereview.chromium.org/1315323006/
Revert: https://codereview.chromium.org/1418513016

Reason for revert:
Reverting to address webgl_conformance_tests break in:
https://build.chromium.org/p/chromium.gpu/builders/Android%20Debug%20%28Nexus...

Reason for reland:
There is a bug in glCopySubTextureCHROMIUM extension. As
https://codereview.chromium.org/1417913004/ resolves the bug, reland it.

TEST=WebglConformance.conformance_textures_video_tex_image_and_sub_image_2d_with_video_*
BUG=349871, 504773
==========

[dshwang, 2015-10-28 19:38:36]

dongseong.hwang@intel.com changed reviewers:
+ dalecurtis@chromium.org, kbr@chromium.org, piman@chromium.org

[dshwang, 2015-10-28 19:39:04]

dongseong.hwang@intel.com changed reviewers:
+ philipj@opera.com

[dshwang, 2015-10-28 19:42:25]

dongseong.hwang@intel.com changed reviewers:
- piman@chromium.org

[dshwang, 2015-10-28 19:42:56]

The original CL was reverted due to a bug in glCopySubTextureCHROMIUM.
As https://codereview.chromium.org/1417913004/ is fixing the bug, ask to review
it.
I checked webgl conformance on Nexus5, linux and ChromeOS.

Could you review again? The code is exactly same to the original CL. Thank you.
DaleCurtis, media/, and content/
kbr, third_party/WebKit/Source/
philpj, third_party/WebKit/public/

[Ken Russell (switch to Gerrit), 2015-10-28 22:27:39]

I think you should fold https://codereview.chromium.org/1417913004/ into this as
patch set 2. It can be difficult to track down these dependent CLs in the
situation where one needs to be reverted.

[DaleCurtis, 2015-10-28 22:28:31]

Once the other CL lands you can just 1-click revert the other revert if you
like.

[DaleCurtis, 2015-10-28 22:28:56]

Or what kbr@ said :) I don't have a strong preference.

[Ken Russell (switch to Gerrit), 2015-10-28 22:30:18]

To be clear, I don't have a strong preference either, though we have had enough
reverts recently that I think it is worth reducing dependent CLs. lgtm either
way

[dshwang, 2015-10-29 09:15:00]

Description was changed from

==========
Reland of "webgl: optimize webgl.texSubImage2D(video) path."

Original CL: https://codereview.chromium.org/1315323006/
Revert: https://codereview.chromium.org/1418513016

Reason for revert:
Reverting to address webgl_conformance_tests break in:
https://build.chromium.org/p/chromium.gpu/builders/Android%20Debug%20%28Nexus...

Reason for reland:
There is a bug in glCopySubTextureCHROMIUM extension. As
https://codereview.chromium.org/1417913004/ resolves the bug, reland it.

TEST=WebglConformance.conformance_textures_video_tex_image_and_sub_image_2d_with_video_*
BUG=349871, 504773
==========

to

==========
Reland of "webgl: optimize webgl.texSubImage2D(video) path."

Original CL: https://codereview.chromium.org/1315323006/
Revert: https://codereview.chromium.org/1418513016

Reason for revert:
Reverting to address webgl_conformance_tests break in:
https://build.chromium.org/p/chromium.gpu/builders/Android%20Debug%20%28Nexus...

Reason for reland:
There is a bug in glCopySubTextureCHROMIUM extension.
GLES2DecoderImpl::DoCopySubTextureCHROMIUM/DoCompressedCopySubTextureCHROMIUM
uses ValidForTexture to check dimensions of source texture. If the source
texture is backed by GL Image, the decoder can report false positive error.
This CL doesn't use Texture::ValidForTexture() for GL Image.

TEST=WebglConformance.conformance_textures_video_tex_image_and_sub_image_2d_with_video_*
BUG=349871, 504773
==========

[dshwang, 2015-10-29 09:15:13]

dongseong.hwang@intel.com changed reviewers:
+ piman@chromium.org, zmo@google.com

[dshwang, 2015-10-29 09:18:09]

Thank you for many comments. Could you review again?
DaleCurtis, media/, and content/
zmo, gpu/
philpj, third_party/WebKit/public/

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

https://codereview.chromium.org/1413833006/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13212: 
As kbr reviewed, I fold https://codereview.chromium.org/1417913004/ into here.
It's fixed as part of cube map CL in
https://codereview.chromium.org/1275773003/diff/80001/gpu/command_buffer/serv...
,
but the CL was reverted.

zmo, piman, could you review?

[philipj_slow, 2015-10-29 10:22:37]

third_party/WebKit/public/platform/WebMediaPlayer.h lgtm

[DaleCurtis, 2015-10-29 15:58:33]

media/ lgtm

https://codereview.chromium.org/1413833006/diff/20001/content/renderer/media/...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/1413833006/diff/20001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.cc:637:
(bitmap_.getTexture())->getTextureHandle());
alignment seems wrong, git cl format?

[dshwang, 2015-10-29 17:44:46]

Thank you for reviewing!
zmo, piman, could you review? gpu/command_buffer/service/gles2_cmd_decoder.cc

https://codereview.chromium.org/1413833006/diff/20001/content/renderer/media/...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/1413833006/diff/20001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.cc:637:
(bitmap_.getTexture())->getTextureHandle());
On 2015/10/29 15:58:33, DaleCurtis wrote:
> alignment seems wrong, git cl format?

"git cl format" produces this indentation.

[Zhenyao Mo, 2015-10-29 23:40:07]

gpu/command_buffer lgtm

[piman, 2015-10-30 00:16:30]

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

https://codereview.chromium.org/1413833006/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13212: 
On 2015/10/29 09:18:09, dshwang wrote:
> As kbr reviewed, I fold https://codereview.chromium.org/1417913004/ into here.
> It's fixed as part of cube map CL in
>
https://codereview.chromium.org/1275773003/diff/80001/gpu/command_buffer/serv...
> ,
> but the CL was reverted.
> 
> zmo, piman, could you review?

Can you explain? It seems that we would want to make sure that the source
contains the rectangle we're trying to copy, even for the GLImage case. Why are
we removing the check?

What would happen if x/y/width/height were out of range?

[dshwang, 2015-10-30 11:06:28]

Thank you for reviewing!
I'll get gles2 cmd decoder more secured in
https://code.google.com/p/chromium/issues/detail?id=549531

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

https://codereview.chromium.org/1413833006/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13212: 
On 2015/10/30 00:16:30, piman (slow to review) wrote:
> On 2015/10/29 09:18:09, dshwang wrote:
> > As kbr reviewed, I fold https://codereview.chromium.org/1417913004/ into
here.
> > It's fixed as part of cube map CL in
> >
>
https://codereview.chromium.org/1275773003/diff/80001/gpu/command_buffer/serv...
> > ,
> > but the CL was reverted.
> > 
> > zmo, piman, could you review?
> 
> Can you explain? It seems that we would want to make sure that the source
> contains the rectangle we're trying to copy, even for the GLImage case. Why
are
> we removing the check?
> 
> What would happen if x/y/width/height were out of range?

That's good concern. Currently GLES2DecoderImpl cannot validation bad dimensions
as well as valid target, because some methods (e.g.
GetLevelSize/ValidForTexture/ValidForTarget) in Texture class are not valid for
GLImage. Let me solve it in
https://code.google.com/p/chromium/issues/detail?id=549531 and then remove this
unfortunate branch together.

[dshwang, 2015-10-30 11:07:37]

Description was changed from

==========
Reland of "webgl: optimize webgl.texSubImage2D(video) path."

Original CL: https://codereview.chromium.org/1315323006/
Revert: https://codereview.chromium.org/1418513016

Reason for revert:
Reverting to address webgl_conformance_tests break in:
https://build.chromium.org/p/chromium.gpu/builders/Android%20Debug%20%28Nexus...

Reason for reland:
There is a bug in glCopySubTextureCHROMIUM extension.
GLES2DecoderImpl::DoCopySubTextureCHROMIUM/DoCompressedCopySubTextureCHROMIUM
uses ValidForTexture to check dimensions of source texture. If the source
texture is backed by GL Image, the decoder can report false positive error.
This CL doesn't use Texture::ValidForTexture() for GL Image.

TEST=WebglConformance.conformance_textures_video_tex_image_and_sub_image_2d_with_video_*
BUG=349871, 504773
==========

to

==========
Reland of "webgl: optimize webgl.texSubImage2D(video) path."

Original CL: https://codereview.chromium.org/1315323006/
Revert: https://codereview.chromium.org/1418513016

Reason for revert:
Reverting to address webgl_conformance_tests break in:
https://build.chromium.org/p/chromium.gpu/builders/Android%20Debug%20%28Nexus...

Reason for reland:
There is a bug in glCopySubTextureCHROMIUM extension.
GLES2DecoderImpl::DoCopySubTextureCHROMIUM/DoCompressedCopySubTextureCHROMIUM
uses ValidForTexture to check dimensions of source texture. If the source
texture is backed by GL Image, the decoder can report false positive error.
This CL doesn't use Texture::ValidForTexture() for GL Image.

TEST=WebglConformance.conformance_textures_video_tex_image_and_sub_image_2d_with_video_*
BUG=349871, 504773, 549531
==========

[dshwang, 2015-10-30 11:56:46]

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

[dshwang, 2015-10-30 11:56:46]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org,
philipj@opera.com, zmo@chromium.org, dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1413833006/#ps40001
(title: "Add TODO comments")

[commit-bot: I haz the power, 2015-10-30 11:57:16]

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

[commit-bot: I haz the power, 2015-10-30 14:42:50]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-10-30 14:43:33]

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

[piman, 2015-10-30 17:53:35]

On 2015/10/30 11:06:28, dshwang wrote:
> Thank you for reviewing!
> I'll get gles2 cmd decoder more secured in
> https://code.google.com/p/chromium/issues/detail?id=549531
> 
>
https://codereview.chromium.org/1413833006/diff/20001/gpu/command_buffer/serv...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> 
>
https://codereview.chromium.org/1413833006/diff/20001/gpu/command_buffer/serv...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:13212: 
> On 2015/10/30 00:16:30, piman (slow to review) wrote:
> > On 2015/10/29 09:18:09, dshwang wrote:
> > > As kbr reviewed, I fold https://codereview.chromium.org/1417913004/ into
> here.
> > > It's fixed as part of cube map CL in
> > >
> >
>
https://codereview.chromium.org/1275773003/diff/80001/gpu/command_buffer/serv...
> > > ,
> > > but the CL was reverted.
> > > 
> > > zmo, piman, could you review?
> > 
> > Can you explain? It seems that we would want to make sure that the source
> > contains the rectangle we're trying to copy, even for the GLImage case. Why
> are
> > we removing the check?
> > 
> > What would happen if x/y/width/height were out of range?
> 
> That's good concern. Currently GLES2DecoderImpl cannot validation bad
dimensions
> as well as valid target, because some methods (e.g.
> GetLevelSize/ValidForTexture/ValidForTarget) in Texture class are not valid
for
> GLImage. Let me solve it in
> https://code.google.com/p/chromium/issues/detail?id=549531 and then remove
this
> unfortunate branch together.

What are the consequences of out-of-range parameters? Are we going to
dereference out-of-bounds memory? If so, this CL must be reverted ASAP. We
cannot introduce a security issue.

[Zhenyao Mo, 2015-10-30 18:22:55]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/1420843005/ by zmo@chromium.org.

The reason for reverting is: Reverting this due to the security concerns piman
raised, also it broke video related texture tests on Android..

[Zhenyao Mo, 2015-10-30 18:26:45]

It also broke
WebglConformance.conformance2_textures_video_tex_image_and_sub_image_2d_with_video*
on GPU FYI windows bots

[rjkroege, 2015-10-30 19:01:15]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/1421873005/ by rjkroege@chromium.org.

The reason for reverting is: Sorry but builder: Android Debug (Nexus 5) (stats)
is failing the webgl_conformance_tests since this patch landed. This seems like
the most logical candidate for introducing the failure:

Expected exception while running
WebglConformance.conformance_textures_video_tex_image_and_sub_image_2d_with_video_rgb_rgb_unsigned_byte

Traceback (most recent call last):
  RunStoryWithRetries at content/test/gpu/gpu_tests/gpu_test_base.py:72
    super(cls, shared_page_state).RunStory(results)
  RunStory at tools/telemetry/telemetry/page/shared_page_state.py:325
    self._current_page, self._current_tab, results)
  ValidateAndMeasurePage at content/test/gpu/gpu_tests/webgl_conformance.py:78
    raise page_test.Failure(_WebGLTestMessages(tab))
Failure: at (4, 4) expected: 0,255,0 was 0,0,0

testing: video/mp4; codecs="avc1.42E01E, mp4a.40.2"
Testing texImage2D with flipY=true bindingTarget=TEXTURE_2D
Checking lower left corner
FAIL at (4, 4) expected: 0,255,0 was 0,0,0
Checking upper left corner
at (4, 24) expected: 255,0,0 was 0,0,0
FAIL at (4, 24) expected: 255,0,0 was 0,0,0
Testing texImage2D with flipY=false bindingTarget=TEXTURE_2D
Checking lower left corner
at (4, 4) expected: 255,0,0 was 0,0,0
FAIL at (4, 4) expected: 255,0,0 was 0,0,0
Checking upper left corner
at (4, 24) expected: 0,255,0 was 0,0,0
FAIL at (4, 24) expected: 0,255,0 was 0,0,0
Testing texSubImage2D with flipY=true bindingTarget=TEXTURE_2D
Checking lower left corner
at (4, 4) expected: 0,255,0 was 0,0,0
FAIL at (4, 4) expected: 0,255,0 was 0,0,0
Checking upper left corner
at (4, 24) expected: 255,0,0 was 0,0,0
FAIL at (4, 24) expected: 255,0,0 was 0,0,0
Testing texSubImage2D with flipY=false bindingTarget=TEXTURE_2D
Checking lower left corner
at (4, 4) expected: 255,0,0 was 0,0,0
FAIL at (4, 4) expected: 255,0,0 was 0,0,0
Checking upper left corner
at (4, 24) expected: 0,255,0 was 0,0,0
FAIL at (4, 24) expected: 0,255,0 was 0,0,0

testing: video/webm; codecs="vp8, vorbis"
Testing texImage2D with flipY=true bindingTarget=TEXTURE_2D
Checking lower left corner
Checking upper left corner
Testing texImage2D with flipY=false bindingTarget=TEXTURE_2D
Checking lower left corner
Checking upper left corner
Testing texSubImage2D with flipY=true bindingTarget=TEXTURE_2D
Checking lower left corner
Checking upper left corner
Testing texSubImage2D with flipY=false bindingTarget=TEXTURE_2D
Checking lower left corner
Checking upper left corner

testing: video/ogg; codecs="theora, vorbis"
video/ogg; codecs="theora, vorbis" unsupported

testing: video/mp4; codecs="avc1.42E01E, mp4a.40.2"
Testing texSubImage2D with flipY=true bindingTarget=TEXTURE_CUBE_MAP
Checking lower left corner
Checking upper left corner
Checking lower left corner
Checking upper left corner
Checking lower left corner
Checking upper left corner
Checking lower left corner
Checking upper left corner
Checking lower left corner
Checking upper left corner
Checking lower left corner
Checking upper left corner
Testing texSubImage2D with flipY=false bindingTarget=TEXTURE_CUBE_MAP
Checking lower left corner
Checking upper left corner
Checking lower left corner
Checking upper left corner
Checking lower left corner
Checking upper left corner
Checking lower left corner
Checking upper left corner
Checking lower left corner
Checking upper left corner
Checking lower left corner
Checking upper left corner

testing: video/webm; codecs="vp8, vorbis"
Testing texSubImage2D with flipY=true bindingTarget=TEXTURE_CUBE_MAP
Checking lower left corner
Checking upper left corner
Checking lower left corner
Checking upper left corner
Checking lower left corner
Checking upper left corner
Checking lower left corner
Checking upper left corner
Checking lower left corner
Checking upper left corner
Checking lower left corner
Checking upper left corner
Testing texSubImage2D with flipY=false bindingTarget=TEXTURE_CUBE_MAP
Checking lower left corner
Checking upper left corner
Checking lower left corner
Checking upper left corner
Checking lower left corner
Checking upper left corner
Checking lower left corner
Checking upper left corner
Checking lower left corner
Checking upper left corner
Checking lower left corner
Checking upper left corner

testing: video/ogg; codecs="theora, vorbis"
video/ogg; codecs="theora, vorbis" unsupported

Locals:
  page    : <gpu_tests.webgl_conformance.WebglConformancePage object at
0x7f90820c3a50>
  results : <telemetry.internal.results.page_test_results.PageTestResults object
at 0x7f90820d7f50>
  tab     : <telemetry.internal.browser.tab.Tab object at 0x7f90820e4f10>
.

[dshwang, 2015-11-02 08:58:22]

sorry for hassle
I'll fix copySubTextureChromium first and then try to re-land it.
copySubTextureChromium CL would be too big to be merged with this CL.