WebRender now exposes resource updates in a way that lets us batch them. This is important to respect transactions, otherwise two pipelines racing each other can cause a frame to be generated with a subset of the updates of a transaction.

This is a preliminary patch that starts separating WebRenderAPI and the resource updates. The idea is that ResourceUpdateQueue accumulates the resource updates that have to be applied within a given transaction and webrender ensures they are applied atomically. For now, in order to avoid churning too much code and colliding with Jerry's work, the resource updates are exposed as a member of the WebRenderAPI, but it's not entirely clear whether it should stay that way. because the bindings generator already produces a placeholder struct named ResourceUpdates that corresponds to the rust type, I named the C++ class that wraps it ResourceUpdateQueue (i'd feel better if we had maybe an ffi namespace for the rust ffi types so that we don't have to bother with finding a different name for the C++ wrapper, but we can worry deal with that later). The next step is to extend this to the child process and have it batch up the resource updates and send them as part of the transaction as well.

Comment on attachment 8900266 [details] [diff] [review] Expose webrender's ResourceUpdates in the C++ API Review of attachment 8900266 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/wr/WebRenderBridgeParent.cpp @@ +1225,5 @@ > > void > WebRenderBridgeParent::DeleteOldImages() > { > for (wr::ImageKey key : mKeysToDelete) { Since the ResourceUpdates struct already buffers key deletion until the transaction we may not need to use mKeysToDelete anymore, but I want to experiment with that separately.

Comment on attachment 8900266 [details] [diff] [review] Expose webrender's ResourceUpdates in the C++ API Review of attachment 8900266 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/webrender_bindings/WebRenderAPI.cpp @@ +252,5 @@ > void > WebRenderAPI::ClearRootDisplayList(Epoch aEpoch, > wr::WrPipelineId pipeline_id) > { > wr_api_clear_root_display_list(mDocHandle, aEpoch, pipeline_id); Is there a reason why it does not pass mResources.Raw() like wr_api_set_display_list()? ::: gfx/webrender_bindings/src/bindings.rs @@ +826,4 @@ > } > > #[no_mangle] > pub unsafe extern "C" fn wr_api_clear_root_display_list(dh: &mut DocumentHandle, You might want to remove "root" from the function name like the above wr_api_set_display_list().

(In reply to Sotaro Ikeda [:sotaro] from comment #3) > Comment on attachment 8900266 [details] [diff] [review] > Expose webrender's ResourceUpdates in the C++ API > > Review of attachment 8900266 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/webrender_bindings/WebRenderAPI.cpp > @@ +252,5 @@ > > void > > WebRenderAPI::ClearRootDisplayList(Epoch aEpoch, > > wr::WrPipelineId pipeline_id) > > { > > wr_api_clear_root_display_list(mDocHandle, aEpoch, pipeline_id); > > Is there a reason why it does not pass mResources.Raw() like > wr_api_set_display_list()? nical: can you answer the above? The patch look OK for me except it.

> Is there a reason why it does not pass mResources.Raw() like > wr_api_set_display_list()? Good point, I'll update the patch with this and the other comment.

Comment on attachment 8900266 [details] [diff] [review] Expose webrender's ResourceUpdates in the C++ API Review of attachment 8900266 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/webrender_bindings/src/bindings.rs @@ +837,5 @@ > None, > LayoutSize::new(0.0, 0.0), > frame_builder.dl_builder.finalize(), > preserve_frame_state, > + ResourceUpdates::new()); // TODO Huh, I even had a TODO about it and forgot.

There we go.

Comment on attachment 8900624 [details] [diff] [review] Expose webrender's ResourceUpdates in the C++ API (v2) Looks good!

This makes sure we don't accidentally cause havoc by trying to assign things around. I realize that I am specifying more than I need to (some ctors and operators would be already disabled by the presence of others), but since I had to look it up to be sure I felt it would be easier to everyone if I erase any possible doubt using extra explicitness.

Pushed by nsilva@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/326d1e6cf7b1 Expose WebRender's ResourceUpdates to C++. r=sotaro https://hg.mozilla.org/integration/mozilla-inbound/rev/663105088e1e Make ResourceUpdateQueue move-only. r=sotaro

Backed out for bustage at gfx/layers/opengl/MacIOSurfaceTextureHostOGL.cpp:178: no member named 'AddExternalImage' in 'mozilla::wr::WebRenderAPI' on OS X and also bustage on Windows: https://hg.mozilla.org/integration/mozilla-inbound/rev/1b82b1c3c9f95bf94817d20cb763d90a58771b65 https://hg.mozilla.org/integration/mozilla-inbound/rev/f833f336b2c9e547dd71200685bda4aa078bbcd6 Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=663105088e1eec5048b04d5a03faffb9f8b13c55&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log OS X: https://treeherder.mozilla.org/logviewer.html#?job_id=125849012&repo=mozilla-inbound > /home/worker/workspace/build/src/gfx/layers/opengl/MacIOSurfaceTextureHostOGL.cpp:178:13: error: no member named 'AddExternalImage' in 'mozilla::wr::WebRenderAPI' etc. Failure log Windows: https://treeherder.mozilla.org/logviewer.html#?job_id=125849014&repo=mozilla-inbound 10:11:41 INFO - Unified_cpp_netwerk_base0.cpp 10:11:41 INFO - z:\build\build\src\obj-firefox\dist\include\mozilla/webrender/WebRenderAPI.h(107): error C2220: warning treated as error - no 'object' file generated 10:11:41 INFO - z:\build\build\src\obj-firefox\dist\include\mozilla/webrender/WebRenderAPI.h(107): warning C4521: 'mozilla::wr::ResourceUpdateQueue': multiple copy constructors specified 10:11:41 INFO - z:\build\build\src\obj-firefox\dist\include\mozilla/webrender/WebRenderAPI.h(107): warning C4522: 'mozilla::wr::ResourceUpdateQueue': multiple assignment operators specified 10:11:41 INFO - z:/build/build/src/config/rules.mk:1064: recipe for target 'Unified_cpp_netwerk_base0.obj' failed 10:11:41 INFO - mozmake.EXE[5]: *** [Unified_cpp_netwerk_base0.obj] Error 2

By the time I am done with this bug, we'll pass the complete serialized ResourceUpdateQueue directly from the content and we won't have individual AddImage/UpdateImage/etc messages. In order to avoid leaks when a process crashes we should use webrender's clear_namespace method instead of tracking live resources manually.

This does not yet remove the ipc messages to add/update/remove images and fonts, but adds the plumbing to serialize resource updates using bincode to build it on the content side thoruh IPDL. It also removes the totally useless DPBegin message, and renames DPEnd into SetDisplayList in the ipc protocol.

Comment on attachment 8901187 [details] [diff] [review] Stop tracking active resource keys on the compositor side. Yes, we could remove them now :)

Next up is external images. It's not as straightforward as regular images and fonts because some code has to happen on the compositor so we can't just record the AddExternalImage calls on the client side and pass them directly to webrender like we do for regular images. We still do need to have them in the transaction so I am thinking of not using the ResourceUpdateQueue for that on the content side and build a list of the external image commands by talking to either WebRenderBridge child or something stored in the displaylist builder. Then that list has to be sent in he same ipc message (SetDisplayList, UpdateResource etc.) to make sure it is properly applied in the transaction (the point of all of this) and on the compositor side, do the glue that needs to be done and add merge the external image related updates with other updates in the ResourceUpdateQueue on that side before sending the transaction to WebRender. Alternatively (small variation), add the updates in the ResourceUpdate Queue on the content side and maintain a separate list of whatever information the compositor will need to do its glue (basically same thing but merge the resource updates on the child side instead of the compositor).

It seems like this introduces an additional copy of the image/font data. Am I correct in this assumption?

(In reply to Jeff Muizelaar [:jrmuizel] from comment #19) > It seems like this introduces an additional copy of the image/font data. Am > I correct in this assumption? Actually, two new copies. One on the way in and one on the way out.

It might be possible to solve the extra copy problem by using ParamTraits for ResourceUpdates that call into bincode of bincoding into a ByteBuf.

> It seems like this introduces an additional copy of the image/font data. Am I correct in this assumption? Correct. It's pretty much the same thing as with regular display lists, so I'd like that we handle both the same way (for example encode the resource updates as we build them as we do for display items, or do the bincode in ParamTraits for both if that's better). Anyway, one step at a time. There is already a lot to chew on this bug.

For convenience, the first patch was storing a ResourceUpdateQueue in the WebRenderAPI object. I wasn't very happy about it and it led to some confusing code and a bug where I was passing the wrong queue to WebRender. This patch removes the ResourceUpdateQueue member and passes the UpdateQueue directly to the various texture host methods which don't need full access to the wr API anyway. This will be folded into the first patch but I kept it separate to make reviewing a bit easier.

This patch makes it so the webrender parent commands which contain external image logic apply in the same transaction as the other commands when calling SetDisplayList.

We don't use mKeysToDelete anymore (with the the other patches applied).

Pushed by nsilva@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/73cbc76a296b Expose webrender resource updates in the C++ wrapper. r=sotaro https://hg.mozilla.org/integration/mozilla-inbound/rev/684dd3236401 Make ResourceUpdateQueue move-only. r=sotaro https://hg.mozilla.org/integration/mozilla-inbound/rev/cf18746c20ea Stop tracking active resource keys on the parent side. r=sotaro https://hg.mozilla.org/integration/mozilla-inbound/rev/44463d723486 Expose webrender transactions at the ipc boundary. r=jrmuizel, r=kanru https://hg.mozilla.org/integration/mozilla-inbound/rev/36260b6ee764 Use the ResourceUpdateQueue API on the content side. r=jrmuizel https://hg.mozilla.org/integration/mozilla-inbound/rev/959919f16fe2 Separate WebRenderAPI and ResourceUpdateQueue. r=sotaro https://hg.mozilla.org/integration/mozilla-inbound/rev/7cb5cd2dd689 Apply external image commands in webrender transactions. r=sotaro

https://hg.mozilla.org/mozilla-central/rev/73cbc76a296b https://hg.mozilla.org/mozilla-central/rev/684dd3236401 https://hg.mozilla.org/mozilla-central/rev/cf18746c20ea https://hg.mozilla.org/mozilla-central/rev/44463d723486 https://hg.mozilla.org/mozilla-central/rev/36260b6ee764 https://hg.mozilla.org/mozilla-central/rev/959919f16fe2 https://hg.mozilla.org/mozilla-central/rev/7cb5cd2dd689

This patch is awfully big I am sorry. Thankfully, it's largely bloated by a tedious addition of a parameter in a gazillion of overloads of the same methods so the overall patch isn't very complicated. I'll highlight the important parts inline. in a nutshell, this patch: - Adds IpcResourceUpdateQueue which is like ResourceUpdateQueue but that is optimized to be passed to ipdl. the later writes blobs of bytes in shared memory to avoid a handful of copies happening all over ipdl. - separates DisplaylistBuilder from (Ipc)ResourceUpdateQueue: I initially glued them together out of laziness, but I've never felt really good about it. now that the update queue we use is the ipdl one, I prefer to keep them separate like they are in webrender's API, and avoid having the c++ wrappers around webrender types depending on ipdl stuff. This is what caused the addition of the paramter that constitute half of the patch. As a followup I want to merge the infamous nsTArray<WebRenderParentMessage> which is used to send external image stuff with IpcResourceUpdateQueue (but that's the next patch because this one is too big already).

Comment on attachment 8906001 [details] [diff] [review] Add IpcResourceUpdates and use Shmem to pass resources Review of attachment 8906001 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/wr/IpcResourceUpdateQueue.cpp @@ +23,5 @@ > + Clear(); > +} > + > +layers::OffsetRange > +ShmSegmentsWriter::Write(Range<uint8_t> aBytes) This deserves a careful review. @@ +120,5 @@ > + } > +} > + > +bool > +ShmSegmentsReader::Read(layers::OffsetRange aRange, wr::Vec_u8& aInto) This deserves a careful review. ::: gfx/webrender_bindings/src/bindings.rs @@ +87,5 @@ > unsafe { Vec::from_raw_parts(self.data, self.length, self.capacity) } > } > + > + // Equivalent to `to_vec` but clears self instead of consuming the value. > + fn flush_into_vec(&mut self) -> Vec<u8> { This deserves a careful review, and the stuff below as well. @@ +676,5 @@ > pub extern "C" fn wr_resource_updates_add_image( > resources: &mut ResourceUpdates, > image_key: WrImageKey, > descriptor: &WrImageDescriptor, > + bytes: &mut WrVecU8, Note: a bunch of these functions have been changed from taking a slice to taking a Vec<u8>. this saves a copy \o/

(In reply to Nicolas Silva [:nical] from comment #28) > in a nutshell, this patch: > - Adds IpcResourceUpdateQueue which is like ResourceUpdateQueue but that is > optimized to be passed to ipdl. the later writes blobs of bytes in shared > memory to avoid a handful of copies happening all over ipdl. > - separates DisplaylistBuilder from (Ipc)ResourceUpdateQueue: I initially > glued them together out of laziness, but I've never felt really good about > it. now that the update queue we use is the ipdl one, I prefer to keep them > separate like they are in webrender's API, and avoid having the c++ wrappers > around webrender types depending on ipdl stuff. This is what caused the > addition of the paramter that constitute half of the patch. This sounds reasonable in principle. I'll try to take a closer look soon.

Rebased plus a few fixes.

This patch begins moving stuff from the WebRenderParentMessage to IpcResourceUpdateQueue. In the process I moved WebRenderLayer::UpdateImageKey to WebRenderImageLayer since it is the only user, and made it recycle the key instead of throwing it away each time something changes. Again, a big part of the patch is tedious but trivial plumbing to pass the update queue as parameter in various places.

I had to fiddle a bit with the ordering of things because some messages are still in the WebRenderParentMessage thing and must be processed before the OpAddExternalImage in the resource update queue. That's why WebRenderBridgeParent::ProcessWebRenderCommands got inlined into its only caller (RecvSetDisplayList).

By now I can type "wr::IpcResourceUpdateQueue& aResources," faster than my own name.

It's now all going through IpcResourceUpdateQueue.

This just removes the unused parameter spread all over the display list building code. This is the last patch for this bug \o/. There's still some followup cleanups that should be done but it is less pressing and can be dealt with in separate bugs.

With this patch queue we have most of the work in place to fix the frame consistency issues (and already have fixed a lot of them), but their are still the places where we use PImageBridge to pass resources that should be part of the transaction to fix before we have correct frame consistency, like Bug 1396588.

Pushed by nsilva@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/131084e007d6 Remove WebRenderBridgeParent::DeleteOldImages. r=sotaro https://hg.mozilla.org/integration/mozilla-inbound/rev/0b78bddcd706 Use shared memory to pass resource update data. r=jrmuizel

Pushed by nsilva@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/67ca40453027 Begin moving OpAddExternalImage to IpcResourceUpdateQueue and recycle image keys in WebRenderImageLayer. r=jrmuizel https://hg.mozilla.org/integration/mozilla-inbound/rev/8291145f0920 Use IpcResourceUpdateQueue in more places. r=jrmuizel https://hg.mozilla.org/integration/mozilla-inbound/rev/980729ab6e94 Remove OpAddExternalImage from WebRenderParentCommands. r=sotaro https://hg.mozilla.org/integration/mozilla-inbound/rev/f34a52244cf9 Remove unused nsTArray<WebRenderParentCommand>& parameter all over the place. r=jrmuizel

Backed out for failing e.g. reftest layout/reftests/image/image-srcset-basic-selection-2x-ref.html on QuantumRender_ https://hg.mozilla.org/integration/mozilla-inbound/rev/edddc72cb9a8b62fa6e921332c32128594fede6c https://hg.mozilla.org/integration/mozilla-inbound/rev/3428a9f92bf4bd18ee78946834a201a670a54752 https://hg.mozilla.org/integration/mozilla-inbound/rev/80142d2029b6d66a281dfd92d44bb08d28b86737 https://hg.mozilla.org/integration/mozilla-inbound/rev/e1d623b0acedbc5ebc19a7a43a09a30d8e2af702 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=f34a52244cf9f7d75b55edb2ca4bbe28ce5717ee&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=131118468&repo=mozilla-inbound > REFTEST TEST-UNEXPECTED-FAIL | file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/image/image-srcset-basic-selection-2x.html == file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/image/image-srcset-basic-selection-2x-ref.html | image comparison, max difference: 50, number of differing pixels: 40000

https://hg.mozilla.org/mozilla-central/rev/131084e007d6 https://hg.mozilla.org/mozilla-central/rev/0b78bddcd706

Pushed by nsilva@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a937c7957c5f Begin moving OpAddExternalImage to IpcResourceUpdateQueue and recycle image keys in WebRenderImageLayer. r=jrmuizel https://hg.mozilla.org/integration/mozilla-inbound/rev/65f9f54bd7bf Use IpcResourceUpdateQueue in more places. r=jrmuizel

Pushed by nsilva@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0ecd18f10203 Remove OpAddExternalImage from WebRenderParentCommands. r=sotaro https://hg.mozilla.org/integration/mozilla-inbound/rev/5c407eb7c7ef Remove unused nsTArray<WebRenderParentCommand>& parameter all over the place. r=jrmuizel

https://hg.mozilla.org/mozilla-central/rev/a937c7957c5f https://hg.mozilla.org/mozilla-central/rev/65f9f54bd7bf https://hg.mozilla.org/mozilla-central/rev/0ecd18f10203 https://hg.mozilla.org/mozilla-central/rev/5c407eb7c7ef