Closed Bug 895116 Opened 6 years ago Closed 6 years ago

BasicCompositor fixes

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: dzbarsky, Assigned: dzbarsky)

References

Details

Attachments

(6 files, 2 obsolete files)

No description provided.
...because that's what BasicCanvasLayer expects.
Assignee: nobody → dzbarsky
Status: NEW → ASSIGNED
Attachment #777371 - Flags: review?(matt.woodrow)
Attachment #777371 - Flags: review?(matt.woodrow) → review+
Attachment #777418 - Flags: review?(matt.woodrow)
Attachment #777418 - Flags: review?(matt.woodrow) → review+
Attached patch Hack needsYFlip (obsolete) — Splinter Review
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?
Attachment #778875 - Flags: review?(matt.woodrow)
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.
Attachment #778884 - Flags: review?(roc)
Attached patch Fix needsYFlipSplinter Review
Thanks for the pointers Matt.  It made this much easier to track down.
Attachment #778875 - Attachment is obsolete: true
Attachment #778875 - Flags: review?(matt.woodrow)
Attachment #778902 - Flags: review?(matt.woodrow)
Comment on attachment 778902 [details] [diff] [review]
Fix needsYFlip

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

\o/
Attachment #778902 - Flags: review?(matt.woodrow) → review+
Attached patch Fix masking (obsolete) — Splinter Review
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.
Attachment #779403 - Flags: review?(matt.woodrow)
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.
Attached patch Fix maskingSplinter Review
Attachment #779403 - Attachment is obsolete: true
Attachment #779403 - Flags: review?(matt.woodrow)
Attachment #779433 - Flags: review?(matt.woodrow)
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?
Attachment #779434 - Flags: review? → review?(bas)
Attachment #779433 - Flags: review?(matt.woodrow) → review+
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_
Depends on: 897409
(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.
Flags: needinfo?(dzbarsky)
(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.
Flags: needinfo?(dzbarsky)
Attachment #779434 - Flags: review?(bas) → review+
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;
+  }
+
Blocks: 865104
All the patches here have landed.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.