Scaling SVG or text with filters should not blur

RESOLVED FIXED in Firefox 54

Status

()

defect
RESOLVED FIXED
4 years ago
8 months ago

People

(Reporter: xidorn, Assigned: u459114)

Tracking

(Blocks 1 bug)

Trunk
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 affected, firefox54 fixed)

Details

(Whiteboard: [parity-blink][parity-webkit])

Attachments

(10 attachments, 2 obsolete attachments)

104 bytes, text/html
Details
59 bytes, text/x-review-board-request
mstange
: review+
Details
59 bytes, text/x-review-board-request
mstange
: review+
Details
59 bytes, text/x-review-board-request
mstange
: review+
Details
59 bytes, text/x-review-board-request
mstange
: review+
Details
59 bytes, text/x-review-board-request
mstange
: review+
Details
59 bytes, text/x-review-board-request
mstange
: review+
Details
59 bytes, text/x-review-board-request
mstange
: review+
Details
59 bytes, text/x-review-board-request
mstange
: review+
Details
1.50 KB, text/html
Details
Reporter

Description

4 years ago
For example, load the following page:
data:text/html,<p style="transform: scale(10); transform-origin: 0 0; filter: grayscale(0.5)">hello</p>

The text is blurred when any filter is applied.
I think this is probably a layout bug. i.e. Layout is asking to draw at particular resolution and we are just respecting that.
Component: Graphics → Layout
ChooseScaleAndSetTransform in FrameLayerBuilder.cpp might be an interesting place to start debugging.
Assignee

Updated

3 years ago
Blocks: svg-enhance
Assignee

Updated

2 years ago
Assignee: nobody → cku
Comment hidden (typo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 6

2 years ago
Posted file test case
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8829083 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8830220 - Flags: review?(mstange)
Attachment #8830221 - Flags: review?(mstange)
Attachment #8830222 - Flags: review?(mstange)
Attachment #8830223 - Flags: review?(mstange)
Attachment #8830224 - Flags: review?(mstange)
Attachment #8830225 - Flags: review?(mstange)
Attachment #8830226 - Flags: review?(mstange)
Attachment #8830227 - Flags: review?(mstange)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 22

2 years ago
mozreview-review
Comment on attachment 8830220 [details]
Bug 1224207 - Part 1. Split SetupContext into ComputeEffectOffset and TranslateToUserSpace.

https://reviewboard.mozilla.org/r/107110/#review108274
Attachment #8830220 - Flags: review?(mstange) → review+

Comment 23

2 years ago
mozreview-review
Comment on attachment 8830221 [details]
Bug 1224207 - Part 2. Fine tune RegularFramePaintCallback.

https://reviewboard.mozilla.org/r/107112/#review108278
Attachment #8830221 - Flags: review?(mstange) → review+

Comment 24

2 years ago
mozreview-review
Comment on attachment 8830222 [details]
Bug 1224207 - Part 3. Remove unused parameters of nsFilterInstance's member functions.

https://reviewboard.mozilla.org/r/107114/#review108280
Attachment #8830222 - Flags: review?(mstange) → review+

Comment 25

2 years ago
mozreview-review
Comment on attachment 8830223 [details]
Bug 1224207 - Part 4. Move internal used member functions into private section.

https://reviewboard.mozilla.org/r/107116/#review108282
Attachment #8830223 - Flags: review?(mstange) → review+

Comment 26

2 years ago
mozreview-review
Comment on attachment 8830224 [details]
Bug 1224207 - Part 5. Fine tune nsFilterInstance::BuildSourcePaint.

https://reviewboard.mozilla.org/r/107118/#review108286
Attachment #8830224 - Flags: review?(mstange) → review+

Comment 27

2 years ago
mozreview-review
Comment on attachment 8830225 [details]
Bug 1224207 - Part 6. Move GetCSSPxToDevPxMatrix to nsSVGUtils.

https://reviewboard.mozilla.org/r/107120/#review108288
Attachment #8830225 - Flags: review?(mstange) → review+

Comment 28

2 years ago
mozreview-review
Comment on attachment 8830226 [details]
Bug 1224207 - Part 7. (Main) Pass frame transform info down to nsFilterInstance.

https://reviewboard.mozilla.org/r/107122/#review108294

::: layout/svg/nsFilterInstance.cpp:240
(Diff revision 2)
>  nsresult
>  nsFilterInstance::ComputeUserSpaceToFilterSpaceScale()
>  {
> -  gfxMatrix canvasTransform;
>    if (mTargetFrame) {
> -    canvasTransform = nsSVGUtils::GetCanvasTM(mTargetFrame);
> +    mUserSpaceToFilterSpaceScale = mPaintTransform.ScaleFactors(true);

This function is also called when we're not painting, right? So you're now using mPaintTransform in cases where we weren't using it before. And in those cases it'll be the identity transform, see the instances of "gfxMatrix unused;" in nsFilterInstance.cpp.

I'm not completely sure whether that's going to cause bugs. I need to debug some testcases.

::: layout/svg/nsSVGIntegrationUtils.cpp:1114
(Diff revision 2)
> +  gfxPoint filterAreaPos =
> +    context.CurrentMatrix().Transform(offsets.offsetToUserSpaceInDevPx);

I'm not sure about the calculations involving filterAreaPos. Are you making any assumptions about the matrix type of context.CurrentMatrix()? Do these calculations work both for scales and for translations? And for rotations?
Or do we know at this point that context.CurrentMatrix() is of a certain type?
Assignee

Comment 29

2 years ago
mozreview-review-reply
Comment on attachment 8830226 [details]
Bug 1224207 - Part 7. (Main) Pass frame transform info down to nsFilterInstance.

https://reviewboard.mozilla.org/r/107122/#review108294

> This function is also called when we're not painting, right? So you're now using mPaintTransform in cases where we weren't using it before. And in those cases it'll be the identity transform, see the instances of "gfxMatrix unused;" in nsFilterInstance.cpp.
> 
> I'm not completely sure whether that's going to cause bugs. I need to debug some testcases.

I will look into it

> I'm not sure about the calculations involving filterAreaPos. Are you making any assumptions about the matrix type of context.CurrentMatrix()? Do these calculations work both for scales and for translations? And for rotations?
> Or do we know at this point that context.CurrentMatrix() is of a certain type?

context.CurrentMatrix() transforms (0,0) to the top-left postion of aParams.frame's user space in aParams.ctx. 
We are goting to pass tm into nsFilterInstance and assign it to mPaintTransform.
mPaintTransform is used with a context, which origin(0,0) is located right at the top-left postion of aParams.frame's user space. So the translation vector in context.CurrentMatrix() has to be removed.

filterAreaPos is kind of confusing, what I really want to do is:
gfxPoint currentOrigin = context.CurrentMatrix().Transform(gfxPoint(0,0));
gfxMatrix tm =
  context.CurrentMatrix() * gfxMatrix::Translation(-currentOrigin) *
  nsSVGUtils::GetCSSPxToDevPxMatrix(frame);
Assignee

Comment 30

2 years ago
Posted file RTS test cases
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 33

2 years ago
mozreview-review-reply
Comment on attachment 8830226 [details]
Bug 1224207 - Part 7. (Main) Pass frame transform info down to nsFilterInstance.

https://reviewboard.mozilla.org/r/107122/#review108294

> I will look into it

New patch fix this problem. We do not handle css scale transform value well on both painting path(such as nsFilterInstance::PaintFilteredFrame) and non-painting path(such as nsFilterInstance::GetFilterDescription). This bug is trying to fix visual incorrectness in painting path. Do we need file anohter bug to fix non-painting path?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 47

2 years ago
Rebase onto the patch in bug 1334554
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Duplicate of this bug: 956134
Assignee

Updated

2 years ago
Duplicate of this bug: 1015575
Assignee

Updated

2 years ago
Duplicate of this bug: 1146903
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8834244 - Flags: review?(mstange)

Comment 62

2 years ago
mozreview-review
Comment on attachment 8830226 [details]
Bug 1224207 - Part 7. (Main) Pass frame transform info down to nsFilterInstance.

https://reviewboard.mozilla.org/r/107122/#review111700

Ok, if this passes reftests, then it should be fine.

I'm worried about filters whose geometry depends on the resolution. For example the convolve matrix filter and the lighting filters default to a kernelUnitLength of one device pixel. This kernel unit length affects a filter's required input bounds and I think it also affects the output bounds. These regions affect invalidation and layer bounds computation, and they are computed in layout at times where we don't have the paint transform.
So with these patches, it may be possible for us to compute input bounds that are smaller than what the filter ends up reading from during the actual paint, and then we might get rendering and invalidation bugs.

Ideally we'd just remove the ability for a filter's geometry to depend on the resolution. There is a bit of discussion around this at https://lists.w3.org/Archives/Public/public-fx/2013OctDec/0119.html .
Attachment #8830226 - Flags: review?(mstange) → review+

Comment 63

2 years ago
mozreview-review
Comment on attachment 8830227 [details]
Bug 1224207 - Part 8. Reftest.

https://reviewboard.mozilla.org/r/107124/#review108322
Attachment #8830227 - Flags: review?(mstange) → review+
Assignee

Comment 64

2 years ago
mozreview-review-reply
Comment on attachment 8830226 [details]
Bug 1224207 - Part 7. (Main) Pass frame transform info down to nsFilterInstance.

https://reviewboard.mozilla.org/r/107122/#review111700

SVG 1.1 defines transfrom attribute. GetCanvasTM can read this attribute correctly. In 2.0, we can also apply a CSS transform onto an SVG element. We read, parsing and apply it in nsDisplayTransform. GetCanvasTM has no idea on it at all, so we have bug Bug 1323962.
I am still thinking of how to fix it. Maybe bring the code in nsDisplayTransform into GetCanvasTM
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8834244 - Attachment is obsolete: true
Attachment #8834244 - Flags: review?(mstange)

Comment 66

2 years ago
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d23079bc188e
Part 1. Split SetupContext into ComputeEffectOffset and TranslateToUserSpace. r=mstange
https://hg.mozilla.org/integration/autoland/rev/958379e4f61c
Part 2. Fine tune RegularFramePaintCallback. r=mstange
https://hg.mozilla.org/integration/autoland/rev/9448e628569e
Part 3. Remove unused parameters of nsFilterInstance's member functions. r=mstange
https://hg.mozilla.org/integration/autoland/rev/15f62c3ec366
Part 4. Move internal used member functions into private section. r=mstange
https://hg.mozilla.org/integration/autoland/rev/7faefd871e00
Part 5. Fine tune nsFilterInstance::BuildSourcePaint. r=mstange
https://hg.mozilla.org/integration/autoland/rev/587cde853b75
Part 6. Move GetCSSPxToDevPxMatrix to nsSVGUtils. r=mstange
https://hg.mozilla.org/integration/autoland/rev/c31e96bf56d0
Part 7. (Main) Pass frame transform info down to nsFilterInstance. r=mstange
https://hg.mozilla.org/integration/autoland/rev/2fce8d53b105
Part 8. Reftest. r=mstange
Assignee

Comment 68

2 years ago
Too bad, Text rendering on win8 need more fuzzy. I am trying to replace text by other graphic element.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f8e9a6627bba71381adb5b04ebd0f72da9a136c
Flags: needinfo?(cku)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 78

2 years ago
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/19933020136b
Part 1. Split SetupContext into ComputeEffectOffset and TranslateToUserSpace. r=mstange
https://hg.mozilla.org/integration/autoland/rev/71a60ab7515f
Part 2. Fine tune RegularFramePaintCallback. r=mstange
https://hg.mozilla.org/integration/autoland/rev/14a9100a889c
Part 3. Remove unused parameters of nsFilterInstance's member functions. r=mstange
https://hg.mozilla.org/integration/autoland/rev/7dfb1483b48e
Part 4. Move internal used member functions into private section. r=mstange
https://hg.mozilla.org/integration/autoland/rev/3484e9368176
Part 5. Fine tune nsFilterInstance::BuildSourcePaint. r=mstange
https://hg.mozilla.org/integration/autoland/rev/b1fb2a174fbf
Part 6. Move GetCSSPxToDevPxMatrix to nsSVGUtils. r=mstange
https://hg.mozilla.org/integration/autoland/rev/f4f6790e3926
Part 7. (Main) Pass frame transform info down to nsFilterInstance. r=mstange
https://hg.mozilla.org/integration/autoland/rev/655021ab4e07
Part 8. Reftest. r=mstange
Assignee

Updated

2 years ago
Duplicate of this bug: 1168904
Depends on: 1341672

Comment 81

2 years ago
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/16b2f55f7e9b
(followup) remove an unused local var. r=me

Updated

2 years ago
Depends on: 1343664

Updated

2 years ago
Depends on: 1348430
Depends on: 1349741
Assignee

Updated

2 years ago
No longer depends on: 1349741
Assignee

Updated

2 years ago
Depends on: 1349741

Updated

2 years ago
Depends on: 1385239
Depends on: 1498221
You need to log in before you can comment on or make changes to this bug.