Bug 122369 - Undo texture unit code refactoring - it is ok to bind a texture to multiple locations
Summary: Undo texture unit code refactoring - it is ok to bind a texture to multiple l...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords: InRadar
: revert (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-10-04 19:15 PDT by Dean Jackson
Modified: 2013-10-07 12:18 PDT (History)
6 users (show)

See Also:


Attachments
Patch (11.04 KB, patch)
2013-10-04 19:39 PDT, Dean Jackson
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2013-10-04 19:15:35 PDT
The fix for 117868 is an optimisation that causes us to fail the WebGL conformance test. In particular, it is valid to bind a single texture to multiple locations, as long as each location is in a different program.

Revert the change http://trac.webkit.org/changeset/152351
Comment 1 Radar WebKit Bug Importer 2013-10-04 19:15:45 PDT
<rdar://problem/15158465>
Comment 2 Dean Jackson 2013-10-04 19:39:51 PDT
Created attachment 213432 [details]
Patch
Comment 3 Darin Adler 2013-10-05 00:37:18 PDT
Comment on attachment 213432 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=213432&action=review

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:607
> +        m_textureUnits[i].m_texture2DBinding = 0;
> +        m_textureUnits[i].m_textureCubeMapBinding = 0;

nullptr is what we now use instead of 0 in new code like this

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:1634
> +            m_textureUnits[i].m_texture2DBinding = 0;

nullptr

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:1636
> +            m_textureUnits[i].m_textureCubeMapBinding = 0;

nullptr

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4877
> +    WebGLTexture* tex = 0;

nullptr

Also, can we call this texture instead of tex?

> Source/WebCore/html/canvas/WebGLRenderingContext.h:453
> +    class TextureUnitState {
> +    public:

struct seems better than class/public, although normally we do not use the m_ prefix on public data members in structs like this
Comment 4 Dean Jackson 2013-10-05 12:14:22 PDT
Comment on attachment 213432 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=213432&action=review

I was right to be implicitly chastised for not taking the opportunity to clean up code :)

>> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4877
>> +    WebGLTexture* tex = 0;
> 
> nullptr
> 
> Also, can we call this texture instead of tex?

Done.

>> Source/WebCore/html/canvas/WebGLRenderingContext.h:453
>> +    public:
> 
> struct seems better than class/public, although normally we do not use the m_ prefix on public data members in structs like this

I changed this to a struct, and removed the m_ prefixes.
Comment 5 Dean Jackson 2013-10-05 12:21:09 PDT
Committed r156970: <http://trac.webkit.org/changeset/156970>
Comment 6 Dean Jackson 2013-10-07 12:18:33 PDT
*** Bug 120285 has been marked as a duplicate of this bug. ***