Closed Bug 1162824 Opened 4 years ago Closed 4 years ago

Change box shadow cache to cache the colored blurred box shadow

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox40 --- affected
firefox41 --- fixed

People

(Reporter: mchang, Assigned: mchang)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file, 1 obsolete file)

Right now, the box shadow cache tries to cache a full size rect blurred surface. With bug 1155828, we use a small minimal blurred surface and stretch it out to fill the target rect. Change the blur cache to cache the small minimal blurred surface.
Attached patch Cache min box shadow (obsolete) — Splinter Review
Changes the current caching strategy from aRect, to only caching the minimum colored box shadow. Relies on bug 1155828. If we want to fix [2], we can do that in a follow up bug. 

Initial results look good. Blurring the small min rect for [1] takes ~0.3 - 04 ms. Using the cache takes ~1 us. While scrolling the whole page w/ APZ on a retina mbp + APZ + e10s + 1024x1024 tile sizes, we spend ~12ms blurring with this patch + Bug 1155828. The original implementation of blurring before this work took ~305ms, so ~25x improvement!

[1] http://www.reddit.com/r/gameofthrones/comments/33zfzq/s5_postpremiere_discussion_503_high_sparrow/
[2] https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxBlur.cpp?from=gfxBlur.cpp&case=true#300
Attachment #8603633 - Flags: review?(mstange)
Comment on attachment 8603633 [details] [diff] [review]
Cache min box shadow

Review of attachment 8603633 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxBlur.cpp
@@ +165,4 @@
>    typedef const BlurCacheKey* KeyTypePointer;
>    enum { ALLOW_MEMMOVE = true };
>  
> +  IntSize mMinimumSize;

I'd just call this mSize.

@@ +186,5 @@
> +      } else {
> +        mCornerRadii.radii[i] = Size(0, 0);
> +      }
> +    }
> +  }

Can you instead construct mCornerRadii in the initializer list, like this:
, mCornerRadii(aCornerRadii ? *aCornerRadii : RectCornerRadii())

If this doesn't work, can you add a copy constructor to RectCornerRadii so that it does?

@@ +196,5 @@
>      , mBackend(aOther->mBackend)
> +  {
> +    for (size_t i = 0; i < 4; i++) {
> +      mCornerRadii.radii[i] = aOther->mCornerRadii.radii[i];
> +    }

and also use that here
Attachment #8603633 - Flags: review?(mstange) → review+
Carrying r+, updated with feedback from comment 2.
Attachment #8603633 - Attachment is obsolete: true
Attachment #8605519 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/d8a584dddc0c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Backed out to backout bug 1155828.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e6654f7fdea
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/869aca2db34d
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Depends on: 1361787
You need to log in before you can comment on or make changes to this bug.