Closed Bug 1224207 Opened 5 years ago Closed 4 years ago

Scaling SVG or text with filters should not blur

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox45 --- affected
firefox54 --- fixed

People

(Reporter: xidorn, Assigned: u459114)

References

(Blocks 1 open bug)

Details

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

Attachments

(10 files, 2 obsolete files)

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
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.
Blocks: svg-enhance
Assignee: nobody → cku
Attached file test case
Attachment #8829083 - Attachment is obsolete: true
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 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 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 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 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 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 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 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?
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);
Attached file RTS test cases
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?
Rebase onto the patch in bug 1334554
Duplicate of this bug: 956134
Duplicate of this bug: 1015575
Duplicate of this bug: 1146903
Attachment #8834244 - Flags: review?(mstange)
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 on attachment 8830227 [details]
Bug 1224207 - Part 8. Reftest.

https://reviewboard.mozilla.org/r/107124/#review108322
Attachment #8830227 - Flags: review?(mstange) → 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

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
Attachment #8834244 - Attachment is obsolete: true
Attachment #8834244 - Flags: review?(mstange)
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
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)
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
Duplicate of this bug: 1168904
Depends on: 1341672
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/16b2f55f7e9b
(followup) remove an unused local var. r=me
Depends on: 1343664
Depends on: 1349741
No longer depends on: 1349741
Depends on: 1349741
Depends on: 1385239
You need to log in before you can comment on or make changes to this bug.