1272600 - Move gfxSharedReadLock into TextureClient/Host

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nicolas Silva [:nical]]

gfxSharedReadLock was originally only used for tiling, but we have other use cases for it these days (copy-on-write canvas updates, texture recycling, etc.). We should move it into the texture for convenience.

We would avoid the tedious bookkeeping of associating the lock to a texture, and we could probably make it work more implicitly.
For instance the TextureHost could take care of calling ReadUnlock since it has all of the information required to do it at the right moment, whereas currently the type of texture (whether it has an intermediate buffer) leaks in the TiledContentHost logic that manages the locks.

It would also be good that TextureClient::Lock(OPEN_WRITE) fails if the lock is taken on the compositor side which can only happen in racy situations and TextureClient currently doesn't know about the lock so it can't perform this check.

Comment 1

[Nicolas Silva [:nical]]

Attachment: Part 1 - Move gfxSharedReadLock into TextureClient.h + some cleanup

Some prep work, that just moves stuff around and rename the lock classes. I also made it so that the lock type is really abstracted away this time (they are not exposed in the header).

Comment 2

[Nicolas Silva [:nical]]

Attachment: Part 2 - Move the lock into TextureClient and TextureHost (WIP)

Store the lock in the texture without changing the logic any further for now.

Comment 3

[Nicolas Silva [:nical]]

Attachment: Part 3 - Move the locking/unlocking logic into TextureClient/Host

The patch is a tad bigger than I hoped but I am pretty happy with the result:
* Added a GetCompositor() method to TextureHost in order to implement some of the logic directly in TextureHost rather than in each derived class.
* moved the locking logic completely outside of the tiling code. When a TextureHost is replaced by another TextureHost, UnbindTextureSource is called. This is when we can tell the compositor to hold the lock and ReadUnlock() it after the next composition. Textures that have an intermediate buffer will ReadUnlock() earlier as soon as the upload is done.
* we only set a lock to textures that have been painted into (and therefore has some unsychronized changes). This simplifies things a great deal, and makes it easier to understand what's going on by looking at the state of the texture (we don't accumulate ReadLocks anymore when a tile is not painted into). Every time a TextureHost receives a lock, it has to unlock it once and only once before it receives another one.
* With this, non tiled layers can trivially use ReadLocks. The only thing required is to pass the locks to the TextureHosts during transactions. This will be usefull for the canvas work I am doing and most likely for the unified texture recycling thing.

There's a big comment on top of the TextureReadLock class that explains things a bit more.

Comment 4

[Nicolas Silva [:nical]]

Attachment: Part 3 - Move the locking/unlocking logic into TextureClient/Host

Comment 5

[Nicolas Silva [:nical]]

Attachment: Part 4 - Let TextureHost ReadUnlock automatically and let textures on white have their own lock

Comment 6

[Nicolas Silva [:nical]]

Attachment: Part 5 - Handle read locks for non-tiled layers

Comment 7

[Nicolas Silva [:nical]]

Attachment: Part 1 - Some preliminary cleanups

Comment 8

[Nicolas Silva [:nical]]

Attachment: Part 2 - Store the read locks into TextureClient/Host

Comment 9

[Nicolas Silva [:nical]]

Attachment: Part 3 - Move the locking/unlocking logic out of compositables into textures

This is where the most important changes are. Textures are now responsible for locking/unlocking at the best time, in a way that is transparent to Compositables. Like in sotaro's recycling patch queue, this makes it important that compositable implementations properly store textures in CompositableTextureRef slots and don't ignore textures they receive.

Comment 10

[Nicolas Silva [:nical]]

Attachment: Part 4 - Add support for read-locks in non-tiled layers

With read locks managed by textures, this is now only a matter of passing the lock around in the ipdl messages, etc.
We don't currently make use of this outside of tiling but will soon in canvas2d (at least).

Comment 11

[Nicolas Silva [:nical]]

Attachment: Part 5 - Make ImageHost store timed textures in CompositableTextureRef slots

Without this, there could be a chance that a video frame that don't end up getting displayed don't get read-unlocked. This might also be important for sotaro's texture recycling work.

Comment 12

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8756865 [details] [diff] [review]
Part 1 - Some preliminary cleanups

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

::: gfx/layers/client/TextureClient.h
@@ +28,5 @@
>  #include "nsAutoPtr.h"                  // for nsRefPtr
>  #include "nsCOMPtr.h"                   // for already_AddRefed
>  #include "nsISupportsImpl.h"            // for TextureImage::AddRef, etc
>  #include "GfxTexturesReporter.h"
> +#include "pratom.h"

Is there a reason to include "pratom.h" here? You might want to include it in TextureClient.cpp.

Comment 13

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8756872 [details] [diff] [review]
Part 5 - Make ImageHost store timed textures in CompositableTextureRef slots

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

::: gfx/layers/composite/ImageHost.h
@@ +126,5 @@
>      int32_t mProducerID;
>      int32_t mInputFrameID;
>    };
>  
> +  RefPtr<TextureHost> mCurrentTextureHost;

Is there a reason why it is changed to normal RefPtr?

Comment 14

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8756867 [details] [diff] [review]
Part 2 - Store the read locks into TextureClient/Host

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

::: gfx/layers/composite/TextureHost.cpp
@@ +515,5 @@
> +    mReadLock->ReadUnlock();
> +    mReadLock = nullptr;
> +  }
> +}
> +

It seems to work to TiledLayers. But how does it work to other layers? It seems to assume the following sequence. And it could be broken in ImageLayer. If its mechanism is limited to some type of layers, it seems better to add a comment about it.

-> Client side: Rendered to TextureClient
-> Client side: Forward the TextureClient and TextureReadLock
-> Host side: Receive TextureHost and TextureReadLock
              Hold CompositableRef of the TextureHost
-> When the CompositableRef becomes 0, release TextureReadLock.

Comment 15

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8756867 [details] [diff] [review]
Part 2 - Store the read locks into TextureClient/Host

Review+, if comment is addressed.

Comment 16

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8756869 [details] [diff] [review]
Part 3 - Move the locking/unlocking logic out of compositables into textures

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

If TexutureHost has direct binding buffer, shouldn't we need to hold it until next composition(composition end) like Bug 1260611 ?

Comment 17

[Nicolas Silva [:nical]]

(In reply to Sotaro Ikeda [:sotaro] from comment #12)
> > +#include "pratom.h"
> 
> Is there a reason to include "pratom.h" here? You might want to include it
> in TextureClient.cpp.

I'll look into this. I basically moved stuff around from TiledContentClient.h/cpp to TextureClient, and I assume some of the atomic operations were in the header back then.

> > +  RefPtr<TextureHost> mCurrentTextureHost;
> 
> Is there a reason why it is changed to normal RefPtr?

Yes, basically we have two strong references to the texture: the one in the array of timed textures and mCurrentTextureHost. We need to only keep one CompositableTextureRef per layer for a given texture to be sure not to miss the fast path when the compositableref counter is 1. So in this patch the array of timed textures is new responsible for holding the CompositableTextureRef slots.

(In reply to Sotaro Ikeda [:sotaro] from comment #14)
> It seems to work to TiledLayers. But how does it work to other layers? It
> seems to assume the following sequence. And it could be broken in
> ImageLayer. If its mechanism is limited to some type of layers, it seems
> better to add a comment about it.

Part 4 completes adding support for other layer types. After that it transparently works for all compositables. The only limitation currently is that we don't yet have code to create the TextureReadLock for ImageBridgeChild compositables. But that won't cause bugs since you need to be able to create the ReadLock on the child side to opt into using it. 

(In reply to Sotaro Ikeda [:sotaro] from comment #16)
> If TexutureHost has direct binding buffer, shouldn't we need to hold it
> until next composition(composition end) like Bug 1260611 ?

This is what this patch queue does (unless I made a mistake): the TextureHost calls Compositor::UnlockAfterComposition which will wait for Compositor::EndFrame to do the actual unlock.

The patch queue is intended to land in one go, it's probable that some pieces in part N require part N+1 to work completely. Sorry if this confuses the review.

Comment 18

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8756872 [details] [diff] [review]
Part 5 - Make ImageHost store timed textures in CompositableTextureRef slots

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

Your comment make it clear. It might help there is a comment about it.

Comment 19

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #17)
> (In reply to Sotaro Ikeda [:sotaro] from comment #16)
> > If TexutureHost has direct binding buffer, shouldn't we need to hold it
> > until next composition(composition end) like Bug 1260611 ?
> 
> This is what this patch queue does (unless I made a mistake): the
> TextureHost calls Compositor::UnlockAfterComposition which will wait for
> Compositor::EndFrame to do the actual unlock.
> 
> The patch queue is intended to land in one go, it's probable that some
> pieces in part N require part N+1 to work completely. Sorry if this confuses
> the review.

Sorry, I do not understand well this part well. Can you explain more?

From the patch, Compositor::EndFrame() seems to unlock all read lock of UnlockAfterComposition(). But when compositor is GPU accelerated, Compositor::EndFrame() just says end of queueing composition task to GPU. Actual composition is still ongoing on GPU. Therefore if TextureHost has direct binding buffer, the buffers are still in use by GPU even after Compositor::EndFrame().

Comment 20

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8756869 [details] [diff] [review]
Part 3 - Move the locking/unlocking logic out of compositables into textures

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

Call ReadUnlock() is called in some TextureHosts. The reason of it might not be clear enough just from the calls. It might be better to add a comment or code clean up.

::: gfx/layers/client/TextureClient.cpp
@@ +388,5 @@
>  
>  bool
>  TextureClient::IsReadLocked() const
>  {
> +  return mReadLock && mReadLock->GetReadCount() > 1 && !mPendingReadUnlock;

It might be better to add a comment here. !mPendingReadUnlock part is a bit tricky.

Comment 21

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8756869 [details] [diff] [review]
Part 3 - Move the locking/unlocking logic out of compositables into textures

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

::: gfx/layers/Compositor.cpp
@@ +40,5 @@
> +{
> +  for (auto& lock : mUnlockAfterComposition) {
> +    lock->ReadUnlock();
> +  }
> +  mUnlockAfterComposition.Clear();

Could this handle a use case that TextureHost is used by multiple compositor like Bug 1265468?

Comment 22

[Nicolas Silva [:nical]]

(In reply to Sotaro Ikeda [:sotaro] from comment #18)
> Your comment make it clear. It might help there is a comment about it.

Good point(s), I will add more comments here and in the other places you pointed out.


> Sorry, I do not understand well this part well. Can you explain more?
> 
> From the patch, Compositor::EndFrame() seems to unlock all read lock of
> UnlockAfterComposition(). But when compositor is GPU accelerated,
> Compositor::EndFrame() just says end of queueing composition task to GPU.
> Actual composition is still ongoing on GPU. Therefore if TextureHost has
> direct binding buffer, the buffers are still in use by GPU even after
> Compositor::EndFrame().is

This is interesting. In principle you are right, the GPU's asynchronous nature means we are still potentially using the texture and we could hold the lock for one more frame to be extra certain.
I double-checked and we currently don't do this (extra frame), though. 
In the transaction where a texture is not used anymore, it's lock is put in the mDelayedUnlocks array, and all of these unlocks are at the next composition just after DrawQuad (so it's actually even earlier than my patch because my patch does that at the end of the next composition). 

This works because with our two use cases (Gralloc and D3D11 texture), we have an additional synchronization mechanism which will block TextureClient::Lock (fences, and DXGI keyed mutex), so releasing the lock after composition N rather than N+1 is sufficient for us to have the guarantee that the texture will be available in a finite (and hopefully small) amount of time (so it's safe to block in TextureClient::Lock) while we still we need to rely on gralloc and DXGI to do the extra bit of synchronization.

With this in mind, I think that we should have a separate discussion about adding an extra frame of delay. If we do that we could even remove gralloc fences and use unsynchronized DXGI textures. but there's a trade-off because we would more often trigger the copy-on-write of tiles (and soon canvas), whereas currently we will throttle painting by blocking rather than allocate new back buffers. so I don't know if this is the behavior we want but it's worth experimenting with. We could even have a flag to choose between the two behaviors if we feel that they both have their place. Adding an extra frame of delay would be very easy to add anyhow.

Considering that this patch queue is already pretty complex, I would feel better about not adding the risk of changing the locking behavior in this bug. I'd rather land things as they are now and experiment with the extra frame while we let this patch queue some time to prove its stability on central.

What do you think?

> Could this handle a use case that TextureHost is used by multiple compositor
> like Bug 1265468?

It works with several compositors because since unlocking depends on CompsoitableTextureRef counters, the texture won't be unlocked until no compositor are using it.
The trickiest hypothetical situation would be the following:
 * a texture is used by two compositors A and B
 * suddenly both compositors are not using the texture anymore, and the texture asks compositor A to do the delayed unlock.
 * A composites (and does the unlock) but for some odd reason B doesn't composite (This in itself is already a bug because something changed for this compositor so a composition should be shceduled)

In this case we are saved again by the DXGI mutex and fence. That said, I don't think this is even possible because most textures can't be used with several compositors at the same time (they can be used with several layers on the same compositor).

In any case it is not possible for a texture to be unlocked by a compositor and still be used by another compositor in subsequent frames, thanks to the CompositableTextureRef mechanism.

Comment 23

[Nicolas Silva [:nical]]

Attachment: Part 6 - Don't delay ReadUnlock for BufferTextureHost.

BufferTextureHost's shared data is always ever accessed synchronously on the compositor thread, even when the texture doesn't have an intermediate buffer (beause in this case we are using the (non-x11) BasicCompositor which is synchronous. This means we can optimize the unlock slightly by ReadUnlocking right away when the texture is not used anymore instead of asking the compositor to ReadUnlock layer. It makes a notable difference for copy-on-write canvas.

Comment 24

[Nicolas Silva [:nical]]

Attachment: Part 7 - Some documentation

Comment 25

[Nicolas Silva [:nical]]

Attachment: Part 8 - Simplify the mPendingReadUnlock business on the content side.

This makes things a lot simpler. Instead of calling ReadLock from TextureClient::Unlock and keep mPendingReadUnlock around to override the behavior of TextureClient::IsReadLocked, we just store a mUpdated boolean which is easier to understand, and call ReadLock before serializing the ReadLock.

It makes a lot more sense than the convoluted behavior I came up with before.

Comment 26

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #22)
> With this in mind, I think that we should have a separate discussion about
> adding an extra frame of delay. If we do that we could even remove gralloc
> fences and use unsynchronized DXGI textures. but there's a trade-off because
> we would more often trigger the copy-on-write of tiles (and soon canvas),
> whereas currently we will throttle painting by blocking rather than allocate
> new back buffers. so I don't know if this is the behavior we want but it's
> worth experimenting with. We could even have a flag to choose between the
> two behaviors if we feel that they both have their place. Adding an extra
> frame of delay would be very easy to add anyhow.
> 
> Considering that this patch queue is already pretty complex, I would feel
> better about not adding the risk of changing the locking behavior in this
> bug. I'd rather land things as they are now and experiment with the extra
> frame while we let this patch queue some time to prove its stability on
> central.
> 
> What do you think?

I am ok to split to different bug.

> > Could this handle a use case that TextureHost is used by multiple compositor
> > like Bug 1265468?
> 
> It works with several compositors because since unlocking depends on
> CompsoitableTextureRef counters, the texture won't be unlocked until no
> compositor are using it.
> The trickiest hypothetical situation would be the following:
>  * a texture is used by two compositors A and B
>  * suddenly both compositors are not using the texture anymore, and the
> texture asks compositor A to do the delayed unlock.
>  * A composites (and does the unlock) but for some odd reason B doesn't
> composite (This in itself is already a bug because something changed for
> this compositor so a composition should be shceduled)
> 
> In this case we are saved again by the DXGI mutex and fence. That said, I
> don't think this is even possible because most textures can't be used with
> several compositors at the same time (they can be used with several layers
> on the same compositor).
> 
> In any case it is not possible for a texture to be unlocked by a compositor
> and still be used by another compositor in subsequent frames, thanks to the
> CompositableTextureRef mechanism.

Ok, make sense.

Comment 27

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8756869 [details] [diff] [review]
Part 3 - Move the locking/unlocking logic out of compositables into textures

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

::: gfx/layers/composite/TextureHost.cpp
@@ +328,5 @@
> +TextureHost::UnbindTextureSource()
> +{
> +  if (mReadLock && GetCompositor()) {
> +    GetCompositor()->UnlockAfterComposition(mReadLock.forget());
> +  }

What happens if the TextureHost does not have Compositor?

Comment 28

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8758346 [details] [diff] [review]
Part 8 - Simplify the mPendingReadUnlock business on the content side.

Nice!

Comment 29

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #27)
> Comment on attachment 8756869 [details] [diff] [review]
> Part 3 - Move the locking/unlocking logic out of compositables into textures
> 
> Review of attachment 8756869 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/composite/TextureHost.cpp
> @@ +328,5 @@
> > +TextureHost::UnbindTextureSource()
> > +{
> > +  if (mReadLock && GetCompositor()) {
> > +    GetCompositor()->UnlockAfterComposition(mReadLock.forget());
> > +  }
> 
> What happens if the TextureHost does not have Compositor?

Just from it, TextureHost seems to necessitate Compositor to unlock read lock if TextureHost has direct binding buffer and not BufferTextureHost.

Comment 30

[Nicolas Silva [:nical]]

(In reply to Sotaro Ikeda [:sotaro] from comment #29)
> > What happens if the TextureHost does not have Compositor?
> 
> Just from it, TextureHost seems to necessitate Compositor to unlock read
> lock if TextureHost has direct binding buffer and not BufferTextureHost.

Hm. If it doesn't have a Compositor, the Unlock will happen in ~TextureHost. I don't know for sure how that could happen. I suppose that there are the first few video frames which may arrive before the ImageHost is attached to a layer (so they don't get a Compositor until the attachment happens) and could potentially be removed from the compositable before getting a compositor, maybe?

I hadn't though of that before. In practice I think that UnbindTextureSource should ReadUnlock right away if the texture does not have a Compositor. In this situation the texture can't be used by a compositor (otherwise mCompositor would not be null), so we don't need to wait until the next composition to unlock. I'll update the patch.

Comment 31

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #30)
> 
> Hm. If it doesn't have a Compositor, the Unlock will happen in ~TextureHost.
> I don't know for sure how that could happen. I suppose that there are the
> first few video frames which may arrive before the ImageHost is attached to
> a layer (so they don't get a Compositor until the attachment happens) and
> could potentially be removed from the compositable before getting a
> compositor, maybe?

Yes, on b2g, the situation was observed during start up.

Comment 32

[Nicolas Silva [:nical]]

Attachment: Part 3 - Move the locking/unlocking logic out of compositables into textures (v2)

Same patch with TextureHost::UnbindTextureSource updated according to my previous comment.

Comment 33

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8758655 [details] [diff] [review]
Part 3 - Move the locking/unlocking logic out of compositables into textures (v2)

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

::: gfx/layers/client/TiledContentClient.cpp
@@ +660,5 @@
>    if (mFrontBuffer &&
>        mFrontBuffer->HasIntermediateBuffer() &&
>        !mFrontBuffer->IsReadLocked() &&
> +      (aMode != SurfaceMode::SURFACE_COMPONENT_ALPHA || (
> +        mFrontBufferOnWhite && !mFrontBuffer->IsReadLocked()))) {

!mFrontBuffer->IsReadLocked() seems to appear twice here. Don't we need to update here?

Comment 34

[Nicolas Silva [:nical]]

(In reply to Sotaro Ikeda [:sotaro] from comment #33)
> !mFrontBuffer->IsReadLocked() seems to appear twice here. Don't we need to
> update here?

Ooch! that's a typo, it should have been:

> +        mFrontBufferOnWhite && !mFrontBufferOnWhite->IsReadLocked()))) {

Comment 35

[Nicolas Silva [:nical]]

(In reply to Nicolas Silva [:nical] from comment #34)
> Ooch! that's a typo, it should have been:
> 
> > +        mFrontBufferOnWhite && !mFrontBufferOnWhite->IsReadLocked()))) {

I fixed that locally, I'll upload the new patch later in case you have other review comments.

Comment 36

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8758655 [details] [diff] [review]
Part 3 - Move the locking/unlocking logic out of compositables into textures (v2)

I do not have other comment.

Comment 37

[Nicolas Silva [:nical]]

Attachment: Part 3 - Move the locking/unlocking logic out of compositables into textures (v3) carrying r=sotaro

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/db987382c68e
Part 1 - Preliminary cleanups for the ReadLock patch queue. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/96fcb332dd5a
Part 2 - Store TextureReadLock into TextureClient/Host. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a4f633a167c
Part 3 - Move the ReadUnlock logic from compositable to texture. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/11a2a2511cc9
Part 4 - Support TextureReadLock with all layer types. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/c39f4f5c16fa
Part 5 - Make ImageHost store all timed images in CompositableTextureRef slots. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9255f6b539c
Part 6 - ReadUnlock BufferTextureHost as soon as its data is available. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/21852160f0a5
Part 7 - Add some documentation to TextureReadLock. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/14c774986c26
Part 8 - Simplify the textures ReadLock on the content side. r=sotaro

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/db987382c68e
https://hg.mozilla.org/mozilla-central/rev/96fcb332dd5a
https://hg.mozilla.org/mozilla-central/rev/7a4f633a167c
https://hg.mozilla.org/mozilla-central/rev/11a2a2511cc9
https://hg.mozilla.org/mozilla-central/rev/c39f4f5c16fa
https://hg.mozilla.org/mozilla-central/rev/a9255f6b539c
https://hg.mozilla.org/mozilla-central/rev/21852160f0a5
https://hg.mozilla.org/mozilla-central/rev/14c774986c26

Comment 40

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8759090 [details] [diff] [review]
Part 3 - Move the locking/unlocking logic out of compositables into textures (v3) carrying r=sotaro

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

::: gfx/layers/client/TiledContentClient.cpp
@@ +670,5 @@
>      if (!mBackBuffer || mBackBuffer->IsReadLocked()) {
> +      mBackBuffer.Set(this,
> +        CreateBackBufferTexture(mBackBuffer, mCompositableClient, mAllocator)
> +      );
> +      mInvalidBack = IntRect(0, 0, mBackBuffer->GetSize().width, mBackBuffer->GetSize().height);

We're using mBackBuffer here, but then we check if it's null one line too late.

@@ +683,5 @@
> +        && (!mBackBufferOnWhite || mBackBufferOnWhite->IsReadLocked())) {
> +      mBackBufferOnWhite = CreateBackBufferTexture(
> +        mBackBufferOnWhite, mCompositableClient, mAllocator
> +      );
> +      mInvalidBack = IntRect(0, 0, mBackBuffer->GetSize().width, mBackBuffer->GetSize().height);

Should this be mBackBufferOnWhite->GetSize()... instead?

Also, same comment as above - we're checking if mBackBufferOnWhite is null too late in this case.