Closed Bug 1399692 Opened 7 years ago Closed 7 years ago

Copy the front buffer to the back buffer on the paint thread with OMTP enabled

Categories

(Core :: Graphics, defect, P1)

Product:
Core
Component:
Graphics
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox57 --- unaffected
firefox58 --- fixed

People

(Reporter: mchang, Assigned: rhunt)

References

(Depends on 1 open bug)

Details

(Whiteboard: [gfx-noted])

Attachments

(12 files, 4 obsolete files)

copy-front-to-back.patch
21.03 KB, patch
Details | Diff | Splinter Review
fix black-issues
7.87 KB, patch
Details | Diff | Splinter Review
Remove unneeded lambda capture in paint thread (bug 1399692 part 1, )
59 bytes, text/x-review-board-request
bas.schouten
: review+
Details
Simplify the code for creating a new back buffer (bug 1399692 part 2, )
59 bytes, text/x-review-board-request
bas.schouten
: review+
Details
Remove BufferContentType and add ValidBufferSize (bug 1399692 part 3, )
59 bytes, text/x-review-board-request
bas.schouten
: review+
Details
Don't create back buffer for front buffer until we know what type to create. (bug 1399692 part 4, )
59 bytes, text/x-review-board-request
bas.schouten
: review+
Details
Simplify copying the front buffer to the back buffer (bug 1399692 part 5, )
59 bytes, text/x-review-board-request
bas.schouten
: review+
Details
Record buffer operations to a struct for replaying on paint thread (bug 1399692 part 6, )
59 bytes, text/x-review-board-request
bas.schouten
: review+
Details
Replay buffer commands on paint thread when OMTP is enabled (bug 1399692 part 7, )
59 bytes, text/x-review-board-request
bas.schouten
: review+
Details
Don't copy over regions that will be painted in this frame (bug 1399692 part 8, )
59 bytes, text/x-review-board-request
bas.schouten
: review+
Details
Don't reuse a back buffer after a swap if the content or surface changed (bug 1399692 part 9, )
59 bytes, text/x-review-board-request
bas.schouten
: review+
Details
Don't do PaintThebes after PaintOffMainThread. (bug 1399692 part 10, )
59 bytes, text/x-review-board-request
dvander
: review+
Details
Attached patch Copy Buffer on paint thread (obsolete) — — Splinter Review 
      No description provided.
Attachment #8907894 - Flags: review?(bas)
status-firefox57: --- → unaffected
Priority: -- → P1
Whiteboard: [gfx-noted]
Fixed build issues on warnings as errors.
Attachment #8907894 - Attachment is obsolete: true
Attachment #8907894 - Flags: review?(bas)
Attachment #8908237 - Flags: review?(bas)
Comment on attachment 8908237 [details] [diff] [review]
Copy the front buffer to the back bufer on the paint thread

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

::: gfx/layers/client/ClientPaintedLayer.cpp
@@ +217,5 @@
>    if (!UpdatePaintRegion(state)) {
>      return false;
>    }
>  
> +  MOZ_ASSERT(mContentClient->IsRemoteBuffer());

I think we should move this bit as we just discussed.
Attachment #8908237 - Flags: review?(bas) → review+
Pushed by mchang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/46f0b004bdd2
Copy the front buffer to the back buffer on the paint thread with OMTP enabled. r=bas
https://hg.mozilla.org/mozilla-central/rev/46f0b004bdd2
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Depends on: 1400299
Comment on attachment 8908353 [details] [diff] [review]
Copy the front buffer to the back bufer on the paint thread

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

::: gfx/layers/PaintThread.cpp
@@ +159,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(aContentClient);
> +
> +  RefPtr<ContentClientRemoteBuffer> cc(aContentClient);

Hrm, this actually is a problem, the last reference to ContentClient's needs to be released on the main thread. I suspect this issue is responsible for:

https://treeherder.mozilla.org/logviewer.html#?job_id=132152065&repo=try&lineNumber=4513
Seems backout might be the best option right now. Someone else will need to take this bug as I won't have access to a windows machine soon.
Attachment #8910338 - Flags: review?(bas)
Attachment #8910338 - Flags: review?(bas) → review+
I can take this after it's backed out.
I just formatted my machines and don't have access to inbound anymore. Can you please land this?
Assignee: mchang → rhunt
Flags: needinfo?(rhunt)
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/545dfc344b36
Backed out changeset 46f0b004bdd2 for not cleaning up content client on the right thread. r=bas
Flags: needinfo?(rhunt)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Here's a try run with an updated patch releasing the content clients on the main thread.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6c52006eaf913a1f3af0d70940d67035a9fa1c6

The ipc assertion is gone, but there are some reftest no-paint failures. I'm investigating them.
So after investigating the reftest no-paint failures, it's been determined that the original patch only accidentally worked.

I'm working on implementing a new approach where we don't send the ContentClient's to the paint thread for a Finalize, but instead sync the region to update with each PaintThread::PaintContents (i.e. a RotatedContentBuffer quadrant), and do the copy front to back buffer there.

This get's around the problem of ContentClient being mutated on the main thread and paint thread. It does have the disadvantage of having to reacquire the front buffer texture lock on each PaintThread::AsyncPaintContents. I'd just like to get this working first and then focus on improving performance.
Here's my current WIP.

It changes CapturedPaintState to include information needed to copy the front buffer to the back buffer (if applicable). Then on the paint thread later we do the operation. Care is taken to make sure we get a CapturedPaintState for quadrants that only need a buffer copy and nothing painted.

This patch mostly works, but I'm running into an issue with large sections of black appearing on pages. Not sure what it is yet. I see it for example on https://www.outlook.com.
Attachment #8908353 - Attachment is obsolete: true
Attachment #8910338 - Attachment is obsolete: true
Attached patch fix black-issues — — Splinter Review 
This patch fixes the issues you were seeing Ryan (it applies on top of yours). Basically old front clients could be discarded and no front client would be present by the time we borrowed the target. Meaning CopyFrontBufferToBackBuffer would never be called.

I also added some comments for issues we may still need to address. Do note we should perhaps also further limit the area being updated to the update region in DrawBufferWithRotation, but it may not matter much as we're already restricting it more or less to the bounds.

With these adjustments it seems to mostly work, there seem to be some trickier to reproduce issues left, let's see if those show up in reftests. I think they have to do with rotation, the one time I saw them the wrong part of the layer was showing in the update region area.

Let me know if there's anything else I can do!
Since we're seeing some dependent perf bugs, could you update the bug with your status Ryan? I know you've been making good progress here!
Flags: needinfo?(rhunt)
Latest try run with some fixes:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e05fbca26e51ac8af8e56979d90af4e8d6d9b5b6
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f549b02ff658658ec5830bf13a265fe871a4828e

They're not finished at the time of writing this, but I know there will be some failures carried over from my previous try runs (which I'm looking into at this moment).
Flags: needinfo?(rhunt)
Okay, I fixed one more issue and now I can't reproduce any reftest failures from the new or old try runs. Gonna clean up the patch a bit and push an expanded new try run.
There is still one issue that I haven't seen in the try runs but experience every once in a while when browsing:

Assertion failure: mIsLocked, at /home/rhunt/src/firefox/gfx/layers/client/TextureClient.cpp:677

This happens in PaintThread::CopyFrontBufferToBackBuffer *after* we have successfully locked the front buffer texture client and are unlocking it at the end of the method.

My best guess is that this is a race condition between ContentClient::EndPaint and the paint thread. [1] It seems like this could happen if the copy front buffer code is still executing when ContentClient::EndPaint is run.

I'm not sure of a fix yet, because I don't know the motivation for that line of code.

Bas, why do we need to postpone the deleting of the front buffer(s) in this manner?

[1] http://searchfox.org/mozilla-central/rev/e62604a97286f49993eb29c493672c17c7398da9/gfx/layers/client/ContentClient.cpp#288
Flags: needinfo?(bas)
(In reply to Ryan Hunt [:rhunt] from comment #21)
> There is still one issue that I haven't seen in the try runs but experience
> every once in a while when browsing:
> 
> Assertion failure: mIsLocked, at
> /home/rhunt/src/firefox/gfx/layers/client/TextureClient.cpp:677
> 
> This happens in PaintThread::CopyFrontBufferToBackBuffer *after* we have
> successfully locked the front buffer texture client and are unlocking it at
> the end of the method.
> 
> My best guess is that this is a race condition between
> ContentClient::EndPaint and the paint thread. [1] It seems like this could
> happen if the copy front buffer code is still executing when
> ContentClient::EndPaint is run.
> 
> I'm not sure of a fix yet, because I don't know the motivation for that line
> of code.
> 
> Bas, why do we need to postpone the deleting of the front buffer(s) in this
> manner?
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> e62604a97286f49993eb29c493672c17c7398da9/gfx/layers/client/ContentClient.
> cpp#288

Hrm, how texture client locking works with OMTP I'm not sure, I noticed some interesting code for this which I don't fully understand yet in the OMTP specific bits. Maybe David knows better, but while we're accessing the texture clients they can't be locked, of course. I also can't tell you the motivation of postponing the deletion, but presumably to keep them alive for readback? That's a complete guess though.
Flags: needinfo?(bas)
There's an explanation on mOldTextures in the comment for the ContentClient class in the header. I can't pretend to know this code well enough to understand straight away what it means, but I think we may want to change things so that we lock a little more conservatively.
So after more reading of RotatedContentBuffer, I realized that the current approach wasn't going to work in two different cases:
1. When we allocate a new back buffer and have to copy the old back buffer to the new back buffer [1]
2. When we do an in place buffer unrotate, which happens when the compositor might resample the layer or the compositor doesn't support rotation. [2]

To handle this I've rewrote the patches. Now back buffer copying is done with a 'buffer copy' operation that can be done on the main or the paint thread. This involves some refactoring of the RotatedBuffer.

This doesn't cover #2 though, and I only realized that after getting #1 working. Ideally we could add a 'buffer unrotate' operation and do the same, _but_ the buffer unrotating can fail when we can't LockBits.

Bas, what do you think should be done here? Is it possible to make a Moz2D buffer unrotate implementation that would always work? i.e. not using LockBits.

Reftest run of the current implementation is green except for a few tests that need buffer unrotating. [3]

[1] http://searchfox.org/mozilla-central/rev/b53e29293c9e9a2905f4849f4e3c415e2013f0cb/gfx/layers/RotatedBuffer.cpp#708
[2] http://searchfox.org/mozilla-central/rev/b53e29293c9e9a2905f4849f4e3c415e2013f0cb/gfx/layers/RotatedBuffer.cpp#626
[3] https://treeherder.mozilla.org/#/jobs?repo=try&revision=9188247438d8effbe9f8379d76b696127f0ec0fe
Flags: needinfo?(bas)
(In reply to Ryan Hunt [:rhunt] from comment #24)
> So after more reading of RotatedContentBuffer, I realized that the current
> approach wasn't going to work in two different cases:
> 1. When we allocate a new back buffer and have to copy the old back buffer
> to the new back buffer [1]

I'm sort of confused why that didn't work in the old approach? But if you have something else that's good I'm not complaining! :)

> 2. When we do an in place buffer unrotate, which happens when the compositor
> might resample the layer or the compositor doesn't support rotation. [2]
> 
> To handle this I've rewrote the patches. Now back buffer copying is done
> with a 'buffer copy' operation that can be done on the main or the paint
> thread. This involves some refactoring of the RotatedBuffer.
> 
> This doesn't cover #2 though, and I only realized that after getting #1
> working. Ideally we could add a 'buffer unrotate' operation and do the same,
> _but_ the buffer unrotating can fail when we can't LockBits.
> 
> Bas, what do you think should be done here? Is it possible to make a Moz2D
> buffer unrotate implementation that would always work? i.e. not using
> LockBits.

Well, it's sort of possible, but since selfcopy's aren't supported on the GPU (and this is where lockbits fails for D2D surfaces), we can't really do it fast, that doesn't necessarily matter though. Note that where lockbits fails, even in the current code we just give up and create a new buffer (presumably this is already the path we use when using D2D), couldn't the same approach be used here?
Flags: needinfo?(bas)
(In reply to Bas Schouten (:bas.schouten) from comment #25)
> (In reply to Ryan Hunt [:rhunt] from comment #24)
> > So after more reading of RotatedContentBuffer, I realized that the current
> > approach wasn't going to work in two different cases:
> > 1. When we allocate a new back buffer and have to copy the old back buffer
> > to the new back buffer [1]
> 
> I'm sort of confused why that didn't work in the old approach? But if you
> have something else that's good I'm not complaining! :)
>

The problem in this case is that we need to do two buffer copies. One from the front buffer to the back buffer. And a second from the old back buffer to the new back buffer. This didn't fit as well with doing the buffer copy right before each quadrant paint, which I still had a reftest failure with.

> > 2. When we do an in place buffer unrotate, which happens when the compositor
> > might resample the layer or the compositor doesn't support rotation. [2]
> > 
> > To handle this I've rewrote the patches. Now back buffer copying is done
> > with a 'buffer copy' operation that can be done on the main or the paint
> > thread. This involves some refactoring of the RotatedBuffer.
> > 
> > This doesn't cover #2 though, and I only realized that after getting #1
> > working. Ideally we could add a 'buffer unrotate' operation and do the same,
> > _but_ the buffer unrotating can fail when we can't LockBits.
> > 
> > Bas, what do you think should be done here? Is it possible to make a Moz2D
> > buffer unrotate implementation that would always work? i.e. not using
> > LockBits.
> 
> Well, it's sort of possible, but since selfcopy's aren't supported on the
> GPU (and this is where lockbits fails for D2D surfaces), we can't really do
> it fast, that doesn't necessarily matter though. Note that where lockbits
> fails, even in the current code we just give up and create a new buffer
> (presumably this is already the path we use when using D2D), couldn't the
> same approach be used here?

Yes, I actually did a try run with this and it was green so I think that is viable.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f0cf3941e0dc73a02ab8f20de181c72b9faaa11
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c9fd4693e79fc587f3cd99385256d32390f480e

Updated patches after the new content client code. The first few patches are some refactoring and then an optimization and then the actual change.
Comment on attachment 8923936 [details]
Record buffer operations to a struct for replaying on paint thread (bug 1399692 part 6, )

https://reviewboard.mozilla.org/r/195108/#review200160


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: gfx/layers/RotatedBuffer.cpp:327
(Diff revision 1)
>      WrapRotationAxis(&newRotation.y, mBufferRect.Height());
>      NS_ASSERTION(gfx::IntRect(gfx::IntPoint(0,0), mBufferRect.Size()).Contains(newRotation),
>                   "newRotation out of bounds");
>  
> -    int32_t xBoundary = aDestBufferRect.XMost() - newRotation.x;
> -    int32_t yBoundary = aDestBufferRect.YMost() - newRotation.y;
> +    return Parameters{aDestBufferRect, newRotation};
> +  } else {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]
Comment on attachment 8923931 [details]
Remove unneeded lambda capture in paint thread (bug 1399692 part 1, )

https://reviewboard.mozilla.org/r/195098/#review200210
Attachment #8923931 - Flags: review?(bas) → review+
Comment on attachment 8923932 [details]
Simplify the code for creating a new back buffer (bug 1399692 part 2, )

https://reviewboard.mozilla.org/r/195100/#review200402

::: gfx/layers/client/ContentClient.cpp:212
(Diff revision 1)
>                          << dest.mBufferRect.x << ", "
>                          << dest.mBufferRect.y << ", "
>                          << dest.mBufferRect.Width() << ", "
>                          << dest.mBufferRect.Height();
>        }
> +      Clear();

As far as I can tell this is correct, but I do believe this is a functional change. :-)
Attachment #8923932 - Flags: review?(bas) → review+
Comment on attachment 8923933 [details]
Remove BufferContentType and add ValidBufferSize (bug 1399692 part 3, )

https://reviewboard.mozilla.org/r/195102/#review200406
Attachment #8923933 - Flags: review?(bas) → review+
Comment on attachment 8923934 [details]
Don't create back buffer for front buffer until we know what type to create. (bug 1399692 part 4, )

https://reviewboard.mozilla.org/r/195104/#review200408

Good work!

::: gfx/layers/client/ContentClient.cpp:217
(Diff revision 1)
>      }
>  
> -    // If we have an existing back buffer, copy it into the new back buffer
> -    if (mBuffer) {
> -      newBuffer->UpdateDestinationFrom(*mBuffer, newBuffer->BufferRect());
> +    // If we have an existing front buffer, copy it into the new back buffer
> +    if (RefPtr<RotatedBuffer> frontBuffer = GetFrontBuffer()) {
> +      bool locked = false;
> +      if (!frontBuffer->IsLocked()) {

In the IsLocked() case, how do we know -we're- the ones holding the lock?

::: gfx/layers/client/ContentClient.cpp:234
(Diff revision 1)
> +      }
> +    }
>  
> -      // We are done with the old back buffer now and it is about to be
> +    // We are done with the old back buffer now and it is about to be
> -      // destroyed, so unlock it
> +    // destroyed, so unlock it if necessary
> +    if (mBuffer && mBuffer->IsLocked()) {

This again seems a little fragile, I'd rather we track whether we locked it through a separate bool.

::: gfx/layers/client/ContentClient.cpp:447
(Diff revision 1)
>      // have transitioned into/out of component alpha, then we need to recreate it.
> +    RefPtr<RotatedBuffer> frontBuffer = GetFrontBuffer();
>      if (canKeepBufferContents &&
> -        mBuffer &&
> -        (contentType != mBuffer->GetContentType() ||
> -        (mode == SurfaceMode::SURFACE_COMPONENT_ALPHA) != mBuffer->HaveBufferOnWhite()))
> +        frontBuffer &&
> +        (contentType != frontBuffer->GetContentType() ||
> +        (mode == SurfaceMode::SURFACE_COMPONENT_ALPHA) != frontBuffer->HaveBufferOnWhite()))

nit: Not your fault, but while you're in here, could you rewrite this clause to be more readible? Like... (mode == SurfaceMode::SURFACE_COMPONENT_ALPHA) != frontBuffer->HaveBufferOnWhite() .. really?

bool shouldHaveBufferOnWhite = mode == SurfaceMode::SURFACE_COMPONENT_ALPHA;

(shouldHaveBufferOnWhite == != frontBuffer->HaveBufferOnWhite())

is much more readible in my opinion :). Also, is this a real thing? where the frontBuffer says its content type is a certain type, but then is lying and incorrectly has/doesn't have a BufferOnWhite? We should fix that.
Attachment #8923934 - Flags: review?(bas) → review+
Comment on attachment 8923935 [details]
Simplify copying the front buffer to the back buffer (bug 1399692 part 5, )

https://reviewboard.mozilla.org/r/195106/#review200414

::: gfx/layers/client/ContentClient.cpp:869
(Diff revision 1)
>  // back buffer (i.e. as they were for the old back buffer)
>  void
>  ContentClientDoubleBuffered::FinalizeFrame(const nsIntRegion& aRegionToDraw)
>  {
>    if (!mFrontAndBackBufferDiffer) {
> -    MOZ_ASSERT(!mFrontBuffer->DidSelfCopy(), "If we have to copy the world, then our buffers are different, right?");
> +    MOZ_ASSERT(!mFrontBuffer || !mFrontBuffer->DidSelfCopy(), "If we have to copy the world, then our buffers are different, right?");

nit: Update assert message. :)
Attachment #8923935 - Flags: review?(bas) → review+
Comment on attachment 8923936 [details]
Record buffer operations to a struct for replaying on paint thread (bug 1399692 part 6, )

https://reviewboard.mozilla.org/r/195108/#review200418

::: gfx/layers/PaintThread.h:65
(Diff revision 1)
>  };
>  
> +// Holds the key operations for a ContentClient to prepare
> +// its buffers for painting
> +class CapturedBufferState {
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(CapturedBufferState)

At least in this patch it seems UniquePtr is more appropriate here than refcounting, I don't think that will change in subsequent patches :).

::: gfx/layers/PaintThread.h:67
(Diff revision 1)
> +// Holds the key operations for a ContentClient to prepare
> +// its buffers for painting
> +class CapturedBufferState {
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(CapturedBufferState)
> +public:
> +  CapturedBufferState()

nit: Aren't you just redefining the default ctor here?

::: gfx/layers/PaintThread.h:113
(Diff revision 1)
> +
> +  Maybe<Copy> mBufferCopy;
> +  Maybe<Unrotate> mBufferUnrotate;
> +
> +protected:
> +  virtual ~CapturedBufferState() {}

nit: Mark class as final and remove virtual dtor.

::: gfx/layers/PaintThread.cpp:27
(Diff revision 1)
>  
> +bool
> +CapturedBufferState::Copy::CopyBuffer()
> +{
> +  bool locked = false;
> +  if (!mSource->IsLocked()) {

This still confuses me a little.. do we understand why sometimes it's locked and sometimes it isn't?

::: gfx/layers/RotatedBuffer.cpp:327
(Diff revision 1)
>      WrapRotationAxis(&newRotation.y, mBufferRect.Height());
>      NS_ASSERTION(gfx::IntRect(gfx::IntPoint(0,0), mBufferRect.Size()).Contains(newRotation),
>                   "newRotation out of bounds");
>  
> -    int32_t xBoundary = aDestBufferRect.XMost() - newRotation.x;
> -    int32_t yBoundary = aDestBufferRect.YMost() - newRotation.y;
> +    return Parameters{aDestBufferRect, newRotation};
> +  } else {

Clangbot's right :-)
Attachment #8923936 - Flags: review?(bas) → review+
Comment on attachment 8923937 [details]
Replay buffer commands on paint thread when OMTP is enabled (bug 1399692 part 7, )

https://reviewboard.mozilla.org/r/195110/#review200430

::: gfx/layers/PaintThread.cpp:56
(Diff revision 1)
>    return (!mBufferCopy || mBufferCopy->CopyBuffer()) &&
>           (!mBufferUnrotate || mBufferUnrotate->UnrotateBuffer());
>  }
>  
> +void
> +CapturedBufferState::GetTextureClients(nsTArray<RefPtr<TextureClient>>* aTextureClients)

I'd personally prefer using std::vector over nsTArray, but I'll leave that up to you. Also, pass it byRef since you're not allowing for it to be nullptr.

::: gfx/layers/PaintThread.cpp:208
(Diff revision 1)
>  {
>    return sThreadId == PlatformThread::CurrentId();
>  }
>  
>  void
> +PaintThread::PrepareBuffer(CapturedBufferState* aState)

Once you change this to UniquePtr, make this take an r-value reference, i.e. UniquePtr<CapturedBufferState>&& aState

::: gfx/layers/client/ClientPaintedLayer.cpp:216
(Diff revision 1)
>  
>    PaintState state = mContentClient->BeginPaint(this, flags | ContentClient::PAINT_ASYNC);
> +  bool didUpdate = false;
> +
> +  if (state.mBufferState) {
> +    PaintThread::Get()->PrepareBuffer(state.mBufferState);

Once you've changed to uniqueptr obviously this should std::move(state.mBufferState)

::: gfx/layers/d3d11/TextureD3D11.cpp
(Diff revision 1)
>  
>  
>  already_AddRefed<DrawTarget>
>  D3D11TextureData::BorrowDrawTarget()
>  {
> -  MOZ_ASSERT(NS_IsMainThread());

Please replace with Or PaintThread, this assert has caught people doing nasty things before.

::: gfx/layers/d3d11/TextureD3D11.cpp:769
(Diff revision 1)
>  {
> -  MOZ_ASSERT(NS_IsMainThread());
> -
>    if (!mDrawTarget && mTexture) {
>      // This may return a null DrawTarget
>      mDrawTarget = Factory::CreateDrawTargetForD3D11Texture(mTexture, mFormat);

Are we certain this DT creation is threadsafe? I suspect so, but we should probably mark it explicitly as such, and keep an eye out for bugs.
Attachment #8923937 - Flags: review?(bas) → review+
Comment on attachment 8923932 [details]
Simplify the code for creating a new back buffer (bug 1399692 part 2, )

https://reviewboard.mozilla.org/r/195100/#review200518
Comment on attachment 8923934 [details]
Don't create back buffer for front buffer until we know what type to create. (bug 1399692 part 4, )

https://reviewboard.mozilla.org/r/195104/#review200506

::: gfx/layers/client/ContentClient.cpp:217
(Diff revision 1)
>      }
>  
> -    // If we have an existing back buffer, copy it into the new back buffer
> -    if (mBuffer) {
> -      newBuffer->UpdateDestinationFrom(*mBuffer, newBuffer->BufferRect());
> +    // If we have an existing front buffer, copy it into the new back buffer
> +    if (RefPtr<RotatedBuffer> frontBuffer = GetFrontBuffer()) {
> +      bool locked = false;
> +      if (!frontBuffer->IsLocked()) {

Yeah that's a confusing piece of code, I should have commented on it.

The reason behind this, is that GetFrontBuffer() is mBuffer when we are not double buffered, and mBuffer may have been locked earlier in BeginPaint as we were trying to reuse it and failed to lock or adjust the buffer.

There shouldn't be any other case where GetFrontBuffer() is locked.

Thinking about all the cases, I believe this can be simplified by always unlocking mBuffer if we determine we need to create a new buffer and locked it.

::: gfx/layers/client/ContentClient.cpp:447
(Diff revision 1)
>      // have transitioned into/out of component alpha, then we need to recreate it.
> +    RefPtr<RotatedBuffer> frontBuffer = GetFrontBuffer();
>      if (canKeepBufferContents &&
> -        mBuffer &&
> -        (contentType != mBuffer->GetContentType() ||
> -        (mode == SurfaceMode::SURFACE_COMPONENT_ALPHA) != mBuffer->HaveBufferOnWhite()))
> +        frontBuffer &&
> +        (contentType != frontBuffer->GetContentType() ||
> +        (mode == SurfaceMode::SURFACE_COMPONENT_ALPHA) != frontBuffer->HaveBufferOnWhite()))

That's confusing, I'll add in a fix for it.

It is a real thing though, (mode == SURFACE_COMPONENT_ALPHA) is whether this frame needs to be component alpha, and frontBuffer->HaveBufferOnWhite() is whether the previous frame was component alpha.

If this condition gets messed up it leads to fun things like bug 1412150.
Comment on attachment 8923935 [details]
Simplify copying the front buffer to the back buffer (bug 1399692 part 5, )

https://reviewboard.mozilla.org/r/195106/#review200520
Comment on attachment 8923936 [details]
Record buffer operations to a struct for replaying on paint thread (bug 1399692 part 6, )

https://reviewboard.mozilla.org/r/195108/#review200522
Comment on attachment 8923936 [details]
Record buffer operations to a struct for replaying on paint thread (bug 1399692 part 6, )

https://reviewboard.mozilla.org/r/195108/#review200524
Comment on attachment 8923937 [details]
Replay buffer commands on paint thread when OMTP is enabled (bug 1399692 part 7, )

https://reviewboard.mozilla.org/r/195110/#review200526
Comment on attachment 8923937 [details]
Replay buffer commands on paint thread when OMTP is enabled (bug 1399692 part 7, )

https://reviewboard.mozilla.org/r/195110/#review200430

> I'd personally prefer using std::vector over nsTArray, but I'll leave that up to you. Also, pass it byRef since you're not allowing for it to be nullptr.

I'm not well versed on the difference between the two, but at the least here it's required by CompositorBridgeChild. I will change it to be by ref though.

> Once you change this to UniquePtr, make this take an r-value reference, i.e. UniquePtr<CapturedBufferState>&& aState

Will change.

> Please replace with Or PaintThread, this assert has caught people doing nasty things before.

Will change.

> Are we certain this DT creation is threadsafe? I suspect so, but we should probably mark it explicitly as such, and keep an eye out for bugs.

How should this be explicitly marked as threadsafe?
Comment on attachment 8923936 [details]
Record buffer operations to a struct for replaying on paint thread (bug 1399692 part 6, )

https://reviewboard.mozilla.org/r/195108/#review200418

> At least in this patch it seems UniquePtr is more appropriate here than refcounting, I don't think that will change in subsequent patches :).

Yes it is, I'll make that change.

> nit: Aren't you just redefining the default ctor here?

Yes, will remove that.

> nit: Mark class as final and remove virtual dtor.

Yes, I'll change that.

> Clangbot's right :-)

Will change that, it originally came from porting the code straightforward.
Comment on attachment 8923935 [details]
Simplify copying the front buffer to the back buffer (bug 1399692 part 5, )

https://reviewboard.mozilla.org/r/195106/#review200414

> nit: Update assert message. :)

Will do.
Comment on attachment 8923932 [details]
Simplify the code for creating a new back buffer (bug 1399692 part 2, )

https://reviewboard.mozilla.org/r/195100/#review200402

> As far as I can tell this is correct, but I do believe this is a functional change. :-)

Ah, you're right. I believe I remove that in a later patch anyways, so I'll either not add it in this patch or remove the message about no functional changes.
Wow I really messed up publishing the mozreview replies. Sorry about that..
Comment on attachment 8923936 [details]
Record buffer operations to a struct for replaying on paint thread (bug 1399692 part 6, )

https://reviewboard.mozilla.org/r/195108/#review200418

> Yes it is, I'll make that change.

So this is complicated by the fact that we need to send this to the paint thread by lambda capture in the next patch (as in [1]) and capture by move isn't supported in lambdas until C++14.

We can get around this by writing a class which does a move inside the copy operator, but it might not be more pretty than just using a RefPtr. I think CapturedPaintState may have had the same problem.

[1] http://searchfox.org/mozilla-central/rev/aa1343961fca52e8c7dab16788530da31aab7c8d/gfx/layers/PaintThread.cpp#167
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/744c8fabaa1c
Remove unneeded lambda capture in paint thread (bug 1399692 part 1, r=bas)
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fcb661df79e
Simplify the code for creating a new back buffer (bug 1399692 part 2, r=bas)
https://hg.mozilla.org/integration/mozilla-inbound/rev/b57a3f0d0847
Remove BufferContentType and add ValidBufferSize (bug 1399692 part 3, r=bas)
https://hg.mozilla.org/integration/mozilla-inbound/rev/926af2eca400
Don't create back buffer for front buffer until we know what type to create. (bug 1399692 part 4, r=bas)
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d0da6564096
Simplify copying the front buffer to the back buffer (bug 1399692 part 5, r=bas)
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb6507b560aa
Record buffer operations to a struct for replaying on paint thread (bug 1399692 part 6, r=bas)
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9349ad2f1f8
Replay buffer commands on paint thread when OMTP is enabled (bug 1399692 part 7, r=bas)
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fac6f595967b
Mark ShallowCopy as override to fix bustage (bug 1399692, r=me)
Backout by nbeleuzu@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/b2375c59ecd9
Backed out changeset fac6f595967b 
https://hg.mozilla.org/mozilla-central/rev/28647c01f828
Backed out 7 changesets failing on browser chrome browser/base/content/test/performance/browser_urlbar_search_reflows.js
I've added two commits that address the speedometer regression. The bug was causing us to always create a back buffer each frame. I'm sad I didn't catch it.

Running a try run now to see if that resolves the test timeouts.
Flags: needinfo?(rhunt)
(In reply to Ryan Hunt [:rhunt] from comment #76)
> I've added two commits that address the speedometer regression. The bug was
> causing us to always create a back buffer each frame. I'm sad I didn't catch
> it.
> 
> Running a try run now to see if that resolves the test timeouts.

I've merged the commit that forced us to create a new back buffer each frame into the commit that introduced it, part 4.

I also added a fix for an issue that could happen with double buffering getting out of sync on surface modes.
Comment on attachment 8925322 [details]
Don't copy over regions that will be painted in this frame (bug 1399692 part 8, )

https://reviewboard.mozilla.org/r/196480/#review201898
Attachment #8925322 - Flags: review?(bas) → review+
Comment on attachment 8925323 [details]
Don't reuse a back buffer after a swap if the content or surface changed (bug 1399692 part 9, )

https://reviewboard.mozilla.org/r/196482/#review201900
Attachment #8925323 - Flags: review?(bas) → review+
FinalizeFrame relies on the front and back buffer having the same size, and this was implicitly enforced with EnsureBackBufferIfFrontBuffer and by clearing the front buffer whenever we created a new buffer from either a visible region increase, failing to AdjustTo, or failing to lock the back buffer. I've updated the code to enforce this constraint again.

In the future, I'd like to remove this constraint as long as the back buffer is big enough for the visible region. But nothing in here depends on that, and it's slightly complicated.
Comment on attachment 8923931 [details]
Remove unneeded lambda capture in paint thread (bug 1399692 part 1, )

https://reviewboard.mozilla.org/r/195098/#review202586


C/C++ static analysis found 0 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Comment on attachment 8923932 [details]
Simplify the code for creating a new back buffer (bug 1399692 part 2, )

https://reviewboard.mozilla.org/r/195100/#review202594


C/C++ static analysis found 0 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Comment on attachment 8923933 [details]
Remove BufferContentType and add ValidBufferSize (bug 1399692 part 3, )

https://reviewboard.mozilla.org/r/195102/#review202598


C/C++ static analysis found 0 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Comment on attachment 8923934 [details]
Don't create back buffer for front buffer until we know what type to create. (bug 1399692 part 4, )

https://reviewboard.mozilla.org/r/195104/#review202602


C/C++ static analysis found 0 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Comment on attachment 8923935 [details]
Simplify copying the front buffer to the back buffer (bug 1399692 part 5, )

https://reviewboard.mozilla.org/r/195106/#review202606


C/C++ static analysis found 0 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Comment on attachment 8923936 [details]
Record buffer operations to a struct for replaying on paint thread (bug 1399692 part 6, )

https://reviewboard.mozilla.org/r/195108/#review202608


C/C++ static analysis found 0 defects in this patch (only the first 30 are reported here).

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Comment on attachment 8923937 [details]
Replay buffer commands on paint thread when OMTP is enabled (bug 1399692 part 7, )

https://reviewboard.mozilla.org/r/195110/#review202618


C/C++ static analysis found 0 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Comment on attachment 8925322 [details]
Don't copy over regions that will be painted in this frame (bug 1399692 part 8, )

https://reviewboard.mozilla.org/r/196480/#review202622


C/C++ static analysis found 0 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Comment on attachment 8925323 [details]
Don't reuse a back buffer after a swap if the content or surface changed (bug 1399692 part 9, )

https://reviewboard.mozilla.org/r/196482/#review202626


C/C++ static analysis found 0 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Updated try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=06526424b045caccfdaa1b09d030e4589538ed21

There is one test failure that could be related to OMTP in dt6. But I've been unable to see how my patches would have caused it, and unable to reproduce it further in another try run. [1]

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=42b712ca30191b0ce6e1a01691cde782dfb3ac12
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c41a712dff2
Remove unneeded lambda capture in paint thread (bug 1399692 part 1, r=bas)
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ba8bda8521a
Simplify the code for creating a new back buffer (bug 1399692 part 2, r=bas)
https://hg.mozilla.org/integration/mozilla-inbound/rev/b971c1aa5a78
Remove BufferContentType and add ValidBufferSize (bug 1399692 part 3, r=bas)
https://hg.mozilla.org/integration/mozilla-inbound/rev/dce585be0737
Don't create back buffer for front buffer until we know what type to create. (bug 1399692 part 4, r=bas)
https://hg.mozilla.org/integration/mozilla-inbound/rev/467532fd5b7a
Simplify copying the front buffer to the back buffer (bug 1399692 part 5, r=bas)
https://hg.mozilla.org/integration/mozilla-inbound/rev/f235b12eda6e
Record buffer operations to a struct for replaying on paint thread (bug 1399692 part 6, r=bas)
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fc2414f146d
Replay buffer commands on paint thread when OMTP is enabled (bug 1399692 part 7, r=bas)
https://hg.mozilla.org/integration/mozilla-inbound/rev/76bf99decf09
Don't copy over regions that will be painted in this frame (bug 1399692 part 8, r=bas)
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d9324e2ab34
Don't reuse a back buffer after a swap if the content or surface changed (bug 1399692 part 9, r=bas)
The problem was that PaintOffMainThread can return false and then the layer will fallback to PaintThebes. This would cause that assert, and isn't necessary so I say we remove this behavior.

With this patch I'm unable to reproduce that assert after 5 retriggers.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5397beb38962e509f4917b2d81bb50c1e5b6a67d
Flags: needinfo?(rhunt)
Comment on attachment 8927113 [details]
Don't do PaintThebes after PaintOffMainThread. (bug 1399692 part 10, )

https://reviewboard.mozilla.org/r/198328/#review203554

::: gfx/layers/client/ClientPaintedLayer.cpp:222
(Diff revision 1)
>      didUpdate = true;
>    }
>  
>    if (!UpdatePaintRegion(state)) {
> -    return false;
> +    if (didUpdate) {
> +      ClientManager()->SetQueuedAsyncPaints();

A good candidate for RAII? But fine as-is for landing this patch.
Attachment #8927113 - Flags: review?(dvander) → review+
Comment on attachment 8927113 [details]
Don't do PaintThebes after PaintOffMainThread. (bug 1399692 part 10, )

https://reviewboard.mozilla.org/r/198328/#review203568


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: gfx/layers/client/ClientPaintedLayer.cpp:278
(Diff revision 1)
>  
>    if (didUpdate) {
>      UpdateContentClient(state);
>      ClientManager()->SetQueuedAsyncPaints();
>    }
> -  return true;
> +  return;

Warning: Redundant return statement at the end of a function with a void return type [clang-tidy: readability-redundant-control-flow]
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/646cb76259a5
Remove unneeded lambda capture in paint thread (bug 1399692 part 1, r=bas)
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9261720d6c2
Simplify the code for creating a new back buffer (bug 1399692 part 2, r=bas)
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1ba79c560d8
Remove BufferContentType and add ValidBufferSize (bug 1399692 part 3, r=bas)
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a045db296d2
Don't create back buffer for front buffer until we know what type to create. (bug 1399692 part 4, r=bas)
https://hg.mozilla.org/integration/mozilla-inbound/rev/dec1d0b6db69
Simplify copying the front buffer to the back buffer (bug 1399692 part 5, r=bas)
https://hg.mozilla.org/integration/mozilla-inbound/rev/a327b4cb9dca
Record buffer operations to a struct for replaying on paint thread (bug 1399692 part 6, r=bas)
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9ffbdc01598
Replay buffer commands on paint thread when OMTP is enabled (bug 1399692 part 7, r=bas)
https://hg.mozilla.org/integration/mozilla-inbound/rev/42de52097f20
Don't copy over regions that will be painted in this frame (bug 1399692 part 8, r=bas)
https://hg.mozilla.org/integration/mozilla-inbound/rev/885688ba4f3d
Don't reuse a back buffer after a swap if the content or surface changed (bug 1399692 part 9, r=bas)
https://hg.mozilla.org/integration/mozilla-inbound/rev/713c3225f148
Don't do PaintThebes after PaintOffMainThread. (bug 1399692 part 10, r=dvander)
Depends on: 1416726
Depends on: 1416873
Depends on: 1416921
Target Milestone: mozilla57 → mozilla58
Depends on: 1422597
No longer depends on: 1416726
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: