...because that's what BasicCanvasLayer expects.

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

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

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 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?).

Looks like you may have checked these in by accident.

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

Comment on attachment 778902 [details] [diff] [review] Fix needsYFlip Review of attachment 778902 [details] [diff] [review]: ----------------------------------------------------------------- \o/

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

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

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 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.

See comment 13.

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 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.

(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.

(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?

(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.

(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_

(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.

(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 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; + } +

All the patches here have landed.