1345891 - threejs CSS3D panorama has graphical issues

Status: VERIFIED FIXED

(Core :: Graphics, defect, P3)

Description

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

[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.

Comment 1

[Sotaro Ikeda [:sotaro]]

:CristiComo, could you get the regression range? Thanks!

Comment 2

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

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!

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Bug 1274962 looks highly likely in that range.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

This definitely reproduces for me on Fx52 as well (as would be expected given the regression range).

Comment 5

[Sotaro Ikeda [:sotaro]]

:sinker might also know about the problem.

Comment 6

[Thinker Li [:sinker]]

I can't reproduce it with m-c and Linux.  Does it have been fixed?

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Very janky for me on Win10 still.

Comment 8

[Miko Mynttinen]

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/

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

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.

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, new ComputeChangeInternal signature

This patch changes ComputeChangeInternal's signature so that we can return false on overflow. This is just a refactoring, no functionality changes.

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, change ComputeDifferences

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.

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 3, fix advanced layers

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.

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 4, handle overflow in LTI

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.)

Comment 14

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 5, fix PresShell API

This changes the NotifySubDocInvalidation callback to accept a pointer instead of a reference. If it's null, the entire layer should be invalidated.

Comment 15

[Matt Woodrow (:mattwoodrow)]

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?

Comment 16

[David Anderson [:dvander] - inactive, e-mail if emergency]

(Yes, it's handled in the 3rd+4th patches - in retrospect should have handled it here.)

Comment 17

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 4, handle overflow in LTI

Sorry, part 4 had the wrong patch attached. This is the correct one.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
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)

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/8d4e23b12403
https://hg.mozilla.org/mozilla-central/rev/1c752b3b2520
https://hg.mozilla.org/mozilla-central/rev/5d7badf38b24
https://hg.mozilla.org/mozilla-central/rev/a8ce1cb14303
https://hg.mozilla.org/mozilla-central/rev/f886c403b88a

Comment 20

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

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.

Comment 21

[Aki Sasaki (not active)]

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)

Comment 22

[Mohammad Maruf Rahman]

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

Comment 23

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

Thank you, Mohammad!