Bug 65473 - [Texmap][Qt] Enable TextureMapperGL in platforms where BGRA is not present
Summary: [Texmap][Qt] Enable TextureMapperGL in platforms where BGRA is not present
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Unspecified
: P2 Normal
Assignee: Noam Rosenthal
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2011-08-01 08:44 PDT by Noam Rosenthal
Modified: 2011-09-26 03:51 PDT (History)
2 users (show)

See Also:


Attachments
Patch (3.62 KB, patch)
2011-08-02 13:57 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (3.62 KB, patch)
2011-08-02 13:58 PDT, Noam Rosenthal
kling: review+
Details | Formatted Diff | Diff
Patch (8.09 KB, patch)
2011-08-02 17:00 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (3.62 KB, patch)
2011-09-26 02:34 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Noam Rosenthal 2011-08-01 08:44:56 PDT
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.
Comment 1 Noam Rosenthal 2011-08-02 13:57:13 PDT
Created attachment 102693 [details]
Patch
Comment 2 Noam Rosenthal 2011-08-02 13:58:18 PDT
Created attachment 102695 [details]
Patch
Comment 3 Benjamin Poulain 2011-08-02 16:36:17 PDT
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.
Comment 4 Benjamin Poulain 2011-08-02 16:52:52 PDT
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? :)
Comment 5 Noam Rosenthal 2011-08-02 17:00:52 PDT
Created attachment 102716 [details]
Patch
Comment 6 Noam Rosenthal 2011-08-02 17:07:31 PDT
(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.
Comment 7 Noam Rosenthal 2011-09-23 07:53:09 PDT
(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.
Comment 8 Andreas Kling 2011-09-26 02:30:26 PDT
Comment on attachment 102695 [details]
Patch

r=me for correctness now.
Comment 9 Noam Rosenthal 2011-09-26 02:34:33 PDT
Created attachment 108648 [details]
Patch
Comment 10 WebKit Review Bot 2011-09-26 03:51:07 PDT
Comment on attachment 108648 [details]
Patch

Clearing flags on attachment: 108648

Committed r95939: <http://trac.webkit.org/changeset/95939>
Comment 11 WebKit Review Bot 2011-09-26 03:51:14 PDT
All reviewed patches have been landed.  Closing bug.