Bug 183892 – [GTK][WPE] Avoid software color conversion inside BitmapTextureGL

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Miguel Gomez]

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.

[Miguel Gomez]

Created attachment 336269 [details]
Patch

[Zan Dobersek]

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.

[Miguel Gomez]

(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.

[Miguel Gomez]

Created attachment 336361 [details]
Patch

[Miguel Gomez]

Created attachment 336364 [details]
Patch

[Miguel Gomez]

Created attachment 336368 [details]
Patch

[Miguel Gomez]

Created attachment 336369 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 336369 [details]
Patch

Clearing flags on attachment: 336369

Committed r229895: <https://trac.webkit.org/changeset/229895>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ms2ger (he/him; ⌚ UTC+1/+2)]

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?

[Miguel Gomez]

(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!!