Closed
Bug 1341524
Opened 7 years ago
Closed 7 years ago
Fix NotifyDidComposite() and WebRender epoch
Categories
(Core :: Graphics: WebRender, defect)
Core
Graphics: WebRender
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: sotaro, Assigned: sotaro)
References
Details
Attachments
(1 file, 6 obsolete files)
14.34 KB,
patch
|
sotaro
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Updated•7 years ago
|
Assignee: nobody → sotaro.ikeda.g
Assignee | ||
Comment 1•7 years ago
|
||
Assignee | ||
Comment 2•7 years ago
|
||
Attachment #8840303 -
Attachment is obsolete: true
Assignee | ||
Comment 3•7 years ago
|
||
Attachment #8840308 -
Attachment is obsolete: true
Assignee | ||
Comment 4•7 years ago
|
||
Attachment #8840309 -
Attachment is obsolete: true
Assignee | ||
Comment 5•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7116ad894ac4a78dc3e6ed633358e08f89122b80
Assignee | ||
Comment 6•7 years ago
|
||
Attachment #8840314 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8840325 -
Flags: review?(nical.bugzilla)
Comment 7•7 years ago
|
||
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+
Assignee | ||
Comment 8•7 years ago
|
||
(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.
Assignee | ||
Comment 9•7 years ago
|
||
Apply the comment.
Attachment #8840325 -
Attachment is obsolete: true
Attachment #8840687 -
Flags: review+
Assignee | ||
Comment 10•7 years ago
|
||
Rebased.
Attachment #8840687 -
Attachment is obsolete: true
Attachment #8840691 -
Flags: review+
Assignee | ||
Comment 11•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8f7a638afffa984a3e3aff8ed7ed5fd0b59939f
Comment 12•7 years ago
|
||
Pushed by sikeda@mozilla.com: https://hg.mozilla.org/projects/graphics/rev/b545b4883b89 Fix NotifyDidComposite() and WebRender epoch r=nical
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Comment 13•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b545b4883b89
status-firefox54:
--- → fixed
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•