1391499 - Apply correct scale transform with nested transform divs

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Description

[Peter Chang[:pchang]]

With nested transform divs, layers-free mode got the wrong result from the following test case.

layout/reftests/transform/compound-1-ref.html

[Test case]
  <div style="left:0px; top:200px;">                                                                                                                                                                                                                                          
    <div style="-moz-transform: translate(100px);">                                                                                                                                                                                                                           
      <div style="-moz-transform:">                                                                                                                                                                                                                                           
        <div style="-moz-transform: scale(2) rotate(90deg);">                                                                                                                                                                                                                 
            <div class="testr">                                                                                                                                                                                                                                   
            </div>                                                                                                                                                                                                                                                            
        </div>                                                                                                                                                                                                                                                                
      </div>                                                                                                                                                                                                                                                                  
    </div>                                                                                                                                                                                                                                                                    
  </div>

Comment 1

[Peter Chang[:pchang]]

We need to apply inherited scale from parent to get correct transform in layers-free mode.

Comment 2

[Peter Chang[:pchang]]

Attachment: Bug 1391499 - Apply the inherited scale from ancestors,

Review commit: https://reviewboard.mozilla.org/r/170180/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/170180/

Comment 3

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

Comment on attachment 8898787 [details]
Bug 1391499 - Apply the inherited scale from ancestors,

https://reviewboard.mozilla.org/r/170180/#review175460

Overall looks reasonable but there's enough nits/fixes that I'd like to see an updated version. Thanks!

::: gfx/layers/wr/StackingContextHelper.h:91
(Diff revision 1)
>  private:
>    wr::DisplayListBuilder* mBuilder;
>    LayerPoint mOrigin;
>    gfx::Matrix4x4 mTransform;
> +
> +  float mXScale, mYScale;

One declaration per line please

::: gfx/layers/wr/StackingContextHelper.cpp:16
(Diff revision 1)
> -  : mBuilder(nullptr)
> +  : mBuilder(nullptr),
> +    mXScale(1.0),
> +    mYScale(1.0)

Please line up the commas with the colon
  : mBuilder(nullptr)
  , mXScale(1.0)
  , mYScale(1.0)

::: gfx/layers/wr/StackingContextHelper.cpp:28
(Diff revision 1)
>    : mBuilder(&aBuilder)
>  {

You should initialize mXScale and mYScale in all four constructors (and definitely in the one you're modifying, because right now there are codepaths where it can be left as uninitialized and then can be later read).

::: gfx/layers/wr/StackingContextHelper.cpp:91
(Diff revision 1)
> +    aTransformPtr->PostScale(aParentSC.mXScale, aParentSC.mYScale, 1.0);
> +    aTransformPtr->NudgeToIntegersFixedEpsilon();

I don't really like that you're modifying the passed-in argument as a side-effect here. Since this function later makes a copy of *aTransformPtr into mTransform anyway, can we move that earlier, and then make these changes on mTransform? That way it won't unexpectedly modify the caller's Matrix.

::: gfx/layers/wr/StackingContextHelper.cpp:94
(Diff revision 1)
> +
> +    // Apply the inherited scale from parent
> +    aTransformPtr->PostScale(aParentSC.mXScale, aParentSC.mYScale, 1.0);
> +    aTransformPtr->NudgeToIntegersFixedEpsilon();
> +
> +    gfxSize scale = ThebesMatrix(aTransformPtr->As2D()).ScaleFactors(true);

ThebesMatrix is now just a typedef for MatrixDouble, so please use that directly. Likewise s/gfxSize/SizeDouble/

::: gfx/layers/wr/StackingContextHelper.cpp:95
(Diff revision 1)
> +    if (fabs(scale.width) < 1e-8 || fabs(scale.height) < 1e-8) {
> +      scale = gfxSize(1.0, 1.0);

It's not clear to me why this is needed. Is a scale of 0 not valid here? If so please add a comment why. Also instead of hard-coding 1e-8 here I'd prefer if you used one of the existing FuzzyEquals methods in the codebase, for example the FuzzyEqualsAdditive in MFBT.

Comment 4

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> ThebesMatrix is now just a typedef for MatrixDouble, so please use that
> directly.

Oh, sorry, I didn't realize ThebesMatrix was the function to convert from Matrix to MatrixDouble. Using that is ok.

Comment 5

[Peter Chang[:pchang]]

Now I couldn't reproduce the try failures with latest nightly, I will check which bug fixed this problem later.

Comment 6

[Peter Chang[:pchang]]

Comment on attachment 8898787 [details]
Bug 1391499 - Apply the inherited scale from ancestors,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170180/diff/1-2/

Comment 7

[Peter Chang[:pchang]]

Comment on attachment 8898787 [details]
Bug 1391499 - Apply the inherited scale from ancestors,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170180/diff/2-3/

Comment 8

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

Comment on attachment 8898787 [details]
Bug 1391499 - Apply the inherited scale from ancestors,

https://reviewboard.mozilla.org/r/170180/#review177952

Much better, thanks. I'm kind of wondering if this is a bug in webrender though, it seems like if stacking contexts are nested the semantics should be that the scale gets inherited down automatically. It seems odd that we need to do this manually on our side.

Comment 9

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 342ee7d2072c -d f06ff175be4f: rebasing 416228:342ee7d2072c "Bug 1391499 - Apply the inherited scale from ancestors, r=kats" (tip)
merging gfx/layers/wr/StackingContextHelper.cpp
merging gfx/layers/wr/StackingContextHelper.h
warning: conflicts while merging gfx/layers/wr/StackingContextHelper.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 10

[Peter Chang[:pchang]]

Comment on attachment 8898787 [details]
Bug 1391499 - Apply the inherited scale from ancestors,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170180/diff/3-4/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/22d7763100aa
Apply the inherited scale from ancestors, r=kats

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for build bustage on Windows at gfx/layers/wr/StackingContextHelper.cpp(105): 'Size': undeclared identifier:

https://hg.mozilla.org/integration/autoland/rev/c137b3d03017b8449ae19bfc58752aa688888ab8

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=22d7763100aad8ac76f9c36b84e67f6349371fac&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=126387680&repo=autoland

10:20:20     INFO -  z:/build/build/src/gfx/layers/wr/StackingContextHelper.cpp(105): error C2065: 'Size': undeclared identifier
10:20:20     INFO -  z:/build/build/src/gfx/layers/wr/StackingContextHelper.cpp(105): error C2146: syntax error: missing ';' before identifier 'scale'
10:20:20     INFO -  z:/build/build/src/gfx/layers/wr/StackingContextHelper.cpp(105): error C2065: 'scale': undeclared identifier
10:20:20     INFO -  z:/build/build/src/gfx/layers/wr/StackingContextHelper.cpp(108): error C2065: 'scale': undeclared identifier
10:20:20     INFO -  z:/build/build/src/gfx/layers/wr/StackingContextHelper.cpp(108): error C2228: left of '.width' must have class/struct/union
10:20:20     INFO -  z:/build/build/src/gfx/layers/wr/StackingContextHelper.cpp(108): note: type is 'unknown-type'

etc.

Comment 13

[Peter Chang[:pchang]]

Comment on attachment 8898787 [details]
Bug 1391499 - Apply the inherited scale from ancestors,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170180/diff/4-5/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a36884ff708d
Apply the inherited scale from ancestors, r=kats

Comment 15

[Peter Chang[:pchang]]

I just fixed the build failure on windows in comment 13.

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 17

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

So I backed out 4 patches locally: the ones from bug 1399050, bug 1394308, and bug 1391499, and ran the reftest layout/reftests/transform/compound-1a.html in layers-free WR. What I see is that the reference image is shifted over to the right by around 200px but other than looks fine. Is this the same as what you were seeing when you wrote comment 0?

I'm wondering if there's a better way to go about fixing this, because extracting the scale here and then manually applying it to everything is turning out to be quite complicated (e.g. in bug 1399043).

Comment 18

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

(Clearing needinfo here, let's keep the discussion in bug 1400034)