Bug 50869 – [cairo] Rendering a lot of arcs on top of each other causes time outs in some tests

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Alejandro G. Castro]

When the start angle and the end angle difference is big, like in the test canvas-largedraws.html, we are rendering arcs on top of each other when we could draw just one arc and move to the end position, like it is being done in other ports.

[Alejandro G. Castro]

Created attachment 77010 [details]
Proposed patch

[Eric Seidel (no email)]

I dont' trust my graphics knowledege w/o seeing examples of the output, but we have lots of graphics-capable folks in webkit...

[Martin Robinson]

Comment on attachment 77010 [details]
Proposed patch

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

> WebCore/ChangeLog:10
> +        We avoid the situation where we have to render the same arc
> +        multiple times over itself. Now it renders just one oval and
> +        moves to the end angle.
> +

Are we guaranteed that this will produce the proper rendering? What if the stroke is partially opaque? Is this hitting some weird slow path in Cairo? Maybe it would be more accurate to break the stroke up into pieces and render them in a loop?

> WebCore/platform/graphics/cairo/PathCairo.cpp:154
> +    float sweep = ea - sa;

This is a good opportunity to expand variable names into something more readable.

> WebCore/platform/graphics/cairo/PathCairo.cpp:163
> +    } else

It's appropriate to use a "{" here since the block is more than one line.

[Alejandro G. Castro]

(In reply to comment #3)
> (From update of attachment 77010 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77010&action=review
> 
> > WebCore/ChangeLog:10
> > +        We avoid the situation where we have to render the same arc
> > +        multiple times over itself. Now it renders just one oval and
> > +        moves to the end angle.
> > +
> 
> Are we guaranteed that this will produce the proper rendering? What if the stroke is partially opaque? Is this hitting some weird slow path in Cairo?

Hrm, good point, it would not be accurate with oppacity, we will have to sum it. I think chrome is doing actually something similar I assume it is very costly to render a lot (like millions) of huge arcs (18s just to render one arc) that are exactly the same one, it is true that cairo could do this instead of us.

> Maybe it would be more accurate to break the stroke up into pieces and render them in a loop?
>

Not sure if I know what you mean, what I'm thinking now is to use the opacity to render exactly that opacity but just one arc.

> > WebCore/platform/graphics/cairo/PathCairo.cpp:154
> > +    float sweep = ea - sa;
> 
> This is a good opportunity to expand variable names into something more readable.
> 
> > WebCore/platform/graphics/cairo/PathCairo.cpp:163
> > +    } else
> 
> It's appropriate to use a "{" here since the block is more than one line.

Thanks, I'll add both.

[Alejandro G. Castro]

(In reply to comment #4)
> (In reply to comment #3)
> > Are we guaranteed that this will produce the proper rendering? What if the stroke is partially opaque? Is this hitting some weird slow path in Cairo?
> 
> Hrm, good point, it would not be accurate with oppacity, we will have to sum it. I think chrome is doing actually something similar I assume it is very costly to render a lot (like millions) of huge arcs (18s just to render one arc) that are exactly the same one, it is true that cairo could do this instead of us.
> 

I've just checked that it does not matter the amount of arcs you add to the path one over the other for the opacity, I guess the opacity of the tint is just applied when stroking so you will have the same result if you draw one or 100, at least in cairo. I've also checked that other ports like skia or qt are applying similar solutions.

I like skia solution (it avoids the modff) but it seems slower for cairo, I'll try to check it.

[Alejandro G. Castro]

Created attachment 78225 [details]
Proposed patch

I made a mistake checking the performance of the solution, we can do it without the modff. This patch avoids using modff to avoid precision errors.

Not sure why in Qt solution they do not pay attention to the end angle in this situation, maybe it is a bug, and in case of skia they do not use anticlockwise property, which could cause issues with fill rules AFAIK.

[Martin Robinson]

Comment on attachment 78225 [details]
Proposed patch

Thanks!

[Alejandro G. Castro]

Landed http://trac.webkit.org/changeset/75256

[Xan Lopez]

Reverted r75256 for reason:

Broke GTK+ canvas tests

Committed r75260: <http://trac.webkit.org/changeset/75260>

[Alejandro G. Castro]

Created attachment 78394 [details]
Proposed patch

[WebKit Review Bot]

Attachment 78394 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/gtk/Skipped', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/cairo/PathCairo.cpp']" exit_code: 1
Source/WebCore/platform/graphics/cairo/PathCairo.cpp:157:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/platform/graphics/cairo/PathCairo.cpp:158:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alejandro G. Castro]

Created attachment 78395 [details]
Proposed patch

I've added a check for the case where the end angle is in the opposite direction of the requested angle, we can not do this in that situation and cairo is already painting a small angle because it increases or decreases the start angle progressively by 2*PI until it is bigger than end angle.

Replaced sa and ea with startAngle and endAngle as suggested.

[Alejandro G. Castro]

Landed http://trac.webkit.org/changeset/75381