WebGL 2: fix the bug in invalidate-framebuffer.html

WebGL 2: fix the bug in invalidate-framebuffer.html

BUG=554902
TEST=conformance2/renderbuffers/invalidate-framebuffer.html, gpu_unittests

Committed: https://crrev.com/6bed6e20b113887528f4ec63a88688ce40fbb911
Cr-Commit-Position: refs/heads/master@{#361068}

[yunchao, 2015-11-12 04:39:26]

Description was changed from

==========
fix the bug in invalidate-framebuffer.html

BUG=429053
==========

to

==========
fix the bug in invalidate-framebuffer.html

BUG=429053
==========

[yunchao, 2015-11-12 04:39:26]

yunchao.he@intel.com changed reviewers:
+ bajones@chromium.org, kbr@chromium.org, zmo@chromium.org

[yunchao, 2015-11-12 05:05:40]

Description was changed from

==========
fix the bug in invalidate-framebuffer.html

BUG=429053
==========

to

==========
fix the bug in invalidate-framebuffer.html

BUG=295792
==========

[yunchao, 2015-11-12 11:57:47]

Description was changed from

==========
fix the bug in invalidate-framebuffer.html

BUG=295792
==========

to

==========
fix the bug in invalidate-framebuffer.html

BUG=554902
==========

[yunchao, 2015-11-12 11:57:47]

yunchao.he@intel.com changed reviewers:
+ piman@chromium.org

[yunchao, 2015-11-12 11:58:18]

Description was changed from

==========
fix the bug in invalidate-framebuffer.html

BUG=554902
==========

to

==========
WebGL 2: fix the bug in invalidate-framebuffer.html

BUG=554902
==========

[yunchao, 2015-11-12 11:59:38]

Description was changed from

==========
WebGL 2: fix the bug in invalidate-framebuffer.html

BUG=554902
==========

to

==========
WebGL 2: fix the bug in invalidate-framebuffer.html

BUG=554902

TEST=conformance2/renderbuffers/invalidate-framebuffer.html, gpu_unittests
==========

[yunchao, 2015-11-12 12:14:51]

PTAL. Thanks a lot!

https://codereview.chromium.org/1435183002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/1435183002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:390: case
GL_BACK:
In GLES spec, it is GL_COLOR, but in WebGL spec, it is GL_BACK (say in
drawBuffer). Both of them means GL_COLOR_ATTACHMENT0 of the internal fbo
(default framebuffer of WebGL).

I would like it is GL_BACK, then it is aligned with other APIs in WebGL spec.

https://codereview.chromium.org/1435183002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:451: if
(width < 0 || height < 0) {
Not sure about this code snippet, In OGL spec, it will raise a GL_INVALID_VALUE
for this case, but it seems be OK in GLES spec.

[Zhenyao Mo, 2015-11-13 03:18:11]

https://codereview.chromium.org/1435183002/diff/20001/gpu/command_buffer/buil...
File gpu/command_buffer/build_gles2_cmd_buffer.py (left):

https://codereview.chromium.org/1435183002/diff/20001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:606: 'InvalidateFrameBufferTarget':
{
You should not remove this.  For invalidateFramebuffer(), only GL_FRAMEBUFFER.

[yunchao, 2015-11-13 04:16:36]

On 2015/11/13 03:18:11, Zhenyao Mo wrote:
>
https://codereview.chromium.org/1435183002/diff/20001/gpu/command_buffer/buil...
> File gpu/command_buffer/build_gles2_cmd_buffer.py (left):
> 
>
https://codereview.chromium.org/1435183002/diff/20001/gpu/command_buffer/buil...
> gpu/command_buffer/build_gles2_cmd_buffer.py:606:
'InvalidateFrameBufferTarget':
> {
> You should not remove this.  For invalidateFramebuffer(), only GL_FRAMEBUFFER.

I think this link
https://www.khronos.org/opengles/sdk/docs/man3/html/glInvalidateFramebuffer.x...
is not correct. From the GLES 3 spec
https://www.khronos.org/registry/gles/specs/3.0/es_spec_3.0.3.pdf.
Invalidate{Sub}Framebuffer can support GL_READ_FRAMEBUFFER and
GL_DRAW_FRAMEBUFFER. Firefox browser pass invalidate-framebuffer.html tests with
these two targets. 

WDYT? 

BTW, could have a look at this CL: https://codereview.chromium.org/1236053003/ ?

[Ken Russell (switch to Gerrit), 2015-11-17 23:06:37]

Thanks for this contribution and sorry for the long delay reviewing. Mostly
looks OK, but a few updates are needed.

https://codereview.chromium.org/1435183002/diff/20001/gpu/command_buffer/buil...
File gpu/command_buffer/build_gles2_cmd_buffer.py (left):

https://codereview.chromium.org/1435183002/diff/20001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:606: 'InvalidateFrameBufferTarget':
{
On 2015/11/13 03:18:11, Zhenyao Mo wrote:
> You should not remove this.  For invalidateFramebuffer(), only GL_FRAMEBUFFER.

Actually it looks like this was changed in ES spec 3.0.1 (see section E.5 of the
ES 3.0.4 spec). The spec allows all 3 values to be passed. The man page is out
of date. Filed Khronos internal bug 15181 about this.

https://codereview.chromium.org/1435183002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/1435183002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:378: bool
WebGL2RenderingContextBase::checkAndTranslateAttachments(const char*
functionName, GLenum target, Vector<GLenum>& attachments)
It's a little nasty to modify the Vector which comes in from the JavaScript
bindings. I think it would be better to mark it as a const Vector& of GLenum in
WebGL2RenderingContextBase.h and here, and allocate a new vector with the
translated values.

https://codereview.chromium.org/1435183002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:381:
GLenum* buffer = attachments.data();
It's not necessary to fetch the data pointer out of "attachments" -- just
reference attachments[i] directly.

https://codereview.chromium.org/1435183002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:390: case
GL_BACK:
On 2015/11/12 12:14:50, yunchao wrote:
> In GLES spec, it is GL_COLOR, but in WebGL spec, it is GL_BACK (say in
> drawBuffer). Both of them means GL_COLOR_ATTACHMENT0 of the internal fbo
> (default framebuffer of WebGL).
> 
> I would like it is GL_BACK, then it is aligned with other APIs in WebGL spec.

Sorry, but no, this should follow the OpenGL ES specification. WebGL does not
deviate arbitrarily from the ES specs.

https://codereview.chromium.org/1435183002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:435: if
(!checkAndTranslateAttachments("invalidateFramebuffe", target, attachments))
invalidateFramebuffe -> invalidateFramebuffer

https://codereview.chromium.org/1435183002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:451: if
(width < 0 || height < 0) {
On 2015/11/12 12:14:51, yunchao wrote:
> Not sure about this code snippet, In OGL spec, it will raise a
GL_INVALID_VALUE
> for this case, but it seems be OK in GLES spec.

In this case it seems better to be safer by generating INVALID_VALUE as is
already tested by conformance2/renderbuffers/invalidate-framebuffer.html.
Otherwise it'll be necessary to clamp these to 0 to avoid INVALID_VALUE errors
from OpenGL drivers.

https://codereview.chromium.org/1435183002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:456: if
(!checkAndTranslateAttachments("invalidateSubFramebuffe", target, attachments))
invalidateSubFramebuffe -> invalidateSubFramebuffer

[yunchao, 2015-11-19 00:12:16]

Patchset #3 (id:40001) has been deleted

[yunchao, 2015-11-19 00:40:13]

Patchset #3 (id:60001) has been deleted

[yunchao, 2015-11-19 01:43:00]

Thanks for your review, Ken. 

Code has been updated accordingly.

https://codereview.chromium.org/1435183002/diff/20001/gpu/command_buffer/buil...
File gpu/command_buffer/build_gles2_cmd_buffer.py (left):

https://codereview.chromium.org/1435183002/diff/20001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:606: 'InvalidateFrameBufferTarget':
{
On 2015/11/17 23:06:37, Ken Russell wrote:
> On 2015/11/13 03:18:11, Zhenyao Mo wrote:
> > You should not remove this.  For invalidateFramebuffer(), only
GL_FRAMEBUFFER.
> 
> Actually it looks like this was changed in ES spec 3.0.1 (see section E.5 of
the
> ES 3.0.4 spec). The spec allows all 3 values to be passed. The man page is out
> of date. Filed Khronos internal bug 15181 about this.

Agreed.

https://codereview.chromium.org/1435183002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/1435183002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:378: bool
WebGL2RenderingContextBase::checkAndTranslateAttachments(const char*
functionName, GLenum target, Vector<GLenum>& attachments)
On 2015/11/17 23:06:37, Ken Russell wrote:
> It's a little nasty to modify the Vector which comes in from the JavaScript
> bindings. I think it would be better to mark it as a const Vector& of GLenum
in
> WebGL2RenderingContextBase.h and here, and allocate a new vector with the
> translated values.

Done.

https://codereview.chromium.org/1435183002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:381:
GLenum* buffer = attachments.data();
On 2015/11/17 23:06:37, Ken Russell wrote:
> It's not necessary to fetch the data pointer out of "attachments" -- just
> reference attachments[i] directly.

Done.

https://codereview.chromium.org/1435183002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:381:
GLenum* buffer = attachments.data();
On 2015/11/17 23:06:37, Ken Russell wrote:
> It's not necessary to fetch the data pointer out of "attachments" -- just
> reference attachments[i] directly.

Done.

https://codereview.chromium.org/1435183002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:390: case
GL_BACK:
On 2015/11/17 23:06:37, Ken Russell wrote:
> On 2015/11/12 12:14:50, yunchao wrote:
> > In GLES spec, it is GL_COLOR, but in WebGL spec, it is GL_BACK (say in
> > drawBuffer). Both of them means GL_COLOR_ATTACHMENT0 of the internal fbo
> > (default framebuffer of WebGL).
> > 
> > I would like it is GL_BACK, then it is aligned with other APIs in WebGL
spec.
> 
> Sorry, but no, this should follow the OpenGL ES specification. WebGL does not
> deviate arbitrarily from the ES specs.

Done.

https://codereview.chromium.org/1435183002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:435: if
(!checkAndTranslateAttachments("invalidateFramebuffe", target, attachments))
On 2015/11/17 23:06:37, Ken Russell wrote:
> invalidateFramebuffe -> invalidateFramebuffer

Done.

https://codereview.chromium.org/1435183002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:451: if
(width < 0 || height < 0) {
On 2015/11/17 23:06:37, Ken Russell wrote:
> On 2015/11/12 12:14:51, yunchao wrote:
> > Not sure about this code snippet, In OGL spec, it will raise a
> GL_INVALID_VALUE
> > for this case, but it seems be OK in GLES spec.
> 
> In this case it seems better to be safer by generating INVALID_VALUE as is
> already tested by conformance2/renderbuffers/invalidate-framebuffer.html.
> Otherwise it'll be necessary to clamp these to 0 to avoid INVALID_VALUE errors
> from OpenGL drivers.

OK, let's generate GL_INVALID_ENUM for this case.

https://codereview.chromium.org/1435183002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:456: if
(!checkAndTranslateAttachments("invalidateSubFramebuffe", target, attachments))
On 2015/11/17 23:06:37, Ken Russell wrote:
> invalidateSubFramebuffe -> invalidateSubFramebuffer

Done.

[Ken Russell (switch to Gerrit), 2015-11-19 02:24:05]

Thanks. LGTM. One typo.

https://codereview.chromium.org/1435183002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/1435183002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:383: //
Translate GL_BACK/GL_DEPTH/GL_STENCIL, because the default framebuffer of WebGL
is not fb 0, it is an internal fbo
GL_BACK -> GL_COLOR

[yunchao, 2015-11-19 05:25:16]

The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2015-11-19 05:26:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1435183002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1435183002/100001

[commit-bot: I haz the power, 2015-11-19 10:34:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-19 10:34:23]

Dry run: This issue passed the CQ dry run.

[Ken Russell (switch to Gerrit), 2015-11-20 23:29:12]

zmo, piman: OWNERS review of gpu/command_buffer please.

[zmo, 2015-11-20 23:31:26]

LGTM

Thank you for checking the spec and identifying the mistake in the manual page.

[piman, 2015-11-20 23:49:43]

lgtm

[Ken Russell (switch to Gerrit), 2015-11-21 00:22:29]

The CQ bit was checked by kbr@chromium.org

[Ken Russell (switch to Gerrit), 2015-11-21 00:22:29]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org
Link to the patchset: https://codereview.chromium.org/1435183002/#ps100001
(title: "fix typo")

[commit-bot: I haz the power, 2015-11-21 00:24:08]

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

[commit-bot: I haz the power, 2015-11-21 03:05:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-21 03:05:14]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[yunchao, 2015-11-23 02:10:18]

The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2015-11-23 02:10:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1435183002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1435183002/100001

[commit-bot: I haz the power, 2015-11-23 03:33:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-23 03:33:01]

Dry run: This issue passed the CQ dry run.

[yunchao, 2015-11-23 04:26:52]

The CQ bit was checked by yunchao.he@intel.com

[commit-bot: I haz the power, 2015-11-23 04:27:14]

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

[commit-bot: I haz the power, 2015-11-23 04:32:31]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2015-11-23 04:33:24]

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