Closed
Bug 1390755
Opened 6 years ago
Closed 6 years ago
Painting deadlocks with Skia and async OMTP
Categories
(Core :: Graphics: Layers, enhancement, P3)
Core
Graphics: Layers
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: rhunt, Assigned: rhunt)
References
Details
(Whiteboard: [gfx-noted])
Attachments
(4 files, 1 obsolete file)
The paint thread is restricted to being one frame behind the main thread before the main thread will block on it. The main thread is currently unblocked in these cases by a runnable that happens after async painting if texture clients need to synchronize. This runnable isn't queued with Skia currently.
Assignee | ||
Comment 1•6 years ago
|
||
The logic for when textures can/should be synced is similar and different from the logic for when we need to notify OMTP to unblock the main thread. I decided to make them separate methods on the paint thread and that seems to have worked out easier than the other way around.
Assignee | ||
Comment 2•6 years ago
|
||
Attachment #8897735 -
Flags: review?(mchang)
Comment 3•6 years ago
|
||
Comment on attachment 8897735 [details] [diff] [review] omtp-end-transaction.patch Review of attachment 8897735 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/PaintThread.cpp @@ +243,5 @@ > + MOZ_ASSERT(NS_IsMainThread()); > + > + // If we are forcing synchronization, then the main thread won't > + // get ahead of the paint thread, and there is no need to unlock it > + if (gfxPrefs::LayersOMTPForceSync()) { I'd still prefer we go through all the logic even if it doesn't have to happen. Sync OMTP is really there to test everything and i'd still like to ensure EndAsyncLayerTransaction works. @@ +263,5 @@ > +{ > + MOZ_ASSERT(IsOnPaintThread()); > + MOZ_ASSERT(!gfxPrefs::LayersOMTPForceSync()); > + > + CompositorBridgeChild::Get()->NotifyFinishedAsyncPaintTransaction(); nit: Can we still assert CompositorBridgeChild::Get() doesn't return null here? ::: gfx/layers/client/ClientLayerManager.cpp @@ +742,5 @@ > } > > + // If a texture sync wasn't requested then no async painting happened, > + // and we don't need to notify OMTP that this transaction is done. > + if (PaintThread::Get() && mTextureSyncOnPaintThread) { Can we just roll up SynchronizePaintTextures and remove the lines above: e.g.: if (mForwarder->GetSyncObject() && !mTextureSyncOnPaintThread) { mForwarder->GetSyncObject()->Sync() } if (PaintThread::Get() && mTextureSyncOnPaintThread) { PaintThread::Get()->FinishedLayerTransaction(mForwrarder->GetSyncObject()) }
Updated•6 years ago
|
Flags: needinfo?(rhunt)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8897735 -
Attachment is obsolete: true
Attachment #8897735 -
Flags: review?(mchang)
Comment 8•6 years ago
|
||
mozreview-review |
Comment on attachment 8898023 [details] Bug 1390755 - Notify the paint thread that a layer transaction is completed so it can unblock the main thread. https://reviewboard.mozilla.org/r/169320/#review174674 ::: gfx/layers/client/ClientLayerManager.cpp:732 (Diff revision 1) > TimeStamp start = TimeStamp::Now(); > > // Skip the synchronization for buffer since we also skip the painting during > // device-reset status. With OMTP, we have to wait for async paints > // before we synchronize and it's done on the paint thread. > + SyncObjectClient* syncObject = nullptr; Should be a RefPtr<SyncObjetClient> ::: gfx/layers/client/ClientLayerManager.cpp:747 (Diff revision 1) > + // all async painting is finished to do a texture sync and unblock the main > + // thread if it is waiting before doing a new layer transaction. > - if (mTextureSyncOnPaintThread) { > + if (mTextureSyncOnPaintThread) { > - // We have to wait for all async paints to finish to do this > - PaintThread::Get()->SynchronizePaintTextures(mForwarder->GetSyncObject()); > + MOZ_ASSERT(PaintThread::Get()); > + PaintThread::Get()->FinishedLayerTransaction(syncObject); > - } else { > + } else { nit: else if (syncObject)
Attachment #8898023 -
Flags: review?(mchang) → review+
Comment 9•6 years ago
|
||
mozreview-review |
Comment on attachment 8898024 [details] Bug 1390755 - Ensure PaintThread::EndTransaction runs before IPDL messages are resumed. https://reviewboard.mozilla.org/r/169322/#review174676 ::: gfx/layers/PaintThread.cpp:212 (Diff revision 1) > MOZ_ASSERT(NS_IsMainThread()); > > RefPtr<CompositorBridgeChild> cbc; > if (!gfxPrefs::LayersOMTPForceSync()) { > cbc = CompositorBridgeChild::Get(); > + cbc->NotifyBeginAsyncPaintEndTransaction(); nit: This name is confusing. Maybe NotifyBeginAsyncTransactionComplete? ::: gfx/layers/ipc/CompositorBridgeChild.cpp:1236 (Diff revision 1) > > MonitorAutoLock lock(mPaintLock); > > MOZ_ASSERT(!mIsWaitingForPaint); > > - if (mOutstandingAsyncPaints > 0) { > + if (mOutstandingAsyncPaints > 0 || mOutstandingAsyncEndTransaction) { nit: Add a comment explaining we should wait for the transaction to end because we need the texture synchronization to happen in addition to no outstanding paints.
Attachment #8898024 -
Flags: review?(mchang) → review+
Comment 10•6 years ago
|
||
mozreview-review |
Comment on attachment 8898025 [details] Bug 1390755 - Rename PaintThread and CompositorBridgeChild methods to be more unified. https://reviewboard.mozilla.org/r/169324/#review174682
Attachment #8898025 -
Flags: review?(mchang) → review+
Comment 11•6 years ago
|
||
mozreview-review |
Comment on attachment 8898026 [details] Bug 1390755 - Reorder functions in PaintThread and CompositorBridgeChild. https://reviewboard.mozilla.org/r/169326/#review174684
Attachment #8898026 -
Flags: review?(mchang) → review+
Assignee | ||
Comment 12•6 years ago
|
||
(In reply to Mason Chang [:mchang] from comment #9) > Comment on attachment 8898024 [details] > Bug 1390755 - Ensure PaintThread::EndTransaction runs before IPDL messages > are resumed. > > https://reviewboard.mozilla.org/r/169322/#review174676 > > ::: gfx/layers/PaintThread.cpp:212 > (Diff revision 1) > > MOZ_ASSERT(NS_IsMainThread()); > > > > RefPtr<CompositorBridgeChild> cbc; > > if (!gfxPrefs::LayersOMTPForceSync()) { > > cbc = CompositorBridgeChild::Get(); > > + cbc->NotifyBeginAsyncPaintEndTransaction(); > > nit: This name is confusing. Maybe NotifyBeginAsyncTransactionComplete? > Yeah, I agree that it's not good. I renamed it in part 4 to NotifyBeginAsyncEndLayerTransaction, was this including that rename?
Comment 13•6 years ago
|
||
(In reply to Ryan Hunt [:rhunt] from comment #12) > (In reply to Mason Chang [:mchang] from comment #9) > > Comment on attachment 8898024 [details] > > Bug 1390755 - Ensure PaintThread::EndTransaction runs before IPDL messages > > are resumed. > > > > https://reviewboard.mozilla.org/r/169322/#review174676 > > > > ::: gfx/layers/PaintThread.cpp:212 > > (Diff revision 1) > > > MOZ_ASSERT(NS_IsMainThread()); > > > > > > RefPtr<CompositorBridgeChild> cbc; > > > if (!gfxPrefs::LayersOMTPForceSync()) { > > > cbc = CompositorBridgeChild::Get(); > > > + cbc->NotifyBeginAsyncPaintEndTransaction(); > > > > nit: This name is confusing. Maybe NotifyBeginAsyncTransactionComplete? > > > > Yeah, I agree that it's not good. I renamed it in part 4 to > NotifyBeginAsyncEndLayerTransaction, was this including that rename? Yeah sorry reviewed them in order. It's fine.
Comment 14•6 years ago
|
||
Pushed by rhunt@eqrion.net: https://hg.mozilla.org/integration/mozilla-inbound/rev/53197932a884 Notify the paint thread that a layer transaction is completed so it can unblock the main thread. r=mchang https://hg.mozilla.org/integration/mozilla-inbound/rev/7bd53f7a2116 Ensure PaintThread::EndTransaction runs before IPDL messages are resumed. r=mchang https://hg.mozilla.org/integration/mozilla-inbound/rev/5b0eb3f25b74 Rename PaintThread and CompositorBridgeChild methods to be more unified. r=mchang https://hg.mozilla.org/integration/mozilla-inbound/rev/a2b09cf8f6c5 Reorder functions in PaintThread and CompositorBridgeChild. r=mchang
Comment 15•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/53197932a884 https://hg.mozilla.org/mozilla-central/rev/7bd53f7a2116 https://hg.mozilla.org/mozilla-central/rev/5b0eb3f25b74 https://hg.mozilla.org/mozilla-central/rev/a2b09cf8f6c5
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•