1677929 - APZScroll composition payloads are wrong with WebRender

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Markus Stange [:mstange]]

Profile: https://share.firefox.dev/3lGjOad

The APZScroll markers in this profile have end times that are likely wrong. Specifically, the second, third, fourth and fifth marker seem to end one frame too early. The markers make it look like the first and second APZScroll payload were presented by the same composite, and then there was a composite between the fifth and sixth payload that did not present any payload. But in the screenshots you can see that the composite in question did indeed have a changed scroll position.

Comment 1

[Markus Stange [:mstange]]

Edit: this comment was confused, please ignore.

Looking at the control flow in APZCTreeManager::SampleForWebRender, I'm not sure how the frame delay is supposed to work:

• We call AdvanceAnimationsInternal before we call apzc->NotifyScrollSampling().
• During AdvanceAnimationsInternal, via AdvanceAnimations and UpdateAnimation, we call SampleCompositedAsyncTransform, which moves enqueues the current payloads from mScrollPayload to mSampledState.
• Then we reach the call to apzc->NotifyScrollSampling(), which returns the payload that we just put into pops the oldest enqueued payload from mSampledState.
• Then we enqueue the payload associated with the current (pipeline, epoch) pair.
• When rendering is done, we look up the payload based on the rendered (pipeline, epoch) pairs.

Based on this reading I'm surprised that some of the payloads are correctly delayed.

Comment 2

[Markus Stange [:mstange]]

(In reply to Markus Stange [:mstange] from comment #1)

• Then we reach the call to apzc->NotifyScrollSampling(), which returns the payload that we just put into mSampledState.

Oh, never mind, I missed that mSampledState is a queue. I'll think about this some more.

Comment 3

[Markus Stange [:mstange]]

I'm now thinking this might be a race between AddPendingScrollPayload for the current frame and TakePendingScrollPayload for the previous frame. APZ sampling for the current frame can run just before NotifyPipelineRendered for the previous frame. And if the epoch doesn't increase, there's nothing stopping the payload for the next frame to be included in the current frame's reporting.

Comment 4

[Markus Stange [:mstange]]

Attachment: Bug 1677929 - Extract the 'root' parts of NotifyPipelineRendered into a new method called NotifyDidRender. r=mattwoodrow

This reduces some code duplication, and makes it clear which things we want to
do only once per window, and which things we want to do once per pipeline / CompositorBridge.

This change affects the ordering of messages, but hopefully not in a way that
anybody was relying on. Specifically, the image notifications are now sent before
the NotifyDidPaint for the root CompositorBridge is sent.

Comment 5

[Markus Stange [:mstange]]

Attachment: Bug 1677929 - Record payloads with the correct composition end timestamp. r=kats

Depends on D97533

Comment 6

[Markus Stange [:mstange]]

Attachment: Bug 1677929 - Add a way to specify an ID for a generated frame, and propagate the ID to the APZSampler. r=gw,kats

This ID allows the compositor to track per-frame information from frame
generation, through APZ sampling, to the NotifyDidRender notification.

Depends on D97534

Comment 7

[Markus Stange [:mstange]]

Attachment: Bug 1677929 - Store APZScroll composition payloads based on composition vsync ID, not based on epoch. r=kats

The epoch doesn't change during pure-APZ scrolling, so we were picking up
payloads from a future composite. As a result, we were computing incorrect
latencies, when the actual latencies were in fact one frame higher.

Depends on D97535

Comment 8

[Markus Stange [:mstange]]

Profile with patches: https://share.firefox.dev/3lJfHu4

Comment 9

[Mark]

Is this a (proposed) fix for bug 1658205? If so is there a test build I can try? cheers

Comment 10

[Botond Ballo [:botond]]

(In reply to Mark from comment #9)

Is this a (proposed) fix for bug 1658205?

I don't believe so. This bug is about a telemetry probe related to scrolling latency sometimes reporting incorrect data.

Comment 11

[Markus Stange [:mstange]]

Attachment: Bug 1677929 - Stop passing aEpochsBeingRendered to WebRender's APZ sampling callback, because it's now unused. r=gw

Depends on D97536

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7eb45ae1b6c6
Extract the 'root' parts of NotifyPipelineRendered into a new method called NotifyDidRender. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/7f229cb74fc9
Record payloads with the correct composition end timestamp. r=kats
https://hg.mozilla.org/integration/autoland/rev/ea368af2bb9e
Add a way to specify an ID for a generated frame, and propagate the ID to the APZSampler. r=gw,kats
https://hg.mozilla.org/integration/autoland/rev/b89ced58eb98
Store APZScroll composition payloads based on composition vsync ID, not based on epoch. r=kats
https://hg.mozilla.org/integration/autoland/rev/d83351f78077
Stop passing aEpochsBeingRendered to WebRender's APZ sampling callback, because it's now unused. r=gw

Comment 13

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/7eb45ae1b6c6
https://hg.mozilla.org/mozilla-central/rev/7f229cb74fc9
https://hg.mozilla.org/mozilla-central/rev/ea368af2bb9e
https://hg.mozilla.org/mozilla-central/rev/b89ced58eb98
https://hg.mozilla.org/mozilla-central/rev/d83351f78077