Currently we same to be scaling the shadow surface that we're masking() through. You can see this if you force nearest-neighbour image scaling. This causes us to go down a pixman slowpath for add_a8_scaled and causes great slowness. I expect this is especially painful when drawing screenshots as we'll do the bluring at a much higher resolution than needed.

This seems to work a little bit

Softblocker for now, but let's nom for higher if we end up hitting this all the time or on high-profile pages.

roc, how do you feel about something like this

Comment on attachment 624556 [details] [diff] [review] WIP 2 Review of attachment 624556 [details] [diff] [review]: ----------------------------------------------------------------- Can't we do everything in nsContextBoxBlur and leave gfxBlur alone?

This move's everything into nsContentBoxBlur and should be cleaner.

Comment on attachment 624863 [details] [diff] [review] Do blur's in device space on mobile Review of attachment 624863 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsCSSRendering.cpp @@ +4156,5 @@ > + // estimate the transformed blur radius > + // this will give an approximation of the desired result > + gfxSize s1 = aTransform.Transform(gfxSize(blurStdDev, 0)); > + gfxSize s2 = aTransform.Transform(gfxSize(0, blurStdDev)); > + blurStdDev = (sqrt(s1.width*s1.width + s1.height*s1.height) + sqrt(s2.width*s2.width + s2.height*s2.height))/2.; NS_hypot? @@ +4184,5 @@ > > +#ifdef MOZ_GFX_OPTIMIZE_MOBILE > + // do blurs in device space on mobile, this will give > + // incorrect results (e.g. rotated blurs) but is cheaper > + // and is what Chrome currently does. Why is this cheaper? I thought you'd just get the current transform and use it to increase the resolution of the blur temporary surface, and then blit back the image under the current transform. Is this patch much cheaper than that? ::: layout/base/nsCSSRendering.h @@ +615,4 @@ > gfxAlphaBoxBlur blur; > nsRefPtr<gfxContext> mContext; > gfxContext* mDestinationCtx; > + bool mTransformed; Document this

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7) > > @@ +4184,5 @@ > > > > +#ifdef MOZ_GFX_OPTIMIZE_MOBILE > > + // do blurs in device space on mobile, this will give > > + // incorrect results (e.g. rotated blurs) but is cheaper > > + // and is what Chrome currently does. > > Why is this cheaper? Because when zoomed out we end up with smaller blur radii and smaller blur surface. In addition, scaled mask() operations are not fastpath'd in pixman.

This falls back to the old code path for non-uniform scale transforms.

Comment on attachment 624960 [details] [diff] [review] Do blurs in device space on mobile Review of attachment 624960 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsCSSRendering.cpp @@ +4178,5 @@ > } > > + gfxFloat scale = 1; > +#ifdef MOZ_GFX_OPTIMIZE_MOBILE > + // Do blurs in device space on mobile. We should do this optimization everywhere. @@ +4228,5 @@ > + blurRadius, &dirtyRect, NULL); > + } > + > + if (mContext) // we don't need to blur if skipRect is equal to rect > + mContext->SetMatrix(transform); {} @@ +4240,5 @@ > return; > > + if (mPreTransformed) { > + gfxContextMatrixAutoSaveRestore saveMatrix(mDestinationCtx); > + mDestinationCtx->IdentityMatrix(); this code does nothing since saveMatrix is immediately destroyed and the matrix restored. So I'm deeply confused as to how this patch worked at all.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10) > > @@ +4240,5 @@ > > return; > > > > + if (mPreTransformed) { > > + gfxContextMatrixAutoSaveRestore saveMatrix(mDestinationCtx); > > + mDestinationCtx->IdentityMatrix(); > > this code does nothing since saveMatrix is immediately destroyed and the > matrix restored. So I'm deeply confused as to how this patch worked at all. I'm also confused, though when I was originally testing I was setting mDestinationCtx to IdentityMatrix() unconditionally.

Comment on attachment 624996 [details] [diff] [review] Do blurs in device space on mobile Review of attachment 624996 [details] [diff] [review]: ----------------------------------------------------------------- r+ with that fixed ::: layout/base/nsCSSRendering.cpp @@ +4225,5 @@ > + blurRadius, &dirtyRect, NULL); > + } > + > + if (mContext) // we don't need to blur if skipRect is equal to rect > + mContext->SetMatrix(transform); {}

https://hg.mozilla.org/integration/mozilla-inbound/rev/2de348d84e83 It looks like we need to handle x and y being different scales for screenshotting. I'll handle that in another bug.

(In reply to Jeff Muizelaar [:jrmuizel] from comment #14) > I'll handle that in another bug. bug 757039

Comment on attachment 624996 [details] [diff] [review] Do blurs in device space on mobile [Approval Request Comment] Bug caused by (feature/regressing bug #): None User impact if declined: very slow box-shadow on fennec and potential OOM Testing completed (on m-c, etc.): Been on m-c for a while Risk to taking this patch (and alternatives if risky): Some risk because it impacts desktop as well as mobile. Passes all our tests String or UUID changes made by this patch: None

Comment on attachment 624996 [details] [diff] [review] Do blurs in device space on mobile Please land ASAP for the Fennec beta build.