[Texmap] Remove platform specific code in BitmapImageGL::updateContents by using GraphicsContext3D::extractImageData.
Created attachment 149272 [details] Patch
Created attachment 149273 [details] patch [Texmap] Remove platform specific code in BitmapImageGL::updateContents by using GraphicsContext3D::extractImageData.
Comment on attachment 149273 [details] patch Attachment 149273 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13103029
Comment on attachment 149273 [details] patch Attachment 149273 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13087142
Comment on attachment 149273 [details] patch Attachment 149273 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13099052
Oops, I made mistake. This patch can be compiled after Bug 89866 is accepted.
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
Comment on attachment 149273 [details] patch Please fix nitpick comment before committing.
Sorry, I recognized that nitpick in subway to go home in yesterday night. I'll fix it.
Created attachment 149396 [details] patch v.3
Created attachment 149398 [details] patch v.4
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.
Created attachment 149423 [details] patch v.5 Ok. I'll remember it. :)
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.
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?
(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.
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
Comment on attachment 149423 [details] patch v.5 Clearing flags on attachment: 149423 Committed r121223: <http://trac.webkit.org/changeset/121223>
All reviewed patches have been landed. Closing bug.
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>&)'
I'll try to fix the build. Essentially TextureMapperGL now requires WebGL.
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.
(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?
(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.
(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.
It's still broken :(
(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 ;)
(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. ;-)
(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 :)