992850 - SVG PATH hover in the path curve line not fluid

Status: RESOLVED FIXED

(Core :: SVG, defect, P3)

Description

[berenice]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/33.0.1750.117 Safari/537.36

Steps to reproduce:

In reference the when fixed bug 984796.


When hover in path in SVG appear the circle not fluid.

This is a test case to test:

http://bl.ocks.org/duopixel/3824661




Actual results:


When hover in the point is not fluid in  version Firefox 29 beta5. It make
 some steps caused by obtain getPointAtLength in a PATH of svg.  In firefox
 27 is OK, and previews versions.



Expected results:

Appear the cricle fluid when hover the path,

Comment 1

[Alice0775 White]

Confirmed on Nightly31.0a1,Aurora30.0a2 and Firefox29.0b5
https://hg.mozilla.org/mozilla-central/rev/5fa70bd90a8b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0 ID:20140406030203

Comment 2

[Alice0775 White]

Regression pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=92f737230338&tochange=af0931327e49

Regressed by:
4f086025350f	Jonathan Watt — Bug 930577 - Convert much of the SVG code for calculating path lengths and position at an offset along a path to Moz2D. r=heycam

Comment 3

[Bas Schouten (:bas.schouten)]

Any chance you can get me a reduced testcase, jwatt?

Comment 4

[Alice0775 White]

Attachment: spped test getPointAtLength

Speed path.getPointAtLength(distance) in my pc:

Firefox27: 180 msec
Before landing Bug 930577: 190 msec
After landing Bug 930577: 2100 msec
Nightly31.0a1: 2800 msec

Comment 5

[Sylvestre Ledru [:Sylvestre]]

Tracking because of the important performance regression.

Comment 6

[Lukas Blakk [:lsblakk] use ?needinfo]

Too late now for FF29 - we need an assignee here to continue tracking this for 30 and to ensure it gets attention.

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

Bas - where are we with this? Is there a backout that can be considered at this stage to not continue to ship with this regression?

Comment 8

[Bas Schouten (:bas.schouten)]

(In reply to Lukas Blakk [:lsblakk] from comment #7)
> Bas - where are we with this? Is there a backout that can be considered at
> this stage to not continue to ship with this regression?

I missed JWatt's testcase, I'll write a fix for this tomorrow.

Comment 9

[Bas Schouten (:bas.schouten)]

Attachment: Cache Moz2D path for Length or Position measuring

As promised, a patch to fix this.

This makes us cache the path for length or position measuring. It makes the test speed go down to 44 ms on my machine. I'll leave it up to JWatt to decide if this is safe and desirable to do.

Comment 10

[Bas Schouten (:bas.schouten)]

Note it's probably a good idea to use this for drawing as well, but that should be easy once this is in.

Comment 11

[Jonathan Watt [:jwatt]]

Bas: So I thought the whole reason for having the Moz2D algorithm that we use is because it is _fast_. So how come it's much slower than the old cairo algo in this case?

Comment 12

[Bas Schouten (:bas.schouten)]

(In reply to Jonathan Watt [:jwatt] from comment #11)
> Bas: So I thought the whole reason for having the Moz2D algorithm that we
> use is because it is _fast_. So how come it's much slower than the old cairo
> algo in this case?

D2D path objects are expensive to create. With cairo, we created these objects on demand when actually drawing (a waste, fwiw). Cairo's path measuring code completely bypassed D2D. With Moz2D this is no longer true, and we'll pay the price of D2D path creation for every single getPointAtLength. But not only the D2D path creation, we'll also have to get the path segments back -out- of D2D (because of the way things are implemented) every time. This is considered okay since it's a one-off cost, but if you're not retaining paths (which are considered 'heavy-weight' objects in Moz2D, hence why they're refcounted at all), it's going to cost you.

Comment 13

[Jonathan Watt [:jwatt]]

You also need to reset in:

  nsresult CopyFrom(const SVGPathData& rhs);
  nsresult AppendSeg(uint32_t aType, ...);
  iterator begin() { return mData.Elements(); }
  iterator end() { return mData.Elements() + mData.Length(); }

Comment 14

[Bas Schouten (:bas.schouten)]

Attachment: Cache Moz2D path for Length or Position measuring v2

Comment 15

[Bas Schouten (:bas.schouten)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/8cb1217dc3ff

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/8cb1217dc3ff

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

We're too close to the wire now for FF30 to take this on with no time to properly get user feedback on Beta - let's wontfix for 30 and get an uplift nomination for Aurora so this can get a whole Beta cycle to shake out any further issues (if there are any).

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Tim, can we have an uplift request for aurora? Thanks

Comment 19

[Sylvestre Ledru [:Sylvestre]]

s/Tim/Bas/

Sorry ;)

Comment 20

[Bas Schouten (:bas.schouten)]

Comment on attachment 8430408 [details] [diff] [review]
Cache Moz2D path for Length or Position measuring v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 930577
User impact if declined: Poor path measuring performance
Testing completed (on m-c, etc.): Some nightly testing
Risk to taking this patch (and alternatives if risky): Relatively low risk, no alternatives. 
String or IDL/UUID changes made by this patch: None

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8430408 [details] [diff] [review]
Cache Moz2D path for Length or Position measuring v2

Thanks

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fb60acc600e4

Comment 23

[Bogdan Maris, Desktop QA]

Reproduced the initial issue on old Nightly (2014-04-07), verified that the issue is fixed on Windows 7 64bit, Mac OS X 10.9.4 and Ubuntu 13.04 64bit using Firefox 31 beta 6 and latest Aurora.

Comment 24

[rafa]

It's not working properly on MacOSX 10.9.4 using Firefox 32.0.2.

Comment 25

[Robert Longson [:longsonr]]

Please raise anothr bug, make it depend on this one if you like. This one is done, though.