Closed Bug 1387639 Opened 2 years ago Closed 2 years ago

Async OMTP doesn't display the nightly search bar correctly

Categories

(Core :: Graphics, defect)

x86
Windows 10
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: mchang, Assigned: mchang)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached image Screenshot
See attached screenshot. Sometimes I can get two search bars, but mostly its the cursor is being rendered somewhere wrong. Eventually though, the page repaints correctly.
Oh even happens with sync OMTP.
The problem was that we weren't always syncing the D3D11 sync object correctly and we would have a race. Pre-OMTP, each client painted layer would finish painting into it's textures then at the end of a transaction, we'd sync via the D3D11 sync texture.

With OMTP, we could have a race where the async omtp on the paint thread hasn't finished yet so incomplete data would be in the texture, and we'd sync on the main thread, resulting in incomplete data on the compositor side, therefore showing random things.

This patch does something like having an incomplete transaction. If we have a async omtp paint, we set a flag in the ClientLayerManager. Once a transaction is complete, we check if we have to forward on the main thread or paint thread. The sync code really isn't thread safe, but it won't race since we'll either only call synchronize on the paint thread or the main thread.
Attachment #8895587 - Flags: review?(matt.woodrow)
Also, we're guaranteed that the sync will happen AFTER the async omtp paints because we append it at the end of the paint thread's event loop.
Blocks: 1387326
Comment on attachment 8895587 [details] [diff] [review]
Sync Textures once all async omtp paints are done

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

Looks good, can you also add comments about the sync code not being thread safe, but why we think we won't actually race.

Either that, or just throw a mutex into the sync object and not need to worry about it.

::: gfx/layers/PaintThread.cpp
@@ +235,5 @@
> +{
> +  MOZ_ASSERT(IsOnPaintThread());
> +  MOZ_ASSERT(aForwarder);
> +
> +  aForwarder->GetSyncObject()->Synchronize();

Can we ever have a race where the sync object goes away on the main thread, and GetSyncObject returns null here?

Why don't we grab the sync object on the main thread, and put a reference to that into the runnable, rather than the ShadowLayerForwarder?
Attachment #8895587 - Flags: review?(matt.woodrow) → review+
Carrying r+
Attachment #8895587 - Attachment is obsolete: true
Attachment #8896059 - Flags: review+
(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> Comment on attachment 8895587 [details] [diff] [review]
> Sync Textures once all async omtp paints are done
> 
> Review of attachment 8895587 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, can you also add comments about the sync code not being thread
> safe, but why we think we won't actually race.
> 
> Either that, or just throw a mutex into the sync object and not need to
> worry about it.

I threw a mutex in it. Also fixed lots of crashes!

> 
> ::: gfx/layers/PaintThread.cpp
> @@ +235,5 @@
> > +{
> > +  MOZ_ASSERT(IsOnPaintThread());
> > +  MOZ_ASSERT(aForwarder);
> > +
> > +  aForwarder->GetSyncObject()->Synchronize();
> 
> Can we ever have a race where the sync object goes away on the main thread,
> and GetSyncObject returns null here?

It shouldn't since we don't start the next transaction until all paints are done. If someone closes the process, then it doesn't matter. But I added a ref to the sync object instead. 


> Why don't we grab the sync object on the main thread, and put a reference to
> that into the runnable, rather than the ShadowLayerForwarder?

Good idea, did that in the updated version. Thanks!
Pushed by mchang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7f2be7b6b95
Sync Textures once all async OMTP paints are done. r=mattwoodrow
https://hg.mozilla.org/mozilla-central/rev/e7f2be7b6b95
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.