Created attachment 205163 [details] TextureUnit code otpimization WebGL specification points to OpenGL ES 2.0 specification (http://www.khronos.org/registry/gles/specs/2.0/es_full_spec_2.0.25.pdf) and according to OpenGL ES 2.0 specification section 2.10: "It is not allowed to have variables of different sampler types pointing to the same texture image unit within a program object. This situation can only be detected at the next rendering command issued, and an INVALID_OPERATION error will then be generated" So now it is possible to bind both texture2D and textureCubeMap in the same texture unit. Variables m_texture2DBinding and m_textureCubeMapBinding in WebGLRenderingContext::TextureUnitState are not necessary. We can use just one variable m_textureBinding and provide less code for textureUnit operations and also slightly increase rendering performance
Created attachment 205165 [details] Texture unit code refactoring
It is not possible to bind both texture2D and textureCubeMap in the same texture unit
Comment on attachment 205165 [details] Texture unit code refactoring some bugs in this patch
Created attachment 205172 [details] Texture unit code refactoring
Comment on attachment 205172 [details] Texture unit code refactoring View in context: https://bugs.webkit.org/attachment.cgi?id=205172&action=review > Source/WebCore/ChangeLog:7 > + Please add a Changelog.
Comment on attachment 205172 [details] Texture unit code refactoring View in context: https://bugs.webkit.org/attachment.cgi?id=205172&action=review R- due to missing changelog. > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:-612 > - m_textureUnits[i].m_texture2DBinding = 0; 4 spaces for indent please > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4811 > + const GC3Denum target = m_textureUnits[ii].m_textureBinding->getTarget(); We don't use local const in webkit.
Created attachment 205571 [details] Texture unit code refactoring Update after review
Noam Rosenthal, could you review again my update? Thank you
Comment on attachment 205571 [details] Texture unit code refactoring View in context: https://bugs.webkit.org/attachment.cgi?id=205571&action=review > Source/WebCore/ChangeLog:8 > + According to OpenGL ES 2.0 specification there is not possible to use both there -> it > Source/WebCore/ChangeLog:11 > + and makes WebKit more consistent with OpenGL Period at end of sentence. > Source/WebCore/ChangeLog:13 > + No new tests. Covered by existing tests ditto. btw always nice to put a reference to one of the tests that covers this. > Source/WebCore/html/canvas/WebGLRenderingContext.h:454 > + struct TextureUnitState { > + RefPtr<WebGLTexture> m_textureBinding; > }; Does this need to be a struct?
Created attachment 205791 [details] Texture unit code refactoring
Comment on attachment 205791 [details] Texture unit code refactoring View in context: https://bugs.webkit.org/attachment.cgi?id=205791&action=review > Source/WebCore/ChangeLog:8 > + According to OpenGL ES 2.0 specification there it is not possible to use both "there it" -> "it" > Source/WebCore/html/canvas/WebGLRenderingContext.h:452 > + struct TextureUnitState { As Noam said, why use a struct here when there is only one member?
In my opinion it should be a struct due to direct access to its member(s) just like in structs. And in this case it also a little simplifies the code. We can make it (in a future patch) a class with all features e.g. member initialization in the constructor, member clear (null) in the destructor, etc.
Comment on attachment 205791 [details] Texture unit code refactoring View in context: https://bugs.webkit.org/attachment.cgi?id=205791&action=review > Source/WebCore/html/canvas/WebGLRenderingContext.h:455 > Vector<TextureUnitState> m_textureUnits; My proposal was not to make TextureUnitState a class but to get rid of TextureUnitState altogether. Then we could have the following member: Vector<RefPtr<WebGLTexture> > m_textureUnits; Would that make sense? Noam should confirm but this is how I interpreted his comment.
Created attachment 205792 [details] Texture unit code refactoring change log modified: there it -> it
Please don't r+ your own patches. Leave the r flag unset and set cq? flag only if you have already updated the reviewer in the Changelog.
In my opinion making a vector: Vector<RefPtr<WebGLTexture> > m_textureUnits; is not a good idea because of a future patch(es). There could be another members so it will be refactored again to struct/class. For example, we prepare another patch with texture units which adds another member to TextureUnitState.
Comment on attachment 205792 [details] Texture unit code refactoring sorry, missclick
(In reply to comment #17) > (From update of attachment 205792 [details]) > sorry, missclick cq? not r?
Comment on attachment 205792 [details] Texture unit code refactoring cq? If this struct is ok for you. Thank you.
(In reply to comment #19) > (From update of attachment 205792 [details]) > cq? > If this struct is ok for you. Thank you. Looks fine to me based on your explanation but I'll let Noam take one last look.
binding TEXTURE_2D and TEXTURE_CUBE_MAP to the same texture unit should be fine as long as they are not used at the same time in a shader program. conformance/textures/texture-active-bind-2.html test covers this, seems missing in your change log. Have you checked this test??
Maybe I didn't wirte patch description clear. OpenGL allow to bind textures of different types in the same texture unit as you mentioned and I didn't change it in the WebKit code. I only removed unnecessary texture variable from TextureUnitState because OpenGL allow to make operations only on actually bound texture at the same time. So in the WebKit code there is used only one pointer to texture at the same time in the same texture unit. So it is still possible to: bindTexture(TEXTURE_2D); bindTexture(TEXTURE_CUBE_MAP); with no erros, but in WebKit after each texture bind TextureUnitState.m_textureBinding is changed (first texture2D then textureCubeMap). And to your question: yes, my patch is covered also by conformance/textures/texture-active-bind-2.html and conformance/textures/texture-active-bind.html My patch is covered by all existing texture tests which are pass without my patch (so my patch doesn't break any of those tests). I hope it is all clear now. Thank you for your attention.
(In reply to comment #22) > I hope it is all clear now. Thank you for your attention. LGTM
(In reply to comment #22) > Maybe I didn't wirte patch description clear. OpenGL allow to bind textures of different types in the same texture unit as you mentioned and I didn't change it in the WebKit code. > I only removed unnecessary texture variable from TextureUnitState because OpenGL allow to make operations only on actually bound texture at the same time. So in the WebKit code there is used only one pointer to texture at the same time in the same texture unit. > > So it is still possible to: > bindTexture(TEXTURE_2D); > bindTexture(TEXTURE_CUBE_MAP); > with no erros, but in WebKit after each texture bind TextureUnitState.m_textureBinding is changed (first texture2D then textureCubeMap). > Yes, my question was only regarding the tests.
Noam Rosenthal, can you confirm whether this patch is ok now? Thank you
Comment on attachment 205792 [details] Texture unit code refactoring Clearing flags on attachment: 205792 Committed r152351: <http://trac.webkit.org/changeset/152351>
All reviewed patches have been landed. Closing bug.
After some discussion in https://github.com/KhronosGroup/WebGL/pull/391 we've concluded that this optimisation is incorrect. Basically, it's ok to bind the same texture unit to multiple points as long as they are in different programs. I'm going to revert this in 122369 Afterwards, you can decide whether or not to optimise again, taking the program into account.
(In reply to comment #28) > After some discussion in https://github.com/KhronosGroup/WebGL/pull/391 > we've concluded that this optimisation is incorrect. > > Basically, it's ok to bind the same texture unit to multiple points as long as they are in different programs. I'm going to revert this in 122369 > > Afterwards, you can decide whether or not to optimise again, taking the program into account. I tried to revert it in patch https://bugs.webkit.org/show_bug.cgi?id=120285 but seems that noone reviewed it yet
As, sorry I missed it. Anyway, it was reverted (and slightly cleaned up) in https://bugs.webkit.org/show_bug.cgi?id=122369