Closed Bug 929001 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: jwatt, Assigned: jwatt)

References

(Blocks 2 open bugs)

Details

(Keywords: perf)

Attachments

(1 file)

To convert to Moz2D, we need to stop SVGEllipseElement from changing the current matrix between generation and drawing of its path.
Attached patch patchSplinter Review
Attachment #819942 - Flags: review?(dholbert)
This was a deliberate choice originally because arc is more accurate so this is not equivalent.
Comment on attachment 819942 [details] [diff] [review]
patch

Seems sensible ;)

r+
Attachment #819942 - Flags: review?(dholbert) → review+
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?)
(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
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.
Ahh yes the regression bug is 465996. We'll be reopening that can of worms if we do this I suspect.
There's a mochitest in bug 465996 that you wrote Daniel! Surely you can remember it's only 5 years ago :-)
May well only fail on XP or when HWA is off these days since it's a cairo thing.
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.
(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.
The patch seems to pass that Try push just fine.
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
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.
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.
Keywords: perf
https://hg.mozilla.org/mozilla-central/rev/39f4bb8c55d8
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Blocks: 703159
Depends on: 944704
You need to log in before you can comment on or make changes to this bug.