The default bug view has changed. See this FAQ.

Shadows are not drawn in device space on Fennec

RESOLVED FIXED in Firefox 14

Status

()

Core
Layout
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: jrmuizel, Assigned: jrmuizel)

Tracking

unspecified
mozilla15
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox14 fixed, blocking-fennec1.0 soft)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

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

Updated

5 years ago
blocking-fennec1.0: --- → ?
(Assignee)

Updated

5 years ago
Blocks: 753028
(Assignee)

Comment 1

5 years ago
Created attachment 623796 [details]
Screenshot of http://people.mozilla.org/~jmuizelaar/tmp/shadow.html
(Assignee)

Comment 2

5 years ago
Created attachment 624423 [details] [diff] [review]
WIP

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
(Assignee)

Comment 4

5 years ago
Created attachment 624556 [details] [diff] [review]
WIP 2

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?
(Assignee)

Comment 6

5 years ago
Created attachment 624863 [details] [diff] [review]
Do blur's in device space on mobile

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
(Assignee)

Comment 8

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

Comment 9

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

Comment 11

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

Comment 12

5 years ago
Created attachment 624996 [details] [diff] [review]
Do blurs in device space on mobile
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+
(Assignee)

Comment 14

5 years ago
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
Last Resolved: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
(Assignee)

Comment 16

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

bug 757039
(Assignee)

Comment 17

5 years ago
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+
(Assignee)

Comment 19

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/5ee3dbd560b5
status-firefox14: --- → fixed
https://hg.mozilla.org/mozilla-central/rev/80d398ab3ae5
You need to log in before you can comment on or make changes to this bug.