1250947 - Inset box-shadow still clipped in some cases

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Loic]

Attachment: index.html

+++ This bug was initially created as a clone of Bug #1250890 +++

Bug 1227216 has not fixed all cases with inset box-shadow bad drawn.

STR: open testcase.

Result: black inset box-shadow is clipped.

Comment 1

[Loic]

Mason, could you check, please. You didn't fix all inset box-shadow issues.

Comment 3

[Mason Chang [Inactive] [:mchang]]

Attachment: Always draw outer box shadow path

The problem with this test case was that it had a large border radius, a small blur radius, and a slightly large offset. The large blur radius and the offset caused us to fail at this check[1]. This patch always fills in the path between the outer and inner shadow rects, blurs the minimum blur rect, and copies the portions of the blur to the destination rect. 

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxBlur.cpp?case=true&from=gfxBlur.cpp#945

Comment 4

[Liz Henry (:lizzard) (relman/hg->git project)]

Regression since 44, seems like minor edge cases, I don't think we need to track here. But please request uplift if you come up with a fix.

Comment 5

[Markus Stange [:mstange]]

Comment on attachment 8724670 [details] [diff] [review]
Always draw outer box shadow path

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

You can't do this. Compare this testcase before and after the patch: data:text/html,<div style="width:200px; height: 200px; box-shadow: inset 10px 10px 5px black">
The fuzzy edge extends both inwards and outwards. If you draw a non-fuzzy box-shadow beneath the fuzzy part, you destroy half of the fuzziness.

Comment 6

[Markus Stange [:mstange]]

What you could do instead is this:
 1. Compute the size of the "hole" from the size of the clip and from the spread radius. Ignore the offset.
 2. Compute the size of the "fuzzy part" by extending the size of the hole by the blur radius, still ignoring the offset.
 3. Fill everything outside of the "fuzzy part" with the solid shadow color, clipped to the clip. Ignore the radius here.
 4. Fill the fuzzy part either using a minimal box shadow surface or with the direct drawing path, clipped to the clip.

Only step 3 and 4 would look at the offset to find out where to draw. And only step 4 needs to know about the radius.

Your patch here is trying to do 3 but it uses the size of the hole instead of the size fuzzy part.

Comment 7

[Markus Stange [:mstange]]

Oh, every time I said "radius" instead of "blur radius" in comment 6, I was referring to the corner radius.

Comment 8

[Mason Chang [Inactive] [:mchang]]

Attachment: Fill area outside of blur with solid color

An approach based on comment 6, although I had to significantly rework how we drew inset box shadows. I couldn't quite get blends to be correct with the current approach since the min box shadow blurs both inward and outward and there was no clean way to cut the min box shadow such that the blurs would look good.

Instead, I kind of worked my way looking at the min blur and the rect's were given. From comment 6, I did this by calculating (1) via the clip + blur radius + corner radius. The clip rect we're given from layout already includes the spread radius. Compute (2) by taking (1) + the blur + corner radius since we could have a very small blur but a large corner radius (which we'll come back to when creating the min blur. Fill (3), and draw in (4).

How we create (4) is a bit different this time. We needed more frame space around the center white space to blend correctly. I created the min box shadow inside out. We have two margins: (A) The blur + corner margin. (B) The blur margin. We create a white hole the size of (2 * A). We surround this with one frame of black the size of (A), the inner frame. Then expand the inner frame by (B). We then blur this.

When we copy the min blur over, we copy one margin (A) worth outside of the white hole and one margin (A) worth inside the whole hole onto the destination. This is quite nice in that we correctly get the right color blends versus before, we'd still be a bit off because the blur would blur with the white area outside the frame because the frame wasn't big enough.

Comment 11

[Markus Stange [:mstange]]

Comment on attachment 8731298 [details] [diff] [review]
Fill area outside of blur with solid color

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

Sorry, I don't understand most of this. I think part of the problem is that you're trying to recover rectangles inside gfxBlur.cpp that nsCSSRendering has already modified. Can you instead change nsCSSRendering to pass in the information you really need?

::: gfx/thebes/gfxBlur.cpp
@@ +339,5 @@
>                                          const Color& aShadowColor,
>                                          const bool& aHasBorderRadius,
>                                          BackendType aBackendType)
>      {
> +      bool insetBoxShadow = true;

Alternatively you could add an enum class ShadowType : uint8_t { INSET, OUTSET }. This will make the parameter readable without pulling it out into a new variable.

@@ +918,5 @@
> + * We create the rects going inside out. The rects have 4 frames
> + * around an empty white space. This is because blurs
> + * blend both into and out of the frame around them.
> + * The frames from inside to outside:
> + * 1) Whitespace the size of blur * 2 + 1 - +1 so we can copy the edges

Why do we need more than 1px of whitespace?

@@ +921,5 @@
> + * The frames from inside to outside:
> + * 1) Whitespace the size of blur * 2 + 1 - +1 so we can copy the edges
> + * 2) Frame around the whitespace - corner + blur
> + * 3) Filled solid color frame around (2) - So we get the correct blend color
> + * 4) blur around (3).

So 4 contains blurred pixels and not just those not affected by the inner transparency?

@@ +955,5 @@
> +      cornerHeight = std::max(cornerHeight, aInnerClipRadii[i].height);
> +    }
> +  }
> +
> +  // Only the inside layers care about the border radius size.

Inside layers? What does layers mean in this context?

@@ +973,5 @@
> +                       const Rect aDestinationRect,
> +                       Rect& aOutInnerRect,
> +                       Rect& aOutOuterRect)
> +{
> +  // We always copy 2 * blur + corner worth of data to the destination rect

What does blur mean here? Is it the blur radius?

@@ +1053,5 @@
> +    Rect destBlur = aDestinationRect;
> +    destBlur.Inflate(blurMargin);
> +    destDrawTarget->DrawSurface(minBlur, destBlur, srcBlur);
> +  } else {
> +    // This is the inverse of up top plus one more blur due to actually doing teh blur

teh -> the

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Since bug 1256403 is now duped to this bug we may want to track this after all, since it may be causing some sites to not work to the point of Firefox freezing on Windows 10. Tracking for 48.

Comment 13

[Mason Chang [Inactive] [:mchang]]

Attachment: Fill area outside of blur with solid color

Rewritten based on what we discussed. We look a lot more like chrome on your test page as well now!

The really only ugly part is having to deal with the translation matrix on the min gfx context at [1]. This is in GetInsetBlur(). When we don't use the destination rects, we want to fill the area between the inside white space and the outer frame. However, this would be shifted by the expanded blur radius if we use the exact rects given to us in GetInsetBlur, which are calculated based off (0, 0) offset. So we set the matrix to the identity matrix, which does everything correctly. The downside is that the rects given to us by layout account for this translation, so when we have to use the dest rects, we keep the translation. Either way, we have to account for this :/

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxBlur.cpp?from=gfxBlur.cpp#85

Comment 14

[Markus Stange [:mstange]]

Comment on attachment 8734862 [details] [diff] [review]
Fill area outside of blur with solid color

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

This seems to work well. Looks ok code-wise, too.

I think we're still creating unnecessarily large cached surfaces for useDestRect blurs. It would be nice to use the "fill outside shape" path for them, too. But this can happen in another bug.
Most of the shadows on http://tests.themasta.com/inset-box-shadows.html are useDestRect shadows.

::: gfx/thebes/gfxBlur.cpp
@@ +963,5 @@
> +      cornerHeight = std::max(cornerHeight, aInnerClipRadii[i].height);
> +    }
> +  }
> +
> +  // Only the inside layers care about the border radius size.

Still don't know what layers means here.

Comment 15

[Mason Chang [Inactive] [:mchang]]

(In reply to Markus Stange [:mstange] from comment #14)
> Comment on attachment 8734862 [details] [diff] [review]
> Fill area outside of blur with solid color
> 
> Review of attachment 8734862 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems to work well. Looks ok code-wise, too.
> 
> I think we're still creating unnecessarily large cached surfaces for
> useDestRect blurs. It would be nice to use the "fill outside shape" path for
> them, too. But this can happen in another bug.
> Most of the shadows on http://tests.themasta.com/inset-box-shadows.html are
> useDestRect shadows.
> 

We're not caching useDestRect blurs. I'm not sure how this would actually work since the formula of for how we determine the minimum whitespace is bigger than the shadow clip rect we're getting from layout. That means, we'd probably have to do some more hacks to get that to work or feed more information from layout.

> ::: gfx/thebes/gfxBlur.cpp
> @@ +963,5 @@
> > +      cornerHeight = std::max(cornerHeight, aInnerClipRadii[i].height);
> > +    }
> > +  }
> > +
> > +  // Only the inside layers care about the border radius size.
> 
> Still don't know what layers means here.

Fixed this in the final patch.

Successful try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=6763aa7a9e22&selectedJob=18708682

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/98818a65c221

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/98818a65c221

Comment 19

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8734862 [details] [diff] [review]
Fill area outside of blur with solid color

Approval Request Comment
[Feature/regressing bug #]: Rewrite of inset box shadows
[User impact if declined]: Some graphics artifacts, some pages freeze due to slow box shadows, potential security problem as shown in bug 1251503.
[Describe test coverage new/current, TreeHerder]: Manual, two added reftests
[Risks and why]: Medium - We changed how inset box shadows render, which in the past have caused new regressions.
[String/UUID change made/needed]: None

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Loic, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Mason, Milan: I get anxious when I see "rewrite" kinds of patches being requested for uplift. The risk associated is also medium and this has baked on Nightly for only a few days. Is there a pressing reason to uplift this (read user impact but it's hard to know how often does it happen)? I prefer to let this one ride the trains. Please let me know.

Comment 22

[Milan Sreckovic [:milan] (needinfo for best results)]

We could go either way - see comment 12, in case that makes it a clearer candidate for an uplift.

Comment 23

[Loic]

(In reply to Ritu Kothari (:ritu) from comment #20)
> Hi Loic, could you please verify this issue is fixed as expected on a latest
> Nightly build? Thanks!

Fixed with the latest Nightly (Win 7), with HWA on/off and e10s on/off.

Comment 24

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8734862 [details] [diff] [review]
Fill area outside of blur with solid color

The fix was verified, it has been on Nightly for a few days, Milan is not too worried about uplifting this to Aurora. Let's take it!

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d29bb8d3fa23

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

Fixing flags to reflect fixes here and in duped bugs.

Comment 28

[Lawrence Mandel [:lmandel] (use needinfo)]

Dup bug 1257054 shows ESR45 as affected. Do we need to backport to ESR?

Comment 29

[Mason Chang [Inactive] [:mchang]]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #28)
> Dup bug 1257054 shows ESR45 as affected. Do we need to backport to ESR?

I think it's fairly risky. We've had a lot of random bugs with box-shadows before and this rewrote a lot of it to try to get it right. We haven't had any regressions so far, but I'm still weary. Please see comment 21.

Comment 30

[Lawrence Mandel [:lmandel] (use needinfo)]

Without any complaints from the ESR branch, the best option is likely to leave this as is for now. If complaints do start to come in, we can assess the risk based on the latest information that we have at that time.

Comment 33

[Markus Stange [:mstange]]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #30)
> Without any complaints from the ESR branch, the best option is likely to
> leave this as is for now. If complaints do start to come in, we can assess
> the risk based on the latest information that we have at that time.

Three weeks later, we still haven't seen any regressions. However, we've seen more reports of this bug making sites completely unusable due to extremely bad performance: Facebook reason (bug 1274819) and http://solnic.eu/2016/05/22/my-time-with-rails-is-up.html , both of which were on the front page of HN.
I think we should uplift this patch to ESR 45. I'm not sure if this bug deserves a point release to 46.

Comment 34

[Mason Chang [Inactive] [:mchang]]

(In reply to Markus Stange [:mstange] from comment #33)
> (In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #30)
> > Without any complaints from the ESR branch, the best option is likely to
> > leave this as is for now. If complaints do start to come in, we can assess
> > the risk based on the latest information that we have at that time.
> 
> Three weeks later, we still haven't seen any regressions. However, we've
> seen more reports of this bug making sites completely unusable due to
> extremely bad performance: Facebook reason (bug 1274819) and
> http://solnic.eu/2016/05/22/my-time-with-rails-is-up.html , both of which
> were on the front page of HN.
> I think we should uplift this patch to ESR 45. I'm not sure if this bug
> deserves a point release to 46.

I can go either way, I don't feel strongly enough either way. Lawrence?

Comment 35

[Lawrence Mandel [:lmandel] (use needinfo)]

Transferring my ni to Liz, who is the release manager for ESR this cycle.

Comment 36

[Liz Henry (:lizzard) (relman/hg->git project)]

Sounds like with the fix, we are more correct to specs and performance is also better. mstange says test coverage is now improved for box-shadow use. With all the duplicate bugs reported on 45/46, I think putting this in esr45 (45.2.0esr) is justifiable.

Comment 37

[Markus Stange [:mstange]]

Comment on attachment 8734862 [details] [diff] [review]
Fill area outside of blur with solid color

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: regular web pages can make Firefox completely unresponsive
User impact if declined: 1. missing or wrong shadows in some cases, 2. extreme slowdowns on innocent web pages
Fix Landed on Version: 48 + 47
Risk to taking this patch (and alternatives if risky): medium. It is a rather big patch, but it has baked for two months now, no regressions have been reported, test coverage has been improved for box-shadow recently.
String or UUID changes made by this patch: none

Comment 38

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8734862 [details] [diff] [review]
Fill area outside of blur with solid color

Includes many tests, fix to spec and performance. 
Please uplift for esr 45.

Comment 39

[Liz Henry (:lizzard) (relman/hg->git project)]

Marking affected for esr45 as well based on comment 37.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

This'll need rebasing for esr45.

Comment 41

[Mason Chang [Inactive] [:mchang]]

Attachment: Rebased for ESR 45

Is there a Mozilla-esr45 try I can push to? I had to do a lot of merging. Thanks!

Comment 42

[Ryan VanderMeulen [:RyanVM]]

You can push esr45 to Try with the understanding that it's going to attempt to schedule jobs that were never intended to run on the esr45 codebase and will burn accordingly. Just pay close attention to what runs normally on esr45 and things should be mostly fine, though.

Comment 43

[Carsten Book [:Tomcat]]

mchang is this now ok for landing ?

Comment 44

[Liz Henry (:lizzard) (relman/hg->git project)]

This won't make it into esr45.2.0, but I'll mark it for 45.3.0 which should release in about 6 weeks.

Comment 45

[Mason Chang [Inactive] [:mchang]]

(In reply to Carsten Book [:Tomcat] from comment #43)
> mchang is this now ok for landing ?

Sorry was waiting for try results with ESR 45 - https://treeherder.mozilla.org/#/jobs?repo=try&revision=e44b805cdeca

I think it's ok. From comment 42, lots of burn which I'm not sure is normal, but I don't see any reftest failures I would expect from this patch.

Comment 46

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/50bc30b35369

Comment 47

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/1357b2bbb5b0e31645152cbdd8ac78c9f87d38a3
Bug 1250947 [esr45] followup - Remove duplicate variable declaration to fix -Werror=shadow build failure. r=me, a=bustage