Shadows are not drawn in device space on Fennec

RESOLVED FIXED in Firefox 14

Status

()

defect
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: jrmuizel, Assigned: jrmuizel)

Tracking

unspecified
mozilla15
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox14 fixed, blocking-fennec1.0 soft)

Details

Attachments

(2 attachments, 4 obsolete attachments)

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.
blocking-fennec1.0: --- → ?
Blocks: 753028
Posted patch WIP (obsolete) — Splinter Review
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.
Assignee: nobody → jmuizelaar
blocking-fennec1.0: ? → soft
Posted patch WIP 2 (obsolete) — Splinter Review
roc, how do you feel about something like this
Attachment #624423 - Attachment is obsolete: true
Attachment #624556 - Flags: feedback?(roc)
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.
Attachment #624556 - Attachment is obsolete: true
Attachment #624556 - Flags: feedback?(roc)
Attachment #624863 - Flags: review?(roc)
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.
Attachment #624863 - Attachment is obsolete: true
Attachment #624863 - Flags: review?(roc)
Attachment #624960 - Flags: review?(roc)
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.
Attachment #624960 - Attachment is obsolete: true
Attachment #624960 - Flags: review?(roc)
Attachment #624996 - Flags: review?(roc)
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);

{}
Attachment #624996 - Flags: review?(roc) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/2de348d84e83
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
(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
Attachment #624996 - Flags: approval-mozilla-aurora?
Comment on attachment 624996 [details] [diff] [review]
Do blurs in device space on mobile

Please land ASAP for the Fennec beta build.
Attachment #624996 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.