WIP patch, seems to run at least. Obviously still need the OpenGL layers implementations of this to be hooked up, and I think there are still plenty of bugs here.

These don't appear to do anything useful except take up sapce.

This doesn't appear to be used by anything post-refactoring

This is pretty hideous. Basically copies the approach taken by the OGL layer buffer code.

These values are pretty much meaningless on the client side (when we have a compositor), except the need to match what the compositor will compute. I think this really highlights the need to split BasicLayerManager and ClientLayerManager into entirely separate code paths. I might work on that next.

Comment on attachment 738797 [details] [diff] [review] Add code handling dual buffers in ThebesLayerBuffer (client side) Adding feedback?Bas since I haven't implemented the Moz2D paths for this yet. Someone will need to do that before we can use this on d3d11.

Dead code.

Comment on attachment 738790 [details] [diff] [review] Add an AutoLockTextureHost class Review of attachment 738790 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/composite/TextureHost.h @@ +304,5 @@ > + { > + if (mTextureHost) { > + mIsValid = mTextureHost->Lock(); > + } else { > + mIsValid = true; Why not set mIsValid = false here and then you can skip the !mTextureHost check in ContentHost? Also, just initialise mIsValid and drop the else clause

(In reply to Nick Cameron [:nrc] from comment #10) > Why not set mIsValid = false here and then you can skip the !mTextureHost > check in ContentHost? Because I wanted a nullptr texture host to be considered valid (since this is often the case for the on-white buffer). > > Also, just initialise mIsValid and drop the else clause Sure.

Comment on attachment 738797 [details] [diff] [review] Add code handling dual buffers in ThebesLayerBuffer (client side) Review of attachment 738797 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/client/ContentClient.cpp @@ +251,4 @@ > } > > void > +ContentClientDoubleBuffered::CreateFrontBufferAndNotify(const nsIntRect& aBufferRect, uint32_t aFlags) You don't need to pass aFlags, you can check mTextureInfo.mTextureFlags - this way might be confusing because the call to CreatedDoubleBuffer uses these flags rather than aFlags ::: gfx/layers/ipc/CompositableForwarder.h @@ +58,5 @@ > */ > virtual void CreatedSingleBuffer(CompositableClient* aCompositable, > const SurfaceDescriptor& aDescriptor, > + const TextureInfo& aTextureInfo, > + bool aWhiteBuffer = false) = 0; Please use a flag in aTextureInfo rather than adding this bool

Comment on attachment 738793 [details] [diff] [review] Let ContentHost create an EffectComponentAlpha if necessary Review of attachment 738793 [details] [diff] [review]: ----------------------------------------------------------------- r=me with these little changes ::: gfx/layers/Effects.h @@ +187,5 @@ > MOZ_NOT_REACHED("unhandled program type"); > } > > + if (aTextureHostOnWhite) { > + return new EffectComponentAlpha(aTextureHost, aTextureHostOnWhite, aFilter); should probably assert that the type we figured out above is compatible with EffectComponentAlpha @@ +188,5 @@ > } > > + if (aTextureHostOnWhite) { > + return new EffectComponentAlpha(aTextureHost, aTextureHostOnWhite, aFilter); > + } else { doesn't need to be an else branch

Comment on attachment 738794 [details] [diff] [review] Remove the old buffer provider code Review of attachment 738794 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/ThebesLayerBuffer.cpp @@ +290,5 @@ > gfxASurface::gfxContentType > ThebesLayerBuffer::BufferContentType() > { > if (mBuffer) { > return mBuffer->GetContentType(); should we assert we don't have a buffer provider? ::: gfx/layers/ThebesLayerBuffer.h @@ +257,5 @@ > mDTBuffer = nullptr; > } else { > // Only this buffer provider can give us a buffer. If we > // already have one, something has gone wrong. > MOZ_ASSERT(!mBuffer && !mDTBuffer); I think this method can be written more nicely with an assertion up front no else branch

Comment on attachment 738796 [details] [diff] [review] Add code for handling dual buffers in ContentHost Review of attachment 738796 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/composite/ContentHost.cpp @@ +214,5 @@ > const TextureInfo& aTextureInfo) > { > + MOZ_ASSERT(aTextureId == TextureFront || > + aTextureId == TextureOnWhiteFront); > + RefPtr<TextureHost> *newHost = trailing whitespace @@ +222,1 @@ > aTextureInfo.mTextureHostFlags, indent @@ +257,5 @@ > return; > } > > if (mNewFrontHost) { > DestroyFrontHost(); either we should modify DestroyFrontHost to take account of the OnWhite texture, or not call it here (the only benefit seems to be the assertion) or both. @@ +348,5 @@ > MOZ_ASSERT(mNewFrontHost->GetDeAllocator(), > "We won't be able to destroy our SurfaceDescriptor"); > mNewFrontHost = nullptr; > } > + whitespace @@ +360,5 @@ > MOZ_ASSERT(mBackHost->GetDeAllocator(), > "We won't be able to destroy our SurfaceDescriptor"); > mBackHost = nullptr; > } > + whitespace

Comment on attachment 738789 [details] [diff] [review] Remove unnecessary Effects classes LGTM, but better check with Bas

(In reply to Nick Cameron [:nrc] from comment #12) > Comment on attachment 738797 [details] [diff] [review] > Add code handling dual buffers in ThebesLayerBuffer (client side) > > Review of attachment 738797 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/layers/client/ContentClient.cpp > @@ +251,4 @@ > > } > > > > void > > +ContentClientDoubleBuffered::CreateFrontBufferAndNotify(const nsIntRect& aBufferRect, uint32_t aFlags) > > You don't need to pass aFlags, you can check mTextureInfo.mTextureFlags - > this way might be confusing because the call to CreatedDoubleBuffer uses > these flags rather than aFlags > > ::: gfx/layers/ipc/CompositableForwarder.h > @@ +58,5 @@ > > */ > > virtual void CreatedSingleBuffer(CompositableClient* aCompositable, > > const SurfaceDescriptor& aDescriptor, > > + const TextureInfo& aTextureInfo, > > + bool aWhiteBuffer = false) = 0; > > Please use a flag in aTextureInfo rather than adding this bool\ I'm not entirely sure how this would work, since these two functions would need different flags. The flag passed to CreateFrontBufferAndNotify marks if there are two buffers being created, the flag to CreatedSingleBuffer marks if this SurfaceDescriptor is for the white or black TextureIdentifier. We also can't easily just pass a TextureIdentifier to this function, because CreatedDoubleBuffer would make that confusing.

(In reply to Matt Woodrow (:mattwoodrow) from comment #17) > (In reply to Nick Cameron [:nrc] from comment #12) > > Comment on attachment 738797 [details] [diff] [review] > > Add code handling dual buffers in ThebesLayerBuffer (client side) > > > > Review of attachment 738797 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: gfx/layers/client/ContentClient.cpp > > @@ +251,4 @@ > > > } > > > > > > void > > > +ContentClientDoubleBuffered::CreateFrontBufferAndNotify(const nsIntRect& aBufferRect, uint32_t aFlags) > > > > You don't need to pass aFlags, you can check mTextureInfo.mTextureFlags - > > this way might be confusing because the call to CreatedDoubleBuffer uses > > these flags rather than aFlags > > > > ::: gfx/layers/ipc/CompositableForwarder.h > > @@ +58,5 @@ > > > */ > > > virtual void CreatedSingleBuffer(CompositableClient* aCompositable, > > > const SurfaceDescriptor& aDescriptor, > > > + const TextureInfo& aTextureInfo, > > > + bool aWhiteBuffer = false) = 0; > > > > Please use a flag in aTextureInfo rather than adding this bool\ > > I'm not entirely sure how this would work, since these two functions would > need different flags. > > The flag passed to CreateFrontBufferAndNotify marks if there are two buffers > being created, the flag to CreatedSingleBuffer marks if this > SurfaceDescriptor is for the white or black TextureIdentifier. > > We also can't easily just pass a TextureIdentifier to this function, because > CreatedDoubleBuffer would make that confusing. How about making a single call to Created*Buffer and have optional arguments for the extra descriptors? Then Created*Buffer can turn it into 2 or 4 IPC messages?

Fixed nrc's suggestions.

Comment on attachment 738888 [details] [diff] [review] Add code handling dual buffers in ThebesLayerBuffer (client side) v2 Review of attachment 738888 [details] [diff] [review]: ----------------------------------------------------------------- This is quite invasive. Are you sure there's no cleaner way to do it? I guess we can't move it down into a new texture type, because that will create more per-backend issues. What if we moved it up so that a ThebesLayer had two ThebesLayerBuffers? ::: gfx/layers/ThebesLayerBuffer.cpp @@ +504,5 @@ > } > > + if (HaveBuffer() && > + (contentType != BufferContentType() || > + mode == Layer::SURFACE_COMPONENT_ALPHA) != (HaveBufferOnWhite())) { I do not understand this. Comment? @@ +627,5 @@ > + nsRefPtr<gfxContext> tmpCtx = new gfxContext(destBufferOnWhite); > + nsIntPoint offset = -destBufferRect.TopLeft(); > + tmpCtx->SetOperator(gfxContext::OPERATOR_SOURCE); > + tmpCtx->Translate(gfxPoint(offset.x, offset.y)); > + // TODO: Make this draw the on white buffer! What does this TODO mean? ::: gfx/layers/ThebesLayerBuffer.h @@ +58,5 @@ > > + enum ContextSource{ > + BUFFER_BLACK, > + BUFFER_WHITE, > + BUFFER_BOTH Please document these @@ +218,5 @@ > uint32_t aFlags); > > enum { > + ALLOW_REPEAT = 0x01, > + BUFFER_ON_WHITE = 0x02 Document this

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20) > > This is quite invasive. Are you sure there's no cleaner way to do it? I > guess we can't move it down into a new texture type, because that will > create more per-backend issues. What if we moved it up so that a ThebesLayer > had two ThebesLayerBuffers? > I don't think that will be easy, ThebesLayer has a ContentClient (compositable), and that inherits from ThebesLayerBuffer. I'm pretty sure that we don't want multiple compositables.

Comment on attachment 739408 [details] [diff] [review] Add code handling dual buffers in ThebesLayerBuffer (client side) v3 Review of attachment 739408 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/ThebesLayerBuffer.h @@ +58,5 @@ > > + /* > + * Which buffer should be drawn to/read from. > + */ > + enum ContextSource{ Space before { @@ +61,5 @@ > + */ > + enum ContextSource{ > + BUFFER_BLACK, // The normal buffer, or buffer with black background when using component alpha. > + BUFFER_WHITE, // The buffer with white background, only valid with component alpha. > + BUFFER_BOTH // The combined black/white buffers, on valid for writing operations, not reading. "only valid"

https://hg.mozilla.org/integration/mozilla-inbound/rev/da9cb9f7c845 https://hg.mozilla.org/integration/mozilla-inbound/rev/20f7119bb059 https://hg.mozilla.org/integration/mozilla-inbound/rev/ecb9f51c5c4c https://hg.mozilla.org/integration/mozilla-inbound/rev/6619f5967ee4 https://hg.mozilla.org/integration/mozilla-inbound/rev/9c63f8f4a341 https://hg.mozilla.org/integration/mozilla-inbound/rev/ff6187ed9e2c https://hg.mozilla.org/integration/mozilla-inbound/rev/b26c44283ccd

https://hg.mozilla.org/mozilla-central/rev/da9cb9f7c845 https://hg.mozilla.org/mozilla-central/rev/20f7119bb059 https://hg.mozilla.org/mozilla-central/rev/ecb9f51c5c4c https://hg.mozilla.org/mozilla-central/rev/6619f5967ee4 https://hg.mozilla.org/mozilla-central/rev/9c63f8f4a341 https://hg.mozilla.org/mozilla-central/rev/ff6187ed9e2c https://hg.mozilla.org/mozilla-central/rev/b26c44283ccd