Closed
Bug 1425453
Opened 6 years ago
Closed 6 years ago
Make WebRender's API transaction based
Categories
(Core :: Graphics: WebRender, enhancement, P1)
Core
Graphics: WebRender
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: nical, Assigned: nical)
References
Details
Attachments
(3 files)
15.75 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
11.42 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
34.57 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
We need some way to make WebRender apply several commands atomically to guarantee frame consistency and avoid doing unnecessary work (building scenes, etc) before the end of a series of commands.
Updated•6 years ago
|
Blocks: stage-wr-trains
Priority: -- → P1
Assignee | ||
Comment 1•6 years ago
|
||
the C++ wrapper for Transaction is TransactionBuilder because the other name is already taken. This patch adds the bindings and wrappers but do not use them yet (that's the next one).
Attachment #8941497 -
Flags: review?(bugmail)
Assignee | ||
Comment 2•6 years ago
|
||
Attachment #8941498 -
Flags: review?(bugmail)
Comment 3•6 years ago
|
||
Comment on attachment 8941497 [details] [diff] [review] Update the WebRender bindings to support transactions Review of attachment 8941497 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/webrender_bindings/WebRenderAPI.h @@ +153,5 @@ > + void UpdateResources(ResourceUpdateQueue& aUpdates); > + > + bool IsEmpty() const; > + > + Transaction* Raw() { return mTxn; } I'd rather avoid exposing this raw pointer as a public function. The best alternative I can come up with is this: - Make the TransactionBuilder constructor take a wr::DocumentHandle* as an argument - Add a function on WebRenderAPI that creates a new TransactionBuilder instance using its mDocHandle pointer and returns it - Call sites would use this WebRenderAPI function to create a TransactionBuilder instead of just allocating one directly - Instead of calling mApi->SendTransaction, callers would be able to do txn.Send(), because the TransactionBuilder would be able to call wr_api_send_transaction(mDocHandle, mTxn); directly So the call site would look more like this: TransactionBuilder txn = mApi->NewTransaction(); // do stuff to txn txn.Send(); It seems a little more encapsulated to me, but I'm not sure if it makes sense to make TransactionBuilder subordinate to WebRenderAPI like this. If there are going to be scenarios where we want to build a transaction without having a WebRenderAPI or knowing which WebRenderAPI instance it will get sent to, then your approach is better. Another advantage of doing this is that you can set a mSent flag in Send() so that we don't accidentally reuse a TransactionBuilder after sending the transaction. ::: gfx/webrender_bindings/src/bindings.rs @@ +807,5 @@ > + > +/// cbindgen:postfix=WR_DESTRUCTOR_SAFE_FUNC > +#[no_mangle] > +pub extern "C" fn wr_transaction_delete(txn: *mut Transaction) { > + unsafe { let _ = Box::from_raw(txn) ; } nit: extra space before = and before ; @@ +907,5 @@ > + transform_array: *const WrTransformProperty, > + transform_count: usize, > +) { > + if transform_count == 0 && opacity_count == 0 { > + return; Maybe just assert that one of these is nonzero? It looks like the call site already does this check so doing it again here is kind of redundant.
Attachment #8941497 -
Flags: review?(bugmail) → review+
Comment 4•6 years ago
|
||
Comment on attachment 8941498 [details] [diff] [review] Use the transaction API. Review of attachment 8941498 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/wr/WebRenderBridgeParent.cpp @@ +734,5 @@ > } > > void > +WebRenderBridgeParent::ProcessWebRenderParentCommands(const InfallibleTArray<WebRenderParentCommand>& aCommands, > + wr::TransactionBuilder& aTxn) Do we need this argument here, since it's not used?
Attachment #8941498 -
Flags: review?(bugmail) → review+
Assignee | ||
Comment 5•6 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #3) > I'd rather avoid exposing this raw pointer as a public function. The best I agree that it's not pretty. We already do this in ResourceUpdateQueue, ThreadPool, DisplayListBuilder, WebRenderProgramCache, et. In fact it looks like we have a Raw method for all webrender wrappers except WebRenderAPI, so I would prefer to come up with a nicer design separately for all of them if we want to change this. > So the call site would look more like this: > > TransactionBuilder txn = mApi->NewTransaction(); A property that I like about the current way we create the transaction is that since it is only accumulating data and independent of the WebRenderAPI, it could be created/built on any thread, move between threads (as long as it is only accessed by a thread at a time), and later submitted on the API's thread. It would be nice if more of our code was structured in ways that allowed that sort of things (OMTP would have been a lot easier to achieve for example). > Another advantage of doing this is that you can set a mSent flag in Send() > so that we don't accidentally reuse a TransactionBuilder after sending the > transaction. Sending the transaction leaves the TransactionBuilder object in a valid and empty state, so it's fine to reuse it afterwards (it even saves a (completely insignificant) heap allocation by reusing the place where the rust Transaction struct is allocated)). Maybe we should just make all of these wrappers "friend class WebRenderAPI". It's not super nice either but I suppose that's what this keyword is for. > nit: extra space before = and before ; Oops, fixed. > Maybe just assert that one of these is nonzero? It looks like the call site > already does this check so doing it again here is kind of redundant. Good idea, fixed. > Do we need this argument here, since it's not used? Oops not at all, fixed.
Comment 6•6 years ago
|
||
Sounds good, feel free to land with the Raw() as-is (after the WR update lands).
Updated•6 years ago
|
See Also: → https://github.com/servo/webrender/pull/1963
Assignee | ||
Comment 7•6 years ago
|
||
Removes most of the remaining usage of the previous API.
Attachment #8941891 -
Flags: review?(bugmail)
Updated•6 years ago
|
Attachment #8941891 -
Flags: review?(bugmail) → review+
Pushed by nsilva@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/438bbffd59b0 Bindings for WebRender's transaction API. r=kats https://hg.mozilla.org/integration/mozilla-inbound/rev/c86b10bbd7b8 Move some WebRender api calls into transactions. r=kats https://hg.mozilla.org/integration/mozilla-inbound/rev/61c9a681836a Move more WebRender api calls into transactions. r=kats
Comment 9•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/438bbffd59b0 https://hg.mozilla.org/mozilla-central/rev/c86b10bbd7b8 https://hg.mozilla.org/mozilla-central/rev/61c9a681836a
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in
before you can comment on or make changes to this bug.
Description
•