Closed Bug 1425453 Opened 2 years ago Closed 2 years ago

Make WebRender's API transaction based

Categories

(Core :: Graphics: WebRender, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: nical, Assigned: nical)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

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.
Blocks: 1425510
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)
Attachment #8941498 - Flags: review?(bugmail)
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 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+
(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.
Sounds good, feel free to land with the Raw() as-is (after the WR update lands).
Removes most of the remaining usage of the previous API.
Attachment #8941891 - Flags: review?(bugmail)
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
You need to log in before you can comment on or make changes to this bug.