733941 - Fennec: In DrawPixelSnapped we seem to hit CreateSamplingRestrictedDrawable a lot

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

An example of us hitting this is with the following conditions:

aSourceRect =  {
    x = 0, 
    y = 0, 
    width = 6.822037169397273, 
    height = 6.822037169397273
  }
aImageRect = {
    x = 0, 
    y = 0, 
    width = 7, 
    height = 6
}

In the caller
aFill = {
    x = 10, 
    y = 585, 
    width = 4, 
    height = 4
}

aUserSpaceToImageSpace = {
  xx = 1.7055092923493183, 
  yx = 0, 
  xy = 0, 
  yy = 1.7055092923493183, 
  x0 = -17.055092923493184, 
  y0 = -997.72293602435116
}


Hitting CreateSamplingRestrictedDrawable seems like it would be a waste of time in this case.

Comment 1

[Joe Drew (not getting mail)]

Timothy, I suspect this is related to bug 733607.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #0)
> Hitting CreateSamplingRestrictedDrawable seems like it would be a waste of
> time in this case.

Why? It looks like the bottom edge in image space ends up at 6.8 or thereabouts. If aSubimage is 7x6 then we do need to create a sampling-restricted drawable to avoid sampling the top row of the image source when we draw the bottom row of pixels.

Comment 3

[Joe Drew (not getting mail)]

We need to know why we're spending time in here before the beta. It's possible that it's unavoidable, but we need to know whether that's the case.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Here's another example when drawing an image of size w=40,h=14

nsLayoutUtils::DrawBackgroundImage(
aFill =     x = 52880, 
            y = 21890, 
            width = 2400, 
            height = 840 (840/60 = 14)

in ComputeSnappedImageDrawingParameters
we get a devPixelFill with height = 16.8 (14/0.83333333333333)
This gets rounded up to 17 because we're snapping.

Later on in imgFrame::Draw() we compute a sourceRect from aFill which has the height of 17, this gives us a sourceRect with a height of 14.166666666666668 which is just outside of the bounds of imageRect.

Then in CreateSamplingRestrictedDrawable we create a new image of size 40x14 and paint an identical copy of the 40x14 image into the new drawable.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Maybe imgFrame::Draw should take a clamp-or-repeat flag. Then if callers (like nsLayoutUtils in this case) don't want to tile, they can say so, and imgFrame::Draw can respect that and we can draw with clamp without creating a temporary.

Comment 6

[Ali Juma [:ajuma]]

Attachment: Let callers of imgFrame::Draw choose not to tile

This is an attempt at following the approach suggested in Comment 5.

With this patch, we spend about 12% less time in nsCSSRendering::PaintBackground while loading cnn.com, 10% less time in PaintBackground when panning, and 9% less time in PaintBackground when zooming. Given the amount of time we spend in PaintBackground on cnn (see Bug 735895), these savings are pretty significant.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 614911 [details] [diff] [review]
Let callers of imgFrame::Draw choose not to tile

Review of attachment 614911 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/public/imgIContainer.idl
@@ +156,5 @@
>     * FLAG_DECODE_NO_COLORSPACE_CONVERSION: Do not do any colorspace conversion;
>     * ignore any embedded profiles, and don't convert to any particular destination
>     * space.
> +   *
> +   * FLAG_NO_TILING: Do not tile; this prevents creating a temporary subimage copy.

The comment should say that this only affects 'draw'.

Avoiding negatives is good, so I think this should be FLAG_CLAMP, and the comment should just say that the image is extended to the aFill area by clamping image sample coordinates instead of by tiling. (That it prevents making temporaries is an implementation detail.)

Fix the comment on aFill in 'draw' to match.

::: layout/base/nsLayoutUtils.cpp
@@ +3738,5 @@
>  {
>    SAMPLE_LABEL("layout", "nsLayoutUtils::DrawBackgroundImage");
>    return DrawImageInternal(aRenderingContext, aImage, aGraphicsFilter,
>                             aDest, aFill, aAnchor, aDirty,
> +                           aImageSize, aImageFlags | imgIContainer::FLAG_NO_TILING);

This doesn't seem right. This method can be used for tiling, so it's not right to just disable tiling! This should be causing zillions of tests to fail :-).

Maybe you can test here whether aDest contains aFill and disabling tiling if it does?

Comment 8

[Ali Juma [:ajuma]]

Attachment: Let callers of imgIContainer::draw choose to clamp instead of tile

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> Comment on attachment 614911 [details] [diff] [review]
> Let callers of imgFrame::Draw choose not to tile
> 
> Review of attachment 614911 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: layout/base/nsLayoutUtils.cpp
> @@ +3738,5 @@
> >  {
> >    SAMPLE_LABEL("layout", "nsLayoutUtils::DrawBackgroundImage");
> >    return DrawImageInternal(aRenderingContext, aImage, aGraphicsFilter,
> >                             aDest, aFill, aAnchor, aDirty,
> > +                           aImageSize, aImageFlags | imgIContainer::FLAG_NO_TILING);
> 
> This doesn't seem right. This method can be used for tiling, so it's not
> right to just disable tiling! This should be causing zillions of tests to
> fail :-).

Indeed it did :)

> Maybe you can test here whether aDest contains aFill and disabling tiling if
> it does?

This new patch does this, though it does so in DrawImageInternal rather than DrawBackgroundImage (where the flag was set in the previous patch), since there doesn't seem to be any reason to restrict this to background images. 

With this patch, we call CreateSamplingRestrictedDrawable about 180 times rather than about 540 times when loading cnn.com. However, the time savings in nsCSSRendering::PaintBackground are much more modest now -- between 1.5% and 2.5% when loading/panning/zooming cnn.com.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 615850 [details] [diff] [review]
Let callers of imgIContainer::draw choose to clamp instead of tile

Review of attachment 615850 [details] [diff] [review]:
-----------------------------------------------------------------

Great!

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Ali Juma [:ajuma] from comment #8)
> With this patch, we call CreateSamplingRestrictedDrawable about 180 times
> rather than about 540 times when loading cnn.com. However, the time savings
> in nsCSSRendering::PaintBackground are much more modest now -- between 1.5%
> and 2.5% when loading/panning/zooming cnn.com.

Interesting. However, you were probably getting a performance boost before by not tiling at all when we actually need to tile, so there were some false savings.

Still seems worth looking into the remaining cases of CreateSamplingRestrictedDrawable to see if we can avoid them.

Comment 11

[Ali Juma [:ajuma]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fdfd683899fd

Comment 12

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/fdfd683899fd