Red and Blue channels are swapped in images with --enable-accelerated-drawing flag in chromium. Image layers are fine. It only affects images in content and root layers that use skia to do accelerated drawing. Test case: http://www.satine.org/research/webkit/snowleopard/snowstack.html The images on top three rows are on content layers and have red and blue channels swapped. The bottom row (reflection) is a on image layer which is fine.
Created attachment 97308 [details] proposed patch
If we cleaned up chrome to not confuse SkColor and SkPMColor, I think this would all be unnecessary, since chrome could always define the byte-order for SkPMColor to be whatever it wants (i.e. matching GL's order).
Created attachment 97316 [details] proposed patch Fixed merge issues
(In reply to comment #2) > If we cleaned up chrome to not confuse SkColor and SkPMColor, I think this would all be unnecessary, since chrome could always define the byte-order for SkPMColor to be whatever it wants (i.e. matching GL's order). I have not looked at SkPMColor. But I think this patch is still relevant because it consolidates the decision about byte order into one place (PlatformColor) instead of using RGBA everywhere.
Comment on attachment 97316 [details] proposed patch Attachment 97316 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8866127
Sorry. Yes, still relevant, since I'm not offering to fix chrome yet. I just intended to make an oh-that-reminds-me comment.
Comment on attachment 97316 [details] proposed patch Attachment 97316 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/8866131
Created attachment 97320 [details] proposed patch Fixed compile errors on linux and mac.
(In reply to comment #8) > Created an attachment (id=97320) [details] > proposed patch > > Fixed compile errors on linux and mac. I'm not that comfortable with the idea repacking pixels on the CPU for cards that don't support BGRA textures. This is possibly because I don't have enough information about which cards/platforms support this extension, other than knowing that ANGLE always does. I would much rather just see us continue to choose a different shader to use based on extension support rather than always uploading via BGRA.
(In reply to comment #9) > (In reply to comment #8) > > Created an attachment (id=97320) [details] [details] > > proposed patch > > > > Fixed compile errors on linux and mac. > > I'm not that comfortable with the idea repacking pixels on the CPU for cards that don't support BGRA textures. This is possibly because I don't have enough information about which cards/platforms support this extension, other than knowing that ANGLE always does. > > I would much rather just see us continue to choose a different shader to use based on extension support rather than always uploading via BGRA. I thought of using a different shader if BGRA is not supported. But in the current LayerRendererChromium architecture, shader selection is done at compile time using a templated class. Refactoring it may be a big change. AFAIK most GPUs on little-endian platform like BGRA. Using RGBA is in fact slower because the drivers may be unpacking in software. So the current implementation may already be incurring this cost. Could someone with driver experience comment? We could also look at gpu stats if we collect such information.
(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > Created an attachment (id=97320) [details] [details] [details] > > > proposed patch > > > > > > Fixed compile errors on linux and mac. > > > > I'm not that comfortable with the idea repacking pixels on the CPU for cards that don't support BGRA textures. This is possibly because I don't have enough information about which cards/platforms support this extension, other than knowing that ANGLE always does. > > > > I would much rather just see us continue to choose a different shader to use based on extension support rather than always uploading via BGRA. > > I thought of using a different shader if BGRA is not supported. But in the current LayerRendererChromium architecture, shader selection is done at compile time using a templated class. Refactoring it may be a big change. There's no refactoring needed. The templatization of ProgramBinding is just glue to remove duplicated uniform code. There's nothing really special about it. One option would be to just add your own small ProgramBindingBase-derived class that took an extra texture format parameter during initialization and picked between two fragment shaders. LayerRendererChromium could just own that instead of its current tiling shader.
(In reply to comment #9) > (In reply to comment #8) > > Created an attachment (id=97320) [details] [details] > > proposed patch > > > > Fixed compile errors on linux and mac. > > I'm not that comfortable with the idea repacking pixels on the CPU for cards that don't support BGRA textures. This is possibly because I don't have enough information about which cards/platforms support this extension, other than knowing that ANGLE always does. > > I would much rather just see us continue to choose a different shader to use based on extension support rather than always uploading via BGRA. BTW ANGLE always supports BGRA and it is the only format that does not require swizzling. It swizzles RGBA. RGBA: http://code.google.com/p/angleproject/source/browse/trunk/src/libGLESv2/Texture.cpp#667 BGRA: http://code.google.com/p/angleproject/source/browse/trunk/src/libGLESv2/Texture.cpp#755 BGRA is the optimal data format on MAC. In fact RGBA is not recommended. http://developer.apple.com/library/mac/#documentation/GraphicsImaging/Conceptual/OpenGL-MacProgGuide/opengl_texturedata/opengl_texturedata.html#//apple_ref/doc/uid/TP40001987-CH407-SW22 So I think the only question here is linux.
Created attachment 97643 [details] proposed patch Eliminated software swizzling by choosing the shader at run time.
Comment on attachment 97643 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=97643&action=review I don't understand the extra indirection added to LayerTilerChromium.h. Why can't you just typedef two different ProgramBinding<>s to ProgramRGBA and ProgramBGRA? Otherwise this seems fairly reasonable. > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:164 > + if (!m_skiaContext && m_contextSupportsTextureFormatBGRA && m_contextSupportsReadFormatBGRA) { so what happens if these extensions are not available? we always return NULL from this function? > Source/WebCore/platform/graphics/chromium/LayerTilerChromium.h:99 > - typedef ProgramBinding<VertexShaderPosTexTransform, FragmentShaderTexAlpha> Program; > + class Program { > + public: > + virtual unsigned program() const = 0; > + virtual int vertexMatrixLocation() const = 0; > + virtual int vertexTexTransformLocation() const = 0; > + virtual int fragmentAlphaLocation() const = 0; > + virtual int fragmentSamplerLocation() const = 0; > + }; > + > + template<class VertexShader, class FragmentShader> > + class ProgramT : public Program { > + public: > + explicit ProgramT(GraphicsContext3D* context) : m_binding(context) { } > + > + bool initialized() const { return m_binding.initialized(); } > + void initialize() { m_binding.initialize(); } > + > + virtual unsigned program() const { return m_binding.program(); } > + virtual int vertexMatrixLocation() const { return m_binding.vertexShader().matrixLocation(); } > + virtual int vertexTexTransformLocation() const { return m_binding.vertexShader().texTransformLocation(); } > + virtual int fragmentAlphaLocation() const { return m_binding.fragmentShader().alphaLocation(); } > + virtual int fragmentSamplerLocation() const { return m_binding.fragmentShader().samplerLocation(); } > + > + private: > + typedef ProgramBinding<VertexShader, FragmentShader> Binding; > + Binding m_binding; > + }; I'm confused - why do these two types exist?
Comment on attachment 97643 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=97643&action=review >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:164 >> + if (!m_skiaContext && m_contextSupportsTextureFormatBGRA && m_contextSupportsReadFormatBGRA) { > > so what happens if these extensions are not available? we always return NULL from this function? Yes in that case this function always returns NULL and we fall back to non-accelerated path. >> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.h:99 >> + }; > > I'm confused - why do these two types exist? Because LayerTilerChromium::drawTexturedQuad takes LayerTilerChromium::Program argument and I was trying to avoid making it a template function. In hindsight making it a template function is better. I will upload an updated patch shortly.
Created attachment 98004 [details] proposed patch Added template functions drawTiles and drawTexturedQuad to avoid the indirection pointed out by jamesr.
Comment on attachment 98004 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=98004&action=review I wonder if this would be a little clearer if we expressed things in terms of whether the pixel shader swizzled colors or not rather than comparing different texture formats. Vangelis, Enne - any thoughts? > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:140 > + extensions->ensureEnabled("GL_CHROMIUM_map_sub"); > + m_contextSupportsTextureFormatBGRA = extensions->supports("GL_EXT_texture_format_BGRA8888"); > + if (m_contextSupportsTextureFormatBGRA) > + extensions->ensureEnabled("GL_EXT_texture_format_BGRA8888"); > + m_contextSupportsReadFormatBGRA = extensions->supports("GL_EXT_read_format_bgra"); > + if (m_contextSupportsReadFormatBGRA) > + extensions->ensureEnabled("GL_EXT_read_format_bgra"); nitpick: I am pretty sure we don't need to call ensureEnabled() for anything with the compositor context, since that notion only exists for WebGL contexts. I'll double check with Ken/Gregg on this. > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1167 > + if (!m_tilerProgramBGRA) > + m_tilerProgramBGRA = adoptPtr(new LayerTilerChromium::ProgramBGRA(m_context.get())); nitpick: I think in practice we'll nearly always use the BGRA program, since BGRA_EXT is nearly always available, so it's probably better to eagerly create this program in initializeSharedObjects() instead of the tilerProgramRGBA. > Source/WebCore/platform/graphics/chromium/PlatformColor.h:39 > + return SK_B32_SHIFT ? GraphicsContext3D::SourceFormatRGBA8 : GraphicsContext3D::SourceFormatBGRA8; what's SK_B32_SHIFT? What header defines it (and should we explicitly be including that header in here)?
(In reply to comment #17) > (From update of attachment 98004 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=98004&action=review > > I wonder if this would be a little clearer if we expressed things in terms of whether the pixel shader swizzled colors or not rather than comparing different texture formats. Vangelis, Enne - any thoughts? I'd find that much less confusing. As I'm reading it, BGRA pixels with a BGRA texture format use the RGBA shader, because it wouldn't need swizzling. Anybody looking at this for the first time might be confused by that and think that mismatch was an error. I know that the RGBA/BGRA shader names predate this patch by a good while, but it'd be nice to see that confusion cleaned up a little.
Comment on attachment 98004 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=98004&action=review I could rename the shader programs to reflect swizzling. Should I rename the original classes as well? http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/chromium/ShaderChromium.h#L117 >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:140 >> + extensions->ensureEnabled("GL_EXT_read_format_bgra"); > > nitpick: I am pretty sure we don't need to call ensureEnabled() for anything with the compositor context, since that notion only exists for WebGL contexts. I'll double check with Ken/Gregg on this. I would be more than happy to remove this kludge if not required. >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1167 >> + m_tilerProgramBGRA = adoptPtr(new LayerTilerChromium::ProgramBGRA(m_context.get())); > > nitpick: I think in practice we'll nearly always use the BGRA program, since BGRA_EXT is nearly always available, so it's probably better to eagerly create this program in initializeSharedObjects() instead of the tilerProgramRGBA. Actually that is backwards (due to confusing names). If BGRA_EXT is available, you would use the RGBA program because there is no need for swizzling. >> Source/WebCore/platform/graphics/chromium/PlatformColor.h:39 >> + return SK_B32_SHIFT ? GraphicsContext3D::SourceFormatRGBA8 : GraphicsContext3D::SourceFormatBGRA8; > > what's SK_B32_SHIFT? What header defines it (and should we explicitly be including that header in here)? That is actually a compile-time flag that instructs skia to use bgra. I will include appropriate header here.
Yes, if you could rename the shaders in ShaderChromium.h I think that'd be a big improvement. Thanks!
Created attachment 98260 [details] proposed patch Addressed most of the comments. - Renamed shader program. - Included file where SK_B32_SHIFT is defined. - Did not removed ensureEnabled() calls because I was not sure if they are necessary. If they are not necessary we should remove them in a separate patch.
Comment on attachment 98260 [details] proposed patch R=me
(In reply to comment #21) > - Did not removed ensureEnabled() calls because I was not sure if they are necessary. If they are not necessary we should remove them in a separate patch. I agree, I'll try to figure out wtf is up with these
Comment on attachment 98260 [details] proposed patch Rejecting attachment 98260 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-02', '--port..." exit_code: 2 Last 500 characters of output: 60e16085ae5a2e413e0f3754fb1607935957f1d5 r89619 = a874ed9ac633d9541bc963a7e4ebe6a72062c53c Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/origin/master. Updating chromium port dependencies using gclient... ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Full output: http://queues.webkit.org/results/8933235
Created attachment 98414 [details] proposed patch The last patch was missing ChangeLog.
Comment on attachment 98414 [details] proposed patch Rejecting attachment 98414 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-01', '--port..." exit_code: 1 Last 500 characters of output: f/SkPDFGraphicState.h U /mnt/git/webkit-commit-queue/Source/WebKit/chromium/third_party/skia/include/views/SkWindow.h U /mnt/git/webkit-commit-queue/Source/WebKit/chromium/third_party/skia/include/views/SkOSWindow_Android.h U /mnt/git/webkit-commit-queue/Source/WebKit/chromium/third_party/skia/include/views/SkView.h Updated to revision 1684. ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Full output: http://queues.webkit.org/results/8936205
You have to edit the "Reviewed by ..." line in the changelog by hand before updating to cq+ without waiting for review
Comment on attachment 98414 [details] proposed patch Clearing flags on attachment: 98414 Committed r89634: <http://trac.webkit.org/changeset/89634>
All reviewed patches have been landed. Closing bug.