Closed Bug 1442748 Opened 2 years ago Closed 2 years ago

Don't throttle frames if we haven't sent the root pipeline DL to WR yet

Categories

(Core :: Graphics: WebRender, enhancement, P1)

Other Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: kats, Assigned: nical)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(2 files)

The root cause of bug 1442608 appears to be this scenario:
- We create the WR instance and set the root pipeline id on it. This gets set in WR on the "pending" scene. However since there is no display list for that pipeline yet, it doesn't trigger a scene build
- Then we send a GenerateFrame request. Since there hasn't been a scene build, the root pipeline id was never copied from the "pending" scene to the "current" scene. Therefore the current scene has no root pipeline id set, and we skip the render/composite steps
- Since the render/composite never happens, the C++ RenderThread code doesn't get notified that the render is done, and so it forevermore denies WRBP [1] the ability to send more GenerateFrame requests. This results in nothing ever getting rendered or composited, and so we end up with a blank screen.

This is something of a race condition because if the root pipeline's display list does get to WR before the root pipeline id is set, then step 1 in the above sequence does cause a scene build and all is well.

This situation was introduced by the async scene building patches in WR. Prior to that, there was no "pending" scene vs "current" scene distinction. So in step 1 when we set the root pipeline id, it would get set on "the" scene, and the GenerateFrame would then render it even though it had no real content to render.

In bug 1442608 I plan to land a workaround where we detect the above scenario and force a scene build when we get the GenerateFrame request. That should work around the problem, but a better long-term fix is to have the frame throttling code only take effect once we've actually sent a display list for the root pipeline. Until that point WR is not guaranteed to render anything in response to the GenerateFrame requests.

This will need to be resolved before the next WR update, because the WR update will clobber the workaround and result in the bad situation again.

[1] https://searchfox.org/mozilla-central/rev/61d400da1c692453c2dc2c1cf37b616ce13dea5b/gfx/layers/wr/WebRenderBridgeParent.cpp#1209
Assignee: nobody → nical.bugzilla
Flags: needinfo?(nical.bugzilla)
Priority: -- → P1
This is improves two things:
 - If we haven't received a display list, this WebRender bridge doesn't have content to render so generating a frame will waste CPU which is better used elsewhere when creating the widget, and avoid showing a blank frame.
 - This avoids getting the frame throttling mechanism to block subsequent frame because webrender declined to render the empty frame.
Flags: needinfo?(nical.bugzilla)
Attachment #8956444 - Flags: review?(bugmail)
The workaround isn't needed anymore.

With these two patches:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe16d5c916f3ef2004eb2abaa82f23ebfb36fe8c

With the two patches + the commit in issue 2474 that fixes leaking pipeline epochs:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab1ea690cb945343cda7d97188800904b9a7a49c

With the two patches + the two commits from issue 2474:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0a228546227fa35e6861bbe9f979e0ae76564a6
Attachment #8956445 - Flags: review?(bugmail)
Comment on attachment 8956444 [details] [diff] [review]
Don't generate frames if we haven't received a display list.

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

Looks straightforward enough.
Attachment #8956444 - Flags: review?(bugmail) → review+
Attachment #8956445 - Flags: review?(bugmail) → review+
Pushed by nsilva@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cf7f246faed
Don't generate frames if we haven't received a display list. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/31760a89ed73
Remove the workaround from 1442608. r=kats
https://hg.mozilla.org/mozilla-central/rev/4cf7f246faed
https://hg.mozilla.org/mozilla-central/rev/31760a89ed73
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.