Closed Bug 809273 Opened 7 years ago Closed 7 years ago

Support component alpha layers with OMTC

Categories

(Core :: Graphics: Layers, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: mattwoodrow, Unassigned)

References

Details

Attachments

(7 files, 4 obsolete files)

3.09 KB, patch
nrc
: review+
Details | Diff | Splinter Review
4.26 KB, patch
nrc
: review+
Details | Diff | Splinter Review
6.56 KB, patch
nrc
: review+
Details | Diff | Splinter Review
7.94 KB, patch
nrc
: review+
Details | Diff | Splinter Review
4.84 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.70 KB, patch
nrc
: review+
Details | Diff | Splinter Review
61.45 KB, patch
roc
: review+
Details | Diff | Splinter Review
Attached patch WIP (obsolete) — Splinter Review
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.
Attachment #678975 - Attachment is obsolete: true
These don't appear to do anything useful except take up sapce.
Attachment #738789 - Flags: review?
Attachment #738790 - Flags: review?(ncameron)
This doesn't appear to be used by anything post-refactoring
Attachment #738794 - Flags: review?(ncameron)
This is pretty hideous. Basically copies the approach taken by the OGL layer buffer code.
Attachment #738797 - Flags: review?(roc)
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.
Attachment #738801 - Flags: review?(roc)
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.
Attachment #738797 - Flags: feedback?(bas)
Dead code.
Attachment #738803 - Flags: review?(ncameron)
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
Attachment #738790 - Flags: review?(ncameron) → review+
(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
Attachment #738793 - Flags: review?(ncameron) → review+
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
Attachment #738794 - Flags: review?(ncameron) → review+
Attachment #738803 - Flags: review?(ncameron) → review+
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
Attachment #738796 - Flags: review?(ncameron) → review+
Comment on attachment 738789 [details] [diff] [review]
Remove unnecessary Effects classes

LGTM, but better check with Bas
Attachment #738789 - Flags: review? → review?(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.
Attachment #738797 - Attachment is obsolete: true
Attachment #738797 - Flags: review?(roc)
Attachment #738797 - Flags: feedback?(bas)
Attachment #738888 - Flags: review?(roc)
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.
Attachment #738888 - Attachment is obsolete: true
Attachment #738888 - Flags: review?(roc)
Attachment #739408 - Flags: review?(roc)
Attachment #738789 - Attachment is obsolete: true
Attachment #738789 - Flags: review?(bas)
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"
Attachment #739408 - Flags: review?(roc) → review+
You need to log in before you can comment on or make changes to this bug.