Fennec: In DrawPixelSnapped we seem to hit CreateSamplingRestrictedDrawable a lot

RESOLVED FIXED in mozilla14

Status

()

Core
Layout
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jrmuizel, Assigned: ajuma)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla14
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-fennec1.0 beta+)

Details

(Whiteboard: [gfx])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
Blocks: 729391
Timothy, I suspect this is related to bug 733607.
Assignee: nobody → tnikkel
(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.
(Reporter)

Updated

5 years ago
Blocks: 717774
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.
blocking-fennec1.0: --- → ?

Updated

5 years ago
blocking-fennec1.0: ? → beta+
(Reporter)

Comment 4

5 years ago
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.
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.

Updated

5 years ago
Assignee: tnikkel → jmuizelaar
Whiteboard: [gfx]
(Assignee)

Updated

5 years ago
Blocks: 735895
blocking-fennec1.0: beta+ → ?
(Assignee)

Comment 6

5 years ago
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.
Attachment #614911 - Flags: feedback?(roc)
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?
blocking-fennec1.0: ? → beta+

Updated

5 years ago
Assignee: jmuizelaar → ajuma
(Assignee)

Comment 8

5 years ago
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.
Attachment #614911 - Attachment is obsolete: true
Attachment #615850 - Flags: review?(roc)
Attachment #614911 - Flags: feedback?(roc)
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!
Attachment #615850 - Flags: review?(roc) → review+
(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.
(Assignee)

Comment 11

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdfd683899fd
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/fdfd683899fd
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.