Bug 183892

Summary: [GTK][WPE] Avoid software color conversion inside BitmapTextureGL
Product: WebKit Reporter: Miguel Gomez <magomez>
Component: WebKitGTKAssignee: Miguel Gomez <magomez>
Status: RESOLVED FIXED    
Severity: Normal CC: bugs-noreply, cmarcelo, commit-queue, ews-watchlist, kondapallykalyan, luiz, Ms2ger, noam, zan
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Miguel Gomez 2018-03-22 02:08:12 PDT
Currently, when used to upload cairo contents to a texture, BitmapTextureGL can use 2 formats: BGRA and RGBA. Ideally we should use BGRA because that's the internal format cairo uses and we don't need to perform any conversion to upload the data. But in order to use BGRA, we need to be using OpenGL or GLES2 with the extension GL_EXT_texture_format_BGRA8888. If none of that happens (like several embedded devices), we fallback to RGBA, where we need to swap the R and B components before uploading the data.

To avoid this conversion what we can do is to stop using BGRA and always use RGBA, and tell the shader to perform a color conversion when drawing the texture (now that the shader has the capability to do that). This also simplifies the code as we don't need to handle the BGRA case, don't need to check for the GL_EXT_texture_format_BGRA8888 extension, and we don't need to keep separate pools for RGBA and BGRA textures in BitmapTexturePool.
Comment 1 Miguel Gomez 2018-03-22 02:28:57 PDT
Created attachment 336269 [details]
Patch
Comment 2 Zan Dobersek 2018-03-22 05:59:42 PDT
Comment on attachment 336269 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/BitmapTextureGL.cpp:90
> +void BitmapTextureGL::updateContents(const void* srcData, const IntRect& targetRect, const IntPoint& sourceOffset, int bytesPerLine, UpdateContentsFlag)

Anything preventing UpdateContentsFlag removal? Or will you remove it in a following patch, adjusting all the call sites?

> Source/WebCore/platform/graphics/texmap/BitmapTextureGL.cpp:95
> +    m_textureMapperColorConvertFlags = TextureMapperGL::ShouldConvertTextureBGRAToRGBA;

The flag value should be ORed into the variable.

> Source/WebCore/platform/graphics/texmap/BitmapTextureGL.h:95
> +    TextureMapperGL::Flags m_textureMapperColorConvertFlags { TextureMapperGL::NoFlag };

m_colorConvertFlags is verbose enough IMO.
Comment 3 Miguel Gomez 2018-03-22 07:13:35 PDT
(In reply to Zan Dobersek from comment #2)
> Comment on attachment 336269 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=336269&action=review
> 
> > Source/WebCore/platform/graphics/texmap/BitmapTextureGL.cpp:90
> > +void BitmapTextureGL::updateContents(const void* srcData, const IntRect& targetRect, const IntPoint& sourceOffset, int bytesPerLine, UpdateContentsFlag)
> 
> Anything preventing UpdateContentsFlag removal? Or will you remove it in a
> following patch, adjusting all the call sites?

I can remove it, yes :)

> > Source/WebCore/platform/graphics/texmap/BitmapTextureGL.cpp:95
> > +    m_textureMapperColorConvertFlags = TextureMapperGL::ShouldConvertTextureBGRAToRGBA;
> 
> The flag value should be ORed into the variable.

Not really necessary. This variable will only be ShouldConvertTextureBGRAToRGBA or NoFlag, there aren't other values involved, so the OR would produce the same result.

> > Source/WebCore/platform/graphics/texmap/BitmapTextureGL.h:95
> > +    TextureMapperGL::Flags m_textureMapperColorConvertFlags { TextureMapperGL::NoFlag };
> 
> m_colorConvertFlags is verbose enough IMO.

Sure, I'll change it.


I'll give it a while before uploading the new patch as the wincairo bot is failing to apply the patches, and that's precisely the one I can break with this change, so I wanna be sure that it works.
Comment 4 Miguel Gomez 2018-03-23 02:44:37 PDT
Created attachment 336361 [details]
Patch
Comment 5 Miguel Gomez 2018-03-23 04:51:53 PDT
Created attachment 336364 [details]
Patch
Comment 6 Miguel Gomez 2018-03-23 06:32:25 PDT
Created attachment 336368 [details]
Patch
Comment 7 Miguel Gomez 2018-03-23 06:47:38 PDT
Created attachment 336369 [details]
Patch
Comment 8 WebKit Commit Bot 2018-03-23 08:29:35 PDT
Comment on attachment 336369 [details]
Patch

Clearing flags on attachment: 336369

Committed r229895: <https://trac.webkit.org/changeset/229895>
Comment 9 WebKit Commit Bot 2018-03-23 08:29:37 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Ms2ger (he/him; ⌚ UTC+1/+2) 2018-03-23 09:50:55 PDT
Comment on attachment 336369 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/BitmapTextureGL.cpp:135
> +    glTexSubImage2D(GL_TEXTURE_2D, 0, targetRect.x(), targetRect.y(), targetRect.width(), targetRect.height(), m_format, m_type, srcData);

Should this `srcData` be `data` instead?
Comment 11 Miguel Gomez 2018-03-26 06:03:02 PDT
(In reply to Ms2ger from comment #10)
> Comment on attachment 336369 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=336369&action=review
> 
> > Source/WebCore/platform/graphics/texmap/BitmapTextureGL.cpp:135
> > +    glTexSubImage2D(GL_TEXTURE_2D, 0, targetRect.x(), targetRect.y(), targetRect.width(), targetRect.height(), m_format, m_type, srcData);
> 
> Should this `srcData` be `data` instead?

Indeed!!! Good catch!!