Bug 117868 – Texture unit code refactoring

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Przemyslaw Szymanski]

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

[Przemyslaw Szymanski]

Created attachment 205165 [details]
Texture unit code refactoring

[Przemyslaw Szymanski]

It is not possible to bind both texture2D and textureCubeMap in the same texture unit

[Przemyslaw Szymanski]

Comment on attachment 205165 [details]
Texture unit code refactoring

some bugs in this patch

[Przemyslaw Szymanski]

Created attachment 205172 [details]
Texture unit code refactoring

[Chris Dumez]

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.

[Noam Rosenthal]

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.

[Przemyslaw Szymanski]

Created attachment 205571 [details]
Texture unit code refactoring

Update after review

[Przemyslaw Szymanski]

Noam Rosenthal, could you review again my update? 
Thank you

[Noam Rosenthal]

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?

[Przemyslaw Szymanski]

Created attachment 205791 [details]
Texture unit code refactoring

[Chris Dumez]

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?

[Przemyslaw Szymanski]

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.

[Chris Dumez]

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.

[Przemyslaw Szymanski]

Created attachment 205792 [details]
Texture unit code refactoring

change log modified: there it -> it

[Chris Dumez]

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.

[Przemyslaw Szymanski]

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.

[Przemyslaw Szymanski]

Comment on attachment 205792 [details]
Texture unit code refactoring

sorry, missclick

[Chris Dumez]

(In reply to comment #17)
> (From update of attachment 205792 [details])
> sorry, missclick

cq? not r?

[Przemyslaw Szymanski]

Comment on attachment 205792 [details]
Texture unit code refactoring

cq?
If this struct is ok for you. Thank you.

[Chris Dumez]

(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.

[Kalyan]

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??

[Przemyslaw Szymanski]

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.

[Kalyan]

(In reply to comment #22)
> I hope it is all clear now. Thank you for your attention.

LGTM

[Kalyan]

(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.

[Przemyslaw Szymanski]

Noam Rosenthal, can you confirm whether this patch is ok now? Thank you

[WebKit Commit Bot]

Comment on attachment 205792 [details]
Texture unit code refactoring

Clearing flags on attachment: 205792

Committed r152351: <http://trac.webkit.org/changeset/152351>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dean Jackson]

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.

[Przemyslaw Szymanski]

(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

[Dean Jackson]

As, sorry I missed it. Anyway, it was reverted (and slightly cleaned up) in https://bugs.webkit.org/show_bug.cgi?id=122369