Closed Bug 1348430 Opened 7 years ago Closed 7 years ago

SVG: blur filters break patterns

Categories

(Core :: SVG, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox-esr45 --- unaffected
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: mpk, Assigned: u459114)

References

Details

(Keywords: regression, testcase)

Attachments

(8 files)

simple tescase
600 bytes, image/svg+xml
Details
Bug 1348430 - Part 1. (Main) Correct value pass to nsFilterInstance::PaintFilteredFrame in nsSVGUtil.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1348430 - Part 2. Rename a local variable in nsSVGPatternFrame::PaintPattern.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1348430 - Part 4. Reftest.
59 bytes, text/x-review-board-request
mstange
: review+
Details
simple testcase #2
809 bytes, image/svg+xml
Details
simple testcase #2 (corrected)
810 bytes, image/svg+xml
Details
Bug 1348430 - Part 3. Correct mTargetBBoxInFilterSpace.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Part 1 for aurora
2.47 KB, patch
gchang
: approval-mozilla-aurora+
Details | Diff | Splinter Review
Attached image simple tescase — 
+++ This bug was initially created as a clone of Bug #1343664 +++
Another regression has crept up in the wake of bug #1224207 (same regression window as in bug #1343664). This time blur filters seem to break patterns.

The two circles in the testcase should both be filled identically, although the left one in red and the right one in green.
Flags: needinfo?(cku)
Thanks for reporting this bug. I will fix it
Assignee: nobody → cku
Flags: needinfo?(cku)
There are two paths that use nsFilterInstance::PaintFilteredFrame. One is nsSVGIntegrationUtil::PaintFilter, another one is nsSVGUtils::PaintFrameWithEffects. We only take care of the first path in bug 1224207
Attached image simple testcase #2 — 
While the first testcase passes with the patches applied, I'm afraid there's still something wrong (see simple testcase #2).

As in the first testcase, both patterns should be rendered identically (apart from the colors).
Flags: needinfo?(cku)
(In reply to Marco Perez from comment #9)
> Created attachment 8848938 [details]
> simple testcase #2
> 
> While the first testcase passes with the patches applied, I'm afraid there's
> still something wrong (see simple testcase #2).
> 
> As in the first testcase, both patterns should be rendered identically
> (apart from the colors).

I don't think left circle can be the same with the one. At least, I read testcase #2 on both FF53(before Bug 1224207 landed) and chrome on linux. None of them render both circle identically.
Flags: needinfo?(cku)
Attachment #8848782 - Flags: review?(mstange)
Attachment #8848783 - Flags: review?(mstange)
Attachment #8848784 - Flags: review?(mstange)
Comment on attachment 8848782 [details]
Bug 1348430 - Part 1. (Main) Correct value pass to nsFilterInstance::PaintFilteredFrame in nsSVGUtil.

https://reviewboard.mozilla.org/r/121658/#review124038

::: commit-message-9a95a:6
(Diff revision 3)
> +Bug 1348430 - Part 1. (Main) Correct value pass to nsFilterInstance::PaintFilteredFrame in nsSVGUtil.
> +
> +There are two places that use nsFilterInstance::PaintFilteredFrame. One is
> +nsSVGIntegrationUtil::PaintFilter, we do take care of it in bug 1224207.
> +Another path is at nsSVGUtils::PaintFrameWithEffects, apparently I missed that
> +path while working on bug 1224207.

I wonder how many more times we will run into this duplication before we remember to check both places. :)
(Or one of them goes away)
Attachment #8848782 - Flags: review?(mstange) → review+
Comment on attachment 8848783 [details]
Bug 1348430 - Part 2. Rename a local variable in nsSVGPatternFrame::PaintPattern.

https://reviewboard.mozilla.org/r/121660/#review124040
Attachment #8848783 - Flags: review?(mstange) → review+
Comment on attachment 8848784 [details]
Bug 1348430 - Part 4. Reftest.

https://reviewboard.mozilla.org/r/121662/#review124042
Attachment #8848784 - Flags: review?(mstange) → review+
Sorry C.J. Looks like I overdid the minimization with the last testcase. (In the end the blur factor was big enough for the line to hit the edges of its viewBox.) Shame on me...
 
I re-tested the new testcase with a v53 test build and with the stock browser of LineageOS (Android).
(In reply to Marco Perez from comment #17)
> Created attachment 8849276 [details]
> simple testcase #2 (corrected)
> 
> Sorry C.J. Looks like I overdid the minimization with the last testcase. (In
> the end the blur factor was big enough for the line to hit the edges of its
> viewBox.) Shame on me...
>  
> I re-tested the new testcase with a v53 test build and with the stock
> browser of LineageOS (Android).

Hmm...you are right, still have something not right. I will figure it out. Thanks for keep update test case! They are awesome.
I fixed Bug 1287492 first, and then Bug 1336480 several week later.The solution in Bug 1336480 can actually fix bug 128y492 too.

The root cause of test case #2 failure is because of the patch in bug 1287492. Filter space is not really bounded by svg-outer frame. Default filter region is (-10%, 10%) ~ (120%, 120%). Because of the patch in  Bug 1287492, we limit filter region to  (0%, 0%) ~ (100%, 100%).

I am going to create a new patch to remove the work in Bug 1287492
Attachment #8849658 - Flags: review?(mstange)
Comment on attachment 8849658 [details]
Bug 1348430 - Part 3. Correct mTargetBBoxInFilterSpace.

https://reviewboard.mozilla.org/r/122446/#review124804

::: layout/svg/nsFilterInstance.cpp:244
(Diff revision 2)
>    if (!gfxUtils::GfxRectToIntRect(targetBBoxInFilterSpace,
>                                    &mTargetBBoxInFilterSpace)) {
>      // The target's bbox is way too big if there is float->int overflow.
>      return false;
>    }
>  

just  return the result of gfxUtils::GfxRectToIntRect. No if test required.
Comment on attachment 8849658 [details]
Bug 1348430 - Part 3. Correct mTargetBBoxInFilterSpace.

https://reviewboard.mozilla.org/r/122446/#review124932

Sorry for not catching that mistake in bug 1287492.
Attachment #8849658 - Flags: review?(mstange) → review+
Blocks: 1287492
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 2651bee3f262 -d b88f35ad48cc: rebasing 383318:2651bee3f262 "Bug 1348430 - Part 1. (Main) Correct value pass to nsFilterInstance::PaintFilteredFrame in nsSVGUtil. r=mstange"
merging layout/svg/nsSVGUtils.cpp
warning: conflicts while merging layout/svg/nsSVGUtils.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a7893368789d
Part 1. (Main) Correct value pass to nsFilterInstance::PaintFilteredFrame in nsSVGUtil. r=mstange
https://hg.mozilla.org/integration/autoland/rev/6084537f2ecd
Part 2. Rename a local variable in nsSVGPatternFrame::PaintPattern. r=mstange
https://hg.mozilla.org/integration/autoland/rev/8db840662319
Part 3. Correct mTargetBBoxInFilterSpace. r=mstange
https://hg.mozilla.org/integration/autoland/rev/5f8ed7886148
Part 4. Reftest. r=mstange

    
    

    
  
Comment on attachment 8850286 [details] [diff] [review]
Part 1 for aurora

Approval Request Comment
[Feature/Bug causing the regression]: bug 1287492
[User impact if declined]: Apply a filter effect to a pattern object will have wrong rendering result.
[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]: no
[Is the change risky?]:no
[Why is the change risky/not risky?]:pass manual test and auto test
[String changes made/needed]: N/A

Please update this branch patch and Part 2~ Part 4 nightly patch.

        Attachment #8850286 -
        Flags: approval-mozilla-aurora?

    
    

    
  
Comment on attachment 8850286 [details] [diff] [review]
Part 1 for aurora

Fix a regression. Aurora54+.

        Attachment #8850286 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

    
  
Depends on: 1385239

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: