Add a capability to allocate PipelineId with IPC MozPromise

RESOLVED FIXED in Firefox 55

Status

()

Core
Graphics: WebRender
RESOLVED FIXED
7 months ago
7 months 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

7 months 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

7 months ago
Blocks: 1345054
(Assignee)

Comment 1

7 months 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

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

Comment 3

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

Comment 4

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

Updated

7 months ago
Duplicate of this bug: 1353627
(Assignee)

Comment 6

7 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=12ff8906d3419c985a24d05adbf49f217b2577f6
(Assignee)

Updated

7 months 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

7 months 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

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

Comment 12

7 months 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

7 months ago
Attachment #8859948 - Flags: review?(nical.bugzilla)
(Assignee)

Updated

7 months ago
Attachment #8860319 - Flags: review?(nical.bugzilla)
(Assignee)

Updated

7 months 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

7 months ago
Depends on: 1358697
(Assignee)

Updated

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

Updated

7 months ago
Attachment #8859948 - Attachment is obsolete: true
(Assignee)

Comment 14

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

Comment 15

7 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a64a7cc21e90ab3a4e36ab3f48d247e24c9beffb

Comment 16

7 months 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: 7 months 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

7 months ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 18

7 months ago
Backed out.

https://hg.mozilla.org/projects/graphics/rev/b74ee6d5fbcc
(Assignee)

Comment 19

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

Comment 20

7 months 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

7 months 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

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

Comment 23

7 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4ffcac8475d1c0b28d630ad1ea3ea3e37b35d33
(Assignee)

Updated

7 months ago
Attachment #8861949 - Flags: review+

Comment 24

7 months 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: 7 months ago7 months ago
Resolution: --- → FIXED
https://hg.mozilla.org/mozilla-central/rev/524eeea1b28b
status-firefox55: --- → fixed
Target Milestone: --- → mozilla55
https://hg.mozilla.org/mozilla-central/rev/b74ee6d5fbcc
https://hg.mozilla.org/mozilla-central/rev/e5082309e675
You need to log in before you can comment on or make changes to this bug.