895116 - BasicCompositor fixes

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Comment 1

[David Zbarsky (:dzbarsky)]

Attachment: Use an ImageSurface in CanvasClient2DUpdate

...because that's what BasicCanvasLayer expects.

Comment 2

[David Zbarsky (:dzbarsky)]

Attachment: Use RGBARectProgram in nsChildView

Comment 3

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/128e7da5f9b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/d07610709b5d

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/128e7da5f9b2
https://hg.mozilla.org/mozilla-central/rev/d07610709b5d

Comment 5

[David Zbarsky (:dzbarsky)]

Attachment: Hack needsYFlip

This is almost certainly not what we want, but it does fix the rendering.  The issue is that we paint twice - once with CanvasLayerComposite::RenderLayer, which comes out upside down, and a second time with CopyableCanvasLayer::PaintWithOpacity.  Perhaps we shouldn't be doing that?

Comment 6

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 778875 [details] [diff] [review]
Hack needsYFlip

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

We shouldn't need to do this!

The NeedsYFlip should already be shared with the compositor using texture flags.

We read it here and produce texture coords that have a negative height: http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/ImageHost.cpp#125
Then we use that to produce a pattern matrix that should include a y-flip: http://mxr.mozilla.org/mozilla-central/source/gfx/layers/basic/BasicCompositor.cpp#248

And that should result in the image being flipped. Which of these things isn't happening?

Could possibly be a bug with pattern transforms in your azure backend (which are you using?).

Comment 7

[David Zbarsky (:dzbarsky)]

Attachment: Remove duplicate reftests

Looks like you may have checked these in by accident.

Comment 8

[David Zbarsky (:dzbarsky)]

Attachment: Fix needsYFlip

Thanks for the pointers Matt.  It made this much easier to track down.

Comment 9

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 778902 [details] [diff] [review]
Fix needsYFlip

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

\o/

Comment 10

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/821e984ef423
https://hg.mozilla.org/integration/mozilla-inbound/rev/039e13f659a6

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/821e984ef423
https://hg.mozilla.org/mozilla-central/rev/039e13f659a6

Comment 12

[David Zbarsky (:dzbarsky)]

Attachment: Fix masking

This fixes some of the failing border-radius tests.  The rest seem to be failing because of opacity problems.  I'll try to figure that out next.

Comment 13

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 779403 [details] [diff] [review]
Fix masking

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

::: gfx/layers/basic/BasicCompositor.cpp
@@ +58,5 @@
>      MOZ_ASSERT(mThebesImage);
> +    mFormat =
> +      (mThebesImage->Format() == gfxASurface::ImageFormatA8) ? FORMAT_A8 :
> +      (surf.ContentType() == gfxASurface::CONTENT_COLOR_ALPHA) ? FORMAT_B8G8R8A8 :
> +                                                                 FORMAT_B8G8R8X8;

Just use this function instead: http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfx2DGlue.h#201

@@ +255,5 @@
>                                    gfxPoint(aDestRect.XMost(), aDestRect.y),
>                                    gfxPoint(aDestRect.XMost(), aDestRect.YMost()));
>    Matrix matrix = ToMatrix(transform);
>    if (aMask) {
> +    // xxxdz handle the case when our mask transform is more than just a translation

NS_ASSERTION that we're only getting translations for now?

@@ +291,5 @@
>    RefPtr<SourceSurface> sourceMask;
>    Matrix maskTransform;
>    if (aEffectChain.mSecondaryEffects[EFFECT_MASK]) {
>      EffectMask *effectMask = static_cast<EffectMask*>(aEffectChain.mSecondaryEffects[EFFECT_MASK].get());
> +    static_cast<DeprecatedTextureHost*>(effectMask->mMaskTexture)->Lock();

No matching unlock!

Put this in a separate patch, and then get Bas to review/feedback it.

We're going to want to do this for all the compositor backends, or move it up to shared code.

It's not obvious what the best spot would be though, I'm sure Bas will have an opinion.

Comment 14

[David Zbarsky (:dzbarsky)]

Attachment: Fix masking

Comment 15

[David Zbarsky (:dzbarsky)]

Attachment: BasicCompositor needs to lock the TextureHost for the mask

See comment 13.

Comment 16

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 779434 [details] [diff] [review]
BasicCompositor needs to lock the TextureHost for the mask

Bas, we probably need this for all the Compositor backend.

Any thoughts on the best place to do this locking?

Comment 17

[David Zbarsky (:dzbarsky)]

Comment on attachment 779433 [details] [diff] [review]
Fix masking

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

::: gfx/layers/basic/BasicCompositor.cpp
@@ +255,5 @@
>    if (aMask) {
> +    NS_ASSERTION(matrix._11 == 1.0f && matrix._12 == 0.0f &&
> +                 matrix._21 == 0.0f && matrix._22 == 1.0f,
> +                 "Can only handle translations for mask transform");
> +    aDest->MaskSurface(SurfacePattern(aSource, EXTEND_CLAMP, matrix),

This assertion should be checking aMaskTransform.  Fixed locally.

Comment 18

[Nick Cameron [:nrc]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #16)
> Comment on attachment 779434 [details] [diff] [review]
> BasicCompositor needs to lock the TextureHost for the mask
> 
> Bas, we probably need this for all the Compositor backend.
> 
> Any thoughts on the best place to do this locking?

I don't think there is anywhere shared we can put it. I think it just needs to be like it is here, but for all compositors.

Comment 19

[Nick Cameron [:nrc]]

(In reply to David Zbarsky (:dzbarsky) from comment #17)
> Comment on attachment 779433 [details] [diff] [review]
> Fix masking
> 
> Review of attachment 779433 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/basic/BasicCompositor.cpp
> @@ +255,5 @@
> >    if (aMask) {
> > +    NS_ASSERTION(matrix._11 == 1.0f && matrix._12 == 0.0f &&
> > +                 matrix._21 == 0.0f && matrix._22 == 1.0f,
> > +                 "Can only handle translations for mask transform");
> > +    aDest->MaskSurface(SurfacePattern(aSource, EXTEND_CLAMP, matrix),
> 
> This assertion should be checking aMaskTransform.  Fixed locally.

I don't think this is right. Why can't we handle other kinds of transforms?

Comment 20

[David Zbarsky (:dzbarsky)]

(In reply to Nick Cameron [:nrc] from comment #19)
> (In reply to David Zbarsky (:dzbarsky) from comment #17)
> > Comment on attachment 779433 [details] [diff] [review]
> > Fix masking
> > 
> > Review of attachment 779433 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: gfx/layers/basic/BasicCompositor.cpp
> > @@ +255,5 @@
> > >    if (aMask) {
> > > +    NS_ASSERTION(matrix._11 == 1.0f && matrix._12 == 0.0f &&
> > > +                 matrix._21 == 0.0f && matrix._22 == 1.0f,
> > > +                 "Can only handle translations for mask transform");
> > > +    aDest->MaskSurface(SurfacePattern(aSource, EXTEND_CLAMP, matrix),
> > 
> > This assertion should be checking aMaskTransform.  Fixed locally.
> 
> I don't think this is right. Why can't we handle other kinds of transforms?

The MaskSurface api only takes an offset, not a transform matrix.  According to Jeff, we'll "need to transform the draw target and adjust the SurfacePattern's matrix to compensate".  This may not be perfect, but it is better than what we have currently.

Comment 21

[Nick Cameron [:nrc]]

(In reply to David Zbarsky (:dzbarsky) from comment #20)
> (In reply to Nick Cameron [:nrc] from comment #19)
> > (In reply to David Zbarsky (:dzbarsky) from comment #17)
> > > Comment on attachment 779433 [details] [diff] [review]
> > > Fix masking
> > > 
> > > Review of attachment 779433 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: gfx/layers/basic/BasicCompositor.cpp
> > > @@ +255,5 @@
> > > >    if (aMask) {
> > > > +    NS_ASSERTION(matrix._11 == 1.0f && matrix._12 == 0.0f &&
> > > > +                 matrix._21 == 0.0f && matrix._22 == 1.0f,
> > > > +                 "Can only handle translations for mask transform");
> > > > +    aDest->MaskSurface(SurfacePattern(aSource, EXTEND_CLAMP, matrix),
> > > 
> > > This assertion should be checking aMaskTransform.  Fixed locally.
> > 
> > I don't think this is right. Why can't we handle other kinds of transforms?
> 
> The MaskSurface api only takes an offset, not a transform matrix.  According
> to Jeff, we'll "need to transform the draw target and adjust the
> SurfacePattern's matrix to compensate".  This may not be perfect, but it is
> better than what we have currently.

Ah, sorry, I thought this was a transform on a mask _layer_ not a mask _surface_

Comment 22

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/98d1f6e62fab

Comment 23

[Nick Cameron [:nrc]]

(In reply to David Zbarsky (:dzbarsky) from comment #1)
> Created attachment 777371 [details] [diff] [review]
> Use an ImageSurface in CanvasClient2DUpdate
> 
> ...because that's what BasicCanvasLayer expects.

What actually expects an ImageSurface and why? We just pass that surface to CopyableCanvasLayer::UpdateSurface and I don't see anything which obviously requires an image surface. We should probably fix that requirement whatever it is and undo your change here because we don't always implement LockImageSurface (sadness) and so this breaks all the windows backends. If we can use a non-image surface that is desirable because ImageSurfaces are as slow as can be.

Comment 24

[Nick Cameron [:nrc]]

(In reply to Nick Cameron [:nrc] from comment #23)
> (In reply to David Zbarsky (:dzbarsky) from comment #1)
> > Created attachment 777371 [details] [diff] [review]
> > Use an ImageSurface in CanvasClient2DUpdate
> > 
> > ...because that's what BasicCanvasLayer expects.
> 
> What actually expects an ImageSurface and why? We just pass that surface to
> CopyableCanvasLayer::UpdateSurface and I don't see anything which obviously
> requires an image surface. We should probably fix that requirement whatever
> it is and undo your change here because we don't always implement
> LockImageSurface (sadness) and so this breaks all the windows backends. If
> we can use a non-image surface that is desirable because ImageSurfaces are
> as slow as can be.

The answer (via irc) is http://mxr.mozilla.org/mozilla-central/source/gfx/layers/CopyableCanvasLayer.cpp#78.

Comment 25

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/922e7f0eb53a

Comment 26

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

Comment on attachment 779434 [details] [diff] [review]
BasicCompositor needs to lock the TextureHost for the mask

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

Random drive by review.  To avoid duplicating the logic, I would have done something like this, but I don't know if we guarantee the same pointer would come back.  This code tells me, unlock what was locked, don't care how it got locked...

   DeprecatedTextureHost* toUnlock = nullptr;
   if (aEffectChain.mSecondaryEffects[EFFECT_MASK]) {
     EffectMask *effectMask = static_cast<EffectMask*>(aEffectChain.mSecondaryEffects[EFFECT_MASK].get());
+    toUnlock = static_cast<DeprecatedTextureHost*>(effectMask->mMaskTexture);
+    toUnlock->Lock();
...
   }

...

+  if (toUnlock) {
+    toUnlock->Unlock();
+    toUnlock = nullptr;
+  }
+

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/922e7f0eb53a

Comment 28

[David Zbarsky (:dzbarsky)]

All the patches here have landed.