Improve border rendering code for running on GPUs

RESOLVED FIXED

Status

()

RESOLVED FIXED
8 years ago
2 years ago

People

(Reporter: jrmuizel, Assigned: bas.schouten)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

8 years ago
We currently do some elaborate work to be faster on software, this probably makes us slower on hardware. We should add a version for GPUs.
(Assignee)

Updated

8 years ago
Blocks: 598202
(Assignee)

Comment 1

8 years ago
As Jeff has rightfully been asking for some more concrete numbers here, I'm adding some profiling here, this is from resizing a window showing 'about:blank' i.e. rendering our UI, painting time is distributed as follows:

Process, Callees, Module, Function, Weight, % Weight, Count, TimeStamp
firefox.exe (4292), xul.dll!mozilla::FrameLayerBuilder::DrawThebesLayer, , , 7172.555024, 4.91, 7169, 
,    |- xul.dll!nsDisplayBorder::Paint, , , 3975.897323, 2.72, 3975, 
,    |- xul.dll!nsDisplayBackground::Paint, , , 1103.336155, 0.76, 1103, 
,    |- xul.dll!mozilla::layers::BasicLayerManager::EndTransaction, , , 578.951741, 0.40, 578, 
,    |- xul.dll!nsDisplayXULTextBox::Paint, , , 512.074132, 0.35, 512, 
,    |- xul.dll!nsDisplaySVGEffects::Paint, , , 378.912356, 0.26, 378, 
,    |- xul.dll!nsDisplayXULImage::Paint, , , 197.126124, 0.13, 197, 
,    |- xul.dll!nsDisplayBoxShadowOuter::Paint, , , 193.154272, 0.13, 193, 
,    |- xul.dll!nsDisplayBoxShadowInner::Paint, , , 183.042885, 0.13, 183, 
,    |- xul.dll!nsDisplayItem::RecomputeVisibility, , , 10.007181, 0.01, 10, 
,    |- xul.dll!nsDisplayCaret::Paint, , , 8.999948, 0.01, 9, 
,    |- xul.dll!nsDisplayText::Paint, , , 8.999565, 0.01, 9, 
,    |- xul.dll!mozilla::FrameLayerBuilder::DrawThebesLayer<itself>, xul.dll, mozilla::FrameLayerBuilder::DrawThebesLayer, 8.026816, 0.01, 8, 

i.e. more than 50% of the time painting is spent inside border drawing code.
Some quick ideas, any/all of which are likely to help:

disabling add in some cases:
- disable OPERATOR_ADD optimization for chrome
- disable OPERATOR_ADD optimization for 1px borders (maybe only in chrome -- has an issue in that the pixel right at the join of two edges can still look noticeably wrong)

not needing to use add:
- implement gradients for corners: set up a linear gradient between the source and dest color and stroke with that, instead of doing the pushgroup/operator add dance -- this is a good thing to do regardless and would be quite easy to implement, but hard to get right if you have two different joining border styles.  Different widths is probably possible, if you compute the outline and fill; same with solid-double etc.  But dotted/solid join would be a challenge, but that is so rare that maybe we don't care that it can look wrong (just use OVER in that case with a clip).
(Assignee)

Comment 3

8 years ago
I've concluded a large part of the problem here is not even so much the usage of OPERATOR_ADD, but also the extensive usage of geometric clipping. This should be avoidable a lot.
(Assignee)

Updated

8 years ago
Blocks: 593812
(Assignee)

Comment 4

8 years ago
I've been working on special D2D border rendering code that avoids using any clips and compositions. Just strokes and fill. At this point it's still in the early stages but performance wise it seems to be working out. I've gotten some great performance but I'm currently starting to work on (for the simple, solid border cases still), using gradients for border color transitions. The problem is that gradients are still fairly expensive in D2D (note the gradient code, in a very preliminary first estimate, still seems 4x+ faster for our UI (which is mainly rounded rectangles with different colors) than the current code). I'd really be interested if anyone has any other ideas.
(Assignee)

Comment 5

8 years ago
Created attachment 485613 [details] [diff] [review]
First code on fast borders

I started working on this a bit. I'm putting up my current changes, it's in no way complete, or cleaned up, or anything. It makes all solid color border drawing pretty fast, except the cases where different sides have different widths. I'm working on improving more cases, but this code catches the majority of borders in our UI.
Assignee: nobody → bas.schouten
blocking2.0: --- → betaN+
Duplicate of this bug: 611235
(Assignee)

Comment 7

8 years ago
Created attachment 494109 [details] [diff] [review]
Part 1: Mask corners of some reftest due to AA differences.

We're not very consistent at how we AA single width rectangular border corners, this is fine and doesn't look bad an performs well. But we need to mask those corners in some reftests.
Attachment #494109 - Flags: review?(roc)
(Assignee)

Comment 8

8 years ago
Created attachment 494111 [details] [diff] [review]
Part 2: Update masks to include extra pixels for new border rendering code.
Attachment #494111 - Flags: review?(jmuizelaar)
Why do we need these mask pixels? Even though you're changing the behavior, shouldn't it remain consistent between test and reference?
(Assignee)

Comment 10

8 years ago
Created attachment 494112 [details] [diff] [review]
Part 3: Update reftest that now fails on all platforms due to gradient rendering inconsistencies.
Attachment #494112 - Flags: review?(jmuizelaar)
(Assignee)

Updated

8 years ago
Attachment #494112 - Attachment is patch: true
Attachment #494112 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Updated

8 years ago
Blocks: 597540
(Assignee)

Comment 11

8 years ago
Created attachment 494166 [details] [diff] [review]
Part 4: Specialized border rendering code for common borders

This is the essence of this bug. The patch adds several specialized rendering paths that avoid using intermediate surfaces and use gradients for smooth corner transitions. This passes all tests with the patches acompanied in this patch. There's no clear differences on Talos (presumably because a bunch of the tests apparently don't count painting for automated Talos according to some bugs).

Locally running winopen.xul from stand alone Talos does show clear improvements (this is largely doing border rendering of our UI from what I can see).

D2D, without Patch: 80 ms
D2D, with Patch: 60 ms
D2D, No Borders at all: 50 ms
GDI, without Patch: 50 ms
GDI, with patch: 45ms
GDI, No Borders at all: 45ms

There's still some difference between D2D and GDI for opening a new window, but it's drastically reduced. The reason D2D still spends some considerable time drawing borders is that gradients are not cheap in D2D. I've got some ideas for improving this further on the backend level (a single, hard stop gradient could be done more cleverly than D2D's general gradient approach), but for now I think this patch will offer us the main improvement we're looking for. It also solves all dependent bugs.
Attachment #494166 - Flags: review?(vladimir)
(Assignee)

Comment 12

8 years ago
Created attachment 494193 [details] [diff] [review]
Part 5: Mark reftests as no longer failing

These reftests used to seam slightly due to AA inconsistencies on the two different geometries blended with ADD. They no longer fail with the new gradient approach.
Attachment #494193 - Flags: review?(vladimir)
(Assignee)

Updated

8 years ago
Attachment #485613 - Attachment is obsolete: true
(Reporter)

Updated

8 years ago
Attachment #494111 - Flags: review?(jmuizelaar) → review+
(Reporter)

Comment 13

8 years ago
Comment on attachment 494112 [details] [diff] [review]
Part 3: Update reftest that now fails on all platforms due to gradient rendering inconsistencies.

I don't see why this should fail due to new border rendering. Can you upload the new images somewhere? Also, we should probably replace the reference rendering with something, perhaps per platform, that passes.
Attachment #494112 - Flags: review?(jmuizelaar) → review-
(Assignee)

Comment 14

8 years ago
Created attachment 494466 [details] [diff] [review]
Part 3: Update reftest to mask nose edge

This patch masks the nose edge as per IRC discussion.
Attachment #494112 - Attachment is obsolete: true
Attachment #494466 - Flags: review?(jmuizelaar)
(Reporter)

Comment 15

8 years ago
Comment on attachment 494466 [details] [diff] [review]
Part 3: Update reftest to mask nose edge

Looks good. Does this let us pass on all of the platforms?
Attachment #494466 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 17

8 years ago
(In reply to comment #9)
> Why do we need these mask pixels? Even though you're changing the behavior,
> shouldn't it remain consistent between test and reference?

No, one uses an 'inset' border style, and the other draws the border manually. My new code has no specialized path for 'inset' and this will use the old code.

Updated

8 years ago
Blocks: 590407

Updated

8 years ago
Blocks: 591361

Updated

8 years ago
Depends on: 616510
(Reporter)

Updated

8 years ago
Depends on: 616495

Updated

7 years ago
Depends on: 679335

Updated

7 years ago
Depends on: 646053

Updated

6 years ago
Depends on: 773203
Depends on: 1303094
You need to log in before you can comment on or make changes to this bug.