Closed Bug 1341524 Opened 4 years ago Closed 4 years ago

Fix NotifyDidComposite() and WebRender epoch

Categories

(Core :: Graphics: WebRender, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

Attachments

(1 file, 6 obsolete files)

WebRender epoch handling and NotifyDidComposite() handling seems not correct.
 https://dxr.mozilla.org/mozilla-central/source/gfx/webrender_bindings/RenderThread.cpp#155

For WebRender epoch, TransactionId is used. But it could reset to 0 when RefreshDriver is replaced. And CompositorBridgeParent::NotifyDidComposite() is called incorrectly.
Assignee: nobody → sotaro.ikeda.g
Attachment #8840303 - Attachment is obsolete: true
Attachment #8840308 - Attachment is obsolete: true
Attachment #8840309 - Attachment is obsolete: true
Attachment #8840314 - Attachment is obsolete: true
Attachment #8840325 - Flags: review?(nical.bugzilla)
Comment on attachment 8840325 [details] [diff] [review]
patch - Fix NotifyDidComposite() and WebRender epoch

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

Looks good overall. It could be a lot simpler if we had an equivalence between TransactionId and Epoch.
r=me but i would like your inout on the questions below.

::: gfx/layers/ipc/CompositorBridgeParent.cpp
@@ +1826,5 @@
> +void
> +CompositorBridgeParent::NotifyDidCompositeToPipeline(const wr::PipelineId& aPipelineId, const wr::Epoch& aEpoch, TimeStamp& aCompositeStart, TimeStamp& aCompositeEnd)
> +{
> +  if (mPaused) {
> +    return;

Why don't we send the notification in this case?

::: gfx/layers/wr/WebRenderBridgeParent.cpp
@@ +592,5 @@
> +  return id;
> +}
> +
> +uint64_t
> +WebRenderBridgeParent::NotifyDidComposite(const wr::Epoch& aEpoch)

This does not notify anything, please rename it FlushTransactionIdsForEpoch or something along these lines.

::: gfx/layers/wr/WebRenderBridgeParent.h
@@ +147,5 @@
> +      : mEpoch(aEpoch)
> +      , mId(aId)
> +    {}
> +    wr::Epoch mEpoch;
> +    uint64_t mId;

It would be nice if the epoch and the transaction id were the same value. Do you think something is preventing us from doing that?
Attachment #8840325 - Flags: review?(nical.bugzilla) → review+
(In reply to Nicolas Silva [:nical] from comment #7)
> Comment on attachment 8840325 [details] [diff] [review]
> patch - Fix NotifyDidComposite() and WebRender epoch
> 
> Review of attachment 8840325 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good overall. It could be a lot simpler if we had an equivalence
> between TransactionId and Epoch.
> r=me but i would like your inout on the questions below.
> 
> ::: gfx/layers/ipc/CompositorBridgeParent.cpp
> @@ +1826,5 @@
> > +void
> > +CompositorBridgeParent::NotifyDidCompositeToPipeline(const wr::PipelineId& aPipelineId, const wr::Epoch& aEpoch, TimeStamp& aCompositeStart, TimeStamp& aCompositeEnd)
> > +{
> > +  if (mPaused) {
> > +    return;
> 
> Why don't we send the notification in this case?

mPaused is set when CompositorBridgeParent::PauseComposition() or CompositorBridgeParent shutdown. When PauseComposition() is called on android, client side already received pending did composite. And client side does not expect to receive the event.
  https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorBridgeParent.cpp#690

With tryserver test, NotifyDidCompositeToPipeline() was called after CompositorBridgeParent shutdown. In this case, the event could not be called.

> ::: gfx/layers/wr/WebRenderBridgeParent.cpp
> @@ +592,5 @@
> > +  return id;
> > +}
> > +
> > +uint64_t
> > +WebRenderBridgeParent::NotifyDidComposite(const wr::Epoch& aEpoch)
> 
> This does not notify anything, please rename it FlushTransactionIdsForEpoch
> or something along these lines.

I will change the name.

> ::: gfx/layers/wr/WebRenderBridgeParent.h
> @@ +147,5 @@
> > +      : mEpoch(aEpoch)
> > +      , mId(aId)
> > +    {}
> > +    wr::Epoch mEpoch;
> > +    uint64_t mId;
> 
> It would be nice if the epoch and the transaction id were the same value. Do
> you think something is preventing us from doing that?

It we use same vale, we could not identify each frame end, since transaction id could revert to 1.
Apply the comment.
Attachment #8840325 - Attachment is obsolete: true
Attachment #8840687 - Flags: review+
Rebased.
Attachment #8840687 - Attachment is obsolete: true
Attachment #8840691 - Flags: review+
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/b545b4883b89
Fix NotifyDidComposite() and WebRender epoch r=nical
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.