1383916 - black screen on windows 10 with async omtp

Status: RESOLVED FIXED

(Core :: Graphics, enhancement)

Description

[Mason Chang [Inactive] [:mchang]]

W/ Async OMTP enabled, I get lots of random black screens. Even with just a hello world. This is without bug  1381393.

Comment 1

[Mason Chang [Inactive] [:mchang]]

This is happening because of the Flush here - http://searchfox.org/mozilla-central/source/gfx/layers/client/TextureClient.cpp#547 - We have a race condition with the main thread during ContentClient::EndPaint flushing and the Paaint Thread actually painting, putting the DrawTarget in a bad state.

Also, since DrawTarget is now multi threaded, we need thread safe refcounting since both the PaintThread and Main Thread hold pointers to the DrawTarget.

Comment 2

[Mason Chang [Inactive] [:mchang]]

Attachment: Prep a DrawTarget to be drawn to on the paint thread

Implements the callback routine we talked about. This refactors out BorrowDrawTargetForPainting into two steps. 1) Get the draw target and set the transform depending on the rect to draw. 2) Clears the rects as required before we draw. This makes part 2 as part of a callback.

Once we have the TextureClients held in bug 1385101, I'll start queuing the draw targets on the paint thread. Record all the content on the main thread, then tell the paint thread to draw. For now its quasi broken in that we only hold 1 DT (which most pages have).

Comment 3

[Mason Chang [Inactive] [:mchang]]

Attachment: Prep a DrawTarget to be drawn to on the paint thread

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8891126 [details] [diff] [review]
Prep a DrawTarget to be drawn to on the paint thread

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

::: gfx/layers/PaintThread.h
@@ +64,5 @@
> +                          gfx::DrawTarget* aTarget,
> +                          gfx::Matrix aBorrowedTransform,
> +                          nsIntRegion aRegionToDraw,
> +                          SurfaceMode aSurfaceMode,
> +                          gfxContentType aContentType,

If we box up aTarget...aContentType into a separate struct, it'd make the signature of the callback and this function way cleaner. It'd also make it easier to do move semantics if we ever need them. The callback signature could move into PaintThread if that makes naming easier.

::: gfx/layers/RotatedBuffer.h
@@ +22,5 @@
>  namespace mozilla {
>  namespace layers {
>  
> +typedef bool (*PrepDrawTargetForPaintingCallback)(gfx::DrawTarget* aTarget,
> +                                                  gfx::DrawTarget* aWhiteTarget,

s/aWhiteTarget/aTargetOnWhite/

::: gfx/layers/client/ClientPaintedLayer.cpp
@@ +223,5 @@
> +  int count = 0;
> +  RefPtr<DrawTarget> heldTarget;
> +  Matrix borrowedTransform;
> +  nsIntRegion regionToDraw;
> +  RefPtr<DrawTargetCapture> captureDT;

Can we move these decls down into the loop, and rm count?

@@ +268,2 @@
>  
> +    if (gfxPrefs::LayersOMTPForceSync()) {

I don't think this will work, the state is not necessarily going to be correct outside the loop. Can we drop the async path at the end, and rm this |if| condition? The async case will have bugs but we're going to fix them (and it's already broken anyway.)

I think you can drop the heldTarget and count variables after that.

Comment 5

[Mason Chang [Inactive] [:mchang]]

Attachment: Prep a DrawTarget for the paint thread

Mostly keeps the same thing. Adds a new CapturedPaintContent with a RefPtr that holds the DT. I think we can probably also hold TextureClient here also, which might make life a lot easier. Just another round of review since it's different enough.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8892207 [details] [diff] [review]
Prep a DrawTarget for the paint thread

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

::: gfx/layers/PaintThread.cpp
@@ +171,3 @@
>    {
> +    self->PaintContentsAsync(cbc, capture,
> +                            aState,

nit: alignment

::: gfx/layers/PaintThread.h
@@ +22,5 @@
>  namespace layers {
>  
> +// Holds the key parts from a RotatedBuffer::PaintState
> +// required to draw the captured paint state
> +class  CapturedPaintState {

nit extra space here

@@ +24,5 @@
> +// Holds the key parts from a RotatedBuffer::PaintState
> +// required to draw the captured paint state
> +class  CapturedPaintState {
> +public:
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(CapturedPaintState)

I don't think this class has to be RefCounted. If you want it on the heap, maybe just try UniquePtr? I think we can use generalized lambda captures in Gecko, so you can capture it like:

[ state = Move(state) ] (...) -> { ... }

But if that doesn't pass a try run feel free to revert back to RefPtr.

::: gfx/layers/RotatedBuffer.cpp
@@ +817,5 @@
> +  }
> +
> +  ExpandDrawRegion(aPaintState, aIter, result->GetBackendType());
> +  RefPtr<CapturedPaintState> capturedPaintState =
> +    MakeAndAddRef<CapturedPaintState>(aPaintState.mRegionToDraw,

It's kind of gross to have to allocate a state object just to call this function. If we can switch the other caller to UniquePtr, it will be less weird to just put this on the stack.

::: gfx/layers/client/ClientPaintedLayer.cpp
@@ +236,5 @@
>  
> +    Matrix capturedTransform = target->GetTransform();
> +    captureDT->SetTransform(capturedTransform);
> +
> +    // TODO: Capture AA Flags and reset them in PaintThread

nit: :TODO: deserves a bug blocking the omtp metabug, cite here

@@ +254,5 @@
>  
>      ctx = nullptr;
>  
> +    // TODO: Fixup component alpha
> +    DrawTarget* targetOnWhite = nullptr;

nit: just drop the decl and pass nullptr. We should have a bug on file for fixing this, blocking the omtp metabug.

Comment 7

[Mason Chang [Inactive] [:mchang]]

Attachment: Prep a DrawTarget to be drawn to on the paint thread

Comment 8

[Mason Chang [Inactive] [:mchang]]

Try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=264555c87a01444a78cb7a37c89ddeb4a8146703

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/525c6c747a7a
Prep and flush draw targets on the paint thread with OMTP. r=dvander

Comment 10

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/6ec0e010ff06 for multiple and surprisingly inconsistent reftest bustage on Win8. For a sampling, https://treeherder.mozilla.org/logviewer.html#?job_id=120154901&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=120155570&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=120152005&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=120152006&repo=mozilla-inbound and for a more complete look https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&fromchange=525c6c747a7a0f6f45afa1277b92f4985dd89e31&filter-searchStr=windows%20x64%20reftest&filter-resultStatus=testfailed&tochange=7fe87ddf6ce421714a935ca8c267d96c6bdd7f0c

Comment 11

[Mason Chang [Inactive] [:mchang]]

Updated try with win 8 - https://treeherder.mozilla.org/#/jobs?repo=try&revision=00c928b3bade043fcbe1ff21aecdc42b29bd1df5&selectedJob=120750519

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc1d502dc1ea
Prep a DrawTarget to be drawn to on the paint thread. r=dvander

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fc1d502dc1ea