gpu: support GL_TEXTURE_CUBE_MAP destination target to Copy(Sub)TextureCHROMIUM.

gpu: support GL_TEXTURE_CUBE_MAP destination target to Copy(Sub)TextureCHROMIUM.

It makes it possible to optimize WebGL.

TEST=gl_tests GLCopyTextureCHROMIUMTests/GLCopyTextureCHROMIUMTest.*
BUG=517548
Committed: https://crrev.com/fa25f0e5786527e63c6ff9aeda77da3bd7456ee3
Cr-Commit-Position: refs/heads/master@{#347354}

[dshwang, 2015-08-06 17:57:54]

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

[dshwang, 2015-08-06 17:58:28]

piman, siever, could you review this CL to expand Copy(Sub)TextureCHROMIUM for
GL_TEXTURE_CUBE_MAP?

[dshwang, 2015-08-06 18:02:54]

On 2015/08/06 17:58:28, dshwang wrote:
> piman, siever, could you review this CL to expand Copy(Sub)TextureCHROMIUM for
> GL_TEXTURE_CUBE_MAP?

Both glFramebufferTexture2D() and glCopyTexImage2D() support cube map by
default, so Copy(Sub)TextureCHROMIUM is easy to support also.
https://www.khronos.org/opengles/sdk/docs/man/xhtml/glFramebufferTexture2D.xml
https://www.khronos.org/opengles/sdk/docs/man/xhtml/glCopyTexImage2D.xml

[Ken Russell (switch to Gerrit), 2015-08-06 19:57:53]

kbr@chromium.org changed reviewers:
+ zmo@chromium.org

[Ken Russell (switch to Gerrit), 2015-08-06 19:57:53]

+zmo as he's been doing a lot of work in the command buffer recently.

I note that a lot of CopyTextureCHROMIUM tests are failing on
win_chromium_rel_ng with this CL.

[piman, 2015-08-06 20:43:23]

https://codereview.chromium.org/1275773003/diff/60001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt (right):

https://codereview.chromium.org/1275773003/diff/60001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt:94: been bound as
GL_TEXTURE_2D object.
Please update here (allow for GL_TEXTURE_CUBE_MAP too, and make sure it matches
<target>).

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

https://codereview.chromium.org/1275773003/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:12064: GL_TEXTURE_CUBE_MAP !=
GLES2Util::GLFaceToTarget(target) &&
Did you mean || instead of && ?

if target == GL_TEXTURE_2D but dest_texture's target is GL_TEXTURE_CUBE_MAP, we
should error here.

https://codereview.chromium.org/1275773003/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:12065: dest_texture->target() !=
target) {
did you mean dest_texture->target() != GLES2Util::GLFaceToTarget(target)?

https://codereview.chromium.org/1275773003/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:12259: GLenum binding_target =
GLES2Util::GLFaceToTarget(target);
You can use dest_texture->target(), which saves a switch.

https://codereview.chromium.org/1275773003/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:12457: GLenum binding_target =
GLES2Util::GLFaceToTarget(target);
you can use dest_texture->target() to save a switch.

[dshwang, 2015-08-10 08:04:18]

thx for reviewing. I addressed all comments.

https://codereview.chromium.org/1275773003/diff/60001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt (right):

https://codereview.chromium.org/1275773003/diff/60001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt:94: been bound as
GL_TEXTURE_2D object.
On 2015/08/06 20:43:23, piman (OOO back Aug 17) wrote:
> Please update here (allow for GL_TEXTURE_CUBE_MAP too, and make sure it
matches
> <target>).

Done.

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

https://codereview.chromium.org/1275773003/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:12064: GL_TEXTURE_CUBE_MAP !=
GLES2Util::GLFaceToTarget(target) &&
On 2015/08/06 20:43:23, piman (OOO back Aug 17) wrote:
> Did you mean || instead of && ?
> 
> if target == GL_TEXTURE_2D but dest_texture's target is GL_TEXTURE_CUBE_MAP,
we
> should error here.

Yes, thx for fixing mistake.

https://codereview.chromium.org/1275773003/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:12065: dest_texture->target() !=
target) {
On 2015/08/06 20:43:23, piman (OOO back Aug 17) wrote:
> did you mean dest_texture->target() != GLES2Util::GLFaceToTarget(target)?

Yes. oops, double mistake. Done.

https://codereview.chromium.org/1275773003/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:12259: GLenum binding_target =
GLES2Util::GLFaceToTarget(target);
On 2015/08/06 20:43:23, piman (OOO back Aug 17) wrote:
> You can use dest_texture->target(), which saves a switch.

Done.

https://codereview.chromium.org/1275773003/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:12457: GLenum binding_target =
GLES2Util::GLFaceToTarget(target);
On 2015/08/06 20:43:23, piman (OOO back Aug 17) wrote:
> you can use dest_texture->target() to save a switch.

Done.

https://codereview.chromium.org/1275773003/diff/80001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (left):

https://codereview.chromium.org/1275773003/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:12509:
source_texture->GetLevelType(source_texture->target(), 0, &source_type,
It's existing bug because it's not valid if |image| exists like GetLevelSize().
Move it into !|image| branch.

[Ken Russell (switch to Gerrit), 2015-08-11 16:49:15]

Thanks Dongseong for this patch. With piman's comments addressed this looks
good, but I'm not an owner in the directory. Daniel, could you do a final review
of this as piman and zmo are both out this week?

[Ken Russell (switch to Gerrit), 2015-08-11 16:50:06]

On 2015/08/11 16:49:15, Ken Russell OOO-till-Aug-17 wrote:
> Thanks Dongseong for this patch. With piman's comments addressed this looks
> good, but I'm not an owner in the directory. Daniel, could you do a final
review
> of this as piman and zmo are both out this week?

Also, there are still failures of the CopyTextureCHROMIUM tests in the try jobs;
these should be addressed before further review progresses.

[dshwang, 2015-08-11 18:09:42]

On 2015/08/11 16:50:06, Ken Russell OOO-till-Aug-17 wrote:
> On 2015/08/11 16:49:15, Ken Russell OOO-till-Aug-17 wrote:
> > Thanks Dongseong for this patch. With piman's comments addressed this looks
> > good, but I'm not an owner in the directory. Daniel, could you do a final
> review
> > of this as piman and zmo are both out this week?

Thank you for reviewing. sievers, could you review?

> Also, there are still failures of the CopyTextureCHROMIUM tests in the try
jobs;
> these should be addressed before further review progresses.

Yes, the failure is due to driver bug. surprisingly Android, ANGLE(for Windows),
NVIDIA(for Linux and Mac) has different bugs.
I was narrowing them, and fixing in https://codereview.chromium.org/1280163004/
After resolving the issue, I'll land this CL.

[Ken Russell (switch to Gerrit), 2015-08-11 18:18:24]

On 2015/08/11 18:09:42, dshwang wrote:
> On 2015/08/11 16:50:06, Ken Russell OOO-till-Aug-17 wrote:
> > On 2015/08/11 16:49:15, Ken Russell OOO-till-Aug-17 wrote:
> > > Thanks Dongseong for this patch. With piman's comments addressed this
looks
> > > good, but I'm not an owner in the directory. Daniel, could you do a final
> > review
> > > of this as piman and zmo are both out this week?
> 
> Thank you for reviewing. sievers, could you review?
> 
> > Also, there are still failures of the CopyTextureCHROMIUM tests in the try
> jobs;
> > these should be addressed before further review progresses.
> 
> Yes, the failure is due to driver bug. surprisingly Android, ANGLE(for
Windows),
> NVIDIA(for Linux and Mac) has different bugs.
> I was narrowing them, and fixing in
https://codereview.chromium.org/1280163004/
> After resolving the issue, I'll land this CL.

Per my comment on the other CL, I think this is a bug in the code and/or the
test, not a driver bug.

[dshwang, 2015-08-12 15:02:56]

On 2015/08/11 18:18:24, Ken Russell OOO-till-Aug-17 wrote:
> On 2015/08/11 18:09:42, dshwang wrote:
> > On 2015/08/11 16:50:06, Ken Russell OOO-till-Aug-17 wrote:
> > > On 2015/08/11 16:49:15, Ken Russell OOO-till-Aug-17 wrote:
> > > > Thanks Dongseong for this patch. With piman's comments addressed this
> looks
> > > > good, but I'm not an owner in the directory. Daniel, could you do a
final
> > > review
> > > > of this as piman and zmo are both out this week?
> > 
> > Thank you for reviewing. sievers, could you review?
> > 
> > > Also, there are still failures of the CopyTextureCHROMIUM tests in the try
> > jobs;
> > > these should be addressed before further review progresses.
> > 
> > Yes, the failure is due to driver bug. surprisingly Android, ANGLE(for
> Windows),
> > NVIDIA(for Linux and Mac) has different bugs.
> > I was narrowing them, and fixing in
> https://codereview.chromium.org/1280163004/
> > After resolving the issue, I'll land this CL.
> 
> Per my comment on the other CL, I think this is a bug in the code and/or the
> test, not a driver bug.

You're right. NVidia checks cube completeness strictly according to OpenGL spec.
Other drivers accepts incomplete cube textures as FBO.

After surveying it in https://codereview.chromium.org/1280163004/, the
conclusion is that all drivers are fine with complete cube texture. So this CL
can progress independently.

I need to make one big design decision. See below comment.

https://codereview.chromium.org/1275773003/diff/100001/gpu/GLES2/extensions/C...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt (right):

https://codereview.chromium.org/1275773003/diff/100001/gpu/GLES2/extensions/C...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt:76: and an empty image
data.
Add this semantic, because CopyTextureCHROMIUM implementation requires dest
texture to be bound to FBO.

It's a bit surprising side effect compared to glCopyTexImage2D().

In detail, it can relax ES spec's constraint.
I'll use this to optimize WebGL.texImage2D(video).

According to spec, following FBO should be incomplete, because only
GL_TEXTURE_CUBE_MAP_POSITIVE_X face texture is defined (i.e. not cube complete)
  texImage2D(GL_TEXTURE_CUBE_MAP_POSITIVE_X, video);
  framebufferTexture2D(..., GL_TEXTURE_CUBE_MAP_POSITIVE_X, texture);

However, if WebGL.texImage2D(video) uses this API, the FBO is suddenly complete.

It doesn't make correct code report error. It makes incorrect code become
correct. So WebGL will become more relaxed.

kbr, sievers, what do you think? Is it acceptable?

If it's not acceptable, I'll implement it via more expensive way.
1. copy to GL_TEXTURE_2D temp texture.
2. glCopyTexImage2D() the temp texture to GL_TEXTURE_CUBE_MAP_POSITIVE_X dest
texture.

https://codereview.chromium.org/1275773003/diff/100001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1275773003/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:12484: dest_type, source_width,
source_height)) {
this code makes the cube textures 'cube complete'

[dshwang, 2015-08-12 17:40:51]

https://codereview.chromium.org/1275773003/diff/100001/gpu/GLES2/extensions/C...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt (right):

https://codereview.chromium.org/1275773003/diff/100001/gpu/GLES2/extensions/C...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt:76: and an empty image
data.
On 2015/08/12 15:02:56, dshwang wrote:
> Add this semantic, because CopyTextureCHROMIUM implementation requires dest
> texture to be bound to FBO.
> 
> It's a bit surprising side effect compared to glCopyTexImage2D().
> 
> In detail, it can relax ES spec's constraint.
> I'll use this to optimize WebGL.texImage2D(video).
> 
> According to spec, following FBO should be incomplete, because only
> GL_TEXTURE_CUBE_MAP_POSITIVE_X face texture is defined (i.e. not cube
complete)
>   texImage2D(GL_TEXTURE_CUBE_MAP_POSITIVE_X, video);
>   framebufferTexture2D(..., GL_TEXTURE_CUBE_MAP_POSITIVE_X, texture);
> 
> However, if WebGL.texImage2D(video) uses this API, the FBO is suddenly
complete.
> 
> It doesn't make correct code report error. It makes incorrect code become
> correct. So WebGL will become more relaxed.
> 
> kbr, sievers, what do you think? Is it acceptable?
> 
> If it's not acceptable, I'll implement it via more expensive way.
> 1. copy to GL_TEXTURE_2D temp texture.
> 2. glCopyTexImage2D() the temp texture to GL_TEXTURE_CUBE_MAP_POSITIVE_X dest
> texture.

The side effect causes many test failures in NVIDIA bots. It might not be good
idea. Now I think it's better to implement "more expensive way" than to fix
Nvidia bot failures. WDYT?

[dshwang, 2015-08-13 18:50:36]

https://codereview.chromium.org/1275773003/diff/100001/gpu/GLES2/extensions/C...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt (right):

https://codereview.chromium.org/1275773003/diff/100001/gpu/GLES2/extensions/C...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt:76: and an empty image
data.
On 2015/08/12 17:40:51, dshwang wrote:
> On 2015/08/12 15:02:56, dshwang wrote:
> > Add this semantic, because CopyTextureCHROMIUM implementation requires dest
> > texture to be bound to FBO.
> > 
> > It's a bit surprising side effect compared to glCopyTexImage2D().
> > 
> > In detail, it can relax ES spec's constraint.
> > I'll use this to optimize WebGL.texImage2D(video).
> > 
> > According to spec, following FBO should be incomplete, because only
> > GL_TEXTURE_CUBE_MAP_POSITIVE_X face texture is defined (i.e. not cube
> complete)
> >   texImage2D(GL_TEXTURE_CUBE_MAP_POSITIVE_X, video);
> >   framebufferTexture2D(..., GL_TEXTURE_CUBE_MAP_POSITIVE_X, texture);
> > 
> > However, if WebGL.texImage2D(video) uses this API, the FBO is suddenly
> complete.
> > 
> > It doesn't make correct code report error. It makes incorrect code become
> > correct. So WebGL will become more relaxed.
> > 
> > kbr, sievers, what do you think? Is it acceptable?
> > 
> > If it's not acceptable, I'll implement it via more expensive way.
> > 1. copy to GL_TEXTURE_2D temp texture.
> > 2. glCopyTexImage2D() the temp texture to GL_TEXTURE_CUBE_MAP_POSITIVE_X
dest
> > texture.
> 
> The side effect causes many test failures in NVIDIA bots. It might not be good
> idea. Now I think it's better to implement "more expensive way" than to fix
> Nvidia bot failures. WDYT?

Relaxing the API will cause unexpected bug, so I make a decision to choose
correct and expensive way.

If the dest texture is incomplete cube texture, the implementation copies it via
2 stages. 
1. copy source to temp texture
2. copy temp to dest texture

[Ken Russell (switch to Gerrit), 2015-08-18 00:33:02]

Thanks for continuing to move this patch forward. The continued failures on
win_chromium_rel_ng are concerning. What's the plan for addressing them?

https://codereview.chromium.org/1275773003/diff/120001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right):

https://codereview.chromium.org/1275773003/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:451:
DoCopyTexSubImage2D(decoder, GL_TEXTURE_2D, staging_testure_, dest_target,
The staging_texture_ will stick around after this, allocated to the size
(source_width * source_height). This seems suboptimal; could you reallocate it
to a smaller size after the DoCopyTexSubImage2D call?

https://codereview.chromium.org/1275773003/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:492:
DoCopyTexImage2D(decoder, GL_TEXTURE_2D, staging_testure_, dest_target,
Similar comment about the staging_texture_ staying around for a long period of
time.

https://codereview.chromium.org/1275773003/diff/120001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h (right):

https://codereview.chromium.org/1275773003/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h:138: GLuint
staging_testure_;
typo: should be staging_texture_ .

[dshwang, 2015-08-18 14:42:01]

On 2015/08/18 00:33:02, Ken Russell wrote:
> Thanks for continuing to move this patch forward. The continued failures on
> win_chromium_rel_ng are concerning. What's the plan for addressing them?

Patch set 6/7 is implemented based on wrong understanding, which FBO binding to
not-cube-complete texture is always incomplete.

The wrong understanding makes wrong hypothesis to fix Nvidia failure, which is
https://codereview.chromium.org/1283303002/ and
https://codereview.chromium.org/1299683002/

I need to debug from scratch based on Patch Set 5.
I don't have Nvidia card, so I rely on trybot to fix it like
https://codereview.chromium.org/1295313002/
If have a clue, plz let me know.

https://codereview.chromium.org/1275773003/diff/120001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right):

https://codereview.chromium.org/1275773003/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:451:
DoCopyTexSubImage2D(decoder, GL_TEXTURE_2D, staging_testure_, dest_target,
On 2015/08/18 00:33:02, Ken Russell wrote:
> The staging_texture_ will stick around after this, allocated to the size
> (source_width * source_height). This seems suboptimal; could you reallocate it
> to a smaller size after the DoCopyTexSubImage2D call?

Acknowledged.

https://codereview.chromium.org/1275773003/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:492:
DoCopyTexImage2D(decoder, GL_TEXTURE_2D, staging_testure_, dest_target,
On 2015/08/18 00:33:02, Ken Russell wrote:
> Similar comment about the staging_texture_ staying around for a long period of
> time.

Acknowledged.

https://codereview.chromium.org/1275773003/diff/120001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h (right):

https://codereview.chromium.org/1275773003/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h:138: GLuint
staging_testure_;
On 2015/08/18 00:33:02, Ken Russell wrote:
> typo: should be staging_texture_ .

Acknowledged.

[dshwang, 2015-08-27 06:53:56]

Patchset #6 (id:100001) has been deleted

[dshwang, 2015-09-02 11:01:19]

kbr, piman, could you review again? Finally I figured out how to workaround all
platforms.
This CL is required to fix "Issue 504773: WebGL video test flakiness on Android"

https://codereview.chromium.org/1275773003/diff/180001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right):

https://codereview.chromium.org/1275773003/diff/180001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:325:
texture_state->force_cube_map_positive_x_allocation;
We don't ensure if it's fine for FBO to bind cube map texture on the drivers
with force_cube_complete or force_cube_map_positive_x_allocation workaround.

Note: even if we take care of these drivers by workaround, webgl is not safe, no
matter this CL. Web site can use FBO to bind cube map texture and we don't have
any way to prevent it. It's existing issue.

https://codereview.chromium.org/1275773003/diff/180001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:474:
ScopedStagingTexture staging_texture;
as kbr suggested to destruct texture after workaround, the class don't need to
own the staging texture as member. The texture is created on demand, and then
deleted.

https://codereview.chromium.org/1275773003/diff/180001/gpu/command_buffer/tes...
File gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc (right):

https://codereview.chromium.org/1275773003/diff/180001/gpu/command_buffer/tes...
gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:33: #if
!defined(OS_ANDROID)
As kbr was worry, glReadPixels from FBO binding cube map texture has many
troubles, especially in Nvidia.
Nvidia Android crashes on glRealPixels.

On the other hands, this CopyTextureCHROMIUM change is fine, because
1. CopyTextureCHROMIUMResourceManager implementation doesn't use glReadPixels
2. In the driver with trouble, CopyTextureCHROMIUMResourceManager don't use FBO
binding cube map texture via workaround.

So the driver bugs are problem of not only gl_tests but also webgl user code
using cube map texture, as-is. CopyTextureCHROMIUM implementation itself is
fine.

To make sure crash not happen, we need to prevent Nvidia from calling
glReadPixels from FBO binding cube map texture, but it breaks GLES2 spec and
there is not workaround :(

https://codereview.chromium.org/1275773003/diff/180001/gpu/command_buffer/tes...
gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:53:
base::IntToString(gpu::FORCE_CUBE_COMPLETE));
Only ANGLE and Nvidia needs this workaround, which makes implementation not use
FBO binding cube map texture.
However, gl_tests cannot detect gpu vendor.
After crbug.com/527126 is resolved, I'll narrow the targets for the workaround.

https://codereview.chromium.org/1275773003/diff/180001/gpu/command_buffer/tes...
gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:87: void
MakeCubeComplete(GLenum target,
It's for mac, linux, win with Nvidia.

[piman, 2015-09-03 19:13:27]

lgtm

https://codereview.chromium.org/1275773003/diff/180001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right):

https://codereview.chromium.org/1275773003/diff/180001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:325:
texture_state->force_cube_map_positive_x_allocation;
On 2015/09/02 11:01:19, dshwang_ooo_5.9-27.9 wrote:
> We don't ensure if it's fine for FBO to bind cube map texture on the drivers
> with force_cube_complete or force_cube_map_positive_x_allocation workaround.
> 
> Note: even if we take care of these drivers by workaround, webgl is not safe,
no
> matter this CL. Web site can use FBO to bind cube map texture and we don't
have
> any way to prevent it. It's existing issue.

Could you file a bug to describe the issue (if there isn't one already)?

We may want to at least prevent the crash at the cost of conformance, if not
disable WebGL on problematic drivers altogether.

[dshwang, 2015-09-04 06:23:13]

thx for reviewing!

https://codereview.chromium.org/1275773003/diff/180001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right):

https://codereview.chromium.org/1275773003/diff/180001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:325:
texture_state->force_cube_map_positive_x_allocation;
On 2015/09/03 19:13:26, piman (slow to review) wrote:
> On 2015/09/02 11:01:19, dshwang_ooo_5.9-27.9 wrote:
> > We don't ensure if it's fine for FBO to bind cube map texture on the drivers
> > with force_cube_complete or force_cube_map_positive_x_allocation workaround.
> > 
> > Note: even if we take care of these drivers by workaround, webgl is not
safe,
> no
> > matter this CL. Web site can use FBO to bind cube map texture and we don't
> have
> > any way to prevent it. It's existing issue.
> 
> Could you file a bug to describe the issue (if there isn't one already)?
> 
> We may want to at least prevent the crash at the cost of conformance, if not
> disable WebGL on problematic drivers altogether.

Yes, I filed https://code.google.com/p/chromium/issues/detail?id=528145
And add comments about the crbug in this CL.

[dshwang, 2015-09-04 06:25:08]

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

[dshwang, 2015-09-04 06:25:08]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1275773003/#ps200001
(title: "add comments about crbug.com/528145")

[commit-bot: I haz the power, 2015-09-04 06:25:28]

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

[commit-bot: I haz the power, 2015-09-04 07:33:19]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2015-09-04 07:34:07]

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

[Jamie Madill, 2015-09-04 18:27:57]

A revert of this CL (patchset #10 id:200001) has been created in
https://codereview.chromium.org/1304103005/ by jmadill@chromium.org.

The reason for reverting is: Linux AMD GPU FYI bot has a broken gl_tests, likely
because of this patch.

http://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Release%20%28AT....