Bug 141729 - Drawing an SVG image into a canvas using drawImage() ignores globalAlpha.
Summary: Drawing an SVG image into a canvas using drawImage() ignores globalAlpha.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-02-17 15:05 PST by Said Abou-Hallawa
Modified: 2015-02-23 12:31 PST (History)
7 users (show)

See Also:


Attachments
Test case (667 bytes, text/html)
2015-02-17 15:05 PST, Said Abou-Hallawa
no flags Details
Patch (5.49 KB, patch)
2015-02-19 12:16 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (8.21 KB, patch)
2015-02-19 13:26 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (8.76 KB, patch)
2015-02-19 18:35 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (22.82 KB, patch)
2015-02-20 18:01 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (22.85 KB, patch)
2015-02-20 21:51 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2015-02-17 15:05:58 PST
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.
Comment 1 Radar WebKit Bug Importer 2015-02-17 15:06:53 PST
<rdar://problem/19866858>
Comment 2 Said Abou-Hallawa 2015-02-19 12:16:01 PST
Created attachment 246902 [details]
Patch
Comment 3 Said Abou-Hallawa 2015-02-19 13:26:14 PST
Created attachment 246913 [details]
Patch
Comment 4 Said Abou-Hallawa 2015-02-19 18:34:23 PST
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.
Comment 5 Said Abou-Hallawa 2015-02-19 18:35:12 PST
Created attachment 246932 [details]
Patch
Comment 6 Simon Fraser (smfr) 2015-02-19 18:57:46 PST
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.
Comment 7 Said Abou-Hallawa 2015-02-19 19:06:47 PST
(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.
Comment 8 zalan 2015-02-19 19:27:53 PST
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)
Comment 9 Said Abou-Hallawa 2015-02-20 18:01:10 PST
Created attachment 247027 [details]
Patch
Comment 10 Said Abou-Hallawa 2015-02-20 18:02:21 PST
(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.
Comment 11 zalan 2015-02-20 21:21:06 PST
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.
Comment 12 Said Abou-Hallawa 2015-02-20 21:51:17 PST
Created attachment 247040 [details]
Patch
Comment 13 WebKit Commit Bot 2015-02-23 12:31:21 PST
Comment on attachment 247040 [details]
Patch

Clearing flags on attachment: 247040

Committed r180511: <http://trac.webkit.org/changeset/180511>
Comment 14 WebKit Commit Bot 2015-02-23 12:31:27 PST
All reviewed patches have been landed.  Closing bug.