Send child side built display lists over gecko ipc to compositor

RESOLVED FIXED in Firefox 54

Status

()

Core
Graphics: WebRender
RESOLVED FIXED
10 months ago
9 months ago

People

(Reporter: jrmuizel, Assigned: jrmuizel)

Tracking

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

10 months ago
Instead of using our current approach of having an IPC command buffer that we fill with WebRenderCommands we can build vec[u8]s directly on the client side and send these over. We can then consume these on the rust side using BuiltDisplayList::from_data and AuxiliaryLists::from_data.

The most difficult part of this looks to be the push external image stuff that we do in WebRenderBridgeParent.cpp. Hopefully it can be untangled.
(Assignee)

Comment 1

10 months ago
(In reply to Jeff Muizelaar [:jrmuizel] from comment #0) 
> The most difficult part of this looks to be the push external image stuff
> that we do in WebRenderBridgeParent.cpp. Hopefully it can be untangled.

This should be solvable by decoupling allocation of the image id from the adding the image data so that we can do them on the different side. Adding a add_image_with_key function should be sufficient to make this work.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #0)
> Instead of using our current approach of having an IPC command buffer that
> we fill with WebRenderCommands we can build vec[u8]s directly on the client
> side and send these over. We can then consume these on the rust side using
> BuiltDisplayList::from_data and AuxiliaryLists::from_data.
> 
> The most difficult part of this looks to be the push external image stuff
> that we do in WebRenderBridgeParent.cpp. Hopefully it can be untangled.

If we use BuiltDisplayList::from_data and AuxiliaryLists::from_data only, the add_image() and delete_image() calls will not be covered. The add_image() and delete_image() are handled by render_backend directly.

When WR try to resolve the external image, it should find the corresponding compositableHost with the image id. We use compositableHandle in gecko ipc for compositableClient/Host passing. The compositableHandle(it's just a uint64_t) is easy to be sent in vec[u8]. If we could handle the add/delete_image() problem, I think we could build all commands into the vec at content side.

Comment 3

10 months ago
In yesterday's wr-video meeting, we talked about external image. We allocate external image id in WebRenderLayer and passed it to parent side. It is going to be change as to generate external image id in parent side and client side uses compositableHandle instead. It is because webrender's external image handling is more close to TextureHost than CompositableHost.
(Assignee)

Comment 4

10 months ago
Created attachment 8839777 [details] [diff] [review]
Draft of the first stage of this

This moves id generation to child side and which will allow us to generate the display items on the child side as well.
Attachment #8839777 - Flags: feedback?(sotaro.ikeda.g)
Attachment #8839777 - Flags: feedback?(hshih)

Comment 5

10 months ago
Comment on attachment 8839777 [details] [diff] [review]
Draft of the first stage of this

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

Looks good to me!
Attachment #8839777 - Flags: feedback?(sotaro.ikeda.g) → feedback+

Comment 6

10 months ago
Comment on attachment 8839777 [details] [diff] [review]
Draft of the first stage of this

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

::: gfx/webrender_bindings/WebRenderAPI.cpp
@@ +334,5 @@
>  
>  void
>  DisplayListBuilder::End(WebRenderAPI& aApi, Epoch aEpoch)
>  {
> +  wr_dp_end(mWrState);

Is it accidental change?
Comment on attachment 8839777 [details] [diff] [review]
Draft of the first stage of this

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

::: gfx/webrender_bindings/src/bindings.rs
@@ +628,5 @@
>      );
>  }
>  
>  #[no_mangle]
> +pub extern fn wr_api_alloc_image(api: &mut RenderApi) -> ImageKey {

If we try to generate both image key and font key in gecko, we don't need to export this function to gecko.
Attachment #8839777 - Flags: feedback?(hshih) → feedback+
(Assignee)

Updated

10 months ago
Depends on: 1341878
(Assignee)

Comment 8

10 months ago
Created attachment 8840599 [details] [diff] [review]
Client side id generation
Attachment #8839777 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8840599 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Depends on: 1342246
(Assignee)

Updated

10 months ago
Depends on: 1342558
(Assignee)

Comment 9

10 months ago
Created attachment 8841265 [details] [diff] [review]
Generate display lists on the child side

This patch seems to work. This is the bare minimum to get things working. After this patch we should be able to cleanup the IPC interface quite a bit.
(Assignee)

Comment 10

10 months ago
Created attachment 8842580 [details] [diff] [review]
Build display lists on the child side.
Attachment #8841265 - Attachment is obsolete: true
Attachment #8842580 - Flags: review?(bugmail)
Comment on attachment 8842580 [details] [diff] [review]
Build display lists on the child side.

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

r+ with comments addressed. Are you sure that the reordering of commands (it looks like now the TOpAddExternalImage and TCompositableOperation actions will happen on the parent, and so get reordered to happen at the end) will not break anything? I assume if the try push is good it should be fine.

::: gfx/layers/wr/WebRenderBridgeChild.cpp
@@ +72,5 @@
>  }
>  
> +wr::BuiltDisplayList
> +WebRenderBridgeChild::ProcessWebrenderCommands(const gfx::IntSize &aSize,
> +                                                InfallibleTArray<WebRenderCommand>& aCommands)

nit: overhang indent

@@ +133,5 @@
> +                          op.mask().ptrOr(nullptr), op.filter(), wr::ImageKey(op.key()));
> +        break;
> +      }
> +      case WebRenderCommand::TOpAddExternalImage: {
> +        break;

Add a comment here saying this is done on the parent side

@@ +141,5 @@
> +        builder.PushIFrame(op.bounds(), op.clip(), op.pipelineId());
> +        break;
> +      }
> +      case WebRenderCommand::TCompositableOperation: {
> +        break;

Ditto

@@ +188,5 @@
>  {
>    MOZ_ASSERT(!mDestroyed);
>    MOZ_ASSERT(mIsInTransaction);
> +
> +

nit: remove one of these blank lines

::: gfx/webrender_bindings/WebRenderTypes.h
@@ +248,5 @@
> +struct VecU8 {
> +  WrVecU8 inner;
> +  VecU8() {
> +    inner.data = nullptr;
> +    inner.capacity = 0;

I'd feel happier if inner.length were initialized too. Or even better, add a default constructor to WrVecU8 that initializes itself. It would be even more better to make WrVecU8 internals private and add some helper methods that more robustly allowed putting things into and taking things out of it, so we don't accidentally leave it in an inconsistent state. All this can be done in a follow-up though.

@@ +259,5 @@
> +  }
> +
> +  ~VecU8() {
> +    if (inner.data)
> +      wr_vec_u8_free(inner);

nit: braces

@@ +278,5 @@
> +  {
> +    if (vec.inner.capacity) {
> +      mLength = vec.inner.length;
> +      mData = vec.inner.data;
> +      vec.inner.data = nullptr;

vec.inner.capacity = 0 here

::: gfx/webrender_bindings/src/bindings.rs
@@ -313,5 @@
>  }
>  
>  #[no_mangle]
>  pub extern fn wr_state_new(pipeline_id: PipelineId) -> *mut WrState {
> -    assert!(unsafe { is_in_compositor_thread() });

Instead of removing this, can we make it a is_in_main_thread() assertion instead? Ditto for the rest.
Attachment #8842580 - Flags: review?(bugmail) → review+

Comment 12

9 months ago
Pushed by jmuizelaar@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/6422bc2ab974
Send child side built display lists over gecko ipc to compositor.
Status: NEW → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → FIXED
(Assignee)

Updated

9 months ago
Blocks: 1343770
https://hg.mozilla.org/mozilla-central/rev/6422bc2ab974
Assignee: nobody → jmuizelaar
status-firefox54: --- → fixed
Target Milestone: --- → mozilla54
(Assignee)

Updated

9 months ago
Blocks: 1344396
You need to log in before you can comment on or make changes to this bug.