Bug 65473 – [Texmap][Qt] Enable TextureMapperGL in platforms where BGRA is not present

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Noam Rosenthal]

Right now TextureMapperGL is hard-coded to use the BGRA extension, which is:
1. not always present;
2. not always needed, if the platform has native support for BGRA.

TextureMapperGL should support both cases.

[Noam Rosenthal]

Created attachment 102693 [details]
Patch

[Noam Rosenthal]

Created attachment 102695 [details]
Patch

[Benjamin Poulain]

Comment on attachment 102695 [details]
Patch

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

> Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:539
> +    // FIXME: use shaders for RGBA->BGRA swap if this becomes a performance issue.
> +    m_buffer->swapRGB();

I think it would be better to invert the color components direclty in a shader.
swapRGB() is doomed to be slow.

[Benjamin Poulain]

Comment on attachment 102695 [details]
Patch

Yep, thinking about it, that should really be done on the shader. That way we avoid adding swapRGB() at all on the texture mapper.

You still did not explain why your assign a ARGB buffer as BGRA. My guess is because you are targetting little endian so the component are inverted when assigned in 32 bits compared to passed as byte here??? But in that case you would have to check the endianess in order to check the format. Am I missing something? :)

[Noam Rosenthal]

Created attachment 102716 [details]
Patch

[Noam Rosenthal]

(In reply to comment #4)
> (From update of attachment 102695 [details])
> Yep, thinking about it, that should really be done on the shader. That way we avoid adding swapRGB() at all on the texture mapper.
As you wish.

> You still did not explain why your assign a ARGB buffer as BGRA. My guess is because you are targetting little endian so the component are inverted when assigned in 32 bits compared to passed as byte here??? But in that case you would have to check the endianess in order to check the format. Am I missing something? :)

I believe endianness is not an issue (see http://www.opengl.org/wiki/Pixel_Transfer). The problem is that both OpenGL and QImage are endian-agnostic, but in a non-compatible way. Qt is natively compatible with what OpenGL refers to as BGRA, which is not always available. 
What would really "fix" the problem is adding QImage::Format_ABGR32_Premultiplied, but that's far from trivial.

[Noam Rosenthal]

(In reply to comment #3)
> (From update of attachment 102695 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102695&action=review
> 
> > Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:539
> > +    // FIXME: use shaders for RGBA->BGRA swap if this becomes a performance issue.
> > +    m_buffer->swapRGB();
> 
> I think it would be better to invert the color components direclty in a shader.
> swapRGB() is doomed to be slow.

I thought about this again and I disagree. It's better to do this once in software while uploading, than to do it for every paint, albeit in hardware. That's how QtGL does it, I think it's the right choice.

I still believe that the second patch is good; Let's try to resolve this.

[Andreas Kling]

Comment on attachment 102695 [details]
Patch

r=me for correctness now.

[Noam Rosenthal]

Created attachment 108648 [details]
Patch

[WebKit Review Bot]

Comment on attachment 108648 [details]
Patch

Clearing flags on attachment: 108648

Committed r95939: <http://trac.webkit.org/changeset/95939>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.