Last Comment Bug 754985 - Shadows are not drawn in device space on Fennec
: Shadows are not drawn in device space on Fennec
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla15
Assigned To: Jeff Muizelaar [:jrmuizel]
:
Mentors:
Depends on:
Blocks: 753028
  Show dependency treegraph
 
Reported: 2012-05-14 12:59 PDT by Jeff Muizelaar [:jrmuizel]
Modified: 2015-01-12 02:53 PST (History)
3 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
soft


Attachments
Screenshot of http://people.mozilla.org/~jmuizelaar/tmp/shadow.html (23.96 KB, image/png)
2012-05-14 13:22 PDT, Jeff Muizelaar [:jrmuizel]
no flags Details
WIP (5.75 KB, patch)
2012-05-16 09:47 PDT, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Splinter Review
WIP 2 (6.57 KB, patch)
2012-05-16 15:16 PDT, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Splinter Review
Do blur's in device space on mobile (4.22 KB, patch)
2012-05-17 13:29 PDT, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Splinter Review
Do blurs in device space on mobile (3.92 KB, patch)
2012-05-17 17:06 PDT, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Splinter Review
Do blurs in device space on mobile (3.86 KB, patch)
2012-05-17 19:44 PDT, Jeff Muizelaar [:jrmuizel]
roc: review+
joe: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Jeff Muizelaar [:jrmuizel] 2012-05-14 12:59:03 PDT
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] 2012-05-14 13:22:04 PDT
Created attachment 623796 [details]
Screenshot of http://people.mozilla.org/~jmuizelaar/tmp/shadow.html
Comment 2 Jeff Muizelaar [:jrmuizel] 2012-05-16 09:47:30 PDT
Created attachment 624423 [details] [diff] [review]
WIP

This seems to work a little bit
Comment 3 Joe Drew (not getting mail) 2012-05-16 11:46:33 PDT
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] 2012-05-16 15:16:45 PDT
Created attachment 624556 [details] [diff] [review]
WIP 2

roc, how do you feel about something like this
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-16 15:32:35 PDT
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] 2012-05-17 13:29:41 PDT
Created attachment 624863 [details] [diff] [review]
Do blur's in device space on mobile

This move's everything into nsContentBoxBlur and should be cleaner.
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-17 15:12:44 PDT
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] 2012-05-17 15:36:49 PDT
(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] 2012-05-17 17:06:20 PDT
Created attachment 624960 [details] [diff] [review]
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) (Exited; email my personal email if necessary) 2012-05-17 17:17:36 PDT
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] 2012-05-17 17:27:03 PDT
(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] 2012-05-17 19:44:13 PDT
Created attachment 624996 [details] [diff] [review]
Do blurs in device space on mobile
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-17 19:46:22 PDT
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] 2012-05-18 11:06:02 PDT
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] 2012-05-18 18:25:06 PDT
https://hg.mozilla.org/mozilla-central/rev/2de348d84e83
Comment 16 Jeff Muizelaar [:jrmuizel] 2012-05-22 08:14:47 PDT
(In reply to Jeff Muizelaar [:jrmuizel] from comment #14)
> I'll handle that in another bug.

bug 757039
Comment 17 Jeff Muizelaar [:jrmuizel] 2012-05-25 19:17:28 PDT
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) 2012-05-28 11:38:27 PDT
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] 2012-05-31 20:41:04 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/5ee3dbd560b5
Comment 20 Carsten Book [:Tomcat] 2015-01-12 02:53:30 PST
https://hg.mozilla.org/mozilla-central/rev/80d398ab3ae5

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