Bug 61442 – [chromium] Red and Blue channels are swapped in images with accelerated drawing

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Alok Priyadarshi]

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.

[Alok Priyadarshi]

Created attachment 97308 [details]
proposed patch

[Mike Reed]

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

[Alok Priyadarshi]

Created attachment 97316 [details]
proposed patch

Fixed merge issues

[Alok Priyadarshi]

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

[WebKit Review Bot]

Comment on attachment 97316 [details]
proposed patch

Attachment 97316 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8866127

[Mike Reed]

Sorry. Yes, still relevant, since I'm not offering to fix chrome yet. I just intended to make an oh-that-reminds-me comment.

[WebKit Review Bot]

Comment on attachment 97316 [details]
proposed patch

Attachment 97316 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8866131

[Alok Priyadarshi]

Created attachment 97320 [details]
proposed patch

Fixed compile errors on linux and mac.

[Adrienne Walker]

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

[Alok Priyadarshi]

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

[Adrienne Walker]

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

[Alok Priyadarshi]

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

[Alok Priyadarshi]

Created attachment 97643 [details]
proposed patch

Eliminated software swizzling by choosing the shader at run time.

[James Robinson]

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?

[Alok Priyadarshi]

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.

[Alok Priyadarshi]

Created attachment 98004 [details]
proposed patch

Added template functions drawTiles and drawTexturedQuad to avoid the indirection pointed out by jamesr.

[James Robinson]

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)?

[Adrienne Walker]

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

[Alok Priyadarshi]

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.

[James Robinson]

Yes, if you could rename the shaders in ShaderChromium.h I think that'd be a big improvement.  Thanks!

[Alok Priyadarshi]

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.

[James Robinson]

Comment on attachment 98260 [details]
proposed patch

R=me

[James Robinson]

(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

[WebKit Review Bot]

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

[Alok Priyadarshi]

Created attachment 98414 [details]
proposed patch

The last patch was missing ChangeLog.

[WebKit Review Bot]

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

[James Robinson]

You have to edit the "Reviewed by ..." line in the changelog by hand before updating to cq+ without waiting for review

[WebKit Review Bot]

Comment on attachment 98414 [details]
proposed patch

Clearing flags on attachment: 98414

Committed r89634: <http://trac.webkit.org/changeset/89634>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.