1145730 - "Assertion failure: Is2D() (Matrix is not a 2D affine transform)"

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: How did we end up with a 3D transform here?!: 'maskTransform.Is2D()', file gfx/layers/opengl/CompositorOGL.cpp, line 951

Assertion failure: Is2D() (Matrix is not a 2D affine transform), at Matrix.h:435

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Botond Ballo [:botond]]

I can repro the assertion failure (for me it happens in BasicCompositor rather than CompositorOGL, because I have hardware acceleartion disabled, but it's pretty clear it's the same cause). I'll investigate.

Comment 3

[Botond Ballo [:botond]]

Doing a bit of code archaeology, bug 716439 comment 172 suggests that at the time the assertion was written, the intention was to make sure the transform associated with a mask layer can't be 3D:

> > @@ +346,5 @@
> > > +               (GLint)(mProfile.mTextureCount - 1));
> > > +
> > > +    gfxMatrix maskTransform;
> > > +    bool maskIs2D = aMaskLayer->GetEffectiveTransform().CanDraw2D(&maskTransform);
> > > +    NS_ASSERTION(maskIs2D, "How did we end up with a 3D transform here?!");
> > 
> > Is not clear that LoadMask is for 2D transforms only. I can't see from the
> > context of this patch where that is checked.
> > 
> The way ComputeEffectiveTransform works, we should never get a 3D transform
> here, and we make sure where we create the mask layer that it's transform is
> 2D. This is documented (briefly) on Layers::SetMaskLayer. I've added a few
> words to the documentation of this method about it too. Let me know if you
> think we need more.

I'm not sure yet whether that intention has changed (and the assertion and surrounding code just didn't get updated), or if this is scenario that hadn't been considered in the first place.

Comment 4

[Botond Ballo [:botond]]

This appears to be a regression from bug 1086723; backing out the change made in that bug to Layer::ComputeEffectiveTransformForMaskLayer() makes this assertion failure go away.

Comment 5

[Botond Ballo [:botond]]

Here's a summary of what's happening:

  - The compositor expects that a mask layer's effective transform is 2D,
    and asserts to this effect.

  - Prior to bug 1086723, a mask layer's effective transform was calculated 
    as the product of:
     
      - An incoming "transform to surface", which the calling code ensures is 2D.

      - The mask layer's own transform, which is also 2D.

    The transform of the layer that owns the mask layer does not factor into
    the mask layer's transform, and it can safely be 3D.

  - In bug 1086723, the calculation was modify to also multiply in the following
    quantity for the owning layer:
 
      GetTransform().Inverse() * GetLocalTransform()

    The idea here is to get at the async portion of the shadow transform, which 
    should also be 2D.

  - In this test case, the owning layer's transform is 3D. The async transform
    is 2D (in fact, it's the identity transform), but rounding error during
    the above calculation causes the result to be not the identity matrix,
    but an ever so slightly different 3D matrix:

      [  1             2.23517e-08  -5.96046e-08  0; 
         7.45058e-09   1            -2.98023e-08  0; 
        -2.98023e-08   1.49012e-08   1            0; 
        -3.8147e-06   -1.90735e-06   0            1; ]

    As a result, the mask layer's effective transform becomes 3D, and the
    assertion fires.

Comment 6

[Botond Ballo [:botond]]

While I was discussing possible ways to fix the rounding error with Markus, we realized that there was also a correctness problem with the fix for bug 1086723: it's incorrect to apply all components of the async transform on the owning layer, to the mask layer. We want to apply an async transform induced by a fixed-position adjustment, but not one induced by async scrolling (of the owning layer itself) or by OMTA. (Markus filed bug 1148582 to demonstrate this).

As it happens, Kip's original patch in bug 1086723 only applied an async transform for a fixed-position adjustment, but during review the approach was changed to apply the whole async transform. In hindsight, it seems the original approach is the correct one.

I verified that using that original approach fixes this assertion, while not regressing the test case in bug 1086723. I'd like to also verify that it fixes bug 1148582, but it appears I need to fix bug 1148581 before being able to even test bug 1148582.

Comment 7

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #6)
> I'd like to also verify that it
> fixes bug 1148582, but it appears I need to fix bug 1148581 before being
> able to even test bug 1148582.

Markus has since posted another testcase in bug 1148582 which is not fixed by this patch alone - it needs a more comprehensive rework of how clip rects and mask layers are handled. That rework should also fix bug 1148581.

I don't want to hold up this fix on that rework - let's just land it now.

Comment 8

[Botond Ballo [:botond]]

Attachment: MozReview Request: bz://1145730/botond

/r/6493 - Bug 1145730 - Restrict the async transforms applied to mask layers to those caused by fixed-position adjustment. r=kats

Pull down this commit:

hg pull -r c7948dad4367a7a682478c344fd1d8668b33ef1d https://reviewboard-hg.mozilla.org/gecko/

Comment 9

[Botond Ballo [:botond]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a2455338137

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8586983 [details]
MozReview Request: bz://1145730/botond

https://reviewboard.mozilla.org/r/6491/#review5501

Ship It!

Comment 11

[Botond Ballo [:botond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e58c174b505c

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e58c174b505c

Comment 13

[Botond Ballo [:botond]]

Comment on attachment 8586983 [details]
MozReview Request: bz://1145730/botond

Requesting uplift because the fix ended up being a correctness fix, not just a debug assertion fix. The fix is low-impact, but it's also correspondingly low-risk.

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  Bug 1086723.

User impact if declined: 
  In some cases where content inside a container with border-radius
  is async-scrolled or async-animated, the mask layer for the
  border-radius is incorrectly scrolled or animated along with the
  content.

Testing completed: 
  On m-c since 2015-04-06.

Risk to taking this patch (and alternatives if risky): 
  Low - the only affected scenario involves a combination of
  border-radius and async scrolling or async animation.

String or UUID changes made by this patch:
  None.

Comment 14

[bhavana bajaj [:bajaj]]

Comment on attachment 8586983 [details]
MozReview Request: bz://1145730/botond

Approving given the low risk here.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/72b708ce084c

Does this need uplift to Aurora/Beta?

Comment 16

[Botond Ballo [:botond]]

The patch fixes two issues: a correctness bug, and a debug assertion.

The correctness bug is only triggered by APZ or OMTA, so it only affects B2G.

The debug assertion affects all platforms, but it's harmless in release builds, and it's fairly edge-case.

Considering this, I would lean towards not uplifting to aurora/beta.

Comment 18

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1145730 - Restrict the async transforms applied to mask layers to those caused by fixed-position adjustment. r=kats