Bug 81103 – [Texmap] Implement BGRA swizzling detection

• Component: Platform
• Status: RESOLVED FIXED

[Simon Hausmann]

We used to check if GL_EXT_texture_format_BGRA8888 is listed in the extensions to assume that the driver supports swizzling from GL_BGRA to the GL_RGBA texture format. Unfortunately this does not work reliably with all chipsets.

[Simon Hausmann]

r110703 removed the swizzling for GLES.

[Simon Hausmann]

From http://www.khronos.org/opengles/sdk/docs/man/xhtml/glTexSubImage2D.xml it appears that the format passed to glTexSubImage2D is supposed to be the same:

"GL_INVALID_OPERATION is generated if the texture array has not been defined by a previous glTexImage2D or glCopyTexImage2D operation whose internalformat matches the format of glTexSubImage2D."

[Simon Hausmann]

Another interesting point:

<k5kangas> You might also want to quote OpenGLES2 spec, v2.0.25, page 72, "Each of the TexSubImage commands
<k5kangas> interprets and processes pixel groups in exactly the manner of its TexImage coun-
<k5kangas> terpart, except that the assignment of R, G, B, and A pixel group values to the
<k5kangas> texture components is controlled by the internalformat of the texture array, not
<k5kangas> by an argument to the command.

[Noam Rosenthal]

(In reply to comment #3)
> Another interesting point:
> 
> <k5kangas> You might also want to quote OpenGLES2 spec, v2.0.25, page 72, "Each of the TexSubImage commands
> <k5kangas> interprets and processes pixel groups in exactly the manner of its TexImage coun-
> <k5kangas> terpart, except that the assignment of R, G, B, and A pixel group values to the
> <k5kangas> texture components is controlled by the internalformat of the texture array, not
> <k5kangas> by an argument to the command.

Interesting. I wonder why this is the only thing that had worked for desktop and Harmattan then :)

[Igor Trindade Oliveira]

Created attachment 186167 [details]
Patch

Proposed patch

[Viatcheslav Ostapenko]

Comment on attachment 186167 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:589
> +    return context->getExtensions()->supports("GL_EXT_texture_format_BGRA8888");

It is called so many times. I doubt that swizzling support can change during app execution and would cache result here.
Something like this:

static bool canSwizzle = context->getExtensions()->supports("GL_EXT_texture_format_BGRA8888");
return canSwizzle;

[Igor Trindade Oliveira]

Created attachment 186175 [details]
Patch V2

Proposed patch.

[Igor Trindade Oliveira]

Created attachment 186178 [details]
Patch v3

Fix left over from other patch.

[Noam Rosenthal]

Comment on attachment 186178 [details]
Patch v3

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

I remember this kind of patch had a tendency to break stuff :)
Can someone validate it on more than one GLES2 hardware?

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:591
>  #if defined(TEXMAP_OPENGL_ES_2)
> -    // FIXME: Implement reliable detection. See also https://bugs.webkit.org/show_bug.cgi?id=81103.
> -    return false;
> +    static bool supportsExternalTextureBGRA = context->getExtensions()->supports("GL_EXT_texture_format_BGRA8888");
> +    return supportsExternalTextureBGRA;
>  #else

I think we shoul switch here to isGLES2Compliant() instead of an #ifdef

[Igor Trindade Oliveira]

(In reply to comment #9)
> (From update of attachment 186178 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186178&action=review
> 
> I remember this kind of patch had a tendency to break stuff :)
> Can someone validate it on more than one GLES2 hardware?

I tested in mesa3d llvmpipe, intel and mali t400.
The reverted implementation(changeset r110703) was slightly different. Basically it was just using BGRA in the external format, however in the GLESv2 case, when BGRA is used it also needs to be used in the internal texture format. 

> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:591
> >  #if defined(TEXMAP_OPENGL_ES_2)
> > -    // FIXME: Implement reliable detection. See also https://bugs.webkit.org/show_bug.cgi?id=81103.
> > -    return false;
> > +    static bool supportsExternalTextureBGRA = context->getExtensions()->supports("GL_EXT_texture_format_BGRA8888");
> > +    return supportsExternalTextureBGRA;
> >  #else
> 
> I think we shoul switch here to isGLES2Compliant() instead of an #ifdef

[Noam Rosenthal]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 186178 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=186178&action=review
> > 
> > I remember this kind of patch had a tendency to break stuff :)
> > Can someone validate it on more than one GLES2 hardware?
> 
> I tested in mesa3d llvmpipe, intel and mali t400.
> The reverted implementation(changeset r110703) was slightly different. Basically it was just using BGRA in the external format, however in the GLESv2 case, when BGRA is used it also needs to be used in the internal texture format. 
Yes, in the N9 case that's the only thing that worked (and N9 was the only GLESv2 device running TextureMapper at the time).
But I think this should be ok, if you can address the other comment :)

[Noam Rosenthal]

Comment on attachment 186178 [details]
Patch v3

See previous comment.

[Igor Trindade Oliveira]

Created attachment 186427 [details]
Patch

Patch for landing.

[WebKit Review Bot]

Comment on attachment 186427 [details]
Patch

Clearing flags on attachment: 186427

Committed r141807: <http://trac.webkit.org/changeset/141807>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.