Optimize double border and outline rendering to avoid transparency layers

• Component: Layout and Rendering
• Status: UNCONFIRMED ---

[David Barr]

A simple case of borders and outlines, where all sides are the same color, are DOUBLE style, and don't have rounded corners, can easily be optimized to avoid using expensive transparency layers.

[David Barr]

Created attachment 114203 [details]
Patch

[David Barr]

Tests are still needed as this particular permutation has very little coverage. I think we are approaching the tail end of straightforward optimizations for borders and outlines with alpha.

[Simon Fraser (smfr)]

Comment on attachment 114203 [details]
Patch

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

Do we have any pixel tests for double outlines? If not, we should.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1624
> +static void calculateThirds(const BorderEdge edges[], RoundedRect& outerThird, RoundedRect& innerThird)

Can we use this new method in an existing code?

> Source/WebCore/rendering/RenderObject.cpp:1158
> +                // We need certain integer rounding results
> +                if (outlineWidth % 3 == 2)
> +                    outerWidth += 1;
> +                if (outlineWidth % 3 == 1)
> +                    innerWidth += 1;
> +                LayoutRect innerThird = outer;

This is copied from BorderEdge::getDoubleBorderStripeWidths(). Please share that code.

[David Barr]

> Do we have any pixel tests for double outlines? If not, we should.
For outlines we have good coverage. For rounded/alpha non-solid borders, less so.

> > Source/WebCore/rendering/RenderBoxModelObject.cpp:1624
> > +static void calculateThirds(const BorderEdge edges[], RoundedRect& outerThird, RoundedRect& innerThird)
> 
> Can we use this new method in an existing code?

Probably ought to inline it, I think it ultimately will have only the one caller.

> > Source/WebCore/rendering/RenderObject.cpp:1158
> This is copied from BorderEdge::getDoubleBorderStripeWidths(). Please share that code.

Alternatively, reduce the scope of this patch to just optimize the border case and defer unifying the code to:
https://bugs.webkit.org/show_bug.cgi?id=60749 (Border and outline rendering should share code)

[David Barr]

Created attachment 117837 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 117837 [details]
Patch

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

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1664
>      bool haveAllSolidEdges = true;
> +    bool haveAllDoubleEdges = true;

Another way to do this would be to add allEdgesShareStyle, like allEdgesShareColor.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1718
> +                LayoutRect innerThirdRect = outerThird.rect();
> +                LayoutRect outerThirdRect = outerThird.rect();

Using outerThird.rect for both is intentional?

[David Barr]

> > Source/WebCore/rendering/RenderBoxModelObject.cpp:1664
> >      bool haveAllSolidEdges = true;
> > +    bool haveAllDoubleEdges = true;
> 
> Another way to do this would be to add allEdgesShareStyle, like allEdgesShareColor.

Good point, this would be clearer and cause less duplication.

> > Source/WebCore/rendering/RenderBoxModelObject.cpp:1718
> > +                LayoutRect innerThirdRect = outerThird.rect();
> > +                LayoutRect outerThirdRect = outerThird.rect();
> 
> Using outerThird.rect for both is intentional?

It would be clearer to say outerBorder.rect(). Both third rects are calculated as insets from the outer border.

[David Barr]

(In reply to comment #7)
> > > Source/WebCore/rendering/RenderBoxModelObject.cpp:1664
> > >      bool haveAllSolidEdges = true;
> > > +    bool haveAllDoubleEdges = true;
> > 
> > Another way to do this would be to add allEdgesShareStyle, like allEdgesShareColor.
> 
> Good point, this would be clearer and cause less duplication.

I looked into this, and the gain is marginal. SOLID and DOUBLE are distinct from the other border styles in that they are simple, so we still need special casing for them.

> 
> > > Source/WebCore/rendering/RenderBoxModelObject.cpp:1718
> > > +                LayoutRect innerThirdRect = outerThird.rect();
> > > +                LayoutRect outerThirdRect = outerThird.rect();
> > 
> > Using outerThird.rect for both is intentional?
> 
> It would be clearer to say outerBorder.rect(). Both third rects are calculated as insets from the outer border.

I'll clean this up and upload again.

[Eric Seidel (no email)]

Ping?

[David Barr]

Created attachment 120241 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: Inform the scrolling coordinator when scrollbar layers come and go
Using index info to reconstruct a base tree...
<stdin>:474806: trailing whitespace.
        [Chromium] DatabaseTrackerChromium: iterating DatabaseSet races with Database disposal on worker thread 
<stdin>:474827: trailing whitespace.
        Nothing to test, just removing redundant code. Correct behavior tested by 
<stdin>:475346: trailing whitespace.
    
warning: 3 lines add whitespace errors.
Falling back to patching base and 3-way merge...
warning: too many files (created: 167522 deleted: 3), skipping inexact rename detection
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/chromium/test_expectations.txt
CONFLICT (content): Merge conflict in LayoutTests/platform/chromium/test_expectations.txt
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Source/WebCore/page/ScrollingCoordinator.h
CONFLICT (content): Merge conflict in Source/WebCore/page/ScrollingCoordinator.h
Auto-merging Source/WebCore/page/mac/ScrollingCoordinatorMac.mm
CONFLICT (content): Merge conflict in Source/WebCore/page/mac/ScrollingCoordinatorMac.mm
Auto-merging Source/WebCore/platform/ScrollView.cpp
Auto-merging Source/WebCore/rendering/RenderLayerCompositor.cpp
CONFLICT (content): Merge conflict in Source/WebCore/rendering/RenderLayerCompositor.cpp
Auto-merging Source/WebKit2/ChangeLog
CONFLICT (content): Merge conflict in Source/WebKit2/ChangeLog
Auto-merging Tools/ChangeLog
CONFLICT (content): Merge conflict in Tools/ChangeLog
Auto-merging Tools/Scripts/build-webkit
Auto-merging Tools/Scripts/webkitdirs.pm
CONFLICT (content): Merge conflict in Tools/Scripts/webkitdirs.pm
Failed to merge in the changes.
Patch failed at 0001 Inform the scrolling coordinator when scrollbar layers come and go

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 158.


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

[Eric Seidel (no email)]

I think the commit-queue/stylebot is currently sad-panda.  Possibly relating to the supposed svn/git.webkit outage earlier today.  Discussing with Dr. Barth.

[Adam Barth]

Sorry for the disruption.  Should be fixed.

[WebKit Review Bot]

Comment on attachment 120241 [details]
Patch

Rejecting attachment 120241 [details] from commit-queue.

New failing tests:
fast/writing-mode/border-styles-vertical-lr.html
fast/borders/borderRadiusDouble01.html
fast/borders/borderRadiusDouble03.html
fast/borders/border-styles-split.html
fast/writing-mode/border-styles-vertical-rl.html
fast/borders/fieldsetBorderRadius.html
fast/borders/borderRadiusAllStylesAllCorners.html
fast/borders/borderRadiusDouble02.html
Full output: http://queues.webkit.org/results/11081292

[David Barr]

Created attachment 121378 [details]
Patch

[David Barr]

It turns out that the previous patch was not calculating border radii correctly. The latest patch has a few minor pixel differences under Skia for some rounded corner cases but for fast/borders/borderRadiusDouble02.html it fails spectacularly.

[WebKit Review Bot]

Comment on attachment 121378 [details]
Patch

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

New failing tests:
fast/writing-mode/border-styles-vertical-lr.html
fast/borders/borderRadiusDouble01.html
fast/borders/borderRadiusDouble03.html
fast/borders/border-styles-split.html
fast/writing-mode/border-styles-vertical-rl.html
fast/borders/borderRadiusAllStylesAllCorners.html
fast/borders/borderRadiusDouble02.html

[James Robinson]

Any update? Are you just waiting for a review, David?  This patch has been sitting here for a month.

[David Barr]

I ran into a pathological case with fast/borders/borderRadiusDouble02.html and I've been focusing on higher priority issues in the meantime. Stability and conformance win over performance for me.

[James Robinson]

Comment on attachment 121378 [details]
Patch

OK, that sounds fine. Removing review? flag for now, then.  Feel free to set it again when this is ready to go.

[David Barr]

Created attachment 136362 [details]
Patch

[WebKit Review Bot]

Comment on attachment 136362 [details]
Patch

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

New failing tests:
fast/writing-mode/border-styles-vertical-lr.html
fast/backgrounds/background-leakage-transforms.html
fast/borders/borderRadiusDouble01.html
fast/backgrounds/background-leakage.html
fast/borders/borderRadiusDouble03.html
fast/borders/border-styles-split.html
fast/writing-mode/border-styles-vertical-rl.html
fast/borders/borderRadiusAllStylesAllCorners.html
fast/borders/borderRadiusDouble02.html

[WebKit Review Bot]

Created attachment 136405 [details]
Archive of layout-test-results from ec2-cr-linux-03

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

[David Barr]

(In reply to comment #22)
> (From update of attachment 136362 [details])
> Attachment 136362 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/12374445

The issue appears to be that the internal radii need to be adjusted by the width of the respective strokes.

[David Barr]

A path for strong progress on this bug is to defer the rounded double borders case to another patch.
Simon, are you in favor of making this a meta-bug and breaking out the individual steps into separate bugs?
Note the original summary was:
Optimize double border and outline rendering to avoid transparency layers
I plan to break this down into:
Optimize double border rendering *without* rounded corners to avoid transparency layers
Optimize double border rendering *with* rounded corners to avoid transparency layers
Unify outline rendering and border rendering

[Simon Fraser (smfr)]

(In reply to comment #25)
> A path for strong progress on this bug is to defer the rounded double borders case to another patch.
> Simon, are you in favor of making this a meta-bug and breaking out the individual steps into separate bugs?
> Note the original summary was:
> Optimize double border and outline rendering to avoid transparency layers
> I plan to break this down into:
> Optimize double border rendering *without* rounded corners to avoid transparency layers
> Optimize double border rendering *with* rounded corners to avoid transparency layers
> Unify outline rendering and border rendering

Sounds fine.