Add a capability to allocate PipelineId with IPC MozPromise

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: sotaro, Assigned: sotaro)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

2 years ago
WebRenderLayerManager uses LayersId as wr::PipelineId. It is not good to add support of video pipeline(Bug 1345054), since WebRenderImageLayer needs to allocate wr::PipelineId for video.
(Assignee)

Updated

2 years ago
Blocks: 1345054
(Assignee)

Comment 1

2 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #0)
> WebRenderLayerManager uses LayersId as wr::PipelineId. It is not good to add
> support of video pipeline(Bug 1345054), since WebRenderImageLayer needs to
> allocate wr::PipelineId for video.

LayersId is allocated  by GPUProcessManager.
Assignee: nobody → sotaro.ikeda.g
(Assignee)

Comment 2

2 years ago
Created attachment 8859938 [details] [diff] [review]
patch - Split LayersId and wr::PipelineId
(Assignee)

Comment 3

2 years ago
Created attachment 8859942 [details] [diff] [review]
patch - Split LayersId and wr::PipelineId
Attachment #8859938 - Attachment is obsolete: true
(Assignee)

Comment 4

2 years ago
Created attachment 8859948 [details] [diff] [review]
patch - Split LayersId and wr::PipelineId
Attachment #8859942 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1353627
(Assignee)

Updated

2 years ago
Attachment #8859948 - Flags: review?(nical.bugzilla)
Comment on attachment 8859948 [details] [diff] [review]
patch - Split LayersId and wr::PipelineId

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

Are you certain we can't use keep layersId and PipelineId the same (by making we can allocate layersId for the video elements)? It looks to me like it should possible but I haven't looked hard enough so I may be missing something.
Comment on attachment 8859948 [details] [diff] [review]
patch - Split LayersId and wr::PipelineId

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

You should also be able to remove the AsUint64 function at http://searchfox.org/mozilla-central/rev/d8ac097a1de2544f0e51d186bc850662c5b7430a/gfx/webrender_bindings/WebRenderTypes.h#108 with this patch. That will help ensure that nobody accidentally starts using the raw u64 value for anything. You can also get rid of the AsPipelineId function and just inline it into the one spot that it's still used (in CompositorBridgeChild::GetNextPipelineId).
Blocks: 1331944
(Assignee)

Comment 9

2 years ago
(In reply to Nicolas Silva [:nical] (away until April 18th) from comment #7)
> 
> Are you certain we can't use keep layersId and PipelineId the same (by
> making we can allocate layersId for the video elements)? It looks to me like
> it should possible but I haven't looked hard enough so I may be missing
> something.

I am going to looking into another option.

nical, could you explain your idea?
Flags: needinfo?(nical.bugzilla)
layersId and PipelineId represent the same thing: sub-trees of the overall content tree (layers wr items) that is put together in the compositor. So I was wondering if we could avoid having both of them at the same time since I think it complicates things. Historically we haven't used layersId for video elements because we haven't considered them as sub-trees, but it should be possible to do so by allocating a layersId for them. From far away it looks like it should not be too hard since the layersid allocation is just a monotonically incremented counter in the content process.
That way we'd avoid carrying around pairs of layersId+pipelineId that represent the same thing all over the code.
Flags: needinfo?(nical.bugzilla)
(Assignee)

Comment 11

2 years ago
Created attachment 8860319 [details] [diff] [review]
patch - Allocate PipelineId with IPC MozPromise
(Assignee)

Comment 12

2 years ago
Comment on attachment 8860319 [details] [diff] [review]
patch - Allocate PipelineId with IPC MozPromise

How about this approach?
Attachment #8860319 - Flags: review?(nical.bugzilla)
(Assignee)

Updated

2 years ago
Attachment #8859948 - Flags: review?(nical.bugzilla)
(Assignee)

Updated

2 years ago
Attachment #8860319 - Flags: review?(nical.bugzilla)
(Assignee)

Updated

2 years ago
Attachment #8860319 - Flags: review?(nical.bugzilla)
Comment on attachment 8860319 [details] [diff] [review]
patch - Allocate PipelineId with IPC MozPromise

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

I think I would prefer the layersId to be constructed on the child side but that's beyond the scope of what you are working on, this looks good to me.
Attachment #8860319 - Flags: review?(nical.bugzilla) → review+
(Assignee)

Updated

2 years ago
Depends on: 1358697
(Assignee)

Updated

2 years ago
Summary: Split LayersId and wr::PipelineId → Add a capability to allocate PipelineId with IPC MozPromise
(Assignee)

Updated

2 years ago
Attachment #8859948 - Attachment is obsolete: true
(Assignee)

Comment 14

2 years ago
Created attachment 8861515 [details] [diff] [review]
patch - Allocate PipelineId with IPC MozPromise
Attachment #8860319 - Attachment is obsolete: true
Attachment #8861515 - Flags: review+

Comment 16

2 years ago
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/524eeea1b28b
Add a capability to allocate PipelineId with IPC MozPromise r=nical
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Note that the graphics push above has an orange Linux64-qr tc-M-e10s(mda2) job where the failure looks related to the patch:

###!!! [Child][DispatchAsyncMessage] Error: PBrowser::Reply_AllocPipelineId Route error: message sent to unknown actor ID

Full log at https://treeherder.mozilla.org/logviewer.html#?job_id=94135807&repo=graphics&lineNumber=2467
(Assignee)

Updated

2 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 19

2 years ago
One problem is that attachment 8861515 [details] [diff] [review] extends the Layers lifetime more than LayerManager.
(Assignee)

Comment 20

2 years ago
Created attachment 8861911 [details] [diff] [review]
patch - Allocate PipelineId with IPC MozPromise
Attachment #8861515 - Attachment is obsolete: true
Attachment #8861911 - Flags: review+
(Assignee)

Comment 21

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=87dee13a7951c7708ee92089580ae5ed87f27100

Dangling pointer problem seemed to be addressed. But ipc problem still exists:(
(Assignee)

Comment 22

2 years ago
Created attachment 8861949 [details] [diff] [review]
patch - Allocate PipelineId with IPC MozPromise
Attachment #8861911 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8861949 - Flags: review+

Comment 24

2 years ago
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/e5082309e675
Allocate PipelineId with IPC MozPromise r=nical
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
https://hg.mozilla.org/mozilla-central/rev/524eeea1b28b
status-firefox55: --- → fixed
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.