Bug 76557 - [Chromium] Layout Test svg/custom/transform-with-shadow-and-gradient.svg is failing
Summary: [Chromium] Layout Test svg/custom/transform-with-shadow-and-gradient.svg is f...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Florin Malita
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-18 11:26 PST by James Robinson
Modified: 2013-02-27 08:47 PST (History)
7 users (show)

See Also:


Attachments
Inconsistent shadow. (640 bytes, image/svg+xml)
2013-02-26 07:41 PST, Florin Malita
no flags Details
Patch (233.53 KB, patch)
2013-02-27 07:18 PST, Florin Malita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2012-01-18 11:26:34 PST
The following layout test is failing on chromium linux:

svg/custom/transform-with-shadow-and-gradient.svg

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=svg%2Fcustom%2Ftransform-with-shadow-and-gradient.svg

The stroked circle and rectangle have black centers when they should be white.  So far this has only failed on linux, but it may just have not cycled on the other bots yet.
Comment 1 James Robinson 2012-01-18 11:46:08 PST
Update: yes, this fails on all platforms that use skia (not just linux).
Comment 2 Stephen Chenney 2012-04-06 10:44:05 PDT
This may be an SVG bug, or maybe a Skia bug.

The problematic shapes are these:

    <circle transform="scale(4,4)" style="-webkit-svg-shadow: black 1px 1px 1px;" stroke="url(#gradient)" fill="transparent" cx="25" cy="100" r="20"></circle>
    <rect transform="scale(32,32)" style="-webkit-svg-shadow: black 1px 1px 1px;" stroke="url(#gradient)" fill="transparent" x="10" y="10" width="5" height="5"></rect>

Note the fill is transparent. When constructing the shadow, we are clearly not considering the object to be unfilled. We are instead presumably considering it filled and drawing the shadow for a filled shape.
Comment 3 Stephen White 2012-04-06 11:19:54 PDT
I haven't looked into this in detail, but if SVG is creating a bitmap from the primitive, then drawing a shadow from the bitmap's alpha, that's a known issue in Skia.  I have a fix in the works, but it hasn't landed yet:  http://codereview.appspot.com/5981063/
Comment 4 Stephen White 2012-04-06 11:22:35 PDT
There was also another issue with shadows on hairline paths (width <= 1px), which junov@ recently fix:  http://code.google.com/p/skia/source/detail?r=3592.  I don't know if WebKit's chromium DEPS have updated past that Skia revision yet.
Comment 6 Florin Malita 2013-02-26 07:40:14 PST
Doesn't look like we're rasterizing the shapes - we're just passing drawXXX calls to Skia.

I can confirm that this is caused by fill='transparent' (when using fill='none' the shadows are drawn correctly). I have a couple of thoughts on this, each may be a separate bug & fix this issue independently:

* at a high level, is 'transparent' not equivalent to 'none' for SVG fill/stroke? If so, we need to treat the same.

* shadows appear to observe the fill/stroke alpha when specified via opacity attributes, but not when specified as part of the color (rgba(x,x,x,a)). This seems inconsistent.
Comment 7 Florin Malita 2013-02-26 07:41:07 PST
Created attachment 190281 [details]
Inconsistent shadow.
Comment 8 Florin Malita 2013-02-26 07:59:06 PST
(In reply to comment #6)
> Doesn't look like we're rasterizing the shapes - we're just passing drawXXX calls to Skia.

Never mind, we are drawing offscreen due to shadows being implemented via transparency layers.
Comment 9 Florin Malita 2013-02-26 10:52:27 PST
(In reply to comment #6)
> * shadows appear to observe the fill/stroke alpha when specified via opacity attributes, but not when specified as part of the color (rgba(x,x,x,a)). This seems inconsistent.

The explanation for this discrepancy is that an opacity attribute forces yet another transparency layer.

Mike helped me trace the shadow alpha behavior, and it turns out to be caused by the transfer mode we use in  GraphicsContext::setPlatformShadow(). According to comments there, we ignore the alpha for CSS (and currently SVG), and only use it for Canvas:

        // CSS wants us to ignore the original's alpha, but Canvas wants us to
        // modulate with it. Using shadowsIgnoreTransforms to tell us that we're
        // in a Canvas, we change the colormode to kDst_Mode, so we don't overwrite
        // it with our layer's (default opaque-black) color.
        colorMode = SkXfermode::kDst_Mode;

We've verified that forcing the transfer mode to kDst_Mode (instead of the default kSrc_Mode) fixes the problem and aligns the results with Safari. Surprisingly, this did not appear to break CSS box-shadow. After some digging around, it turns out that RenderBoxModelObject::paintBoxShadow() already forces the paint color to opaque black (and actually draws the shadow on its own, not as part of the background fill) so this precaution appears to be unnecessary:

SVG: shadows are applied as part of the fill/stroke drawing and observe the paint alpha (kDst_Mode is what we want)
CSS: shadows are applied in a separate pass, using an opaque paint per spec (kDst_Mode still OK since the color is opaque)

It seems that we can safely always use kDst_Mode here, what do you think?
Comment 10 Florin Malita 2013-02-27 07:18:32 PST
Created attachment 190516 [details]
Patch
Comment 11 Stephen Chenney 2013-02-27 07:22:31 PST
Comment on attachment 190516 [details]
Patch

lgtm. R=me.
Comment 12 Stephen Chenney 2013-02-27 07:23:20 PST
Wait for style and linux ews before committing.
Comment 13 WebKit Review Bot 2013-02-27 08:21:53 PST
Attachment 190516 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/chromium-linux/svg/css/text-shadow-multiple-expected.png', u'LayoutTests/platform/chromium-linux/svg/custom/transform-with-shadow-and-gradient-expected.png', u'LayoutTests/platform/chromium/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp']" exit_code: 1
LayoutTests/platform/chromium-linux/svg/css/text-shadow-multiple-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 WebKit Review Bot 2013-02-27 08:47:05 PST
Comment on attachment 190516 [details]
Patch

Clearing flags on attachment: 190516

Committed r144195: <http://trac.webkit.org/changeset/144195>
Comment 15 WebKit Review Bot 2013-02-27 08:47:09 PST
All reviewed patches have been landed.  Closing bug.