Bug 65473

Summary: [Texmap][Qt] Enable TextureMapperGL in platforms where BGRA is not present
Product: WebKit Reporter: Noam Rosenthal <noam>
Component: Layout and RenderingAssignee: Noam Rosenthal <noam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, webkit.review.bot
Priority: P2 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: All   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
kling: review+
Patch
none
Patch none

Noam Rosenthal
Reported 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.
Attachments
Patch (3.62 KB, patch)
2011-08-02 13:57 PDT, Noam Rosenthal 		no flags
DetailsFormatted DiffDiff
Patch (3.62 KB, patch)
2011-08-02 13:58 PDT, Noam Rosenthal 		kling: review+
DetailsFormatted DiffDiff
Patch (8.09 KB, patch)
2011-08-02 17:00 PDT, Noam Rosenthal 		no flags
DetailsFormatted DiffDiff
Patch (3.62 KB, patch)
2011-09-26 02:34 PDT, Noam Rosenthal 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Noam Rosenthal
Comment 1 2011-08-02 13:57:13 PDT
Created attachment 102693 [details] Patch
Noam Rosenthal
Comment 2 2011-08-02 13:58:18 PDT
Created attachment 102695 [details] Patch
Benjamin Poulain
Comment 3 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.
Benjamin Poulain
Comment 4 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? :)
Noam Rosenthal
Comment 5 2011-08-02 17:00:52 PDT
Created attachment 102716 [details] Patch
Noam Rosenthal
Comment 6 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.
Noam Rosenthal
Comment 7 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.
Andreas Kling
Comment 8 2011-09-26 02:30:26 PDT
Comment on attachment 102695 [details] Patch r=me for correctness now.
Noam Rosenthal
Comment 9 2011-09-26 02:34:33 PDT
Created attachment 108648 [details] Patch
WebKit Review Bot
Comment 10 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>
WebKit Review Bot
Comment 11 2011-09-26 03:51:14 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.