Closed Bug 1341064 Opened 7 years ago Closed 7 years ago

Send child side built display lists over gecko ipc to compositor

Categories

(Core :: Graphics: WebRender, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: jrmuizel, Assigned: jrmuizel)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
(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.
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.
Attached patch Draft of the first stage of this (obsolete) — Splinter Review
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 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 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+
Depends on: 1341878
Attached patch Client side id generation (obsolete) — Splinter Review
Attachment #8839777 - Attachment is obsolete: true
Attachment #8840599 - Attachment is obsolete: true
Depends on: 1342246
Depends on: 1342558
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.
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+
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
Closed: 7 years ago
Resolution: --- → FIXED
Blocks: 1343770
https://hg.mozilla.org/mozilla-central/rev/6422bc2ab974
Assignee: nobody → jmuizelaar
Target Milestone: --- → mozilla54
Blocks: 1344396
You need to log in before you can comment on or make changes to this bug.