1484173 - mNeedsComposite counter is based on faulty assumption; can result in spurious overcompositing on Android

Status: RESOLVED FIXED

(Core :: Graphics, defect, P2)

Description

[Kartikaya Gupta (email:[e-mail])]

Spinoff from https://bugzilla.mozilla.org/show_bug.cgi?id=1432019#c24:

The mNeedsComposite counter in CompositorVsyncScheduler tracks the number of composite requests that the scheduler has received since the last vsync. On Android, it also forces a composite immediately if that number is >= 2 [1]. I added this code back in bug 1225950, but it has a faulty assumption that in the normal course of events we'll only get one composite request (i.e. one increment of mNeedsComposite) per vsync interval.

This assumption is wrong, because APZ can have multiple scroll animations going, or might receive multiple touch events within a single vsync interval, each of which could call ScheduleComposite. In this case mNeedsComposite can increment rapidly and trigger extra composites in between vsyncs. This is unnecessary. It would be better to change mNeedsComposite to be a timestamp-based mechanism rather than a simple counter. I wrote a WIP at [2] that is what I had in mind but it needs more testing.

[1] https://searchfox.org/mozilla-central/rev/246f2b4fab2c1a6cca99418bc2e4d73d1102cc38/gfx/layers/ipc/CompositorVsyncScheduler.cpp#166-173
[2] https://hg.mozilla.org/try/rev/60bf653625f1ed8bebde7a56961538b9974e06d8

Comment 1

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=da236e90b9e29f1f9f75894a9ecb4ee685381b60

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1484173 - Expose the vsync interval via the CompositorVsyncSchedulerOwner interface. r?sotaro

We need this for the next patch.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1484173 - Replace the mNeedsComposite counter with a timestamp. r?sotaro

The mNeedsComposite counter was used to force a composite immediately if
the scheduler received a number of composite requests without actually
getting a vsync. It was necessary on Fennec because of main-thread
contention. However it was wrong because it assumes only a single
composite gets requested per vsync interval, which is not true. Instead
of using a counter this patch uses a timestamp to ensure that we only
force the vsync after two vsync intervals have elapsed.

Depends on D8765

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b032c73f6f69
Expose the vsync interval via the CompositorVsyncSchedulerOwner interface. r=sotaro
https://hg.mozilla.org/integration/autoland/rev/23ac21b643c2
Replace the mNeedsComposite counter with a timestamp. r=sotaro

Comment 5

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/b032c73f6f69
https://hg.mozilla.org/mozilla-central/rev/23ac21b643c2