Closed Bug 1362000 Opened 7 years ago Closed 7 years ago

New graphics compositor and SVG matrixes bug

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Version:
53 Branch
Platform:
x86
Windows 10
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

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

People

(Reporter: tadeuszwojcik, Assigned: u459114)

References

Details

(Keywords: regression)

Attachments

(3 files, 5 obsolete files)

ff_compositor_bug_example.zip
134.84 KB, application/x-zip-compressed
Details
Bug 1362000 - (follow-up) Simplify match logic in CSSMaskLayerUserData::operator==.
59 bytes, text/x-review-board-request
mstange
: review+
Details
beta.patch
3.01 KB, patch
jcristau
: approval-mozilla-beta+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.81 Safari/537.36

Steps to reproduce:

User agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0 
Operating system: Windows 10 64 bit
Graphcis card Intel HD 520 (reproduced also with different GPU configs on different PCs)


Open: https://codepen.io/tadeuszwojcik/pen/ZKJwRE which is simplified snipped of real world bug inside our web app.

Drag image within black border - drags apply CSSmatrix to SVG <use> element that is used as part of clip mask as well as to 'ghost' group.

Here's another example that shows the issue (random matrix changes cause visible artifacts): 
https://codepen.io/tadeuszwojcik/pen/oWexWW 


Actual results:

Dragged image jumps around and sometimes outside <svg> bounds, and in general 'lags' in rendering creating pretty bad end user experience.




Expected results:

Dragged image should behave the same way as in FF v52 or in Chrome - no image 'leaking'/moving outside SVG, no jumps or visible lag

This bug is most likely related to new graphics compositor enabled in v53 as in 52 and below it works fine. It also works fine when use disabled hardware acceleration in FF options. When trying Chrome it also behaves correctly (as in FF v52).
Interestingly it only happens when applying 'translation', when we modify matrix to rotate/scale (not implemented in sample codepen) it works fine.

We consider FF as first-class browser we support 100% and in general try to find workarounds in our code when finding bugs first, but this one seems to be really hard to fix on JS level.
Summary: New graphics compositor with SVG and matrixes → New graphics compositor and SVG matrixes issue
Summary: New graphics compositor and SVG matrixes issue → New graphics compositor and SVG matrixes bug
Severity: normal → major
OS: Unspecified → Windows 10
Hardware: Unspecified → x86

    
    

    
  
I can reproduce the problem on Windows10 64 and Nightly55 32bit with/without e10s.

Regression window with/without e10s:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=0005d0bfadf72746ce36f4e8d09d9504b814557e&tochange=ba20391edbec448b56725ef01158176614706b4f

Regressed by: Bug 1313898
Blocks: 1313898
Status: UNCONFIRMED → NEW

          status-firefox53:
          --- → wontfix

          status-firefox54:
          --- → affected

          status-firefox55:
          --- → affected

          status-firefox-esr45:
          --- → unaffected

          status-firefox-esr52:
          --- → unaffected
Component: Graphics → Layout
Ever confirmed: true
Keywords: regressionwindow-wanted → regression
Assignee: nobody → cku

    
    

    
  
Looks like the check at http://searchfox.org/mozilla-central/rev/b0e1da2a90ada7e00f265838a3fafd00af33e547/layout/painting/FrameLayerBuilder.cpp#3883-3888 is insufficient and we're reusing a mask layer with a size that's too small.

    
  
See Also:  → 1363855

    
    

    
  
Briefly speaking,
http://searchfox.org/mozilla-central/source/layout/painting/FrameLayerBuilder.cpp#3908

aMaskItem->IsInvalid(dirtyRect) can not detect content change of nsSVGClipPathFrame or nsSVGMaskFrame.

    
    

    
      
  

    
    

    
      
  


  

    
  

        Attachment #8867043 -
        Attachment is obsolete: true

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  


  

    
  

        Attachment #8867146 -
        Attachment is obsolete: true

    
    

    
      
  

    
  

        Attachment #8867447 -
        Flags: review?(mstange)

    
    

    
  
Is the logic in nsDisplayMask::HasInvalidSVGMaskOrClipPath() correct?  It is returning true if a mask or the clippath returns false from IsInvalid.  This doesn't look right to me...

    
    

    
  
(In reply to Gerry Iles from comment #14)
> Is the logic in nsDisplayMask::HasInvalidSVGMaskOrClipPath() correct?  It is
> returning true if a mask or the clippath returns false from IsInvalid.  This
> doesn't look right to me...
Yes, that's wierd. Let me double check again
Flags: needinfo?(bugs)

    
  

        Attachment #8867447 -
        Flags: review?(mstange)

    
  
Flags: needinfo?(bugs)

    
    

    
  
hmm... mask::isinvalid always return fasle, which means I eitehr can not use it or need to make it return true when it is.
Or, I can go back to this version(https://reviewboard.mozilla.org/r/138734/diff/3/)

    
    

    
      
  


  

    
  

        Attachment #8867447 -
        Attachment is obsolete: true

    
    

    
      
  
Status: NEW → ASSIGNED

    
    

    
  
CJ, are you going to be able to fix this soon enough that we could uplift to 54, or should we consider this as a fix targeted at 55?
Flags: needinfo?(cku)

    
    

    
      
  

    
  

        Attachment #8867824 -
        Flags: review?(mstange)

    
    

    
      
  

    
    

    
  
(In reply to Nathan Froyd [:froydnj] from comment #19)
> CJ, are you going to be able to fix this soon enough that we could uplift to
> 54, or should we consider this as a fix targeted at 55?
I will try to make the fix land before 6/12
Flags: needinfo?(cku)

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
mstange is quite busy on other stuff. I will land the patch first since we do not have much time left for 55.

    
  

        Attachment #8867824 -
        Flags: review?(mstange) → review?(cku)

    
    

    
  
Comment on attachment 8867824 [details]
Bug 1362000 - When the position of masked frame changed, always regenerate mask layer if any of maskUnits/maskContentUnits/clipPathUnits is userSpaceOnUse.

https://reviewboard.mozilla.org/r/139350/#review146948

        Attachment #8867824 -
        Flags: review?(cku) → review+

    
    

    
  
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2c4c92d37b9d
When the position of masked frame changed, always regenerate mask layer if any of maskUnits/maskContentUnits/clipPathUnits is userSpaceOnUse. r=cjku

    
    

    
  
Comment on attachment 8867824 [details]
Bug 1362000 - When the position of masked frame changed, always regenerate mask layer if any of maskUnits/maskContentUnits/clipPathUnits is userSpaceOnUse.

https://reviewboard.mozilla.org/r/139350/#review147038

::: layout/painting/FrameLayerBuilder.cpp:1655
(Diff revision 7)
> +    // layer, where the mask was drawn, stay in different coordinate space.
> +    // Since they are in different coordinate space, change the position of any
> +    // one of them makes mask layer not reusable, and modifying the position of
> +    // the masked frame does change the position of mask layer.
> +    //
> +    // CSS image mask's coordinate space is always objectBoudingBox, we do not

typo: objectBoundingBox

::: layout/painting/FrameLayerBuilder.cpp:1656
(Diff revision 7)
> +    // Since they are in different coordinate space, change the position of any
> +    // one of them makes mask layer not reusable, and modifying the position of
> +    // the masked frame does change the position of mask layer.
> +    //
> +    // CSS image mask's coordinate space is always objectBoudingBox, we do not
> +    // have to worry about it.

This seems rather magical and I'm not completely convinced it's true. Could we just always check the TopLeft() position, so that there would be no need for nsSVGEffects::HasUserSpaceOnUseUnitsMaskOrClipPath? Do you have a testcase where doing so would cause unnecessary repaints?

    
    

    
  
Comment on attachment 8867824 [details]
Bug 1362000 - When the position of masked frame changed, always regenerate mask layer if any of maskUnits/maskContentUnits/clipPathUnits is userSpaceOnUse.

https://reviewboard.mozilla.org/r/139350/#review147038

> This seems rather magical and I'm not completely convinced it's true. Could we just always check the TopLeft() position, so that there would be no need for nsSVGEffects::HasUserSpaceOnUseUnitsMaskOrClipPath? Do you have a testcase where doing so would cause unnecessary repaints?

If we have a translation animation on masked element, we may end up keep repaint mask layer during animation becasue of the position chnage, that's the reason why I add one more check(HasUserSpaceOnUseUnitsMaskOrClipPath).

But I think I might worry too much. When we apply an animation onto a mask element, we will put masked frame onto an indivisual layer, so the position of mask layer won't change.
I will add one more patch to fix what you said since this one had been checkin.

    
    

    
      
  

    
  

        Attachment #8871865 -
        Flags: review?(mstange)

    
    

    
  
Comment on attachment 8871865 [details]
Bug 1362000 - (follow-up) Simplify match logic in CSSMaskLayerUserData::operator==.

https://reviewboard.mozilla.org/r/143372/#review147078

Thanks!

        Attachment #8871865 -
        Flags: review?(mstange) → review+

    
  

        Attachment #8867824 -
        Attachment is obsolete: true

    
    

    
  
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/058239ada7a6
(follow-up) Simplify match logic in CSSMaskLayerUserData::operator==. r=mstange

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/2c4c92d37b9d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago

          status-firefox55:
          affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

    
    

    
  
It's getting late in the 54 cycle. Is this severe enough to warrant uplift consideration or should we let it ride the 55 train instead?
Flags: needinfo?(cku)

    
    

    
  
(In reply to Ryan VanderMeulen [:RyanVM] from comment #36)
> It's getting late in the 54 cycle. Is this severe enough to warrant uplift
> consideration or should we let it ride the 55 train instead?

I think we shoud uplift to 54 for two reasons:
1. Final patch is simple.
2. In SVG, by applying any transform animation onto a masked descendant, you might see this glitch. So I think we should get rid of it this regression in release version.
Flags: needinfo?(cku)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        beta.patch (obsolete)
        — 
          — Splinter Review
      

    


  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        beta.patch
        — 
          — Splinter Review
      

    


  

        Attachment #8872023 -
        Attachment is obsolete: true

    
    

    
  
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf76bce5f85b
(followup) remove unused headers. r=me

    
    

    
  
Comment on attachment 8872024 [details] [diff] [review]
beta.patch

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1313898
[User impact if declined]: Applying any transform animation onto a masked descendant in SVG,  this glitch may be seen by users.
[Is this code covered by automated tests?]: no
[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]: this patch only
[Is the change risky?]: no
[Why is the change risky/not risky?]: small change
[String changes made/needed]: N/A

        Attachment #8872024 -
        Flags: approval-mozilla-beta?

    
    

    
  
Comment on attachment 8872024 [details] [diff] [review]
beta.patch

layout fix, beta54+, should be in 54.0b12

        Attachment #8872024 -
        Flags: approval-mozilla-beta? → approval-mozilla-beta+

    
    

    
  
(In reply to C.J. Ku[:cjku](UTC+8) from comment #42)
> [Is this code covered by automated tests?]: no
> [Has the fix been verified in Nightly?]: yes
> [Needs manual test from QE? If yes, steps to reproduce]:  no

Setting qe-verify- based on C.J. Ku's assessment on manual testing needs.
Flags: qe-verify-

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: