1358014 - Add a capability to allocate PipelineId with IPC MozPromise

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement)

Description

[Sotaro Ikeda [:sotaro]]

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.

Comment 1

[Sotaro Ikeda [:sotaro]]

(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.

Comment 2

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Split LayersId and wr::PipelineId

Comment 3

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Split LayersId and wr::PipelineId

Comment 4

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Split LayersId and wr::PipelineId

Comment 6

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=12ff8906d3419c985a24d05adbf49f217b2577f6

Comment 7

[Nicolas Silva [:nical]]

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 8

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

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).

Comment 9

[Sotaro Ikeda [:sotaro]]

(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?

Comment 10

[Nicolas Silva [:nical]]

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.

Comment 11

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Allocate PipelineId with IPC MozPromise

Comment 12

[Sotaro Ikeda [:sotaro]]

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

How about this approach?

Comment 13

[Nicolas Silva [:nical]]

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.

Comment 14

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Allocate PipelineId with IPC MozPromise

Comment 15

[Sotaro Ikeda [:sotaro]]

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/524eeea1b28b
Add a capability to allocate PipelineId with IPC MozPromise r=nical

Comment 17

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

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

Comment 18

[Sotaro Ikeda [:sotaro]]

Backed out.

https://hg.mozilla.org/projects/graphics/rev/b74ee6d5fbcc

Comment 19

[Sotaro Ikeda [:sotaro]]

One problem is that attachment 8861515 [details] [diff] [review] extends the Layers lifetime more than LayerManager.

Comment 20

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Allocate PipelineId with IPC MozPromise

Comment 21

[Sotaro Ikeda [:sotaro]]

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

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

Comment 22

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Allocate PipelineId with IPC MozPromise

Comment 23

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4ffcac8475d1c0b28d630ad1ea3ea3e37b35d33

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/e5082309e675
Allocate PipelineId with IPC MozPromise r=nical

Comment 25

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

https://hg.mozilla.org/mozilla-central/rev/524eeea1b28b

Comment 26

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

https://hg.mozilla.org/mozilla-central/rev/b74ee6d5fbcc
https://hg.mozilla.org/mozilla-central/rev/e5082309e675