Closed Bug 1358014 Opened 2 years ago Closed 2 years ago

Add a capability to allocate PipelineId with IPC MozPromise

Categories

(Core :: Graphics: WebRender, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

Attachments

(1 file, 6 obsolete files)

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.
Blocks: 1345054
(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
Attachment #8859938 - Attachment is obsolete: true
Attachment #8859942 - Attachment is obsolete: true
Duplicate of this bug: 1353627
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
(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)
Comment on attachment 8860319 [details] [diff] [review]
patch - Allocate PipelineId with IPC MozPromise

How about this approach?
Attachment #8860319 - Flags: review?(nical.bugzilla)
Attachment #8859948 - Flags: review?(nical.bugzilla)
Attachment #8860319 - Flags: review?(nical.bugzilla)
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+
Depends on: 1358697
Summary: Split LayersId and wr::PipelineId → Add a capability to allocate PipelineId with IPC MozPromise
Attachment #8859948 - Attachment is obsolete: true
Attachment #8860319 - Attachment is obsolete: true
Attachment #8861515 - Flags: review+
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
Closed: 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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
One problem is that attachment 8861515 [details] [diff] [review] extends the Layers lifetime more than LayerManager.
Attachment #8861515 - Attachment is obsolete: true
Attachment #8861911 - Flags: review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=87dee13a7951c7708ee92089580ae5ed87f27100

Dangling pointer problem seemed to be addressed. But ipc problem still exists:(
Attachment #8861911 - Attachment is obsolete: true
Attachment #8861949 - Flags: review+
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/e5082309e675
Allocate PipelineId with IPC MozPromise r=nical
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.