3D Rendering issue in special condition (border radius and perspective on parent, transform on child)

RESOLVED FIXED in Firefox 49

Status

()

Core
Graphics: Layers
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: roland, Assigned: mattwoodrow)

Tracking

({regression, testcase})

45 Branch
mozilla50
x86_64
Windows 7
regression, testcase
Points:
---

Firefox Tracking Flags

(e10s-, firefox46- wontfix, firefox47+ wontfix, firefox48+ wontfix, firefox49+ fixed, firefox50 fixed)

Details

(URL)

Attachments

(8 attachments, 3 obsolete attachments)

1.25 KB, text/html
Details
4.65 KB, patch
Details | Diff | Splinter Review
986 bytes, text/html
Details
14.57 KB, image/png
Details
3.29 KB, patch
mstange
: review+
Details | Diff | Splinter Review
4.27 KB, patch
sinker
: review+
Details | Diff | Splinter Review
4.38 KB, patch
sinker
: review+
Details | Diff | Splinter Review
4.11 KB, patch
sinker
: review+
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160501030217

Steps to reproduce:

Example #1
https://jsfiddle.net/70yumc2m/1/

Example #2
https://jsfiddle.net/anL2tsp4/1/

Example #3 - border radius removed
https://jsfiddle.net/4nm14xk3/1/

Affected Firefox version 46.0.0, 47.0 Beta 1, 49.0 Alpha 1
Versions prior Firefox 46 works fine


Actual results:

Example #1
Red area with border radius

Example #2
Black area on top and bigger green area on bottom, border radius applied on bottom

Example #3
Green area


Expected results:

Example #1
Green area with border radius

Example #2
Green area with border radius

Example #3
Green area
(Reporter)

Updated

2 years ago
OS: Unspecified → Windows 7
Hardware: Unspecified → x86_64
(Reporter)

Updated

2 years ago
Component: Untriaged → Graphics: Layers
Product: Firefox → Core

Comment 1

2 years ago
Created attachment 8747798 [details]
1269321.html

Comment 2

2 years ago
Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ddd566eb39b7a39d0ff630343f179fedc8fcafd5&tochange=7e18014be68d9e13760b2814862745911caf49a5

With e10s enabled in Nightly, on example #2, the black corners disappear when scrolling.
Blocks: 1168263
Status: UNCONFIRMED → NEW
status-firefox46: --- → affected
status-firefox47: --- → affected
status-firefox48: --- → affected
status-firefox49: --- → affected
tracking-firefox46: --- → ?
tracking-firefox47: --- → ?
tracking-firefox48: --- → ?
tracking-firefox49: --- → ?
Ever confirmed: true
Flags: needinfo?(matt.woodrow)
Keywords: regression, testcase

Updated

2 years ago
tracking-e10s: --- → ?
(Reporter)

Comment 3

2 years ago
It seems like the issue is related with border radius when overflow:hidden and border-radius applied on the same element. Here are few examples where you can see the issue:

Background glitches, images sometimes disappear, etc...
http://smartslider3.com/carousel-template/
http://smartslider3.com/barber/#fourthslider
http://smartslider3.com/steak-bistro/#fourthslider
Triaging with Liz, won't fix for Firefox 46 and not going to track. Tracking for the other branches.
status-firefox46: affected → wontfix
tracking-firefox46: ? → -
tracking-firefox47: ? → +
tracking-firefox48: ? → +
tracking-firefox49: ? → +
(Reporter)

Comment 5

2 years ago
When you scroll and the border-radius crop of the image goes out of the viewport, the image appears well.
(Assignee)

Comment 6

2 years ago
Created attachment 8748968 [details] [diff] [review]
Propagate mask layer down to child

Having a mask layer forces an intermediate surface, which breaks the rendering of perspective.

This moves the mask down onto the transform layer, which is where we want.

I think in the future we could propagate mask layers down onto children more often instead of forcing intermediate surfaces.
Assignee: nobody → matt.woodrow
Flags: needinfo?(matt.woodrow)

Updated

2 years ago
tracking-e10s: ? → -
Matt, did you want somebody to review the patch from comment 6?
Flags: needinfo?(matt.woodrow)
(Assignee)

Comment 8

2 years ago
(In reply to Milan Sreckovic [:milan] from comment #7)
> Matt, did you want somebody to review the patch from comment 6?

Yes, yes I did. Thanks
Flags: needinfo?(matt.woodrow)
(Assignee)

Updated

2 years ago
Attachment #8748968 - Flags: review?(tlee)

Comment 9

2 years ago
I am looking into the patch.  May spend more time than usual, just let you know.
Created attachment 8754334 [details] [diff] [review]
fix-intermeidatesurface-origin.diff

Hi Matt,

I don't really understand how your patch handle cases that there is a translation applied on the element.  It doesn't make sense for me to fix the problem by moving mask layers to child, so I do some study on the code.

I find the problem is the layers with mask create a intermediate surface and the content of descendants had been rendered at wrong position.  For the second testcase, gecko create a buffer with the size of 300x300 pixels (a right size) for the intermediate surface, but drew content of children at 8,77 on my linux machine and running with --layoutdebug.   This is exactly the offset the whole document from the left-top corner of the window, and it also match the result that the green area had been shifted from right position with the exact same offset.

In this patch, I try to fix the origin of intermediate buffers, and it looks right for both test cases.
Flags: needinfo?(matt.woodrow)
(Reporter)

Comment 11

2 years ago
Thank you Thinker Li to take a look at the problem.

Could you try if these examples are working fine with your patch or not?

http://smartslider3.com/carousel-template/
http://smartslider3.com/barber/#fourthslider
http://smartslider3.com/steak-bistro/#fourthslider
(In reply to roland from comment #11)
> Thank you Thinker Li to take a look at the problem.
> 
> Could you try if these examples are working fine with your patch or not?

They all look good with my patch on my linux machine.
(Reporter)

Comment 13

2 years ago
Thanks you, that is a great news!

I have to create a new ticket for a different issue, but maybe it is related to this one. If you have a few minutes, could you take a look at this ticket with your patch? https://bugzilla.mozilla.org/show_bug.cgi?id=1274495
Version: 46 Branch → 45 Branch
(Assignee)

Comment 14

2 years ago
(In reply to Thinker Li [:sinker] from comment #10)
> Created attachment 8754334 [details] [diff] [review]
> fix-intermeidatesurface-origin.diff
> 
> Hi Matt,
> 
> I don't really understand how your patch handle cases that there is a
> translation applied on the element.  It doesn't make sense for me to fix the
> problem by moving mask layers to child, so I do some study on the code.
> 
> I find the problem is the layers with mask create a intermediate surface and
> the content of descendants had been rendered at wrong position.  For the
> second testcase, gecko create a buffer with the size of 300x300 pixels (a
> right size) for the intermediate surface, but drew content of children at
> 8,77 on my linux machine and running with --layoutdebug.   This is exactly
> the offset the whole document from the left-top corner of the window, and it
> also match the result that the green area had been shifted from right
> position with the exact same offset.
> 
> In this patch, I try to fix the origin of intermediate buffers, and it looks
> right for both test cases.

When we have a MaskLayer, we force an intermediate surface so that the input to the mask shader is always a BGRA surface.

Perspective layers can't have a an intermediate surface, since we need to combine their transform with their children to get proper 3d perspective.

The testcase in this bug (and in my patch) don't actually have any depth, so the difference isn't visible.

The possible solutions I came up with are:

* Create an extra layer on the outside of the perspective and attach the mask layer to that instead. This is a real pain and we run into all the issue with z-index and sorting etc.

* Change the compositors to support a mask effect on top of any source effect, not just the BGRA surface, then stop forcing an intermediate surface. This is nice, but it's a lot of changes.

* Push the mask layer down to the transformed child and then require an intermediate surface there. This is fairly similar to what we had before nsDisplayPerspective and so it's what I went for.

I just realised that this won't work for preserve-3d though, we need to keep pushing the mask down to the preserve-3d leaves. I'll work on that unless you have a strong objection.
Flags: needinfo?(matt.woodrow)
Created attachment 8756232 [details] [diff] [review]
fix-p3d-layer-shadow-visible-regions.diff

The solution implemented by this patch is to keep the mask layer as it was.  But, make effective transforms of the sub-tree as the accumulation of transforms from the establisher (the parent of the mask layer owner) of the 3D rendering context to the layer itself, but do not include the transform of the establisher.

The coordination of the intermediate surface is the same as the one of the establisher before transform.  So, it works like an intermediate surface for the establisher but only for the content of a sub-tree, and it is actually held by a child.

This patch is not complete, the changes for basic layers are not included now.
Attachment #8754334 - Attachment is obsolete: true
(Assignee)

Comment 16

2 years ago
So, you're effectively making the intermediate surface a post transform effect instead of pre?

I'm not convinced that that's any simpler, and it's quite different to how we handle other things.

Moving properties down to children via GetEffectiveXXX is how we've solved this sort of thing in the past, so seems easier to understand.
(Assignee)

Comment 18

2 years ago
Created attachment 8756706 [details] [diff] [review]
Propagate mask layer down to child v2

This fixes things when we have preserve-3d too, and adds a new reftest for that.
Attachment #8748968 - Attachment is obsolete: true
Attachment #8748968 - Flags: review?(tlee)
Attachment #8756706 - Flags: review?(tlee)
I agree with you now.  It is would be complicated to handle sorting.  For now, Collect3DContextLeaves() stop at layers with an intermediate surface.  It is wrong for all descendants of layers with an intermediate surface.  They should attend the same sorting instead of breaking into independent sorting.

I mean the solution of pushing masks to leaves would be more simple.
Comment on attachment 8756706 [details] [diff] [review]
Propagate mask layer down to child v2

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

Will it change the result of mask layers with AA?  Gecko's mask layers for rounded clips have applied AA.

::: gfx/layers/Layers.h
@@ +1362,5 @@
>  
> +  Layer* GetEffectiveMaskLayer() const;
> +  size_t GetEffectiveAncestorMaskLayerCount() const;
> +  Layer* GetEffectiveAncestorMaskLayerAt(size_t aIndex) const;
> +  bool HasEffectiveMaskLayers() const;

We should move GetMaskLayer(), GetAncestorMaskLayerCount(), and HasMaskLayers() to private to prevent others from using them in accident.

::: gfx/layers/composite/LayerManagerComposite.h
@@ +602,5 @@
>    size_t maskLayerCount = 0;
>    size_t nextAncestorMaskLayer = 0;
>  
> +  size_t ancestorMaskLayerCount = aLayer->GetEffectiveAncestorMaskLayerCount();
> +  if (Layer* ownMask = aLayer->GetEffectiveMaskLayer()) {

Please change the comment before the function too.
Created attachment 8757128 [details]
rounded-corner-color.html

This example shows the color at the left-top corner of the rounded clip is very weird. Background color (red) and inner's color (green) should not be showed there, but they are.  It results from applying the mask layer independently.

So, the solution may be to create a container layer to hold the mask layer at first place where the nsIFrame owning the clip is.
Created attachment 8757134 [details]
rounded-corner-color.png

This is what I see on my desktop (with the patch to fix transformation).
Duplicate of this bug: 1275213
(Assignee)

Comment 24

2 years ago
Created attachment 8758122 [details] [diff] [review]
Part 1: Add a way to check if the current clip has rounded corners
Attachment #8756706 - Attachment is obsolete: true
Attachment #8756706 - Flags: review?(tlee)
(Assignee)

Comment 25

2 years ago
Created attachment 8758123 [details] [diff] [review]
Part 2: Add a new constructor option to nsDisplayOwnLayer that doesn't force active layers
(Assignee)

Comment 26

2 years ago
Created attachment 8758124 [details] [diff] [review]
Part 3: Create a wrapper layer for masking when we can't support it on the normal layer
Attachment #8758124 - Flags: review?(tlee)
(Assignee)

Comment 27

2 years ago
Created attachment 8758125 [details] [diff] [review]
Part 4: Add tests

Updated

2 years ago
Attachment #8758124 - Flags: review?(tlee) → review+

Updated

2 years ago
status-firefox47: affected → wontfix
(Assignee)

Updated

2 years ago
Attachment #8758122 - Flags: review?(mstange)
(Assignee)

Updated

2 years ago
Attachment #8758123 - Flags: review?(tlee)
(Assignee)

Updated

2 years ago
Attachment #8758125 - Flags: review?(tlee)
Comment on attachment 8758122 [details] [diff] [review]
Part 1: Add a way to check if the current clip has rounded corners

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

I'm not happy about this at all. How bad it would be to create the wrapping layer even if we don't have a mask layer?
Attachment #8758122 - Flags: review?(mstange) → review+
(Assignee)

Comment 29

2 years ago
(In reply to Markus Stange [:mstange] from comment #28)
> Comment on attachment 8758122 [details] [diff] [review]
> Part 1: Add a way to check if the current clip has rounded corners
> 
> Review of attachment 8758122 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not happy about this at all. How bad it would be to create the wrapping
> layer even if we don't have a mask layer?

Not a massive disaster, but it's yet another layer every time we have perspective or preserve-3d. That adds overhead that we've seen show up in profiles many times before.
Hmm, so the only part I don't like is that it accesses the saved clip state. Can you move the check to a place where it can access the current clip state instead? Or maybe a display item's clip / scroll clip? Though I suppose you need to know the information before you create the display item in question.

Updated

2 years ago
Attachment #8758123 - Flags: review?(tlee) → review+

Updated

2 years ago
Attachment #8758125 - Flags: review?(tlee) → review+
Making sure we get to landing this as it's tracked for beta.
Flags: needinfo?(matt.woodrow)

Comment 32

2 years ago
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e492b8253d4
Part 1 - Add a way to check if the current clip has rounded corners. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/0391921abae7
Part 2 - Add a new constructor option to nsDisplayOwnLayer that doesn't force active layers. r=thinker
https://hg.mozilla.org/integration/mozilla-inbound/rev/52e03a7950cc
Part 3 - Create a wrapper layer for masking when we can't support it on the normal layer. r=thinker
https://hg.mozilla.org/integration/mozilla-inbound/rev/090e3a2d0f89
Part 4 - Add tests. r=thinker
(Assignee)

Updated

2 years ago
Flags: needinfo?(matt.woodrow)

Comment 33

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4e492b8253d4
https://hg.mozilla.org/mozilla-central/rev/0391921abae7
https://hg.mozilla.org/mozilla-central/rev/52e03a7950cc
https://hg.mozilla.org/mozilla-central/rev/090e3a2d0f89
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Hi Matt & Milan,
If we let this ride the train on 49/50 only and won't fix in 48, will this be a big impact for user for 48 because these patches seem big to me and I would like the extra bake time.
Flags: needinfo?(milan)
Flags: needinfo?(matt.woodrow)
I would like to see if in 49 at least.  I guess we can keep it out of 48, it was a regression in 45.
Flags: needinfo?(milan)
Thanks. Let's won't fix in 48. 
Hi Matt,
Please create an uplift request for 49.
status-firefox48: affected → wontfix
We're now less than a week left with 49 on aurora, it's getting tight if we're going to uplift.
(Assignee)

Comment 38

2 years ago
Comment on attachment 8758122 [details] [diff] [review]
Part 1: Add a way to check if the current clip has rounded corners

Approval Request Comment
[Feature/regressing bug #]: Bug 1168263 
[User impact if declined]: Incorrect rendering when border-radius is combined with perspective.
[Describe test coverage new/current, TreeHerder]: Manually tested, reftests added.
[Risks and why]: Medium risk, reasonable amount of changes, but should be restricted to perspective.
[String/UUID change made/needed]: None
Flags: needinfo?(matt.woodrow)
Attachment #8758122 - Flags: approval-mozilla-aurora?
Hi Matt,
Is part 1 patch the only uplift request or all the patches needs to be uplift?
Flags: needinfo?(matt.woodrow)
(Assignee)

Comment 40

2 years ago
(In reply to Gerry Chang [:gchang] from comment #39)
> Hi Matt,
> Is part 1 patch the only uplift request or all the patches needs to be
> uplift?

All the patches need to be uplifted (except the tests, but there's no harm in doing so).
Flags: needinfo?(matt.woodrow)
I guess you should create uplift requests for all 3 parts.
Flags: needinfo?(matt.woodrow)
(Assignee)

Comment 42

2 years ago
Comment on attachment 8758123 [details] [diff] [review]
Part 2: Add a new constructor option to nsDisplayOwnLayer that doesn't force active layers

See comment 38.
Flags: needinfo?(matt.woodrow)
Attachment #8758123 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 43

2 years ago
Comment on attachment 8758124 [details] [diff] [review]
Part 3: Create a wrapper layer for masking when we can't support it on the normal layer

See comment 38.
Attachment #8758124 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 44

2 years ago
Comment on attachment 8758125 [details] [diff] [review]
Part 4: Add tests

See comment 38.
Attachment #8758125 - Flags: approval-mozilla-aurora?
Comment on attachment 8758122 [details] [diff] [review]
Part 1: Add a way to check if the current clip has rounded corners

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

This patch fixes a rendering regression. Let's take it in aurora.
Attachment #8758122 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8758123 [details] [diff] [review]
Part 2: Add a new constructor option to nsDisplayOwnLayer that doesn't force active layers

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

The same as comment #45. Let's take it in aurora.
Attachment #8758123 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8758124 [details] [diff] [review]
Part 3: Create a wrapper layer for masking when we can't support it on the normal layer

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

The same as comment #45. Let's take it in aurora.
Attachment #8758124 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8758125 [details] [diff] [review]
Part 4: Add tests

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

The same as comment #45. Let's take it in aurora.
Attachment #8758125 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Duplicate of this bug: 1267690
You need to log in before you can comment on or make changes to this bug.