Bug 98728 – [Texmap] Swizzling BGRA to RGBA in temp buffer while USE(TEXMAP_OPENGL_ES_2)

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Hyungchan Kim]

[Texmap] Swizzling BGRA to RGBA in temp buffer while USE(TEXMAP_OPENGL_ES_2)

[Hyungchan Kim]

Created attachment 167706 [details]
Patch

[Hyungchan Kim]

Created attachment 167708 [details]
Patch

[Hyungchan Kim]

This problem happens in QtWebKit for Pandaboard ES.
Launched QtTestBrowser under X with -graphicsbased, -gl-viewport options.

[kov's GTK+ EWS bot]

Comment on attachment 167708 [details]
Patch

Attachment 167708 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14245039

[Noam Rosenthal]

Comment on attachment 167708 [details]
Patch

This image is usually the backing store of a temporary ImageBuffer. This would create an extra unnecessary deep copy in the common case.

[Hyungchan Kim]

Created attachment 167746 [details]
Patch

[Hyungchan Kim]

Thanks for reviewing, I agree with you.
I didn't consider that common case, but I think swizzleBGRAToRGBA seems have a problem.
In BitmapTextureGL::updateContents, const char *imageData should not be modified by swizzleBGRAToRGBA because 
1) const_cast used
2) seems to be shared textures which point same NativeImagePtr

A and B keep changing its data from BGRA to RGBA or from RGBA to BGRA. So, both A and B cannot have correct color simultaneously.

[Noam Rosenthal]

Comment on attachment 167746 [details]
Patch

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

In some cases it's ok to swizzle the actual data (when it's a backing store for a temporary ImageBuffer), and in some cases it's not (when it's a directly composited image). We should add an enum parameter to updateContents that stands for whether it's ok to swizzle the actual data without copying.

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:726
> +        void* srcData = reinterpret_cast<void *>(const_cast<char*>(imageData));

why do you need srcData? If you're copying, you can use imageData directly...

[Hyungchan Kim]

Created attachment 168839 [details]
Patch

[Hyungchan Kim]

Thanks for your review. I modified some code to use temp buffer only for directly composited image and did refactoring to use same temp buffer which was already used for driverSupportsSubImage() condition.

[Noam Rosenthal]

Comment on attachment 168839 [details]
Patch

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

This is getting closer - however this should be an enum that gets passed along updateContents, not a bool member; Also this is not about directly composited images, it's about whether BitmapTexture is allowed to modify the original image. Something like enum UpdateContentsFlag { UpdateCanModifyOriginalImageData = 0x01 }

> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:76
> +void TextureMapperTile::updateContents(TextureMapper* textureMapper, Image* image, const IntRect& dirtyRect, bool isDirectlyCompositedImage)

This is a boolean trap... it should be an enum, like ContentUpdateOptions

> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:98
> +    if (isDirectlyCompositedImage)
> +        m_texture->setDirectlyCompositedFlag();
>      m_texture->updateContents(image, targetRect, sourceOffset);
> +    if (isDirectlyCompositedImage)
> +        m_texture->resetDirectlyCompositedFlag();

You don't really need this as a member. Pass it through the updateContents functions instead.

[Hyungchan Kim]

Created attachment 169095 [details]
Patch

[Noam Rosenthal]

Comment on attachment 169095 [details]
Patch

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

Great! Please fix nits and then commit or cq?

> Source/WebCore/ChangeLog:13
> +        BitmapTextureGL::updateContents() try to swizzle source Image if opengl
> +        driver doesn't support BGRA format.
> +        In case of directly composited image, swizzling source image
> +        should not be modified because another BitmapTexture will also try
> +        to swizzle.
> +
> +        BitmapTexture now provide UpdateContentsFlag to separate whether
> +        source image can be modified or not

Some rewording:
BitmapTexture swizzles the source image if the OpenGL driver doesn't support the BGRA extension.
In case of directly composited images, the source image should not be modified.

BitmapTexture::updateContents now accepts an UpdateContentsFlag argument to separate the different cases.

> Source/WebCore/ChangeLog:19
> +        No new tests.

This should be tested - there are a few current test cases for directly composited images, do they test this?

> Source/WebCore/platform/graphics/texmap/TextureMapper.h:62
> +    enum UpdateContentsFlag {
> +        UpdateCanModifyOriginalImageData = 0x01,
> +        UpdateCannotModifyOriginalImageData = 0x02
> +    };

Any reason to use 0x01 and 0x02 instead of 0 and 1?

> Source/WebCore/platform/graphics/texmap/TextureMapperImageBuffer.cpp:57
> +void BitmapTextureImageBuffer::updateContents(Image* image, const IntRect& targetRect, const IntPoint& offset, UpdateContentsFlag /*updatecontentsflag*/)

You don't need the comment, just omit the argument name.

[Noam Rosenthal]

Comment on attachment 169095 [details]
Patch

Sorry, changed to r- until we figure out if the existing tests actually cover this. I don't want this regressing in the future...

[Hyungchan Kim]

Created attachment 169353 [details]
Patch

[Hyungchan Kim]

I fixed ChangeLog and removed some comments of unused parameters.
But, I have no idea how to write test only for Qt Linux with OpenGLES.

It would be great if you can give me some guide for writing test for this issue.
I can compose a test for this, but it will always succeed under OpenGL support.

[Noam Rosenthal]

(In reply to comment #16)
> I fixed ChangeLog and removed some comments of unused parameters.
> But, I have no idea how to write test only for Qt Linux with OpenGLES.
> 
> It would be great if you can give me some guide for writing test for this issue.
> I can compose a test for this, but it will always succeed under OpenGL support.

OK - please update the changelog to say that this is only testable on specific hardware not currently available in the bots.

[Hyungchan Kim]

Created attachment 169423 [details]
Patch

[Hyungchan Kim]

Can someone set cq+ for me? 
Or please let me know if there is something to get cq+.

[WebKit Review Bot]

Comment on attachment 169423 [details]
Patch

Clearing flags on attachment: 169423

Committed r132019: <http://trac.webkit.org/changeset/132019>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.