1405952 - Remove PWebRenderBridge::SetDisplayListSync

Status: RESOLVED FIXED

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

Description

[Sotaro Ikeda [:sotaro]]

SetDisplayListSync seems not necessary any more. It is used only when WebRenderLayerManager::mTarget exists. But WebRenderLayerManager::MakeSnapshotIfRequired() does not need it.

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Remove PWebRenderBridge::SetDisplayListSync

Comment 2

[Sotaro Ikeda [:sotaro]]

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

Comment 3

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

(In reply to Sotaro Ikeda [:sotaro] from comment #0)
> SetDisplayListSync seems not necessary any more. It is used only when
> WebRenderLayerManager::mTarget exists. But
> WebRenderLayerManager::MakeSnapshotIfRequired() does not need it.

I don't understand this. It seems like MakeSnapshotIfRequired() still draws into mTarget, so doesn't it need the display list to have been sent to webrender synchronously? How we can we guarantee that the correct stuff ends up in mTarget otherwise?

Comment 4

[Sotaro Ikeda [:sotaro]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> 
> I don't understand this. It seems like MakeSnapshotIfRequired() still draws
> into mTarget, so doesn't it need the display list to have been sent to
> webrender synchronously? How we can we guarantee that the correct stuff ends
> up in mTarget otherwise?

SetDisplayListSync seems to be used to make that the peer WebRenderBridgeParent already received the DisplayList when MakeSnapshotIfRequired() is called. But it is not necessary. Because the order of messages(between 'async SetDisplayList' and 'sync GetSnapshot') is not changed if the message are sent by same WebRenderBridgeChild instance. This IPC characteristic is used for example by Bug 1364626 to address a reftest problem. Then when WebRenderBridgeParent::RecvGetSnapshot() was called, the peer WebRenderBridgeParent pushed the correct display list. WebRenderBridgeParent::RecvGetSnapshot() takes the snap shot synchronously, then we could get expected snapshot. Therefor I could not find the necessity of SetDisplayListSync usage. If reftest want to wait transaction complete WebRenderLayerManager::WaitOnTransactionProcessed() is used for it.

It is my understanding for taking snapshot. Is there a problem that I did not recognize?

Comment 5

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

Comment on attachment 8915468 [details] [diff] [review]
patch - Remove PWebRenderBridge::SetDisplayListSync

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

That makes sense, thanks for the explanation!

Comment 6

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Remove PWebRenderBridge::SetDisplayListSync

Rebased.

Comment 7

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8916174 [details] [diff] [review]
patch - Remove PWebRenderBridge::SetDisplayListSync

:kanru, can you review the patch. The change to sync-messages.ini needs IPC peer review.

Comment 8

[Sotaro Ikeda [:sotaro]]

:kanru, review ping.

Comment 9

[Sotaro Ikeda [:sotaro]]

Thanks!

Comment 10

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Remove PWebRenderBridge::SetDisplayListSync

Rebased.

Comment 11

[Sotaro Ikeda [:sotaro]]

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbecf147fb53
Remove PWebRenderBridge::SetDisplayListSync r=kats,kanru

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/bbecf147fb53