1412078 - Rocket chat [version 0.48.1] is not working properly

Status: VERIFIED FIXED

(Core :: Graphics: Layers, defect)

Description

[cladech_le_vrai]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20171025230440

Steps to reproduce:

I've been here : https://chat.lescommuns.org/channel/communecter_accueil

and it's not working properly


Actual results:

When you scroll, it takes a lot of time to display.


Expected results:

To display directly.

Comment 1

[cladech_le_vrai]

It's probably because of the version of rocket chat 0.48.1 . But it's working on chrome for example.

Comment 2

[Cristina Badescu [:cbadescu], Experiments QA]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20171030103605

Hello,

I have tested this issue on latest Nightly build and managed to reproduce it. I have accessed the provided link and scrolled the channel: the web painting isn't correctly rendered and also it takes a long time to display while scrolling.

[Regression]
Given the fact that this issue is not reproducible on latest Firefox Release 56.0.2, I've ran the Mozregression tool and managed to find a regression window.
Here are the results:
Last good revision: 8dba09333346ba9f445c6648622596a96a841e8d
First bad revision: 5bf42ef768d80ebeba9f0a634a38446c33f1c125
Pushlog: https://goo.gl/YNu5Va

From the pushlog, it seems that the bug 1408781 has caused this. David, can you please take a look?

This issue is not reproducible on Mac 10.12, nor on Ubuntu 14.04 x64, using latest Nightly build 58.0a1 (2017-10-30).

Comment 3

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

Okay, I can reproduce this. I don't think it's a regression from bug 1408781. It looks like it's been broken on Advanced Layers for a while, but bug 1408781 made it much more obvious. The site is not very smooth on the old D3D11 compositor but Advanced Layers definitely is clipping the invalid region incorrectly.

Comment 4

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

Not related to bug 1408781 after all. Also not an invalidation bug since it happens with invalidation disabled.

What's happening is that there are two painted layers, A and B, with A sandwiched in between two visible strips of B. This means B has two rects as its visible region and A's visible region is between them:


    B B B B B B B
    A A A A A A A
    B B B B B B B     

A and B both are inside a container with FrameMetrics, which causes PaintedLayer::MayResample to return true, and that collapses B's visible region to one giant rect. Advanced Layers then adds that whole rect to the occlusion set, which makes A no longer visible.

There's two easy ways to fix this, one is to consider may-resample layers as never opaque. Another fix is to track the opaque and visible regions separately.

Comment 5

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

Attachment: patch

Comment 6

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8925427 [details] [diff] [review]
patch

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

::: gfx/layers/mlgpu/LayerMLGPU.h
@@ +83,5 @@
>    }
>  
> +  // If the layer is opaque, this returns the subset of the visible region
> +  // that can be considered opaque.
> +  virtual const LayerIntRegion& GetOpaqueRegion() {

Somewhat confusing that this is only valid to call if the layer is opaque.

Maybe return an empty region if !IsContentOpaque()? Or assert that we don't call this if it's false?

::: gfx/layers/mlgpu/PaintedLayerMLGPU.cpp
@@ +61,5 @@
>    // Note that when AL performs CPU-based occlusion culling (the default
>    // behavior), we might break up the visible region again. If that turns
>    // out to be a problem, we can factor this into ForEachDrawRect instead.
>    if (MayResample()) {
> +    mOpaqueRegion = Move(mRenderRegion);

Maybe just always write to mOpaqueRegion (before this branch)? That way GetOpaqueRegion can be a trivial getter, which makes this a bit simpler.

Comment 7

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

Attachment: v2

Matt suggested not having two MayResample() calls since it's confusing. Here, PaintedLayerMLGPU always stores the correct visible region. RenderPassMLGPU then calls a separate method to extract the draw rects, and that's where we handle MayResample().

(I didn't make GetDrawRects virtual and replace all the callers of GetRenderRegion - but that's quite doable.)

Comment 8

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8925607 [details] [diff] [review]
v2

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

::: gfx/layers/mlgpu/PaintedLayerMLGPU.cpp
@@ +65,5 @@
>    // Note that when AL performs CPU-based occlusion culling (the default
>    // behavior), we might break up the visible region again. If that turns
>    // out to be a problem, we can factor this into ForEachDrawRect instead.
>    if (MayResample()) {
> +    mDrawRects = mRenderRegion.GetBounds();

Do we need to store this? The bounds should already be cached as part of nsRegion, so I think we can just return mRenderRegion.GetBounds() directly here.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/02dd65e55fd1
Don't treat resampled layers as entirely opaque. (bug 1412078, r=mattwoodrow)

Comment 10

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

Comment on attachment 8925607 [details] [diff] [review]
v2

Approval Request Comment
[Feature/Bug causing the regression]: Advanced Layers
[User impact if declined]: broken rendering on some complex pages, like RocketChat
[Is this code covered by automated tests?]:
[Has the fix been verified in Nightly?]: 
[Needs manual test from QE? If yes, steps to reproduce]: STR in comment #0
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: The bug is that Advanced Layers considers too many pixels to be opaque, and thus fails to render pixels that should be visible. This just restricts how many are considered opaque, which worst case is a very small deoptimization but not a cause for further correctness problems.
[String changes made/needed]:

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/02dd65e55fd1

Comment 12

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

Comment on attachment 8925607 [details] [diff] [review]
v2

I got a second opinion from Milan on this one and he feels this is a low risk, potential high reward. Taking it based on his recommendation, Beta57+

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cdbba9c49a70 (FIREFOX_57b_RELBRANCH)
https://hg.mozilla.org/releases/mozilla-release/rev/56086a67fdd5

Comment 14

[Petruta Horea [:phorea], Desktop QA]

Reproduced on 2017-10-31 Nightly.
Verified as fixed using Firefox 57.0 build 3 and latest Nightly 58.0a1 2017-10-09 under Win 10 64-bit and Win 7 64-bit.