Closed Bug 1381666 Opened 2 years ago Closed 2 years ago

Twitter avatars get out of sync on long threads

Categories

(Core :: Graphics, defect)

x86_64
Windows 10
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 --- fixed

People

(Reporter: kbrosnan, Assigned: dvander)

References

Details

(Keywords: regression, Whiteboard: [gfx-noted])

Attachments

(5 files, 2 obsolete files)

Attached image twitterjank.png
Steps
1. open a long twitter thread ex. https://twitter.com/bilalfarooqui/status/887025375754166272
2. scroll down over the tweet area so that many comments are loaded
3. scroll up and down over the tweet area until the avatars start scrolling with 'jank'
It appears that setting layers.mlgpu.dev-enabled false fixes the issue so bug 1375743 is at fault. 

Seems to be some interaction between the gif/<video> embedded in the tweets and the avatar desync.
Blocks: 1375743
Assignee: nobody → dvander
Whiteboard: [gfx-noted]
(In reply to Kevin Brosnan [:kbrosnan] from comment #0)
> Created attachment 8887252 [details]
> twitterjank.png
> 
> Steps
> 1. open a long twitter thread ex.
> https://twitter.com/bilalfarooqui/status/887025375754166272
> 2. scroll down over the tweet area so that many comments are loaded
> 3. scroll up and down over the tweet area until the avatars start scrolling
> with 'jank'

What build ID did you reproduce this on? Do I have to scroll with the mouse wheel or the scrollbar (or does it not matter)?
Flags: needinfo?(kbrosnan)
Scroll with a touchpad or mouse wheel while having the pointer over the body of the tweets. It does not seem to occur using the scrollbar or when the pointer is to the left or right of the main thread.
Flags: needinfo?(kbrosnan)
I can reproduce this on my laptop. Like the other Twitter it involves scrolling over video. Twitter has a very complicated layer tree here.
This patch adds some helpers to debug issues like this with RenderDoc. First, there's a pref to Sleep() before creating a D3D11 device. This lets us spawn Firefox and then inject with RenderDoc. If RenderDoc launches Firefox we can't log console spew.

Second, it's hard to correlate RenderDoc frames back to AL spew. So this patch also logs a frame number, and stores the frame number in a per-frame constant buffer. This number can be extracted by looking at WorldConstants (buffer 0) of the vertex shader of most draw calls.
Attachment #8890166 - Flags: review?(matt.woodrow)
Some more helpers I used to debug this. One function to copy MLGTextures for readback, and another to dump readback textures as PNGs.
Attachment #8890168 - Flags: review?(matt.woodrow)
Attached patch part 3, clear surfaces (obsolete) — Splinter Review
I'm not 100% sure if this is necessary, but it seems like a good idea. My concern is if for some reason we don't create an intermediate surface one frame, and then do the next, the invalid rect might not be coherent. We could maybe just set mInvalidRect = mRenderTarget->GetSize() here too, if you're worried about texture creation spam.
Attachment #8890170 - Flags: review?(matt.woodrow)
Comment on attachment 8890170 [details] [diff] [review]
part 3, clear surfaces

Actually I'll fold this into the next patch.
Attachment #8890170 - Attachment is obsolete: true
Attachment #8890170 - Flags: review?(matt.woodrow)
This is not *the* bug, but while we're here, it looks quite wrong to me. We translate the invalid rect from layer coordinates to render target coordinates based on the render target offset from the previous frame, but the invalid rect is relative to the current frame.

To fix this, mInvalidRect is stored as-is, and when we prepare to render the container we perform the translation. For the case where the invalid rect overflows, that is now stored as a separate flag.
Attachment #8890172 - Flags: review?(matt.woodrow)
Attachment #8890166 - Flags: review?(matt.woodrow) → review+
Attachment #8890168 - Flags: review?(matt.woodrow) → review+
Attachment #8890172 - Flags: review?(matt.woodrow) → review+
Attached patch part 4, fix bug (obsolete) — Splinter Review
The bug is that visible region changes of the container aren't included in invalidation. This never mattered before because CompositorD3D11 redraws the entire container every frame, whereas AL tries to be more precise.
Attachment #8890190 - Flags: review?(matt.woodrow)
This version fixes the problem local to the compositor, since there's no reason to have this change on client layers.
Attachment #8890692 - Flags: review?(matt.woodrow)
Attachment #8890190 - Attachment is obsolete: true
Attachment #8890190 - Flags: review?(matt.woodrow)
Attachment #8890692 - Flags: review?(matt.woodrow) → review+
Status: NEW → ASSIGNED
Pushed by danderson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb46fd08bcff
Add helpers for debugging Advanced Layers with RenderDoc. (bug 1381666 part 1, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/368331e8a462
Add helper functions for dumping MLGTextures as files. (bug 1381666 part 2, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/2736371868ec
Fix a coordinate space bug in the cached invalid region of container layers. (bug 1381666 part 3, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3e6fac14d49
Include visible region changes in the cached invalid region for ContainerLayers. (bug 1381666 part 4, r=mattwoodrow)
Flags: qe-verify+
I tried to reproduce this bug using old version of Nightly from 2017-07-17 on Windows 10 x64 and Windows 10 x32, both on Desktop and Laptop, but I couldn't. 
Does anybody that managed to reproduce have the time to take a look at the bug on the latest Beta (https://archive.mozilla.org/pub/firefox/candidates/56.0b12-candidates/build1/) and see if it's fixed? Kevin?
Flags: needinfo?(kbrosnan)
Removing qe-verify+ as there's nothing else we can do here.
Flags: qe-verify+
I can reproduce on the June 17th build using a current long Twitter thread. Used one of the popular tweets from https://twitter.com/NFL/

I am not able to reproduce this behavior in a current nightly.
Flags: needinfo?(kbrosnan)
You need to log in before you can comment on or make changes to this bug.