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

RESOLVED FIXED in mozilla54

Status

()

Core
Graphics: WebRender
P3
normal
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: jerry, Assigned: jerry)

Tracking

(Blocks: 1 bug)

53 Branch
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 affected)

Details

Attachments

(1 obsolete attachment)

(Assignee)

Description

a year ago
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
(Assignee)

Comment 1

a year ago
Created attachment 8814403 [details] [diff] [review]
flush and wait the last frame's data before webrender render() call
Attachment #8814403 - Flags: review?(nical.bugzilla)
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).
Attachment #8814403 - Flags: review?(nical.bugzilla) → review?(jmuizelaar)
(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).
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
(Assignee)

Comment 5

a year ago
(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.
(Assignee)

Updated

a year ago
Flags: needinfo?(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.
Flags: needinfo?(mchang)
Depends on: 1321095
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.
Jerry, you should be able to just use force_sync_composite from bug 1321417 instead of this patch. Will that work for you? Thanks!
Flags: needinfo?(hshih)
(Assignee)

Comment 9

a year ago
(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()?
Flags: needinfo?(hshih) → needinfo?(mchang)

Comment 10

a year ago
Pushed by hshih@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/2d3e477fd282
flush and wait the last frame's data before webrender render() call. r=jmuizelaar?
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
(Assignee)

Updated

a year ago
Attachment #8814403 - Attachment is obsolete: true
Attachment #8814403 - Flags: review?(jmuizelaar)
The function is already replaced by Bug 1321417.
(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.
Flags: needinfo?(mchang)

Comment 13

a year ago
Pushed by hshih@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/cd238a069da7
handle pipeline epoch in WR binding. r=mchang?
(In reply to Pulsebot from comment #10)
> Pushed by hshih@mozilla.com:
> 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.
(In reply to Pulsebot from comment #13)
> Pushed by hshih@mozilla.com:
> 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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 16

a year ago
Pushed by hshih@mozilla.com:
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?
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
Resolution: --- → FIXED

Comment 17

a year ago
Pushed by hshih@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/b5e0e2b971bd
remove the missed-epoch-checking panic in wait_for_epoch() WR bindings. r=mchang?
Please address the comments here - https://gfx.tacowolf.com/rQ225505af7cfbd322d1ae83824d116916dd19373c
Flags: needinfo?(hshih)
(Assignee)

Updated

a year ago
Depends on: 1323143
(Assignee)

Comment 19

a year ago
I will back out these synchronization patches after bug 1323143.
Flags: needinfo?(hshih)
Depends on: 1325646
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.