Bug 74578 – Incorrect stroke near borders

• Component: SVG
• Status: RESOLVED CONFIGURATION CHANGED

[Philip Rogers]

Created attachment 119359 [details]
Test case

A line drawn along the border of an SVG element will be clipped if opacity != 1.

In the attached test case there should be two lines of length 50px. The line with opacity=0.9 is not drawn but the line with opacity=1.0 is.

(Found while diving into http://code.google.com/p/chromium/issues/detail?id=107510)

[Philip Rogers]

Created attachment 119654 [details]
Fix incorrect stroke near borders

[WebKit Review Bot]

Comment on attachment 119654 [details]
Fix incorrect stroke near borders

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

New failing tests:
http/tests/inspector-enabled/dedicated-workers-list.html
fast/repaint/scroll-fixed-reflected-layer.html
fast/repaint/fixed-scale.html
media/video-poster-blocked-by-willsendrequest.html
css2.1/20110323/abspos-containing-block-initial-004b.htm
fast/repaint/fixed-tranformed.html
svg/batik/text/textAnchor.svg
css2.1/20110323/abspos-containing-block-initial-004d.htm
fast/repaint/moving-shadow-on-path.html
fast/repaint/fixed-table-overflow-zindex.html
http/tests/inspector/resource-tree/resource-tree-document-url.html

[Eric Seidel (no email)]

I believe you that this works... but it feels like a bit of a one-off-hack for this situation.  Should we be doing this inflate in a more central place?

[Dirk Schulze]

Comment on attachment 119654 [details]
Fix incorrect stroke near borders

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

Eric is right, the patch is not correct. Also what if the border is thicker than 1? You can not hard code the inflation.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:94
>          FloatRect repaintRect = object->repaintRectInLocalCoordinates();

The repaintRectInLocalCoordinates should already include the border. If we clip sty of the border away, it seems like a bug in repaintRectInLocalCoordinates(). Is it just for lines? Do you see that for SVGRects or SVGCircles as well?

[Philip Rogers]

Created attachment 121433 [details]
Example of clipping fixed with this patch

(In reply to comment #3)
> I believe you that this works... but it feels like a bit of a one-off-hack for this situation.  Should we be doing this inflate in a more central place?

I agree it feels like a one-off hack but it's the result of clipping so deep in the stack. This codepath only occurs because a separate transparency layer is created for objects with opacity or shadow and we have to clip when drawing.

That said, I really don't like a random 0.5 inflate in the middle of the code. I'll have a followup patch that switches to enclosingLayoutRect(object->repaintRectInLocalCoordinates()), which is used for similar purposes elsewhere. Instead of adding 0.5, enclosingLayoutRect uses floor on the top and left coordinates, and ceil on the right and bottom coordinates, which should be exactly what we need.


(In reply to comment #4)
> (From update of attachment 119654 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119654&action=review
> 
> Eric is right, the patch is not correct. Also what if the border is thicker than 1? You can not hard code the inflation.

This extra padding is only to account for the "overdraw" outside the repaint rect, not the border itself. For instance, the rightmost line in the test extends from x=99.5 to x=100.5, but is actually drawn using pixels at x=99. Previously, we were clipping the pixels at x=99 and not drawing them.

> 
> > Source/WebCore/rendering/svg/SVGRenderSupport.cpp:94
> >          FloatRect repaintRect = object->repaintRectInLocalCoordinates();
> 
> The repaintRectInLocalCoordinates should already include the border. If we clip sty of the border away, it seems like a bug in repaintRectInLocalCoordinates(). Is it just for lines? Do you see that for SVGRects or SVGCircles as well?

Unfortunately it looks like repaintRect is just the minimum enclosing rect for the path and does not account for the overdraw. This bug occurs for all shapes (lines, rects, even circles) and this patch fixes them all. Looking at the diffs of the new failing tests, the current expected results clearly have clipping, which is fixed with this patch (see the attached image clipping.png).

[Philip Rogers]

Created attachment 121434 [details]
Fix incorrect stroke near borders

[WebKit Review Bot]

Comment on attachment 121434 [details]
Fix incorrect stroke near borders

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

New failing tests:
svg/css/stars-with-shadow.html
svg/css/composite-shadow-example.html
svg/css/composite-shadow-with-opacity.html
fast/repaint/moving-shadow-on-path.html
svg/custom/container-opacity-clip-viewBox.svg
svg/custom/marker-opacity.svg
svg/custom/js-update-bounce.svg

[Philip Rogers]

The reviewers were right to question an inflate() so deep in the code--in exploring this more I found this clip bug affects Skia and CG differently. I've played with some other ways of fixing this but the end result is that this patch here is too simplistic.

I'd like to abandon my approach here for now, as it's clear there are at least a couple bugs that I need to tackle separately.

(Some details)
Skia's clip() is not antialiased, which causes the clipped aliasing on all non-integer-aligned shapes. This is actually different from the border bug on semi-transparent lines, which seems to affect CG and Skia, but was erroneously "fixed" by inflating the repaint rect.

[Dirk Schulze]

(In reply to comment #8)
> The reviewers were right to question an inflate() so deep in the code--in exploring this more I found this clip bug affects Skia and CG differently. I've played with some other ways of fixing this but the end result is that this patch here is too simplistic.

Can your remove the review flag please? Or should it still get reviewed?

[Philip Rogers]

Created attachment 141743 [details]
Fix SVG clipping in SVGRenderingContext

This bit me again over the weekend so I'd like to resurrect this bug.

Looking into this issue again, I think the proposed fix before actually wasn't too local after all. It looks like every other instance of clipping for transparency layers snaps the values; there are many examples in RenderLayer. Eric and Dirk, do you still think this approach is too local?

The new patch has a much better testcase but needs rebaselines and is not ready to land yet--just looking for a review of the approach.

[WebKit Review Bot]

Comment on attachment 141743 [details]
Fix SVG clipping in SVGRenderingContext

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

New failing tests:
svg/as-background-image/svg-as-background-6.html
fast/backgrounds/svg-as-mask.html
svg/css/stars-with-shadow.html
svg/css/composite-shadow-example.html
svg/css/composite-shadow-with-opacity.html
svg/as-background-image/svg-as-background-5.html
fast/repaint/moving-shadow-on-path.html
svg/custom/container-opacity-clip-viewBox.svg
svg/custom/marker-opacity.svg
svg/custom/js-update-bounce.svg

[WebKit Review Bot]

Created attachment 141787 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Eric Seidel (no email)]

do you still want this reviewed?

[Ahmad Saleem]

I took the test case from Chrome bug into JSFiddle:

Link - https://jsfiddle.net/2b4avqng/show

It seems to work fine in Safari 16.2 and matching Chrome Canary 111 and Firefox Nightly 110. Similarly the attached test case is also matching in all browser, although it is not showing the expected result of two lines except just one.

Do we need to do anything further? Thanks!

[Ahmad Saleem]

I am unable to reproduce this bug, so marking this as 'RESOLVED CONFIGURATION CHANGED', please reopen if it is still reproducible.