Bug 89867 – [Texmap] Remove platform specific code in BitmapImageGL::updateContents by using GraphicsContext3D::extractImageData.

• Component: New Bugs
• Status: RESOLVED FIXED

[Dongseong Hwang]

[Texmap] Remove platform specific code in BitmapImageGL::updateContents by using GraphicsContext3D::extractImageData.

[Dongseong Hwang]

Created attachment 149272 [details]
Patch

[Dongseong Hwang]

Created attachment 149273 [details]
patch

[Texmap] Remove platform specific code in BitmapImageGL::updateContents by using GraphicsContext3D::extractImageData.

[Early Warning System Bot]

Comment on attachment 149273 [details]
patch

Attachment 149273 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13103029

[Early Warning System Bot]

Comment on attachment 149273 [details]
patch

Attachment 149273 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13087142

[Gustavo Noronha (kov)]

Comment on attachment 149273 [details]
patch

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

[Dongseong Hwang]

Oops, I made mistake.
This patch can be compiled after Bug 89866 is accepted.

[Noam Rosenthal]

Comment on attachment 149273 [details]
patch

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

I'd rather if we just moved TExtureMapperGL to use GC3D, see https://bugs.webkit.org/show_bug.cgi?id=78672.
Though this might be a good intermediate step. What do others think?

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:526
> +    GraphicsContext3D::extractImageData(image, GraphicsContext3D::RGBA, GraphicsContext3D::UNSIGNED_BYTE, false, premultiplyAlpha, false, imageData);

Add comments near the "false" arguments

[Noam Rosenthal]

Comment on attachment 149273 [details]
patch

Please fix nitpick comment before committing.

[Dongseong Hwang]

Sorry, I recognized that nitpick in subway to go home in yesterday night.
I'll fix it.

[Dongseong Hwang]

Created attachment 149396 [details]
patch v.3

[Dongseong Hwang]

Created attachment 149398 [details]
patch v.4

[Noam Rosenthal]

Comment on attachment 149398 [details]
patch v.4

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

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:523
> +    // The premultiplyAlpha is true, because the default value of alphaOption in ImageSource is AlphaPremultiplied.
> +    bool premultiplyAlpha = true;
> +    // The ignoreGammaAndColorProfile is false, because the default value of gammaAndColorProfileOption in ImageSource is GammaAndColorProfileApplied.
> +    bool ignoreGammaAndColorProfile = false;
> +    GraphicsContext3D::extractImageData(image, GraphicsContext3D::RGBA, GraphicsContext3D::UNSIGNED_BYTE, flipY, premultiplyAlpha, ignoreGammaAndColorProfile, imageData);

Not exactly what I meant...
something like: GraphicsContext3D::extractImageData(image, GraphicsContext3D::RGBA, GraphicsContext3D::UNSIGNED_BYTE, flipY, true /* premultiplyAlpha */, false /* ignoreGammaAndColorProfile */, imageData);
this way there are no magic "false" and "true" around.

[Dongseong Hwang]

Created attachment 149423 [details]
patch v.5

Ok. I'll remember it. :)

[Noam Rosenthal]

The other optimization we need to do, if we're to use this code, is to load the image directly to ARGB32_Premultiplied instead of to ARGB32 and then add a premultiply step.

[Dongseong Hwang]

When I called GraphicsContext3D::extractImageData(image, GraphicsContext3D::RGBA, GraphicsContext3D::UNSIGNED_BYTE, flipY, false /* premultiplyAlpha */, false /* ignoreGammaAndColorProfile */, imageData);
I can not get proper pixel data.

Could you explain briefly how we can get unpremultiplyAlpha pixel using GC3D?

[Noam Rosenthal]

(In reply to comment #15)
> When I called GraphicsContext3D::extractImageData(image, GraphicsContext3D::RGBA, GraphicsContext3D::UNSIGNED_BYTE, flipY, false /* premultiplyAlpha */, false /* ignoreGammaAndColorProfile */, imageData);
> I can not get proper pixel data.
> 
> Could you explain briefly how we can get unpremultiplyAlpha pixel using GC3D?

Sorry, it's inside GRaphicsContext3D::extractImageData. This comment should have been for bug 89865.

[Dongseong Hwang]

After this patch, if we run WebKit Leaves, you can see strange command message.
 libpng warning: Ignoring attempt to set cHRM RGB triangle with zero area

It is because of GraphicsContext3D::extractImageData.
I filed Bug 89938

[WebKit Review Bot]

Comment on attachment 149423 [details]
patch v.5

Clearing flags on attachment: 149423

Committed r121223: <http://trac.webkit.org/changeset/121223>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Hausmann]

This patch broke the minimal Qt build bot ;( 


/usr/bin/gold: /ramdisk/qt-linux-release-minimal/build/WebKitBuild/Release/Source/WebCore/release/libWebCore.a(TextureMapperGL.o): in function WebCore::BitmapTextureGL::updateContents(WebCore::Image*, WebCore::IntRect const&, WebCore::IntPoint const&):TextureMapperGL.cpp(.text._ZN7WebCore15BitmapTextureGL14updateContentsEPNS_5ImageERKNS_7IntRectERKNS_8IntPointE+0x6a): error: undefined reference to 'WebCore::GraphicsContext3D::extractImageData(WebCore::Image*, unsigned int, unsigned int, bool, bool, bool, WTF::Vector<unsigned char, 0u>&)'

[Simon Hausmann]

I'll try to fix the build. Essentially TextureMapperGL now requires WebGL.

[Simon Hausmann]

Minimal build fixed in http://trac.webkit.org/changeset/121226 . The bot however needs a clean rebuild now though, because the DEFINES in WebCore's Target.pri changed and TextureMapperGL.cpp should not be compiled anymore.

[Noam Rosenthal]

(In reply to comment #21)
> I'll try to fix the build. Essentially TextureMapperGL now requires WebGL.

It doesn't require WebGL, it requires GraphicsContext3D (which is a low-level part of WebGL). We should still be able to use TextureMapperGL without WebGL - should we create a separate bug to support this?

[Simon Hausmann]

(In reply to comment #23)
> (In reply to comment #21)
> > I'll try to fix the build. Essentially TextureMapperGL now requires WebGL.
> 
> It doesn't require WebGL, it requires GraphicsContext3D (which is a low-level part of WebGL). We should still be able to use TextureMapperGL without WebGL - should we create a separate bug to support this?

Yes, because GraphicsContext3D.cpp has a big #if ENABLE(WEBGL) at the top :)

I have the feeling that what's missing are some lower level convenience APIs for OpenGL that don't pull in the entire WebGL stuff, i.e. an OpenGL context abstraction and like in this case convenience for getting data out of a texture.

[Dongseong Hwang]

(In reply to comment #22)
> Minimal build fixed in http://trac.webkit.org/changeset/121226 . The bot however needs a clean rebuild now though, because the DEFINES in WebCore's Target.pri changed and TextureMapperGL.cpp should not be compiled anymore.

Thanks for your favor.
I should be more careful.

[Ryosuke Niwa]

It's still broken :(

[Simon Hausmann]

(In reply to comment #26)
> It's still broken :(

Yeah, but that's an issue on the bot. I built it locally and it works. The bot needs a nudge or somebody touching config.h ;)

[Csaba Osztrogonác]

(In reply to comment #27)
> (In reply to comment #26)
> > It's still broken :(
> 
> Yeah, but that's an issue on the bot. I built it locally and it works. The bot needs a nudge or somebody touching config.h ;)

rgabor triggered a clean build on the bot, it's green again. ;-)

[Noam Rosenthal]

(In reply to comment #24)
> (In reply to comment #23)
> > (In reply to comment #21)
> > > I'll try to fix the build. Essentially TextureMapperGL now requires WebGL.
> > 
> > It doesn't require WebGL, it requires GraphicsContext3D (which is a low-level part of WebGL). We should still be able to use TextureMapperGL without WebGL - should we create a separate bug to support this?
> 
> Yes, because GraphicsContext3D.cpp has a big #if ENABLE(WEBGL) at the top :)
> 
> I have the feeling that what's missing are some lower level convenience APIs for OpenGL that don't pull in the entire WebGL stuff, i.e. an OpenGL context abstraction and like in this case convenience for getting data out of a texture.

We're not missing it. GraphicsContext3D is it - we just need to change that ENABLE flag at the top :)