1385239 - incorrect SVG mask with transformed/filtered graphic object

Status: RESOLVED FIXED

(Core :: SVG, defect, P3)

Description

[kpreisert]

Attachment: test.svg

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170628075643

Steps to reproduce:

Open the attached SVG file in Firefox and compare it to other browsers.


Actual results:

It shows a red background and the text, and a little gray box in the top left corner.


Expected results:

The gray box should be positioned such that the text is directly on top of it

Comment 1

[Alice0775 White]

[Tracking Requested - why for this release]: SVG rendering regression


1st regression window(no gray box is drawn):
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=7d9fb1f4c14ec4e5ed9c8b575e70c2f39a57c983&tochange=655021ab4e0729879f20ad05979e9dad7fb0b24e

Regressed by: Bug 1224207

2nd regression window(a little gray box is drawn on the top left corner):
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=cc308a73ad05ab7b11872d8640f9711a99775094&tochange=5f8ed78861489be8477b1689afb361c3750ff263

Regressed by: Bug 1348430


:@cjku,
your patch seems to cause this problem.

Comment 2

[u459114]

Look like we do not pass this translation into filter after my change in those two bugs.
<g transform="translate(10, 50)">

I will look into it. Thank you for reporting this issue.

Comment 3

[Julien Cristau [:jcristau]]

Too late for 55.

Comment 4

[u459114]

Attachment: test-2.svg

Comment 5

[u459114]

Attachment: test-2.svg

Expected behavior: lime background color.
Actual behavior: red background color

Comment 6

[u459114]

Attachment: Bug 1385239 - Part 1. Correct the transform matix of the given draw target in nsFilterInstance::Render.

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

Comment 7

[u459114]

Attachment: Bug 1385239 - Part 3. A test case of putting a translated filtered element inside an SVG mask.

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

Comment 8

[u459114]

Comment on attachment 8900108 [details]
Bug 1385239 - Part 1. Correct the transform matix of the given draw target in nsFilterInstance::Render.

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

Comment 9

[Markus Stange [:mstange]]

Taking just the translation from mPaintTransform seems like it's not the right thing to do but happens to work in this case. Please describe the various transforms that are involved here in more detail and try to find a way to use mPaintTransform that uses the whole mPaintTransform instead of just its translation component; or alternatively, find a good justification for why just taking the translation is the right thing to do.

Comment 10

[u459114]

Attachment: Bug 1385239 - Part 1. Pass gfxContext, instead of DrawTarget, into nsFilterInstance::PaintFilteredFrame.

This change make the next patch free from doing Matrix to gfxMatrix conversion.

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

Comment 11

[u459114]

Attachment: Bug 1385239 - Part 2. Remove aTransform parameter from PaintFilteredFrame.

There are two callers of nsFilterInstance::PaintFilteredFrame:
1. nsSVGUtils::PaintFrameWithEffects at [1]
   This function is used while painting a filtered element on a path which
   display item painting is not supported yet, such as drawing elemnts inside a
   indirect painted SVG object, such as a mask, a clipPath or a gradient object.

   Let's say we have a masked element, which refers to an SVG mask, and there is
   a filtered element inside that SVG mask.
   Using nsFilterInstance::PaintFilteredFrame to paint that filtered frame in
   the mask, we have to pass a gfxContext and a transform matrix to it. The
   transform of the gfxContext 'target' that we pass in consists of a tranform
   from the referenced frame, of the masked frame, to the masked frame. We also
   pass in a transform matrix 'aTransform', this matrix contains a transform
   from the the masked frame to the filtered frame in *device units*, which
   means it contains css-to-dev-px scaling factor.

2. nsSVGIntegrationUtils::PaintFilter at [2]
   This function is used by normal display item painting.

   The same, we pass a gfxContext 'context' and a transform matrix 'tm' into
   nsFilterInstance::PaintFilteredFrame. The transform matrix of 'context'
   consists of the transform from the referenced frame, of the filtered frame,
   to this filtered frame, but the scale factor was taken out . The transform
   matrix 'tm' we pass in contains scale transform from the referenced frame to
   the filtered frame in *device unit*.

Inside nsFilterInstance::PaintFilteredFrame, we treat the transform matrix of
'aCtx' and 'aTransform' as parameters we pass in in #2 caller. So it can be
failed in #1.

In this patch, I unified the definition of parameters of
nsFilterInstance::PaintFilteredFrame:
1. nsFilterInstance::PaintFilteredFrame(aCtx): the transform matrix of aCtx
   should be a transfrom from the referenced frame to the filtered frame in
   *css units*. Originally, the aCtx we passed in #1 is in device units, which
   should be fixed; the aCtx we passed in #2 does not even inlcude css scaling
   factor, need be fixed too.
2. nsFilterInstance::PaintFilteredFrame(aTransform): this transform matrix
   should contain only scaling factor in device units. And I removed it in the
   end since I found we can get this value easily right inside the callee.

[1]
https://hg.mozilla.org/mozilla-central/file/ef585ac7c476/layout/svg/nsSVGUtils.cpp#l857

[2]
https://hg.mozilla.org/mozilla-central/file/ef585ac7c476/layout/svg/nsSVGIntegrationUtils.cpp#l1114

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

Comment 12

[u459114]

Comment on attachment 8900588 [details]
Bug 1385239 - Part 3. A test case of putting a translated filtered element inside an SVG mask.

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

Comment 13

[u459114]

Comment on attachment 8901178 [details]
Bug 1385239 - Part 1. Pass gfxContext, instead of DrawTarget, into nsFilterInstance::PaintFilteredFrame.

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

Comment 14

[u459114]

Comment on attachment 8901179 [details]
Bug 1385239 - Part 2. Remove aTransform parameter from PaintFilteredFrame.

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

Comment 15

[u459114]

Comment on attachment 8900588 [details]
Bug 1385239 - Part 3. A test case of putting a translated filtered element inside an SVG mask.

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

Comment 16

[u459114]

Comment on attachment 8901179 [details]
Bug 1385239 - Part 2. Remove aTransform parameter from PaintFilteredFrame.

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

Comment 17

[u459114]

Comment on attachment 8900588 [details]
Bug 1385239 - Part 3. A test case of putting a translated filtered element inside an SVG mask.

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

Comment 18

[Markus Stange [:mstange]]

Comment on attachment 8901178 [details]
Bug 1385239 - Part 1. Pass gfxContext, instead of DrawTarget, into nsFilterInstance::PaintFilteredFrame.

https://reviewboard.mozilla.org/r/172652/#review178196

Comment 19

[Markus Stange [:mstange]]

Thanks! I'll probably review this patch next week.

Comment 20

[Markus Stange [:mstange]]

Comment on attachment 8901179 [details]
Bug 1385239 - Part 2. Remove aTransform parameter from PaintFilteredFrame.

https://reviewboard.mozilla.org/r/172654/#review182758

I prefer this patch very much to the previous one. Thanks! And thanks again for the detailed explanation.

::: layout/svg/nsSVGUtils.cpp:858
(Diff revision 3)
> +    // Take out css-to-dev-px scaling from the current transform of 'target'
> +    // since nsFilterInstance::PaintFilteredFrame does not expect it.

Could you rephrase the comment like this? (Is what I'm saying there correct?)

'target' is currently scaled such that its user space units are CSS pixels (SVG user space units). But PaintFilteredFrame expects it to be scaled in such a way that its user space units are device pixels. So we have to adjust the scale.

::: layout/svg/nsSVGUtils.cpp:862
(Diff revision 3)
> +
> +    // Take out css-to-dev-px scaling from the current transform of 'target'
> +    // since nsFilterInstance::PaintFilteredFrame does not expect it.
> +    gfxMatrix reverseScaleMatrix = nsSVGUtils::GetCSSPxToDevPxMatrix(aFrame);
> +    DebugOnly<bool> invertible = reverseScaleMatrix.Invert();
> +    target->SetMatrix(reverseScaleMatrix *aTransform *

missing space between * and aTransform

Comment 21

[Markus Stange [:mstange]]

Comment on attachment 8900588 [details]
Bug 1385239 - Part 3. A test case of putting a translated filtered element inside an SVG mask.

https://reviewboard.mozilla.org/r/171988/#review182760

Comment 22

[u459114]

Comment on attachment 8901178 [details]
Bug 1385239 - Part 1. Pass gfxContext, instead of DrawTarget, into nsFilterInstance::PaintFilteredFrame.

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

Comment 23

[u459114]

Comment on attachment 8901179 [details]
Bug 1385239 - Part 2. Remove aTransform parameter from PaintFilteredFrame.

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

Comment 24

[u459114]

Comment on attachment 8900588 [details]
Bug 1385239 - Part 3. A test case of putting a translated filtered element inside an SVG mask.

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

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/45278d597320
Part 1. Pass gfxContext, instead of DrawTarget, into nsFilterInstance::PaintFilteredFrame. r=mstange
https://hg.mozilla.org/integration/autoland/rev/9a6413d1fd0a
Part 2. Remove aTransform parameter from PaintFilteredFrame. r=mstange
https://hg.mozilla.org/integration/autoland/rev/114d87a3943b
Part 3. A test case of putting a translated filtered element inside an SVG mask. r=mstange

Comment 26

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

https://hg.mozilla.org/mozilla-central/rev/45278d597320
https://hg.mozilla.org/mozilla-central/rev/9a6413d1fd0a
https://hg.mozilla.org/mozilla-central/rev/114d87a3943b

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Can this ride the 57 train or should we consider it for Beta backport? Note that we're spinning the RC build for 56 next week.

Comment 28

[u459114]

Comment on attachment 8900588 [details]
Bug 1385239 - Part 3. A test case of putting a translated filtered element inside an SVG mask.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1348430
[User impact if declined]: a transformed object inside an svg mask will be positioned at wrong place.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: Part 1 ~ Part 3.
[Is the change risky?]: no
[Why is the change risky/not risky?]: Small change covered by a reftest.
[String changes made/needed]:NA

Comment 29

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8900588 [details]
Bug 1385239 - Part 3. A test case of putting a translated filtered element inside an SVG mask.

Fix for SFG regression from 54, has new test coverage, let's uplift it for beta 12.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b663bd73db28
https://hg.mozilla.org/releases/mozilla-beta/rev/45497624d792
https://hg.mozilla.org/releases/mozilla-beta/rev/acd7c5826e6a