929001 - Stop SVGEllipseElement from changing the current matrix between generation and drawing of its path

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

To convert to Moz2D, we need to stop SVGEllipseElement from changing the current matrix between generation and drawing of its path.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Robert Longson [:longsonr]]

This was a deliberate choice originally because arc is more accurate so this is not equivalent.

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 819942 [details] [diff] [review]
patch

Seems sensible ;)

r+

Comment 4

[Daniel Holbert [:dholbert]]

Oh, I missed comment 2.

Is arc still more accurate? Can we just fix the ellipse() API to be as accurate?

(Do we have tests that reveal the innacuracy?)

Comment 5

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson from comment #2)
> This was a deliberate choice originally because arc is more accurate so this
> is not equivalent.

The history there seems to predate HG, so I'm not sure where I can find any prior discourse on that.

I've pushed to Try to see if anything fails. Depending on what happens there, and on any further comments here, I may end up just tweaking some tests to get this landed.

https://tbpl.mozilla.org/?tree=Try&rev=c3bd3bb016a9

Comment 6

[Robert Longson [:longsonr]]

I tried to fix this before in bug 459148 and again in bug 467629.

IIRC what happens is that small ellipses magnified to be visible stop working. I think there's a regression bug somewhere.

Comment 7

[Robert Longson [:longsonr]]

Ahh yes the regression bug is 465996. We'll be reopening that can of worms if we do this I suspect.

Comment 8

[Robert Longson [:longsonr]]

There's a mochitest in bug 465996 that you wrote Daniel! Surely you can remember it's only 5 years ago :-)

Comment 9

[Robert Longson [:longsonr]]

May well only fail on XP or when HWA is off these days since it's a cairo thing.

Comment 10

[Robert Longson [:longsonr]]

So perhaps the answer is to make this change but also simultaneously change cairo so that when we do the ellipse call it works like the SVG implementation we have now rather than doing whatever it currently does.

Comment 11

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson from comment #8)
> There's a mochitest...

Meh, I only got Try to run reftests. Cancelled and repushed:

https://tbpl.mozilla.org/?tree=Try&rev=ec1e43a4c66f

Let's see what fails and then we can think about how to handle it. Some good background and suggestions above though. Thanks, Robert.

Comment 12

[Jonathan Watt [:jwatt]]

The patch seems to pass that Try push just fine.

Comment 13

[Robert Longson [:longsonr]]

I can't get it to go wrong on trunk with/without HWA or SVG display lists enabled/disabled. 

It seems like the underlying code in gfx has undergone some changes over the years which could be relevant but I can't really tell.

Perhaps we should just check Jeff is OK with this now given bug 465996 Comment 12

Comment 14

[Jonathan Watt [:jwatt]]

Five years ago in bug 465996 Comment 12 he said:

> It's also not necessarily desirable to switch to Ellipse() as it will only
> use a single Bezier curve instead of as many as needed to keep the error
> minimized as Arc() does.

Asking him now what he thought the problem was, he can't remember what he was concerned about. He and the other gfx folks would very much like us to land this patch and, if there are issues, Jeff thinks it should be "super easy" to write better code for gfxContext::Ellipse(), and they'll just do that.

Comment 15

[Jonathan Watt [:jwatt]]

The Save()/Restore() that's being removed is also very expensive, so this is a perf improvement even as-is (that is, without the subsequent conversion to Moz2D) in the case that someone was to draw a lot of ellipse elements.

Comment 16

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/39f4bb8c55d8

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/39f4bb8c55d8