Last Comment Bug 733941 - Fennec: In DrawPixelSnapped we seem to hit CreateSamplingRestrictedDrawable a lot
: Fennec: In DrawPixelSnapped we seem to hit CreateSamplingRestrictedDrawable a...
Status: RESOLVED FIXED
[gfx]
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla14
Assigned To: Ali Juma [:ajuma]
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: checkerboarding 735895 729391
  Show dependency treegraph
 
Reported: 2012-03-07 15:53 PST by Jeff Muizelaar [:jrmuizel]
Modified: 2012-04-18 13:20 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta+


Attachments
Let callers of imgFrame::Draw choose not to tile (10.58 KB, patch)
2012-04-13 14:23 PDT, Ali Juma [:ajuma]
no flags Details | Diff | Splinter Review
Let callers of imgIContainer::draw choose to clamp instead of tile (11.68 KB, patch)
2012-04-17 13:33 PDT, Ali Juma [:ajuma]
roc: review+
Details | Diff | Splinter Review

Description Jeff Muizelaar [:jrmuizel] 2012-03-07 15:53:15 PST
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) 2012-03-08 12:50:02 PST
Timothy, I suspect this is related to bug 733607.
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-03-18 18:47:59 PDT
(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) 2012-04-02 08:54:20 PDT
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] 2012-04-03 11:06:43 PDT
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) 2012-04-03 14:02:22 PDT
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] 2012-04-13 14:23:15 PDT
Created attachment 614911 [details] [diff] [review]
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) 2012-04-13 15:28:26 PDT
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] 2012-04-17 13:33:21 PDT
Created attachment 615850 [details] [diff] [review]
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) 2012-04-17 14:53:14 PDT
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) 2012-04-17 14:57:48 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.