Bug 117868 (TextureUnits)

Summary: Texture unit code refactoring
Product: WebKit Reporter: Przemyslaw Szymanski <p.szymanski3>
Component: WebGLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, dino, esprehn+autocc, kalyan.kondapally, kondapallykalyan, noam, ostap73, p.szymanski3, simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
TextureUnit code otpimization
none
Texture unit code refactoring
none
Texture unit code refactoring
noam: review-
Texture unit code refactoring
noam: review+, noam: commit-queue-
Texture unit code refactoring
cdumez: commit-queue-
Texture unit code refactoring none

Description Przemyslaw Szymanski 2013-06-21 02:58:08 PDT
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
Comment 1 Przemyslaw Szymanski 2013-06-21 03:10:44 PDT
Created attachment 205165 [details]
Texture unit code refactoring
Comment 2 Przemyslaw Szymanski 2013-06-21 03:14:05 PDT
It is not possible to bind both texture2D and textureCubeMap in the same texture unit
Comment 3 Przemyslaw Szymanski 2013-06-21 04:56:46 PDT
Comment on attachment 205165 [details]
Texture unit code refactoring

some bugs in this patch
Comment 4 Przemyslaw Szymanski 2013-06-21 04:57:21 PDT
Created attachment 205172 [details]
Texture unit code refactoring
Comment 5 Chris Dumez 2013-06-26 06:26:29 PDT
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 6 Noam Rosenthal 2013-06-26 06:37:47 PDT
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.
Comment 7 Przemyslaw Szymanski 2013-06-27 00:32:44 PDT
Created attachment 205571 [details]
Texture unit code refactoring

Update after review
Comment 8 Przemyslaw Szymanski 2013-06-28 05:42:21 PDT
Noam Rosenthal, could you review again my update? 
Thank you
Comment 9 Noam Rosenthal 2013-06-28 07:17:11 PDT
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?
Comment 10 Przemyslaw Szymanski 2013-07-01 02:47:39 PDT
Created attachment 205791 [details]
Texture unit code refactoring
Comment 11 Chris Dumez 2013-07-01 02:51:34 PDT
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?
Comment 12 Przemyslaw Szymanski 2013-07-01 03:00:24 PDT
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 13 Chris Dumez 2013-07-01 03:06:20 PDT
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.
Comment 14 Przemyslaw Szymanski 2013-07-01 03:09:59 PDT
Created attachment 205792 [details]
Texture unit code refactoring

change log modified: there it -> it
Comment 15 Chris Dumez 2013-07-01 03:12:12 PDT
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.
Comment 16 Przemyslaw Szymanski 2013-07-01 03:14:22 PDT
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 17 Przemyslaw Szymanski 2013-07-01 03:15:00 PDT
Comment on attachment 205792 [details]
Texture unit code refactoring

sorry, missclick
Comment 18 Chris Dumez 2013-07-01 03:16:29 PDT
(In reply to comment #17)
> (From update of attachment 205792 [details])
> sorry, missclick

cq? not r?
Comment 19 Przemyslaw Szymanski 2013-07-01 03:21:45 PDT
Comment on attachment 205792 [details]
Texture unit code refactoring

cq?
If this struct is ok for you. Thank you.
Comment 20 Chris Dumez 2013-07-01 03:25:31 PDT
(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.
Comment 21 Kalyan 2013-07-01 04:47:39 PDT
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??
Comment 22 Przemyslaw Szymanski 2013-07-02 02:40:37 PDT
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.
Comment 23 Kalyan 2013-07-02 03:25:03 PDT
(In reply to comment #22)
> I hope it is all clear now. Thank you for your attention.

LGTM
Comment 24 Kalyan 2013-07-02 03:33:23 PDT
(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.
Comment 25 Przemyslaw Szymanski 2013-07-03 05:31:56 PDT
Noam Rosenthal, can you confirm whether this patch is ok now? Thank you
Comment 26 WebKit Commit Bot 2013-07-03 06:00:39 PDT
Comment on attachment 205792 [details]
Texture unit code refactoring

Clearing flags on attachment: 205792

Committed r152351: <http://trac.webkit.org/changeset/152351>
Comment 27 WebKit Commit Bot 2013-07-03 06:00:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 28 Dean Jackson 2013-10-04 19:16:14 PDT
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.
Comment 29 Przemyslaw Szymanski 2013-10-06 23:28:20 PDT
(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
Comment 30 Dean Jackson 2013-10-07 12:19:13 PDT
As, sorry I missed it. Anyway, it was reverted (and slightly cleaned up) in https://bugs.webkit.org/show_bug.cgi?id=122369