Fix ASSERT failure in RenderGeomtryMap for SVGs

Fix ASSERT failure in RenderGeomtryMap for SVGs

SVGRenderSupport::mapLocalToContainer() was trying to apply transforms
in the incorrect order. Specifically, it would attempt to apply its
localToParentTransform before its localToBorderBoxTransform. This
was causing an ASSERT to fail when the computed transforms didn't
match up to those computed by RenderGeometryMap.

BUG=302719
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=167941

[hartmanng, 2014-01-20 20:21:17]

leviw@, it looks like you've worked in SVGRenderSupport before, could you please
take a look?

[fs, 2014-01-21 09:07:57]

https://codereview.chromium.org/143363004/diff/1/Source/core/rendering/svg/SV...
File Source/core/rendering/svg/SVGRenderSupport.cpp (right):

https://codereview.chromium.org/143363004/diff/1/Source/core/rendering/svg/SV...
Source/core/rendering/svg/SVGRenderSupport.cpp:78:
transformState.applyTransform(object->localToParentTransform());
This doesn't look right to me - I take it that in the relevant case, |object| is
the <g transform="scale(3)">, and hence that |parent| is the <svg ...>? If so,
then the currently local coordinate space is the coordinate space of the <g> -
from which we get to the local coordinate space of the <svg> by applying the
scale(3) - yes? Then to get to the "CSS (border) box" space by applying the
transform produced by the viewBox (mapping the content-box width/height to a
specified local coordinate space). And then RenderBox adds in any extras. Or
maybe it's me missing something? =)

[fs, 2014-01-21 09:34:25]

On 2014/01/21 09:07:57, fs wrote:
>
https://codereview.chromium.org/143363004/diff/1/Source/core/rendering/svg/SV...
> File Source/core/rendering/svg/SVGRenderSupport.cpp (right):
> 
>
https://codereview.chromium.org/143363004/diff/1/Source/core/rendering/svg/SV...
> Source/core/rendering/svg/SVGRenderSupport.cpp:78:
> transformState.applyTransform(object->localToParentTransform());
> This doesn't look right to me - I take it that in the relevant case, |object|
is
> the <g transform="scale(3)">, and hence that |parent| is the <svg ...>? If so,
> then the currently local coordinate space is the coordinate space of the <g> -
> from which we get to the local coordinate space of the <svg> by applying the
> scale(3) - yes? Then to get to the "CSS (border) box" space by applying the
> transform produced by the viewBox (mapping the content-box width/height to a
> specified local coordinate space). And then RenderBox adds in any extras. Or
> maybe it's me missing something? =)

Maybe what is wanted is rather something like:

TransformationMatrix matrix(object->localToParentTransform());
if (parent->isSVGRoot())
   matrix.multiply(toRenderSVGRoot(parent)->localToBorderBoxTransform());
transformState.applyTransform(matrix);

? (To cater to both "directions" of TransformState.)

[Stephen Chennney, 2014-01-21 22:12:10]

Where's the test? If there's an assertion, there should be some test that
generates it without this patch.

https://codereview.chromium.org/143363004/diff/1/Source/core/rendering/svg/SV...
File Source/core/rendering/svg/SVGRenderSupport.cpp (right):

https://codereview.chromium.org/143363004/diff/1/Source/core/rendering/svg/SV...
Source/core/rendering/svg/SVGRenderSupport.cpp:78:
transformState.applyTransform(object->localToParentTransform());
On 2014/01/21 09:07:57, fs wrote:
> This doesn't look right to me - I take it that in the relevant case, |object|
is
> the <g transform="scale(3)">, and hence that |parent| is the <svg ...>? If so,
> then the currently local coordinate space is the coordinate space of the <g> -
> from which we get to the local coordinate space of the <svg> by applying the
> scale(3) - yes? Then to get to the "CSS (border) box" space by applying the
> transform produced by the viewBox (mapping the content-box width/height to a
> specified local coordinate space). And then RenderBox adds in any extras. Or
> maybe it's me missing something? =)

I think that, like me, you are confused by the ambiguity of "applyTransform",
which apparently multiplies it to the current transform on the right. In other
words, the last transform to be applied in code will actually be the first one
applied mathematically. I don't know who wrote this, but it's as confusing as
can be.

[fs, 2014-01-22 08:17:52]

> I think that, like me, you are confused by the ambiguity of "applyTransform",
> which apparently multiplies it to the current transform on the right. In other
> words, the last transform to be applied in code will actually be the first one
> applied mathematically. I don't know who wrote this, but it's as confusing as
> can be.

I don't know if that's the main source of confusion (for me). If the result here
is actually build "left to right", then it seems odd that the method on the
parent is called _after_ the accumulation of the current node, since that would
then logically continue applying from the right...

Request for test seconded! =) Preferably one that also inspires confidence in
the change (after all - there's two sides to an equality...).

[hartmanng, 2014-01-22 15:04:40]

On 2014/01/21 22:12:10, Stephen Chenney wrote:
> Where's the test? If there's an assertion, there should be some test that
> generates it without this patch.

Good point. I've added a layout test which crashes before this fix.


On 2014/01/22 08:17:52, fs wrote:
> Request for test seconded! =) Preferably one that also inspires confidence in
> the change (after all - there's two sides to an equality...).

Hopefully this one inspires confidence, but if not, please let me know :)

[fs, 2014-01-22 15:24:16]

On 2014/01/22 15:04:40, hartmanng wrote:
> On 2014/01/22 08:17:52, fs wrote:
> > Request for test seconded! =) Preferably one that also inspires confidence
in
> > the change (after all - there's two sides to an equality...).
> 
> Hopefully this one inspires confidence, but if not, please let me know :)

In more clear terms, what I was getting at was a TC that exercises the code-path
(in one way or another) giving an observably correct result. (I.e. how to know
that not both LHS and RHS of the comparison is now wrong...).

If schenney (or other owner) is happy this way, then by all means ignore my
ramblings =)

[hartmanng, 2014-01-29 19:40:42]

On 2014/01/22 15:24:16, fs wrote:
> In more clear terms, what I was getting at was a TC that exercises the
code-path
> (in one way or another) giving an observably correct result. (I.e. how to know
> that not both LHS and RHS of the comparison is now wrong...).
> 
> If schenney (or other owner) is happy this way, then by all means ignore my
> ramblings =)


Sorry for the long delay on this one. I may be misinterpreting again, but by
"observable", I assume you mean a test with visible results? If so, I've added
some visible content to the test, so the pixel results should be able to verify
the actual on-screen position of the svg elements.

As for testing LHS vs RHS, to be honest, I'm not quite sure how to show that in
a layout test. Did you have an idea in mind, by any chance?


@schenney, wdyt?

[f(malita), 2014-01-29 21:19:37]

Note that the root issue here is foreignObject content triggering layer creation
(
textarea in this case).

SVG currently doesn't support descendant layers (http://crbug.com/87072), and
this is but one of the many symptoms of a fundamental limitation. Even if the
ASSERT gets silenced, you'll find that the resulting layer is quite borked for
various other reasons.

[Stephen Chennney, 2014-01-29 22:13:27]

On 2014/01/29 21:19:37, Florin Malita wrote:
> Note that the root issue here is foreignObject content triggering layer
creation
> (
> textarea in this case).
> 
> SVG currently doesn't support descendant layers (http://crbug.com/87072), and
> this is but one of the many symptoms of a fundamental limitation. Even if the
> ASSERT gets silenced, you'll find that the resulting layer is quite borked for
> various other reasons.

We can put svg inside foreign object, right? That would allow a red vs. green
test.

The way to test matrix multiply order would be some combinations of transforms
that are not order invariant. So a scale then a transform looks different to a
transform then a scale.

[fs, 2014-01-30 10:27:15]

On 2014/01/29 19:40:42, hartmanng wrote:
> On 2014/01/22 15:24:16, fs wrote:
> > In more clear terms, what I was getting at was a TC that exercises the
> code-path
> > (in one way or another) giving an observably correct result. (I.e. how to
know
> > that not both LHS and RHS of the comparison is now wrong...).
> > 
> > If schenney (or other owner) is happy this way, then by all means ignore my
> > ramblings =)
> 
> 
> Sorry for the long delay on this one. I may be misinterpreting again, but by
> "observable", I assume you mean a test with visible results? If so, I've added
> some visible content to the test, so the pixel results should be able to
verify
> the actual on-screen position of the svg elements.
>
> As for testing LHS vs RHS, to be honest, I'm not quite sure how to show that
in
> a layout test. Did you have an idea in mind, by any chance?

[Sorry for not being clear enough =( - I plead "Swedish chef" (will need to
tweak the deborkenizer it seems...).]

Did the on-screen position change with the fix? If so, the test illustrating
that would be good (and "observable"). Other observable tests would be ones that
provide the results of the computation such that it can be compared (in one way
or another - preferably to the result of the other code-path. If no such exist,
well, then bummer...
 
Anyway, I took a look at the TC, and it seems to me that the fix should rather
be in SVGRenderSupport::pushMappingToContainer (which is what's used to produce
the RHS in the ASSERT). The easiest way to motivate this is probably by saying
"set a breakpoint in RenderSVGContainer::paint and look at the GC transform
before and after the local transform is applied". The slightly more complex
route to enlightenment would look at how we have the viewBox transform (V,
composed of a scale and a translation - in an order which is irrelevant to this
issue), and the local transform (of the <g>) L=scale(3). Now intuitively (I
hope), when L is concatenated to V, the origin remains the same - i.e. the
translation component from V is not affected by the scaling operation.

Transferring this to the case in question (which applies the transform to a
point [the origin even] in the coordinate space of the <textarea>), we can see
that we get a sequence like: p0 = <0, 0>; p1 = p0 * matrix1; p2 = p1 * matrix2;
etc. (a bit dependent on how points and matrices are represented, but let's
ignore that for simplicity...). So what we have is the point "bubbling" up the
ancestors while applying their local->parent transforms (no surprises). When the
point reaches the <g>, it should be scaled (by 3, giving p2 = <p1x, p1y> * 3)
and then there's the (somewhat confusing) application of V, yielding p3 = <p2x,
p2y> * v_scale + v_translation. Notice how v_translation is not subject to
scaling by 3. With your change you'd get those two in differing order, and hence
v_translation would be subject to scaling.

Now back to the meatballs!

[hartmanng, 2014-02-07 02:02:42]

Thanks for the feedback, everyone.


On 2014/01/29 22:13:27, Stephen Chenney wrote:
> We can put svg inside foreign object, right? That would allow a red vs. green
> test.

It looks like we can, afaict, but when I do, everything appears to be working.
There are no more ASSERT crashes, and visually, combinations of transforms
(scale then translate, and vice versa) look how I'd expect.


@fs, your suggestion sounds reasonable, but again, now that I've removed the
limitation Florin described, I'm not actually seeing a problem anymore.


Is it possible that it was the only issue? Or I'm just not figuring out how to
repro this properly? (The latter is certainly possible - I'm very new to SVG).


Thanks to everyone for your help and patience :)

[Stephen Chennney, 2014-02-26 19:56:37]

It's been forever but now that I'm more familiar with why this matters, let's go
ahead and land it.

LGTM

[hartmanng, 2014-02-26 20:17:14]

The CQ bit was checked by hartmanng@chromium.org

[commit-bot: I haz the power, 2014-02-26 20:17:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hartmanng@chromium.org/143363004/220001

[hartmanng, 2014-02-26 20:17:33]

thanks!

[commit-bot: I haz the power, 2014-02-26 23:27:23]

Change committed as 167941

[hartmanng, 2014-04-02 22:51:53]

A revert of this CL has been created in
https://codereview.chromium.org/223063002/ by hartmanng@chromium.org.

The reason for reverting is: This broke rendering in Google Presentations
(https://code.google.com/p/chromium/issues/detail?id=359142)..