threejs CSS3D panorama has graphical issues

VERIFIED FIXED in Firefox 57

Status

()

defect
P3
normal
VERIFIED FIXED
3 years ago
6 months ago

People

(Reporter: ccomorasu, Assigned: dvander)

Tracking

(Blocks 1 bug, {regression})

49 Branch
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 wontfix, firefox-esr52 wontfix, firefox53 wontfix, firefox54 wontfix, firefox55 wontfix, firefox57 verified, firefox65 verified, firefox66 verified, firefox67 verified)

Details

(Whiteboard: [gfx-noted])

Attachments

(5 attachments, 1 obsolete attachment)

[Affected versions]:
 Fx 53.0b1
 Fx 54.0a2
 Fx 55.0a1

[Affected platforms]:
 Windows 10 x64
 Ubuntu 14.04 LTS
 Mac OS X 10.11.6

[Steps to reproduce]:
 1. Launch Firefox.
 2. Go to https://threejs.org/examples/#css3d_panorama .

[Expected result]:
 The panoramic view goes smoothly.

[Actual result]:
 The panoramic view freezes often.

[Regression range]:
 I will write the regression range as soon as possible.
:CristiComo, could you get the regression range? Thanks!
Flags: needinfo?(cristian.comorasu)
Sorry for the long pause!

Last good: 20160605030215

First bad: 20160606131341


Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f75d7afd686e2af034d60e321c032b313e3ba440&tochange=58137ecd12edb7ae1a57c80da811676bc66fd03c


If you have trouble reproducing this issue make sure the browser is maximized or in full screen.

Cheers!
Has Regression Range: --- → yes
Has STR: --- → yes
Flags: needinfo?(cristian.comorasu)
Bug 1274962 looks highly likely in that range.
Blocks: 1274962
Flags: needinfo?(matt.woodrow)
Version: Trunk → 49 Branch
This definitely reproduces for me on Fx52 as well (as would be expected given the regression range).
:sinker might also know about the problem.
Flags: needinfo?(tlee)
Whiteboard: [gfx-noted]
I can't reproduce it with m-c and Linux.  Does it have been fixed?
Flags: needinfo?(tlee)
Very janky for me on Win10 still.
The problem here is at least partially caused by the invalidation in LayerManagerComposite::UpdateAndRender() not working properly. That is, the invalid area in http://searchfox.org/mozilla-central/source/gfx/layers/composite/LayerManagerComposite.cpp#481 is empty when it should not be.

One source of the problem is the usage of Union() with IntRects instead of SafeUnion() in area calculations in LayerTreeInvalidation.cpp, where integer overflow causes the width or height of the rectangle to be negative, and therefore empty.

The test page here seems to also trigger another nsRegion related assertion: "Shouldn't return empty rect: '!mTmp.IsEmpty()', file /mozilla-unified/obj-ff-dbg/dist/include/nsRegion.h, line 389", which might be related.

I think (and hope) that this bug might be related to poor performance on Windows when running this CSS 3D transforms demo: http://www.keithclark.co.uk/labs/css-fps/nojs/
Assignee: nobody → mikokm
Status: NEW → ASSIGNED
Flags: needinfo?(matt.woodrow)
Status: ASSIGNED → NEW
Assignee: mikokm → nobody
Blocks: 1309138
This also affects http://keithclark.co.uk/labs/css-fps/nojs/ which is a great benchmark for Advanced Layers. I'd like to make sure this gets fixed in the same release AL ships.
Assignee: nobody → dvander
Status: NEW → ASSIGNED
This patch changes ComputeChangeInternal's signature so that we can return false on overflow. This is just a refactoring, no functionality changes.
Attachment #8892323 - Flags: review?(matt.woodrow)
This changes LayerTreeInvalidation::ComputeDifferences to return false if overflow occurs. Compositors now handle this scenario by invaliding the entire window. Client-side callers treat this as invalidating the entire frame or treating it like no previous layer tree was available to compare.
Attachment #8892325 - Flags: review?(matt.woodrow)
Advanced Layers tracks a per-container invalid region, which the old compositor does not. This patch changes the per-container callback so that we can specify the entire container should be invalidated, if for example overflow occurred.
Attachment #8892326 - Flags: review?(matt.woodrow)
This patch adds overflow checks to LayerTreeInvalidation, by changing all Rect::Union calls to be Rect::SafeUnion. Failure is propagated up. Maybe there is a way to clamp the failure to within leaf ContainerLayers, but I don't know if it's worth it.

Initially I changed the signature of Old/NewTransformedBounds() and fixed up all their callers. However, most callers are statically operating on leaf layer types (i.e., non-ContainerLayers). So I copied the base definitions of those functions as non-virtuals to avoid refactoring all the derived layer cases.

(The MOZ_RELEASE_ASSERTS are for a try push, I'll change them to MOZ_ASSERTs before landing.)
Attachment #8892329 - Flags: review?(matt.woodrow)
This changes the NotifySubDocInvalidation callback to accept a pointer instead of a reference. If it's null, the entire layer should be invalidated.
Attachment #8892330 - Flags: review?(matt.woodrow)
Attachment #8892323 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8892325 [details] [diff] [review]
part 2, change ComputeDifferences

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

::: gfx/layers/LayerTreeInvalidation.cpp
@@ +390,5 @@
>                MOZ_CRASH("Out of bounds");
>              }
>              // Invalidate any regions of the child that have changed:
> +            nsIntRegion region;
> +            mChildren[childsOldIndex]->ComputeChange(LTI_DEEPER(aPrefix), region, aCallback);

Don't we want to handle the case where this returns false?
Attachment #8892325 - Flags: review?(matt.woodrow) → review+
(Yes, it's handled in the 3rd+4th patches - in retrospect should have handled it here.)
Attachment #8892326 - Flags: review?(matt.woodrow) → review+
Sorry, part 4 had the wrong patch attached. This is the correct one.
Attachment #8892329 - Attachment is obsolete: true
Attachment #8892329 - Flags: review?(matt.woodrow)
Attachment #8892671 - Flags: review?(matt.woodrow)
Attachment #8892671 - Flags: review?(matt.woodrow) → review+
Attachment #8892330 - Flags: review?(matt.woodrow) → review+
Pushed by danderson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d4e23b12403
Refactor LayerTreeInvalidation::ComputeChangeInternal's signature to handle overflow. (bug 1345891 part 1, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c752b3b2520
Handle bounds overflow in consumers of LayerTreeInvalidation. (bug 1345891 part 2, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d7badf38b24
Handle invalidation bounds overflow in ContainerLayerMLGPU. (bug 1345891 part 3, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8ce1cb14303
Handle 3d context bounds overflow in LayerTreeInvalidation. (bug 1345891 part 4, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/f886c403b88a
Change the subdoc invalidation callback to handle overflow. (bug 1345891 part 5, r=mattwoodrow)
I reproduced this issue using Fx 55.0a1, build ID: 20170309030216, on Windows 10 x64.
I can confirm this issue is fixed using build ID: 20170806100257, on Widows 10 x64.

However this issue is still reproducible using the same build on macOS X 10.12.5 and Ubuntu 14.04LTS.
https://hg.mozilla.org/projects/date/rev/8d4e23b1240386699839fa16cccfa67a261e94b3
Refactor LayerTreeInvalidation::ComputeChangeInternal's signature to handle overflow. (bug 1345891 part 1, r=mattwoodrow)

https://hg.mozilla.org/projects/date/rev/1c752b3b2520b0f3fe93c2c1c26a6e39d4e0219c
Handle bounds overflow in consumers of LayerTreeInvalidation. (bug 1345891 part 2, r=mattwoodrow)

https://hg.mozilla.org/projects/date/rev/5d7badf38b240fce314b8466e6d6842d30145753
Handle invalidation bounds overflow in ContainerLayerMLGPU. (bug 1345891 part 3, r=mattwoodrow)

https://hg.mozilla.org/projects/date/rev/a8ce1cb143034ea116c19bc36a9d899c74767885
Handle 3d context bounds overflow in LayerTreeInvalidation. (bug 1345891 part 4, r=mattwoodrow)

https://hg.mozilla.org/projects/date/rev/f886c403b88a78035ae0e2f7fe71c830e40bcb3f
Change the subdoc invalidation callback to handle overflow. (bug 1345891 part 5, r=mattwoodrow)
QA Whiteboard: [good first verify]
I have reproduced this bug with Nightly 55.0a1 (2017-03-09) (64-bit) on Ubuntu 16.04 LTS!

This bug's fix is verified with latest Beta!

Build   ID    20171009192146
User Agent    Mozilla/5.0 (X11; Linuxx86_64; rv:57.0) Gecko/20100101 Firefox/57.0
QA Whiteboard: [good first verify] → [good first verify] [bugday-20171011]
Depends on: 1472733

Thank you, Mohammad!

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.