432453 - dotted/dashed borders rendering with Qpainter surface very slow

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Qt, defect)

Description

[Oleg Romashin (:romaxa)]

Attachment: Dotted border divs testcase

original page maemo.org

Comment 1

[Oleg Romashin (:romaxa)]

Attachment: Oprofile data

Comment 2

[Oleg Romashin (:romaxa)]

Attachment: Backtrace

Comment 3

[Oleg Romashin (:romaxa)]

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

Comment 4

[Oleg Romashin (:romaxa)]

> 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

Comment 5

[Oleg Romashin (:romaxa)]

Attachment: Possible way to fix this bug

Vlad can you check this approach for fixing slow borders rendering?

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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).

Comment 7

[Oleg Romashin (:romaxa)]

(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....

Comment 8

[Oleg Romashin (:romaxa)]

Attachment: Proposed fix rev2

Vlad what do you think about this fix?

Comment 9

[Oleg Romashin (:romaxa)]

Also condition for dotted lines can be simplified:
if (!style->dash[0] || style->dash[0] == style->line_width)

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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).

Comment 12

[Oleg Romashin (:romaxa)]

Attachment: Comment #11 fixes

I think now it should be ok.

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 334390 [details] [diff] [review]
Comment #11 fixes

Yep, lookgs good.. thanks!

Comment 14

[Oleg Romashin (:romaxa)]

fixed in:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/ab84d935f37e