Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
974203002:
update getTexParameter for WebGL 2 (Closed)
Descriptionupdate getTexParameter for WebGL 2
BUG=295792
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194345
Patch Set 1 : #
Total comments: 11
Patch Set 2 : addressed zmo@'s feedback #
Total comments: 3
Patch Set 3 : addressed zmo@'s feedback: fix a bug caused by GLES spec error #
Total comments: 8
Patch Set 4 : addressed zmo@'s feedback #
Messages
Total messages: 24 (6 generated)
yunchao.he@intel.com changed reviewers: + bajones@chromium.org, kbr@chromium.org, zmo@chromium.org
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Please take a look. Thanks a lot! https://codereview.chromium.org/974203002/diff/40001/Source/core/html/canvas/... File Source/core/html/canvas/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/974203002/diff/40001/Source/core/html/canvas/... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1208: // return m_textureUnits[m_activeTextureUnit].m_texture2DArrayBinding.get(); Will Implement TexParameter and add 2D Array and 3D GLTexture later.
https://codereview.chromium.org/974203002/diff/40001/Source/core/html/canvas/... File Source/core/html/canvas/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/974203002/diff/40001/Source/core/html/canvas/... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1208: // return m_textureUnits[m_activeTextureUnit].m_texture2DArrayBinding.get(); On 2015/03/04 07:11:31, yunchao wrote: > Will Implement TexParameter and add 2D Array and 3D GLTexture later. You should add a FIXME and ASSERT(false) for now. https://codereview.chromium.org/974203002/diff/40001/Source/core/html/canvas/... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1228: case GL_TEXTURE_COMPARE_MODE: The above seven return type is GLenum, which is unsigned. https://codereview.chromium.org/974203002/diff/40001/Source/core/html/canvas/... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1229: case GL_TEXTURE_IMMUTABLE_LEVELS: This is defined as GLuint https://codereview.chromium.org/974203002/diff/40001/Source/core/html/canvas/... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1233: case GL_TEXTURE_SWIZZLE_A: These above four are not exposed in WebGL 2 https://codereview.chromium.org/974203002/diff/40001/Source/core/html/canvas/... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1246: case GL_TEXTURE_MAX_LEVEL: The above two return type is GLint.
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
zmo@, Thank you very much. I have updated the code. Please help to have a look. https://codereview.chromium.org/974203002/diff/40001/Source/core/html/canvas/... File Source/core/html/canvas/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/974203002/diff/40001/Source/core/html/canvas/... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1208: // return m_textureUnits[m_activeTextureUnit].m_texture2DArrayBinding.get(); On 2015/03/04 17:55:10, Zhenyao Mo wrote: > On 2015/03/04 07:11:31, yunchao wrote: > > Will Implement TexParameter and add 2D Array and 3D GLTexture later. > > You should add a FIXME and ASSERT(false) for now. Done. https://codereview.chromium.org/974203002/diff/40001/Source/core/html/canvas/... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1228: case GL_TEXTURE_COMPARE_MODE: On 2015/03/04 17:55:10, Zhenyao Mo wrote: > The above seven return type is GLenum, which is unsigned. Acknowledged. https://codereview.chromium.org/974203002/diff/40001/Source/core/html/canvas/... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1229: case GL_TEXTURE_IMMUTABLE_LEVELS: On 2015/03/04 17:55:10, Zhenyao Mo wrote: > This is defined as GLuint Acknowledged. https://codereview.chromium.org/974203002/diff/40001/Source/core/html/canvas/... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1233: case GL_TEXTURE_SWIZZLE_A: On 2015/03/04 17:55:10, Zhenyao Mo wrote: > These above four are not exposed in WebGL 2 Acknowledged. https://codereview.chromium.org/974203002/diff/40001/Source/core/html/canvas/... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1246: case GL_TEXTURE_MAX_LEVEL: On 2015/03/04 17:55:10, Zhenyao Mo wrote: > The above two return type is GLint. Acknowledged. It's my fault. When I implemented this according to WebGL 2 spec, I need to find details in openGL ES 3.0 spec and followed ES 3.0 spec. But forgot to double-check with WebGL 2.0 spec again...
https://codereview.chromium.org/974203002/diff/100001/Source/core/html/canvas... File Source/core/html/canvas/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/974203002/diff/100001/Source/core/html/canvas... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1247: GLfloat value = 0.f; This should be GLint, the function being getTexParameteriv, and no casting necessary.
https://codereview.chromium.org/974203002/diff/100001/Source/core/html/canvas... File Source/core/html/canvas/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/974203002/diff/100001/Source/core/html/canvas... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1247: GLfloat value = 0.f; On 2015/03/06 18:39:46, Zhenyao Mo wrote: > This should be GLint, the function being getTexParameteriv, and no casting > necessary. Thanks for your review, @zmo. Accroding to OpenGL ES 3.0 (and GLES 3.1) linked by WebGL spec, these two parameters are obtained by getTexParameterfv (not getTexParameteriv). See the table 6.9 in https://www.khronos.org/registry/gles/specs/3.0/es_spec_3.0.3.pdf#nameddest=s.... However, these two parameters are set by texParameter with type 'int' in table 3.20 in the same spec. Seems that the type for these two parameters are different in setter (texParameter) and getter (getTexParameter). In fact, I am a little confused here. But I suppose the spec is not wrong. I followed the GLES spec. So I choose getTexParameterfv and cast the type to int to obey WebGL spec. WDYT?
https://codereview.chromium.org/974203002/diff/100001/Source/core/html/canvas... File Source/core/html/canvas/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/974203002/diff/100001/Source/core/html/canvas... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1247: GLfloat value = 0.f; On 2015/03/09 02:25:25, yunchao wrote: > On 2015/03/06 18:39:46, Zhenyao Mo wrote: > > This should be GLint, the function being getTexParameteriv, and no casting > > necessary. > > Thanks for your review, @zmo. Accroding to OpenGL ES 3.0 (and GLES 3.1) linked > by WebGL spec, these two parameters are obtained by getTexParameterfv (not > getTexParameteriv). See the table 6.9 in > https://www.khronos.org/registry/gles/specs/3.0/es_spec_3.0.3.pdf#nameddest=s.... > However, these two parameters are set by texParameter with type 'int' in table > 3.20 in the same spec. > > Seems that the type for these two parameters are different in setter > (texParameter) and getter (getTexParameter). In fact, I am a little confused > here. But I suppose the spec is not wrong. I followed the GLES spec. So I choose > getTexParameterfv and cast the type to int to obey WebGL spec. WDYT? Yes, it seems there are some discrepancies in the ES3 spec. I will clarify them and get back to you.
On 2015/03/09 17:18:45, Zhenyao Mo wrote: > https://codereview.chromium.org/974203002/diff/100001/Source/core/html/canvas... > File Source/core/html/canvas/WebGL2RenderingContextBase.cpp (right): > > https://codereview.chromium.org/974203002/diff/100001/Source/core/html/canvas... > Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1247: GLfloat value = > 0.f; > On 2015/03/09 02:25:25, yunchao wrote: > > On 2015/03/06 18:39:46, Zhenyao Mo wrote: > > > This should be GLint, the function being getTexParameteriv, and no casting > > > necessary. > > > > Thanks for your review, @zmo. Accroding to OpenGL ES 3.0 (and GLES 3.1) linked > > by WebGL spec, these two parameters are obtained by getTexParameterfv (not > > getTexParameteriv). See the table 6.9 in > > > https://www.khronos.org/registry/gles/specs/3.0/es_spec_3.0.3.pdf#nameddest=s.... > > However, these two parameters are set by texParameter with type 'int' in table > > 3.20 in the same spec. > > > > Seems that the type for these two parameters are different in setter > > (texParameter) and getter (getTexParameter). In fact, I am a little confused > > here. But I suppose the spec is not wrong. I followed the GLES spec. So I > choose > > getTexParameterfv and cast the type to int to obey WebGL spec. WDYT? > > Yes, it seems there are some discrepancies in the ES3 spec. I will clarify them > and get back to you. Make sense. Thank you in advance, @zmo.
I asked around and people agree this looks like a spec bug in Table 6.9. Table 3.20 is correct, that these two are int type. So we should use getTexParameteriv for these two.
On 2015/03/10 16:56:32, Zhenyao Mo wrote: > I asked around and people agree this looks like a spec bug in Table 6.9. > > Table 3.20 is correct, that these two are int type. > > So we should use getTexParameteriv for these two. OK. Thank you, @zmo. The code has been updated accordingly. Please have a look.
Mostly looks good, but as kbr pointed out in other CLs, you should get a conformance test before landing this. https://codereview.chromium.org/974203002/diff/120001/Source/core/html/canvas... File Source/core/html/canvas/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/974203002/diff/120001/Source/core/html/canvas... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1209: ASSERT(false); Thinking more, maybe it's better to return nullptr here instead of ASSERTING and crash the renderer. https://codereview.chromium.org/974203002/diff/120001/Source/core/html/canvas... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1212: ASSERT(false); Same here. https://codereview.chromium.org/974203002/diff/120001/Source/core/html/canvas... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1258: case GL_TEXTURE_MAX_ANISOTROPY_EXT: // EXT_texture_filter_anisotropic Actually I am not sure about extension situation. kbr can advise on it. Will WebGL2 just carry over WebGL1 extensions? If yes, probably you want to do the same as validateTextureBinding(), i.e., only handle the WebGL 2 specific enums here, and delegate the rest to the WebGLRenderingContextBase::getTexParameter(). This way, in the future when we add an extension, we don't have to add the handing in multiple RenderingContext. https://codereview.chromium.org/974203002/diff/120001/Source/core/html/canvas... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1264: synthesizeGLError(GL_INVALID_ENUM, "getTexParameter", "invalid parameter name, EXT_texture_filter_anisotropic not enabled"); This is not needed. It can just fall through to the default.
Code has been updated accordingly. And the corresponding conformance test is submitted too: https://github.com/KhronosGroup/WebGL/pull/946. PTAL when you have time. Thanks! https://codereview.chromium.org/974203002/diff/120001/Source/core/html/canvas... File Source/core/html/canvas/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/974203002/diff/120001/Source/core/html/canvas... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1209: ASSERT(false); On 2015/03/11 17:31:40, Zhenyao Mo wrote: > Thinking more, maybe it's better to return nullptr here instead of ASSERTING and > crash the renderer. Done. https://codereview.chromium.org/974203002/diff/120001/Source/core/html/canvas... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1212: ASSERT(false); On 2015/03/11 17:31:40, Zhenyao Mo wrote: > Same here. Done. https://codereview.chromium.org/974203002/diff/120001/Source/core/html/canvas... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1258: case GL_TEXTURE_MAX_ANISOTROPY_EXT: // EXT_texture_filter_anisotropic On 2015/03/11 17:31:40, Zhenyao Mo wrote: > Actually I am not sure about extension situation. kbr can advise on it. Will > WebGL2 just carry over WebGL1 extensions? If yes, probably you want to do the > same as validateTextureBinding(), i.e., only handle the WebGL 2 specific enums > here, and delegate the rest to the WebGLRenderingContextBase::getTexParameter(). > This way, in the future when we add an extension, we don't have to add the > handing in multiple RenderingContext. Done. https://codereview.chromium.org/974203002/diff/120001/Source/core/html/canvas... Source/core/html/canvas/WebGL2RenderingContextBase.cpp:1264: synthesizeGLError(GL_INVALID_ENUM, "getTexParameter", "invalid parameter name, EXT_texture_filter_anisotropic not enabled"); On 2015/03/11 17:31:40, Zhenyao Mo wrote: > This is not needed. It can just fall through to the default. Acknowledged.
LGTM
By the way, just confirmed with bajones, that we don't implicitly carry over the WebGL 1 extensions to WebGL 2. So my apologies, but it seems that you need to revise based on that.
On 2015/04/22 22:54:17, Zhenyao Mo wrote: > By the way, just confirmed with bajones, that we don't implicitly carry over the > WebGL 1 extensions to WebGL 2. So my apologies, but it seems that you need to > revise based on that. @zmo, thanks for your reviewing. Yes, I agree with you that we should not implicitly carry over all WebGL 1 extensions to WebGL 2. Some extensions are removed or promoted into core against WebGL 2, for example, OES_texture_float, OES_texture_half_float and WEBGL_draw_buffers, among many other extensions. These extensions should not be carried to WebGL 2. We can refer to https://www.khronos.org/registry/webgl/extensions/, although descriptions on this website may need to be updated too. However, I think extension EXT_texture_filter_anisotropic should be carried to WebGL 2 according to https://www.khronos.org/registry/webgl/extensions/. Seems that this extension doesn't conflict with any core feature in WebGL 2 and/or GLES 3.0, and it is not subsumed by any core feature. WDYT? @zmo, @bajones, @kbr?
Although I still think this mechanism has the potential of leaking tex params enabled by WebGL 1 extensions into WebGL 2, in reality, I agree EXT_texture_filter_anisotropic will be carried over and there are no other potential WebGL 1 extensions that might cause issues here. So I guess the current CL is OK. kbr as the owner of the code should have the final say in this.
zmo@: thanks for carefully reviewing this. I would have missed the interaction with the anisotropy extension. The form of this fix looks fine. Let's land it and test it. LGTM
The CQ bit was checked by yunchao.he@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/974203002/140001
Message was sent while issue was closed.
Committed patchset #4 (id:140001) as https://src.chromium.org/viewvc/blink?view=rev&revision=194345 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
