Drawing an SVG image into a canvas using drawImage() ignores globalAlpha.

• Component: SVG
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

Created attachment 246772 [details]
Test case

Open the attached test case.  It has a canvas on which two objects are drawn after setting the globalAlpha to 0.2. The first one is a solid red rectangle and the second is an SVG. The SVG simply draws a solid red rectangle also.

Result: The globalAlpha is applied to the rectangle but it is not applied to the SVG.
Expected: Drawing the SVG image should take into account the globalAlpha.

[Radar WebKit Bug Importer]

<rdar://problem/19866858>

[Said Abou-Hallawa]

Created attachment 246902 [details]
Patch

[Said Abou-Hallawa]

Created attachment 246913 [details]
Patch

[Said Abou-Hallawa]

The fix is to force rendering the SVG image to be though a transparent layer if the global alpha is less than 1. Few comments about this fix is necessary:

-- We can check whether the SVG image is drawn on with transparency or not by checking the alpha value set to the graphics context.

-- setAlpha() does not stack the alpha value unless the drawing happens on transparent layer which can be nested as well.

-- The reason for this bug to happen is setAlpha() has to be called before drawing any SVG element since the opacity is part of the element style. So if the graphics context is not set to render on a transparent layer, we end up setting the global alpha of the context and the drawing can be fully opaque if opacity is set to be 1.

[Said Abou-Hallawa]

Created attachment 246932 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 246932 [details]
Patch

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

> Source/WebCore/svg/graphics/SVGImage.cpp:236
> -        context->beginTransparencyLayer(1);
> +        context->beginTransparencyLayer(alpha);

If I understand CG correctly, you don't need this. Ending the transparency layer should use the context alpha.

[Said Abou-Hallawa]

(In reply to comment #6)
> Comment on attachment 246932 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=246932&action=review
> 
> > Source/WebCore/svg/graphics/SVGImage.cpp:236
> > -        context->beginTransparencyLayer(1);
> > +        context->beginTransparencyLayer(alpha);
> 
> If I understand CG correctly, you don't need this. Ending the transparency
> layer should use the context alpha.

GraphicsContext::beginTransparencyLayer() calls GraphicsContext::beginPlatformTransparencyLayer() which is the following:

void GraphicsContext::beginPlatformTransparencyLayer(float opacity)
{
    if (paintingDisabled())
        return;

    save();

    CGContextRef context = platformContext();
    CGContextSetAlpha(context, opacity);
    CGContextBeginTransparencyLayer(context, 0);
    m_data->m_userToDeviceTransformKnownToBeIdentity = false;
}

The first thing it does is setting the global alpha. If I keep passing 1 to context->beginTransparencyLayer(1), the global alpha of the context will change before calling CGContextBeginTransparencyLayer(). The last alpha set to the context will be the one that is used when CGContextEndTransparencyLayer() gets called.

If you want to make beginTransparencyLayer() takes no parameter, this would be a bigger change since this function is used in many places and I can see some callers rely on it to set the global alpha.

[zalan]

Comment on attachment 246932 [details]
Patch

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

> Source/WebCore/platform/graphics/GraphicsContext.cpp:691
> +void GraphicsContext::setAlpha(float alpha)
> +{
> +    m_state.alpha = alpha;
> +    setPlatformAlpha(alpha);
> +}
> +
> +float GraphicsContext::alpha() const
> +{
> +    return m_state.alpha;
> +}

These could go to the .h

> Source/WebCore/platform/graphics/GraphicsContext.h:126
> +            , alpha(1)

Use in-class member initialization, please.
(and it'd be great If you could fix the rest of the initializers too)

[Said Abou-Hallawa]

Created attachment 247027 [details]
Patch

[Said Abou-Hallawa]

(In reply to comment #8)
> Comment on attachment 246932 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=246932&action=review
> 
> > Source/WebCore/platform/graphics/GraphicsContext.cpp:691
> > +void GraphicsContext::setAlpha(float alpha)
> > +{
> > +    m_state.alpha = alpha;
> > +    setPlatformAlpha(alpha);
> > +}
> > +
> > +float GraphicsContext::alpha() const
> > +{
> > +    return m_state.alpha;
> > +}
> 
> These could go to the .h
> 

I moved all the single lines getter/setters to the header file.

> > Source/WebCore/platform/graphics/GraphicsContext.h:126
> > +            , alpha(1)
> 
> Use in-class member initialization, please.
> (and it'd be great If you could fix the rest of the initializers too)

Done.

[zalan]

Comment on attachment 247027 [details]
Patch

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

> Source/WebCore/platform/graphics/GraphicsContext.h:138
> +        float strokeThickness = 0;

Please use { 0 }; syntax instead.

[Said Abou-Hallawa]

Created attachment 247040 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 247040 [details]
Patch

Clearing flags on attachment: 247040

Committed r180511: <http://trac.webkit.org/changeset/180511>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.