Bug 50527 – [Qt] Background image rendering is slow

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Noam Rosenthal]

Even on desktop, it is evident (using WebInspector, valgrind and Shark) that background image rendering is a bottleneck.
LayoutTests/fast/backgrounds/size/backgroundSize02.html, for example, takes 17ms to paint on a 1440x960 window.

This is due to the use of fillRect with a transformed brush, which is an unoptimized code path in Qt.

[Noam Rosenthal]

Created attachment 75618 [details]
Patch

I think this patch needs to be looked at by an additional set of experienced eyes, as it's in a core location that might break some website rendering (or hopefully just make them render faster)

[Jakub Wieczorek]

Comment on attachment 75618 [details]
Patch

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

> WebCore/platform/graphics/qt/ImageQt.cpp:110
> +    if (!destinationRect.width() || !destinationRect.height() || !destinationRect.width() || !destinationRect.height())

This should be:
if (!destinationRect.width() || !destinationRect.height() || !tileRect.width() || !tileRect.height())
or even:
if (!destinationRect.isValid() || !tileRect.isValid())

[Noam Rosenthal]

(In reply to comment #2)
> (From update of attachment 75618 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75618&action=review
> 
> > WebCore/platform/graphics/qt/ImageQt.cpp:110
> > +    if (!destinationRect.width() || !destinationRect.height() || !destinationRect.width() || !destinationRect.height())
> 
> This should be:
> if (!destinationRect.width() || !destinationRect.height() || !tileRect.width() || !tileRect.height())
> or even:
> if (!destinationRect.isValid() || !tileRect.isValid())

Agree. This part was copied from the original code :)

[Benjamin Poulain]

(In reply to comment #3)
> > This should be:
> > if (!destinationRect.width() || !destinationRect.height() || !tileRect.width() || !tileRect.height())
> > or even:
> > if (!destinationRect.isValid() || !tileRect.isValid())
> 
> Agree. This part was copied from the original code :)

That would be incorrect. ::isValid() is >0, what need to be catched here is a size of zero. ::isEmpty() should do the trick.

[Benjamin Poulain]

Comment on attachment 75618 [details]
Patch

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

> WebCore/platform/graphics/qt/ImageQt.cpp:139
> +    painter->drawTiledPixmap(QRect(0, 0, destinationRect.right() - offset.x(), destinationRect.bottom() - offset.y()), tilePixmap);

I don't understand the coordinates here. Why you start at 0, 0? Why the width is destinationRect.right() - offset.x()? The offset could be bigger than destinationRect.right(), and the result should still be correct.

[Benjamin Poulain]

Comment on attachment 75618 [details]
Patch

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

> WebCore/platform/graphics/qt/ImageQt.cpp:125
> +    // FIXME: Might be worth investigating to cache the transformed result, though this has memory implications.
> +    QPixmap tilePixmap;
> +    if (tileRect.x() || tileRect.y() || tileRect.width() != framePixmap->width() || tileRect.height() != framePixmap->height() || !patternTransform.isIdentityOrTranslation()) {
> +        // We can't use the pixmap as is, because we're using only part of it or it needs to be scaled. Because we know the pixmap is going to be
> +        // drawn more than once, we copy/scale it only once to avoid repeating the operation for each tile.
> +        QRect targetTileRect = patternTransform.mapRect(tileRect);
> +        tilePixmap = QPixmap(targetTileRect.size());
> +        QPainter painter(&tilePixmap);
> +        painter.setCompositionMode(QPainter::CompositionMode_Source);
> +        painter.setTransform(patternTransform);
> +        painter.translate(-targetTileRect.topLeft());
> +        painter.drawPixmap(tileRect, *framePixmap);
> +    } else

I don't think that would work with rotation so you don't implement the interface given by the API of ImageBuffer.
I would prefer if you change the function signature to take two arguments scaleX and scaleY so it is explicit that any other type transform is not supported.

Otherwise, future change to support arbitrary transform from CSS would break Qt.

[Benjamin Poulain]

Would you mind attaching the testcases? I would like to know why QBrush is so much slower.

[Noam Rosenthal]

(In reply to comment #7)
> Would you mind attaching the testcases? I would like to know why QBrush is so much slower.

Just look at the stuff under LayoutTests/fast/size

[Noam Rosenthal]

(In reply to comment #6)
> (From update of attachment 75618 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75618&action=review
> 
> > WebCore/platform/graphics/qt/ImageQt.cpp:125
> > +    // FIXME: Might be worth investigating to cache the transformed result, though this has memory implications.
> > +    QPixmap tilePixmap;
> > +    if (tileRect.x() || tileRect.y() || tileRect.width() != framePixmap->width() || tileRect.height() != framePixmap->height() || !patternTransform.isIdentityOrTranslation()) {
> > +        // We can't use the pixmap as is, because we're using only part of it or it needs to be scaled. Because we know the pixmap is going to be
> > +        // drawn more than once, we copy/scale it only once to avoid repeating the operation for each tile.
> > +        QRect targetTileRect = patternTransform.mapRect(tileRect);
> > +        tilePixmap = QPixmap(targetTileRect.size());
> > +        QPainter painter(&tilePixmap);
> > +        painter.setCompositionMode(QPainter::CompositionMode_Source);
> > +        painter.setTransform(patternTransform);
> > +        painter.translate(-targetTileRect.topLeft());
> > +        painter.drawPixmap(tileRect, *framePixmap);
> > +    } else
> 
> I don't think that would work with rotation so you don't implement the interface given by the API of ImageBuffer.
> I would prefer if you change the function signature to take two arguments scaleX and scaleY so it is explicit that any other type transform is not supported.
> 
> Otherwise, future change to support arbitrary transform from CSS would break Qt.
Yes, I agree. Or maybe, we can do this for scale/translate only and go through QPixmap::transformed as a fallback.

[Noam Rosenthal]

(In reply to comment #8)
> (In reply to comment #7)
> > Would you mind attaching the testcases? I would like to know why QBrush is so much slower.
> 
> Just look at the stuff under LayoutTests/fast/size

I meant LayoutTests/fast/backgrounds/size

[Noam Rosenthal]

(In reply to comment #5)
> (From update of attachment 75618 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75618&action=review
> 
> > WebCore/platform/graphics/qt/ImageQt.cpp:139
> > +    painter->drawTiledPixmap(QRect(0, 0, destinationRect.right() - offset.x(), destinationRect.bottom() - offset.y()), tilePixmap);
> 
> I don't understand the coordinates here. Why you start at 0, 0? Why the width is destinationRect.right() - offset.x()? The offset could be bigger than destinationRect.right(), and the result should still be correct.

Since we translate(offset) a line earlier, this would draw the tile pixmap between the points 
(offset -> destinationRect.bottomRight()). Maybe I need to write it in a more verbose way :)

[Benjamin Poulain]

(In reply to comment #11)
> > I don't understand the coordinates here. Why you start at 0, 0? Why the width is destinationRect.right() - offset.x()? The offset could be bigger than destinationRect.right(), and the result should still be correct.
> 
> Since we translate(offset) a line earlier, this would draw the tile pixmap between the points 
> (offset -> destinationRect.bottomRight()). Maybe I need to write it in a more verbose way :)

The offset should be the offset of the pixmap being rendered. The pattern does not necessary starts at (0, 0).

That is not what you are doing, or I am missing completely what you are doing and yet a more verbose way would be nice :)

[Noam Rosenthal]

(In reply to comment #12)
> (In reply to comment #11)
> > > I don't understand the coordinates here. Why you start at 0, 0? Why the width is destinationRect.right() - offset.x()? The offset could be bigger than destinationRect.right(), and the result should still be correct.
> > 
> > Since we translate(offset) a line earlier, this would draw the tile pixmap between the points 
> > (offset -> destinationRect.bottomRight()). Maybe I need to write it in a more verbose way :)
> 
> The offset should be the offset of the pixmap being rendered. The pattern does not necessary starts at (0, 0).
> 
> That is not what you are doing, or I am missing completely what you are doing and yet a more verbose way would be nice :)

Looking it a second time, yes - something is funky with my geometry. I'll revise and submit.
Btw I'm thinking the performance gain is not necessarily from fillRect vs. drawTiledPixmap, it might be because of the optimization to pre-transform the image, while I'm not sure fillRect does it (it's really a raster-specific optimization, in OpenGL it's probably better to transform on each draw).

If anybody is up to figuring it out inside the raster engine, go for it... I'll continue trying to fix it inside GraphicsContext and friends for now.

[Kenneth Rohde Christiansen]

Comment on attachment 75618 [details]
Patch

r- due to Noam's comments.

[Noam Rosenthal]

Created attachment 77439 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 77439 [details]
Patch

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

> WebCore/ChangeLog:8
> +        Special case the simple version of Image::drawTiled in Qt, since we can optimize for the simple QPainter::drawTiledPixmap, rather than go through Image::drawPattern which has to invoke QPainter::drawBrush. 

Could you make this line around 80 chars :-) ?

> WebCore/platform/graphics/qt/ImageQt.cpp:139
> +void Image::drawTiled(GraphicsContext* ctxt, const FloatRect& destRect, const FloatPoint& srcPoint, const FloatSize& scaledTileSize, ColorSpace styleColorSpace, CompositeOperator op)

It would be nice if it was detailed here that the code is not strictly needed but it is an optimization. Maybe such an optimization will not be needed forever.

[Eric Seidel (no email)]

Comment on attachment 77439 [details]
Patch

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

>> WebCore/ChangeLog:8
>> +        Special case the simple version of Image::drawTiled in Qt, since we can optimize for the simple QPainter::drawTiledPixmap, rather than go through Image::drawPattern which has to invoke QPainter::drawBrush. 
> 
> Could you make this line around 80 chars :-) ?

Please!  It won't fit on my punch card otherwise!

[Noam Rosenthal]

Created attachment 77487 [details]
Patch

[Kenneth Rohde Christiansen]

Someone up to reviewing this? or who would be the right reviewer?

[Kenneth Rohde Christiansen]

Anyway it looks good and it passes the tests. No'am do you have any performance numbers? Any change of getting this into Qt?

[Noam Rosenthal]

(In reply to comment #20)
> Anyway it looks good and it passes the tests. No'am do you have any performance numbers? Any change of getting this into Qt?

Out of the 22 tests in fast/backgrounds/size, running on Mac with raster graphics system:

The average painting speed improvement is 50%

9 tests: improvement in performance of 50% or more.
3 tests: improvement of 25%-50%
8 tests: similar before/after numbers
3 tests: 25%-50% regression (see below for explanation).
No test shows a regression of more than 25%.

Those 3 tests that regressed are fast to render both before and after, but the extra rendering step to scale the tile does cause them to not be as fast - those 3 tests are of a large scaled background image that only repeats once or twice - in those cases the extra rendering step just causes overhead. However, I doubt those situations would be very slow in real circumstances, unlike the use case of repeating small images multiple times, which is sometimes improved by 80%.

[Noam Rosenthal]

(In reply to comment #20)
> Anyway it looks good and it passes the tests. No'am do you have any performance numbers? Any change of getting this into Qt?

(In reply to comment #19)
> Someone up to reviewing this? or who would be the right reviewer?
Actually I'd like it if Benjamin could give his go ahead, and then maybe you or Kling can do the actual review? Benjamin would also be the right person know also if this can be fixed further down the stack (i.e. the raster engine).

[Kenneth Rohde Christiansen]

(In reply to comment #22)
> (In reply to comment #20)
> > Anyway it looks good and it passes the tests. No'am do you have any performance numbers? Any change of getting this into Qt?
> 
> (In reply to comment #19)
> > Someone up to reviewing this? or who would be the right reviewer?
> Actually I'd like it if Benjamin could give his go ahead, and then maybe you or Kling can do the actual review? Benjamin would also be the right person know also if this can be fixed further down the stack (i.e. the raster engine).

Sounds like a plan!

[Benjamin Poulain]

Yep that makes sense.

The original have a special case when you have to render only one tile. I think it is a good idea to avoid creating a new potentionally big pixmap.

[Benjamin Poulain]

Oh, and maybe use QPainter::SmoothPixmapTransform if !PLATFORM(ARM)?

[Noam Rosenthal]

Created attachment 79121 [details]
Patch

[Noam Rosenthal]

Created attachment 79134 [details]
Patch

This would only affect pages with -webkit-background-size or border images.
Background rendering would still be slow if the painter has a scale on it (e.g. a scaled QGraphicsWebView).

[WebKit Commit Bot]

Comment on attachment 79134 [details]
Patch

Clearing flags on attachment: 79134

Committed r75926: <http://trac.webkit.org/changeset/75926>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.