1320320 - flush and wait the last frame's data before the webrender render() call

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P3)

Description

[Jerry Shih[:jerry] (UTC+8) (inactive)]

If WebRenderBridgeParent receives several RecvDPEnd() calls during the next scheduled CompositeToTarget(), we can't make sure WR renders the last transaction's display_list data. WR will just render the frame that it was processed. Please reference the webrender_traits::RenderNotifier trait[1].

So, it's hard to find a place to release a gecko image buffer(or a gl_texture). Currently, we delete the images which are in previous transaction at WebRenderBridgeParent::ProcessWebrenderCommands(). But that images will still be used in the future if the WR render_backend haven't done the frame construction for that transaction.

I'm trying to add the flush and wait call before we call renderer.render() until we have a good solution. Then, the render() call will just render the last transaction. And we will only clean up the unused resources in our codebase[2].

[1]
https://github.com/servo/webrender/blob/655cbfc5a5de1d7bdce52205449779f912b4df6d/webrender_traits/src/types.rs#L425
[2]
https://hg.mozilla.org/projects/graphics/file/tip/gfx/layers/wr/WebRenderBridgeParent.cpp#l171

Comment 1

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: flush and wait the last frame's data before webrender render() call

Comment 2

[Nicolas Silva [:nical]]

Comment on attachment 8814403 [details] [diff] [review]
flush and wait the last frame's data before webrender render() call

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

I haven't looked closely at the bindings yet so deferring to Jeff (although this looks like a short term hack).

Comment 3

[Nicolas Silva [:nical]]

(In reply to Nicolas Silva [:nical] from comment #2)
> (although this looks like a short term hack).

Nevermind that, I don't even understand how that part currently works in the integration so I can't really comment on the patch.
I feel like we should specify the epoch of the transaction when we need to block until it is done to avoid races (not necessarily a comment about the patch, mostly thinking out loud).

Comment 4

[Mason Chang [Inactive] [:mchang]]

Is there a specific reason you need this to be a sync call? The better thing to do would be specify epochs and clean things up when webrender has finished rendering that epoch. The flush API is there just for reftests, but I plan to get rid of it in bug 1319170

Comment 5

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Mason Chang [:mchang] from comment #4)
> Is there a specific reason you need this to be a sync call? The better thing
> to do would be specify epochs and clean things up when webrender has
> finished rendering that epoch. The flush API is there just for reftests, but
> I plan to get rid of it in bug 1319170

When we call render(), we need to wait the specific epoch frame done which comes form render_backend. Otherwise, the WR might still render the previous content we sent before. It's not what we want in layer-transaction.
Maybe we don't need to call the flush call, but we should wait the epoch somewhere.

Comment 6

[Mason Chang [Inactive] [:mchang]]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #5)
> (In reply to Mason Chang [:mchang] from comment #4)
> > Is there a specific reason you need this to be a sync call? The better thing
> > to do would be specify epochs and clean things up when webrender has
> > finished rendering that epoch. The flush API is there just for reftests, but
> > I plan to get rid of it in bug 1319170
> 
> When we call render(), we need to wait the specific epoch frame done which
> comes form render_backend. Otherwise, the WR might still render the previous
> content we sent before. It's not what we want in layer-transaction.
> Maybe we don't need to call the flush call, but we should wait the epoch
> somewhere.

Yeah, I think we're talking about the same thing. Let's try not to do anything sync and instead wait for the epoch. If you have other things, I Was hoping to get to that this week.

Comment 7

[Mason Chang [Inactive] [:mchang]]

Spoke to Jerry on IRC. If this is blocking, you can land this now and I'll rewrite the bindings part to use the Epoch. I'm currently doing that now for reftests.

Comment 8

[Mason Chang [Inactive] [:mchang]]

Jerry, you should be able to just use force_sync_composite from bug 1321417 instead of this patch. Will that work for you? Thanks!

Comment 9

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Mason Chang [:mchang] from comment #8)
> Jerry, you should be able to just use force_sync_composite from bug 1321417
> instead of this patch. Will that work for you? Thanks!

It still has a little bit difference.
In force_sync_composite() call, it will call "wr_composite_window()" continuously until the frame's epoch matches our last epoch. Then, the wr_composite_window() calls with previous frame's data still use the old resource we set before. Could we skip the wr_composite_window() in force_sync_composite()?

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/2d3e477fd282
flush and wait the last frame's data before webrender render() call. r=jmuizelaar?

Comment 11

[Sotaro Ikeda [:sotaro]]

The function is already replaced by Bug 1321417.

Comment 12

[Mason Chang [Inactive] [:mchang]]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #9)
> (In reply to Mason Chang [:mchang] from comment #8)
> > Jerry, you should be able to just use force_sync_composite from bug 1321417
> > instead of this patch. Will that work for you? Thanks!
> 
> It still has a little bit difference.
> In force_sync_composite() call, it will call "wr_composite_window()"
> continuously until the frame's epoch matches our last epoch. Then, the
> wr_composite_window() calls with previous frame's data still use the old
> resource we set before. Could we skip the wr_composite_window() in
> force_sync_composite()?

Not really. I think this was actually a bug with the flush api as well. The render backend thread would get the updated contents, but until we render the scene on the main thread, it isn't actually on the screen. That means even with the flush api, we would still composite old frame data, but we didn't know it since we didn't have epochs. The flush API only ensured that the render backend thread had processed the frame, not the actual renderer yet.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/cd238a069da7
handle pipeline epoch in WR binding. r=mchang?

Comment 14

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

(In reply to Pulsebot from comment #10)
> Pushed by [e-mail]:
> https://hg.mozilla.org/projects/graphics/rev/2d3e477fd282
> flush and wait the last frame's data before webrender render() call.
> r=jmuizelaar?

This patch was backed out in https://hg.mozilla.org/projects/graphics/rev/95098568797bbcd89a19e17f4d444acab7aec4d8 (before the patch in comment 13 landed). Please make sure you include the bug number in backout patches.

Comment 15

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

(In reply to Pulsebot from comment #13)
> Pushed by [e-mail]:
> https://hg.mozilla.org/projects/graphics/rev/cd238a069da7
> handle pipeline epoch in WR binding. r=mchang?

... and this was backed out in https://hg.mozilla.org/projects/graphics/rev/954a21ff3d449fddeb02cd5c9758ef89dc34d4ac again without a bug number.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/225505af7cfb
handle pipeline epoch in WR binding. r=mchang?
https://hg.mozilla.org/projects/graphics/rev/11ab9642165e
wait epochs ready before WR render() call. r=mchang?

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/b5e0e2b971bd
remove the missed-epoch-checking panic in wait_for_epoch() WR bindings. r=mchang?

Comment 18

[Mason Chang [Inactive] [:mchang]]

Please address the comments here - https://gfx.tacowolf.com/rQ225505af7cfbd322d1ae83824d116916dd19373c

Comment 19

[Jerry Shih[:jerry] (UTC+8) (inactive)]

I will back out these synchronization patches after bug 1323143.