1779953 - CompositionRecorder records previous frame rather than current frame

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Description

[Lee Salzman [:lsalzman]]

Both I and Timothy Nikkel noticed that CompositionRecorder results seem to be late by a frame, as if it was recording the previous frame rather then the current frame. Timothy suspected that this was because MaybeRecordFrame was called after EndFrame (and thus SwapBuffers).

On further analysis, this was indeed true, and means that the recorded frame was coming from the state of the GL back buffer immediately after a call to SwapBuffers, which is in general undefined, and at best will tend to give you an older version of the back buffer from a previously rendered frame (or a completely new buffer), depending on the GL driver's internal buffer handling.

This has the side effect of skewing test results in harnesses that depend on the frame timestamps being accurate, such as browsertime.

Comment 1

[Lee Salzman [:lsalzman]]

Attachment: Bug 1779953 - Ensure MaybeRecordFrame is called before EndFrame. r?tnikkel

If MaybeRecordFrame is called after EndFrame, this means we are reading from
the back buffer state immediately after a call to SwapBuffers. The state of
the back buffer is undefined in that scenario, and mostly was just returning
old frames. We actually want to call MaybeRecordFrame before EndFrame, so we
get the valid contents of the back buffer before it is swapped out.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1551735

Comment 3

[Lee Salzman [:lsalzman]]

Try run: https://treeherder.mozilla.org/jobs?repo=try&revision=6d7bb7c0adae44e674c22c15097056d4093eee3c
https://treeherder.mozilla.org/perfherder/compare?originalProject=try&newProject=try&newRevision=6d7bb7c0adae44e674c22c15097056d4093eee3c&framework=13&selectedTimeRange=172800&page=1

Comment 4

[Lee Salzman [:lsalzman]]

It seems that for a lot of browsertime tests, the *SpeedIndex and *VisualChange metrics were being inaccurately judged because the duration of the orange frame used to track the first visual change was wrong. The CompositionRecorder was assigning the duration of the current frame to the contents of the previous frame (you could view this inversely as it having assigned the duration of the next frame to the contents of the current frame). Now that the CompositionRecorder is actually outputting the correct screenshot contents with the correct associated timestamp, browsertime is now getting the correct duration of time associated with the content. We will see a lot of browsertime tests seemingly "regress" as a result of this fix, but these are our true results, whereas the other ones were just illusory and somewhat random.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/29943068938a
Ensure MaybeRecordFrame is called before EndFrame. r=tnikkel

Comment 6

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/29943068938a