dotted/dashed borders rendering with Qpainter surface very slow

RESOLVED FIXED

Status

Core Graveyard
Widget: Qt
RESOLVED FIXED
10 years ago
2 years ago

People

(Reporter: romaxa, Assigned: romaxa)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

10 years ago
Created attachment 319585 [details]
Dotted border divs testcase

original page maemo.org
(Assignee)

Comment 1

10 years ago
Created attachment 319586 [details]
Oprofile data
(Assignee)

Comment 2

10 years ago
Created attachment 319588 [details]
Backtrace
(Assignee)

Comment 3

10 years ago
I guess we have to recognize dotted/dashed drawing and draw it something like this:
QPen.setStyle(strokeStyle);
qp->drawLine...
(Assignee)

Comment 4

10 years ago
> qp->drawLine...
No I'm wrong ....

We are asking to draw any borders (dotted/dashed/custom) with Qt::CustomDashLine,
but this is very slow...

I guess we have to use simple patterns Qt::DotLine/Qt::DashLine when it possible

(Assignee)

Comment 5

10 years ago
Created attachment 319776 [details] [diff] [review]
Possible way to fix this bug

Vlad can you check this approach for fixing slow borders rendering?
Attachment #319776 - Flags: review?(vladimir)
Comment on attachment 319776 [details] [diff] [review]
Possible way to fix this bug

Hmm, good idea, but this approach won't work -- you'll end up using Qt::DashLine for every dash length, and Qt::DotLine for lengths 1 and 2.  What I would do instead here is something like:

style = Qt::NoPen;

if (style->num_dashes == 2) {
  if (style->dash[0] == style->line_width &&
      style->dash[1] == style->line_width * 2.0)
  {
    style = Qt::DotLine;
  } else if (style->dash[0] == style->line_width * 4.0 &&
             style->dash[1] == style->line_width * 2.0)
  {
    style = Qt::DashLine;
  }
}

if (style == Qt::NoPen) {
  // do real conversion
}

The 2 and 4 values are what Qt internally uses for DotLine/DashLine.

And then I'd patch layout/base/nsCSSRendering.cpp and add some #ifdef MOZ_WIDGET_QT's around where the dash pattern is set, and make the dash/dot patterns match up to the above.  That'll change behaviour some which means we'll change rendering on the web, and we'll have to figure out if that's ok, but that should have the same effect.

Are you sure that this is the actual performance issue, though?  I guess looking at the X11 paint engine code it handles DotLine/DashLine separately if it can (solid color, straight lines).
(Assignee)

Comment 7

10 years ago
(In reply to comment #6)
> (From update of attachment 319776 [details] [diff] [review])
> Hmm, good idea, but this approach won't work -- you'll end up using
> Qt::DashLine for every dash length, and Qt::DotLine for lengths 1 and 2.  What

That was temporary hack, because in current implementation something wrong...
for me it fails to render properly next url:
data:text/html,<div style="height:1000px;border:5px dotted #c0c0c0;">

It just render dots in corner....

(Assignee)

Comment 8

10 years ago
Created attachment 334334 [details] [diff] [review]
Proposed fix rev2

Vlad what do you think about this fix?
Attachment #319776 - Attachment is obsolete: true
Attachment #334334 - Flags: review?(vladimir)
Attachment #319776 - Flags: review?(vladimir)
(Assignee)

Comment 9

10 years ago
Also condition for dotted lines can be simplified:
if (!style->dash[0] || style->dash[0] == style->line_width)
Comment on attachment 334334 [details] [diff] [review]
Proposed fix rev2

This looks ok, but please add a comment before the #defines saying that this is an optimization based on the assumption that dots are 1*w and dashes are 3*w.

Also please fix the indentation in the added block -- that file uses 4-space indents, you've got 2-space indents happening.
Attachment #334334 - Flags: review?(vladimir) → review+
Comment on attachment 334334 [details] [diff] [review]
Proposed fix rev2


Wait, one problem:

>+       if (style->num_dashes == 2) {
>+           if ((style->dash[0] == style->line_width &&
>+                style->dash[1] == style->line_width && style->line_width <= 2.0) ||

This is ok. (<= 2.0? not < 2.0? I can't remember what the original condition is; <= is probably ok.)

>+               (style->dash[0] == 0.0 &&
>+                style->dash[1] == style->line_width * 2 && style->line_width > 2.0)

This however is only Dot if the line cap is ROUND -- it's not a dot if the line cap is anything else (where it'll be probably pretty solid).

>+               )
>+           {
>+               pstyle = Qt::DotLine;
>+           } else if (style->dash[0] == style->line_width * DASH_LENGTH &&
>+                      style->dash[1] == style->line_width * DASH_LENGTH)
>+           {

This is also only Dash if the line cap is BUTT (and possibly SQUARE, but Ithink just BUTT).
(Assignee)

Comment 12

10 years ago
Created attachment 334390 [details] [diff] [review]
Comment #11 fixes

I think now it should be ok.
Attachment #334334 - Attachment is obsolete: true
Attachment #334390 - Flags: review?(vladimir)
Comment on attachment 334390 [details] [diff] [review]
Comment #11 fixes

Yep, lookgs good.. thanks!
Attachment #334390 - Flags: review?(vladimir) → review+
(Assignee)

Updated

10 years ago
Assignee: nobody → romaxa
(Assignee)

Comment 14

10 years ago
fixed in:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/ab84d935f37e
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.