754985 - Shadows are not drawn in device space on Fennec

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: Screenshot of http://people.mozilla.org/~jmuizelaar/tmp/shadow.html

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Attachment: WIP

This seems to work a little bit

Comment 3

[Joe Drew (not getting mail)]

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

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Attachment: WIP 2

roc, how do you feel about something like this

Comment 5

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

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?

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Attachment: Do blur's in device space on mobile

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

Comment 7

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

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

Comment 8

[Jeff Muizelaar [:jrmuizel]]

(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.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

Attachment: Do blurs in device space on mobile

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

Comment 10

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

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.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

(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 12

[Jeff Muizelaar [:jrmuizel]]

Attachment: Do blurs in device space on mobile

Comment 13

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

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);

{}

Comment 14

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2de348d84e83

Comment 16

[Jeff Muizelaar [:jrmuizel]]

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

bug 757039

Comment 17

[Jeff Muizelaar [:jrmuizel]]

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 18

[Joe Drew (not getting mail)]

Comment on attachment 624996 [details] [diff] [review]
Do blurs in device space on mobile

Please land ASAP for the Fennec beta build.

Comment 19

[Jeff Muizelaar [:jrmuizel]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5ee3dbd560b5

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/80d398ab3ae5