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)
Core
Graphics: WebRender
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: jrmuizel, Assigned: jrmuizel)
References
Details
Attachments
(1 file, 3 obsolete files)
39.74 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
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•7 years 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.
Comment 2•7 years ago
|
||
(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•7 years 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•7 years ago
|
||
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•7 years 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•7 years 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 7•7 years 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/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 | ||
Comment 8•7 years ago
|
||
Attachment #8839777 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8840599 -
Attachment is obsolete: true
Assignee | ||
Comment 9•7 years ago
|
||
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•7 years ago
|
||
Attachment #8841265 -
Attachment is obsolete: true
Attachment #8842580 -
Flags: review?(bugmail)
Comment 11•7 years ago
|
||
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•7 years 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
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•