Closed Bug 1412249 Opened 4 years ago Closed 4 years ago

Fix DidComposite timing for EmptyTransaction

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox57 --- unaffected
firefox58 --- unaffected
firefox59 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

(Whiteboard: [wr-mvp] [gfx-noted])

Attachments

(1 file, 1 obsolete file)

When WebRenderBridgeParent::RecvEmptyTransaction() is implemented, DidComposite handling was implemented as temporal. DidComposite needs to be call at correct timing.

https://dxr.mozilla.org/mozilla-central/source/gfx/layers/wr/WebRenderBridgeParent.cpp#627
Whiteboard: [gfx-noted]
Whiteboard: [gfx-noted] → [wr-mvp] [triage] [gfx-noted]
Priority: P3 → P2
Whiteboard: [wr-mvp] [triage] [gfx-noted] → [wr-mvp] [gfx-noted]
Blocks: 1411472
Assignee: nobody → sotaro.ikeda.g
Status: NEW → ASSIGNED
Priority: P2 → P1
Attachment #8927242 - Flags: review?(nical.bugzilla)
Comment on attachment 8927242 [details] [diff] [review]
patch - Fix DidComposite timing for EmptyTransaction

Review of attachment 8927242 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/wr/WebRenderBridgeParent.cpp
@@ +663,5 @@
> +    // Send empty UpdatePipelineResources to WebRender just to notify a new epoch.
> +    // The epoch is used to know a timing of calling DidComposite().
> +    // This is much simpler than tracking an epoch of AsyncImagePipeline.
> +    wr::ResourceUpdateQueue resourceUpdates;
> +    mApi->UpdatePipelineResources(resourceUpdates, mPipelineId, wr::NewEpoch(wrEpoch));

Thankfully I have something in progress that will make this nicer (you will be able send new epochs without sending empty resource updates).

@@ +671,5 @@
> +    // If WebRenderBridgeParent does not have pending DidComposites,
> +    // send DidComposite now.
> +    if (mPendingTransactionIds.empty()) {
> +      TimeStamp now = TimeStamp::Now();
> +      mCompositorBridge->DidComposite(wr::AsUint64(mPipelineId), now, now);

Why do we need to send DidComposite here?
Comment on attachment 8927242 [details] [diff] [review]
patch - Fix DidComposite timing for EmptyTransaction

Review of attachment 8927242 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/wr/WebRenderBridgeParent.cpp
@@ +671,5 @@
> +    // If WebRenderBridgeParent does not have pending DidComposites,
> +    // send DidComposite now.
> +    if (mPendingTransactionIds.empty()) {
> +      TimeStamp now = TimeStamp::Now();
> +      mCompositorBridge->DidComposite(wr::AsUint64(mPipelineId), now, now);

In this case, we do not need to defer to send DidComposite to client side, since RecvEmptyTransaction() did not update AsyncImagePipeline and there is no pending DidComposite. If WebRenderBridgeParent has pending DidComposites, sending DidComposite needs to be deferred after the pending DidComposites.

In this case, RecvEmptyTransaction() does not trigger updating Webrender, but client side needs DidComposite.
Attachment #8927242 - Flags: review?(nical.bugzilla) → review+
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cc53112a8d5
Fix DidComposite timing for EmptyTransaction r=nical
https://hg.mozilla.org/mozilla-central/rev/4cc53112a8d5
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Depends on: 1418035
You need to log in before you can comment on or make changes to this bug.