WebGL: add targets for WebGL 2 when check FBO targets

WebGL: add targets for WebGL 2 when check FBO targets

BUG=295792
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194706

[yunchao, 2015-04-21 10:11:32]

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

[yunchao, 2015-04-21 10:12:34]

A very simple change, PTAL. Thanks!

[Zhenyao Mo, 2015-04-22 22:39:43]

https://codereview.chromium.org/1099853002/diff/1/Source/core/html/canvas/Web...
File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right):

https://codereview.chromium.org/1099853002/diff/1/Source/core/html/canvas/Web...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:1372: if ((target !=
GL_FRAMEBUFFER && !isWebGL2OrHigher()) || (target != GL_READ_FRAMEBUFFER &&
target != GL_DRAW_FRAMEBUFFER)) {
This is incorrect.

GL_FRAMEBUFFER is also a valid target for ES2.

https://codereview.chromium.org/1099853002/diff/1/Source/core/html/canvas/Web...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:5377: if ((target !=
GL_FRAMEBUFFER && !isWebGL2OrHigher()) || (target != GL_READ_FRAMEBUFFER &&
target != GL_DRAW_FRAMEBUFFER)) {
Ditto.

[yunchao, 2015-04-23 06:46:46]

Thanks for your reviewing, @zmo. Code has been updated accordingly, PTAL. Thanks
a lot.

https://codereview.chromium.org/1099853002/diff/1/Source/core/html/canvas/Web...
File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right):

https://codereview.chromium.org/1099853002/diff/1/Source/core/html/canvas/Web...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:1372: if ((target !=
GL_FRAMEBUFFER && !isWebGL2OrHigher()) || (target != GL_READ_FRAMEBUFFER &&
target != GL_DRAW_FRAMEBUFFER)) {
On 2015/04/22 22:39:43, Zhenyao Mo wrote:
> This is incorrect.
> 
> GL_FRAMEBUFFER is also a valid target for ES2.

Yeah. But I made a mistake by this logic.

https://codereview.chromium.org/1099853002/diff/1/Source/core/html/canvas/Web...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:5377: if ((target !=
GL_FRAMEBUFFER && !isWebGL2OrHigher()) || (target != GL_READ_FRAMEBUFFER &&
target != GL_DRAW_FRAMEBUFFER)) {
On 2015/04/22 22:39:43, Zhenyao Mo wrote:
> Ditto.

Done.

[Zhenyao Mo, 2015-04-23 17:39:38]

LGTM

[Ken Russell (switch to Gerrit), 2015-04-23 17:54:32]

https://codereview.chromium.org/1099853002/diff/20001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right):

https://codereview.chromium.org/1099853002/diff/20001/Source/core/html/canvas...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:1372: if
(!((isWebGL2OrHigher() && (target == GL_READ_FRAMEBUFFER || target ==
GL_DRAW_FRAMEBUFFER)) || target == GL_FRAMEBUFFER)) {
These nested expressions are unreadable. Please factor them out into a
validateFramebufferTarget method (similar to validateBufferTarget) and override
it in WebGL2RenderingContextBase to add the new targets.

[yunchao, 2015-04-24 07:07:35]

Thanks for your reviewing, kbr@. Code has been updated accordingly, PTAL.

https://codereview.chromium.org/1099853002/diff/20001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right):

https://codereview.chromium.org/1099853002/diff/20001/Source/core/html/canvas...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:1372: if
(!((isWebGL2OrHigher() && (target == GL_READ_FRAMEBUFFER || target ==
GL_DRAW_FRAMEBUFFER)) || target == GL_FRAMEBUFFER)) {
On 2015/04/23 17:54:31, Ken Russell wrote:
> These nested expressions are unreadable. Please factor them out into a
> validateFramebufferTarget method (similar to validateBufferTarget) and
override
> it in WebGL2RenderingContextBase to add the new targets.

Agree. It is much clear. In fact, I also thought about adding a function to
validate the targets, but the functions is too simple...

[yunchao, 2015-04-28 05:09:55]

On 2015/04/24 07:07:35, yunchao wrote:
> Thanks for your reviewing, kbr@. Code has been updated accordingly, PTAL.
> 
>
https://codereview.chromium.org/1099853002/diff/20001/Source/core/html/canvas...
> File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right):
> 
>
https://codereview.chromium.org/1099853002/diff/20001/Source/core/html/canvas...
> Source/core/html/canvas/WebGLRenderingContextBase.cpp:1372: if
> (!((isWebGL2OrHigher() && (target == GL_READ_FRAMEBUFFER || target ==
> GL_DRAW_FRAMEBUFFER)) || target == GL_FRAMEBUFFER)) {
> On 2015/04/23 17:54:31, Ken Russell wrote:
> > These nested expressions are unreadable. Please factor them out into a
> > validateFramebufferTarget method (similar to validateBufferTarget) and
> override
> > it in WebGL2RenderingContextBase to add the new targets.
> 
> Agree. It is much clear. In fact, I also thought about adding a function to
> validate the targets, but the functions is too simple...

@kbr, and this one.

[Ken Russell (switch to Gerrit), 2015-04-28 05:18:59]

https://codereview.chromium.org/1099853002/diff/40001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right):

https://codereview.chromium.org/1099853002/diff/40001/Source/core/html/canvas...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:1384: if
(!m_framebufferBinding || !m_framebufferBinding->object())
Looking at this again, it occurs to me that this code won't work properly if
GL_READ_FRAMEBUFFER is passed since the draw and read framebuffers are different
binding points. Making this change without fixing up all of the code in
WebGLRenderingContextBase that accesses m_framebufferBinding makes the code less
correct rather than more correct. A better approach would be to also add a
protected virtual method like "getFramebufferBinding" which is overridden in
WebGL2RenderingContextBase, instead of WebGLRenderingContextBase accessing
m_framebufferBinding directly. Please do this.

[yunchao, 2015-04-29 07:40:05]

Patchset #4 (id:60001) has been deleted

[yunchao, 2015-04-29 07:42:00]

Patchset #4 (id:80001) has been deleted

[yunchao, 2015-04-29 07:43:22]

Thanks for your reviewing, kbr@. Code has been updated accordingly, PTAL when
you have time.

https://codereview.chromium.org/1099853002/diff/40001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right):

https://codereview.chromium.org/1099853002/diff/40001/Source/core/html/canvas...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:1384: if
(!m_framebufferBinding || !m_framebufferBinding->object())
On 2015/04/28 05:18:59, Ken Russell wrote:
> Looking at this again, it occurs to me that this code won't work properly if
> GL_READ_FRAMEBUFFER is passed since the draw and read framebuffers are
different
> binding points. Making this change without fixing up all of the code in
> WebGLRenderingContextBase that accesses m_framebufferBinding makes the code
less
> correct rather than more correct. A better approach would be to also add a
> protected virtual method like "getFramebufferBinding" which is overridden in
> WebGL2RenderingContextBase, instead of WebGLRenderingContextBase accessing
> m_framebufferBinding directly. Please do this.

Agree. I add a function getFramebufferBinding() to get the 'correct' framebuffer
bound to given target.

[yunchao, 2015-04-29 11:23:51]

On 2015/04/29 07:43:22, yunchao wrote:
> Thanks for your reviewing, kbr@. Code has been updated accordingly, PTAL when
> you have time.
> 
>
https://codereview.chromium.org/1099853002/diff/40001/Source/core/html/canvas...
> File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right):
> 
>
https://codereview.chromium.org/1099853002/diff/40001/Source/core/html/canvas...
> Source/core/html/canvas/WebGLRenderingContextBase.cpp:1384: if
> (!m_framebufferBinding || !m_framebufferBinding->object())
> On 2015/04/28 05:18:59, Ken Russell wrote:
> > Looking at this again, it occurs to me that this code won't work properly if
> > GL_READ_FRAMEBUFFER is passed since the draw and read framebuffers are
> different
> > binding points. Making this change without fixing up all of the code in
> > WebGLRenderingContextBase that accesses m_framebufferBinding makes the code
> less
> > correct rather than more correct. A better approach would be to also add a
> > protected virtual method like "getFramebufferBinding" which is overridden in
> > WebGL2RenderingContextBase, instead of WebGLRenderingContextBase accessing
> > m_framebufferBinding directly. Please do this.
> 
> Agree. I add a function getFramebufferBinding() to get the 'correct'
framebuffer
> bound to given target.

Other Framebuffer related APIs need to be updated too. To keep this one small
and easy to be reviewed, I will submit another patch to add GL_DRAW_FRAMEBUFFER
and GL_READ_FRAMEBUFFER to those APIs tomorrow.

[yunchao, 2015-04-29 11:27:07]

On 2015/04/29 11:23:51, yunchao wrote:
> On 2015/04/29 07:43:22, yunchao wrote:
> > Thanks for your reviewing, kbr@. Code has been updated accordingly, PTAL
when
> > you have time.
> > 
> >
>
https://codereview.chromium.org/1099853002/diff/40001/Source/core/html/canvas...
> > File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1099853002/diff/40001/Source/core/html/canvas...
> > Source/core/html/canvas/WebGLRenderingContextBase.cpp:1384: if
> > (!m_framebufferBinding || !m_framebufferBinding->object())
> > On 2015/04/28 05:18:59, Ken Russell wrote:
> > > Looking at this again, it occurs to me that this code won't work properly
if
> > > GL_READ_FRAMEBUFFER is passed since the draw and read framebuffers are
> > different
> > > binding points. Making this change without fixing up all of the code in
> > > WebGLRenderingContextBase that accesses m_framebufferBinding makes the
code
> > less
> > > correct rather than more correct. A better approach would be to also add a
> > > protected virtual method like "getFramebufferBinding" which is overridden
in
> > > WebGL2RenderingContextBase, instead of WebGLRenderingContextBase accessing
> > > m_framebufferBinding directly. Please do this.
> > 
> > Agree. I add a function getFramebufferBinding() to get the 'correct'
> framebuffer
> > bound to given target.
> 
> Other Framebuffer related APIs need to be updated too. To keep this one small
> and easy to be reviewed, I will submit another patch to add
GL_DRAW_FRAMEBUFFER
> and GL_READ_FRAMEBUFFER to those APIs tomorrow.

The other one is in preparation, but seems that I can not finish it today...

[Ken Russell (switch to Gerrit), 2015-04-30 01:41:51]

https://codereview.chromium.org/1099853002/diff/100001/Source/core/html/canva...
File Source/core/html/canvas/WebGLRenderingContextBase.h (right):

https://codereview.chromium.org/1099853002/diff/100001/Source/core/html/canva...
Source/core/html/canvas/WebGLRenderingContextBase.h:888: virtual
RefPtrWillBeMember<WebGLFramebuffer> getFramebufferBinding(GLenum target);
I don't think it's necessary or correct to return RefPtrWillBeMember<T> here.
You should just return a WebGLFramebuffer* here and call
m_framebufferBinding.get(). See https://www.chromium.org/blink/blink-gc and the
linked design document.

[yunchao, 2015-04-30 02:42:49]

Thanks for your reviewing, Ken. Code has been updated accordingly, PTAL.

https://codereview.chromium.org/1099853002/diff/100001/Source/core/html/canva...
File Source/core/html/canvas/WebGLRenderingContextBase.h (right):

https://codereview.chromium.org/1099853002/diff/100001/Source/core/html/canva...
Source/core/html/canvas/WebGLRenderingContextBase.h:888: virtual
RefPtrWillBeMember<WebGLFramebuffer> getFramebufferBinding(GLenum target);
On 2015/04/30 01:41:51, Ken Russell wrote:
> I don't think it's necessary or correct to return RefPtrWillBeMember<T> here.
> You should just return a WebGLFramebuffer* here and call
> m_framebufferBinding.get(). See https://www.chromium.org/blink/blink-gc and
the
> linked design document.

Done.

[Ken Russell (switch to Gerrit), 2015-04-30 02:54:01]

lgtm

[yunchao, 2015-04-30 05:38:24]

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

[yunchao, 2015-04-30 05:38:24]

The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org
Link to the patchset: https://codereview.chromium.org/1099853002/#ps120001
(title: "addressed kbr@'s feedback")

[commit-bot: I haz the power, 2015-04-30 05:38:28]

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

[commit-bot: I haz the power, 2015-04-30 07:28:49]

Committed patchset #5 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=194706