Closed Bug 1224207 Opened 8 years ago Closed 7 years ago

Scaling SVG or text with filters should not blur

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
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)

test case
104 bytes, text/html
Details
Bug 1224207 - Part 1. Split SetupContext into ComputeEffectOffset and TranslateToUserSpace.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1224207 - Part 2. Fine tune RegularFramePaintCallback.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1224207 - Part 3. Remove unused parameters of nsFilterInstance's member functions.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1224207 - Part 4. Move internal used member functions into private section.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1224207 - Part 5. Fine tune nsFilterInstance::BuildSourcePaint.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1224207 - Part 6. Move GetCSSPxToDevPxMatrix to nsSVGUtils.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1224207 - Part 7. (Main) Pass frame transform info down to nsFilterInstance.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1224207 - Part 8. Reftest.
59 bytes, text/x-review-board-request
mstange
: review+
Details
RTS test cases
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

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  


  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  

        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
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: 1348430

    
  
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.