Bug 89888 – use scale() instead of getCTM/normalizeTransform/setCTM for svg text decorations

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Mike Reed]

use scale() instead of getCTM/normalizeTransform/setCTM for svg text decorations

[Mike Reed]

Created attachment 149302 [details]
Patch

[Eric Seidel (no email)]

Why wouldn't all platforms want this path?  Why does Skia?

[Mike Reed]

Why:

Skia-specific:
The current code does not play well if we try to record/playback the drawing commands, since the getCTM() is done once, and may not be the same matrix that is present during subsequent playback.

General:
The old code introduces a fair amount of complexity and heuristics (e.g. why do we normalize only around identity, and not other integral scale factors or integral translates?)

If other platforms want to opt-in to this change, I presume they will either remove the check, or add their platform to the list.

[David Reveman]

Comment on attachment 149302 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149302&action=review

> Source/WebCore/rendering/svg/SVGInlineTextBox.cpp:652
> +            context->save();

if we consider save/restore() lightweight, I'd prefer the save/restore pair outside the conditional so that only one "scalingFactor != 1" check is needed.

[Mike Reed]

I can make that change. My original CL attempted to keep the existing flow as much as possible.

I do think save/restore are very efficient in skia, so it should not be a perf issue there.

[Mike Reed]

always calling save/restore might be another reason to make this Skia specific, unless the other platforms feel the same way about their relative perf cost...

[Eric Seidel (no email)]

I don't know what the current view on the cost of GraphicsContext::save/restore is.

[Eric Seidel (no email)]

I'd prefer if we just removed the "old" code and used scale instead, if that's perceived as the better way.  We can re-visit adding the "old" code back for other platforms if scale is too slow.  The perf bots will answer these questions (but I doubt it matters).

So my suggestion would be to just change the code instead of adding yet another if-def.

[Mike Reed]

I'll give it a try, and hope there are rebaselines on other platforms...

[Mike Reed]

Created attachment 149319 [details]
Patch

[Mike Reed]

patch #2 removes #ifdefs

[Mike Reed]

PTAL

[Eric Seidel (no email)]

Comment on attachment 149319 [details]
Patch

Am I reading correctly that this patch basically undoes this change: http://trac.webkit.org/changeset/78704/trunk/Source/WebCore/rendering/svg/SVGInlineTextBox.cpp?  Do you anticipate that this will cause rendering differences between 64 and 32 bit machines (for skia or otherwise)?

[Mike Reed]

I would think it would, though zimmermann might comment on that. This is why I thought my change should be per-platform.

[Eric Seidel (no email)]

I see.  I can understand your desire for this to be per-platform then.  I'd like to give Niko a day to comment.  He's working from Germany last I knew.

[Dirk Schulze]

Comment on attachment 149319 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149319&action=review

In general I like to do this, instead of the "normalizeTransform" logic. I reviewed the original patch and thought that it might be worth at that time. But changed my mind. Even if we see rounding differences. I still think it is a good idea if Niko looks over the patch as well.

r- because of the issues listed below. I am fine if someone else reviews a new patch, since I won't be available for the next weeks.

> Source/WebCore/ChangeLog:8
> +
> +        use scale() instead of getCTM/normalizeTransform/setCTM for svg text decorations.
> +        https://bugs.webkit.org/show_bug.cgi?id=89888
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        No new tests. Current layouttests exercise this code path.

A detailed comment is missing what you are doing and why you are doing it.

> Source/WebCore/rendering/svg/SVGInlineTextBox.cpp:618
> +        context->save();

We have GraphicsContextStateSaver for save/restore. Can't this be used here? Also, why are you using save and restore here? I can remember that for some platforms it was quite an expensive operation. Can't you just unscale?

> Source/WebCore/rendering/svg/SVGInlineTextBox.cpp:623
> +        context->restore();

Some lines later there is a condition and another restore  is called. This seems dangerous, since no save was called before here.

[Mike Reed]

(In reply to comment #16)
> (From update of attachment 149319 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149319&action=review
> 
> In general I like to do this, instead of the "normalizeTransform" logic. I reviewed the original patch and thought that it might be worth at that time. But changed my mind. Even if we see rounding differences. I still think it is a good idea if Niko looks over the patch as well.
> 
> r- because of the issues listed below. I am fine if someone else reviews a new patch, since I won't be available for the next weeks.
> 
> > Source/WebCore/ChangeLog:8
> > +
> > +        use scale() instead of getCTM/normalizeTransform/setCTM for svg text decorations.
> > +        https://bugs.webkit.org/show_bug.cgi?id=89888
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        No new tests. Current layouttests exercise this code path.
> 
> A detailed comment is missing what you are doing and why you are doing it.
> 
> > Source/WebCore/rendering/svg/SVGInlineTextBox.cpp:618
> > +        context->save();
> 
> We have GraphicsContextStateSaver for save/restore. Can't this be used here? Also, why are you using save and restore here? I can remember that for some platforms it was quite an expensive operation. Can't you just unscale?

Too numerically unstable. That is why we have save/restore.

> 
> > Source/WebCore/rendering/svg/SVGInlineTextBox.cpp:623
> > +        context->restore();
> 
> Some lines later there is a condition and another restore  is called. This seems dangerous, since no save was called before here.

This is from code that has always been here. The restore is balance a hidden save inside applyShadowToGraphicsContext().

[Mike Reed]

Created attachment 149516 [details]
Patch

[Mike Reed]

Updated comment, and replied to Dirk's comments.

PTAL

[Nikolas Zimmermann]

Comment on attachment 149516 [details]
Patch

If there are no regressions, r=me. Reverting the mess is just right, it didn't fix anything after all, and was my rude attempt to get consistent results between 32bit/64bit mac machines, but as it effectively breaks things for Chromium, it should just go away.

[WebKit Review Bot]

Comment on attachment 149516 [details]
Patch

Clearing flags on attachment: 149516

Committed r121343: <http://trac.webkit.org/changeset/121343>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[David Reveman]

*** Bug 89506 has been marked as a duplicate of this bug. ***