BasicCompositor fixes

RESOLVED FIXED in mozilla25

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dzbarsky, Assigned: dzbarsky)

Tracking

unspecified
mozilla25
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 777371 [details] [diff] [review]
Use an ImageSurface in CanvasClient2DUpdate

...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+
(Assignee)

Comment 2

5 years ago
Created attachment 777418 [details] [diff] [review]
Use RGBARectProgram in nsChildView
Attachment #777418 - Flags: review?(matt.woodrow)
Attachment #777418 - Flags: review?(matt.woodrow) → review+
(Assignee)

Comment 5

5 years ago
Created attachment 778875 [details] [diff] [review]
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?
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?).
(Assignee)

Comment 7

5 years ago
Created attachment 778884 [details] [diff] [review]
Remove duplicate reftests

Looks like you may have checked these in by accident.
Attachment #778884 - Flags: review?(roc)
(Assignee)

Comment 8

5 years ago
Created attachment 778902 [details] [diff] [review]
Fix needsYFlip

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+
(Assignee)

Comment 12

5 years ago
Created attachment 779403 [details] [diff] [review]
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.
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.
(Assignee)

Comment 14

5 years ago
Created attachment 779433 [details] [diff] [review]
Fix masking
Attachment #779403 - Attachment is obsolete: true
Attachment #779403 - Flags: review?(matt.woodrow)
Attachment #779433 - Flags: review?(matt.woodrow)
(Assignee)

Comment 15

5 years ago
Created attachment 779434 [details] [diff] [review]
BasicCompositor needs to lock the TextureHost for the mask

See comment 13.
Attachment #779434 - Flags: review?
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+
(Assignee)

Comment 17

5 years ago
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

5 years ago
(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

5 years ago
(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?
(Assignee)

Comment 20

5 years ago
(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

5 years ago
(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_

Updated

5 years ago
Depends on: 897409

Comment 23

5 years ago
(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)

Comment 24

5 years ago
(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;
+  }
+
(Assignee)

Updated

5 years ago
Blocks: 865104
(Assignee)

Comment 28

5 years ago
All the patches here have landed.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Whiteboard: [leave open]
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.