Bug 89506 – GraphicsContext::setCTM usage in SVGInlineTextBox::paintTextWithShadows breaks per-tile painting in chromium.

• Component: SVG
• Status: RESOLVED DUPLICATE

[David Reveman]

GraphicsContext::save()/restore() should be used instead.

[Mike Reed]

or (possibly)

void setMyMatrix(const SkMatrix& mat) {
    SkMatrix inverse;
    if (canvas->getTotalMatrix().invert(&inverse)) {
        canvas->concat(mat);
        canvas->concat(inverse);
    }
}

[James Robinson]

Why can't the picture record the CTM?

[David Reveman]

Created attachment 148416 [details]
Patch

[David Reveman]

The attached patch just switches to using a save()/restore(). I'd prefer to just remove the "scalingFactor != 1" conditions completely but I guess that might cause a performance regression as I assume they are there for a reason.

[Mike Reed]

Comment on attachment 148416 [details]
Patch

looks better than the old code, and will work with pictures. nice.

[David Reveman]

(In reply to comment #2)
> Why can't the picture record the CTM?

We use a different translation offsets for each tile. setCTM playback will only be correct when the tile offset is 0,0 as it depends on the result from the getCTM call during recording.

Maybe we should abort when getCTM is call during recording?

[David Reveman]

Created attachment 148431 [details]
Patch

Add a bit more information to the ChangeLog entry about why the getCTM/setCTM usage is causing breakage in chromium.

[Eric Seidel (no email)]

Comment on attachment 148431 [details]
Patch

It's not immediately clear to me how to review this.  I would expect there to be some performance impact to this change, no?  Saving a context is expensive in many implementations, and this will make scaled text save the context for every paint of every text run?  Is Chromium's tile-based model the future?  Are other platforms going to see this sort of correctness concern?

[James Robinson]

I think we should just make GraphicsContext::(set|get)CTM work with the skia implementation, or if we can't/won't do that see if we can remove it.  I don't think it is acceptable to have an implementation of GraphicsContext where some functions simply do not work.

This is separate from the question of whether the SVGInlineTextBox's implementation strategy is ideal or not.

[David Reveman]

(In reply to comment #9)
> I think we should just make GraphicsContext::(set|get)CTM work with the skia implementation, or if we can't/won't do that see if we can remove it.  I don't think it is acceptable to have an implementation of GraphicsContext where some functions simply do not work.
> 

I'm with you on that. Maybe we could add a device offset to SkCanvas that effects all rendering just like an initial translation but is not included in the CTM.

[James Robinson]

Comment on attachment 148431 [details]
Patch

R- for now - this isn't an obvious improvement or even obviously correct (there was some logic in normalizeTransform that is no longer called, although I'm not sure what it is all about).

If all this wants to do is locally set a scale, could we just undo the scale by hand instead of save()/restore()ing the whole context?

[David Reveman]

(In reply to comment #11)
> (From update of attachment 148431 [details])
> R- for now - this isn't an obvious improvement or even obviously correct (there was some logic in normalizeTransform that is no longer called, although I'm not sure what it is all about).
> 
> If all this wants to do is locally set a scale, could we just undo the scale by hand instead of save()/restore()ing the whole context?

It's a shame that save()/restore()ing is not lightweight on all platforms. I think it's more intuitive that optimizations such as integer translation can be tracked when save()/restore() pairs are used.

[James Robinson]

I don't know, maybe it is in fact cheap.  None of this really gets to the point of the patch, however.

[Mike Reed]

my read of the checkin when normalizeTransform() was added, was it was a concession to DRT to make images between 32bit CG and 64bit CG more the same. Not clear it "fixed" anything at the spec or user level, and was CG specific.

For skia: save/scale/restore is by far the most compact and speed-efficient, matches the rest of webkit's usage pattern, and will work seamlessly in all of its backends.

[Nikolas Zimmermann]

(In reply to comment #11)
> (From update of attachment 148431 [details])
> R- for now - this isn't an obvious improvement or even obviously correct (there was some logic in normalizeTransform that is no longer called, although I'm not sure what it is all about).
> 
> If all this wants to do is locally set a scale, could we just undo the scale by hand instead of save()/restore()ing the whole context?

The existing code only tried to avoid 32bit vs. 64bit differences that we saw in text rendering.
Short story: it failed - normalizeTransform, as well as the whole setCTM() stuff, was a test, and it failed, and can be removed.

[Mike Reed]

sweet!

Such a change may create a *lot* of rebaselines. Perhaps we can opt-in to that cleanup/change per platform, so as not to force it for platforms that don't want it (or don't care). I can post such a CL, unless David wants to.

[David Reveman]

(In reply to comment #16)
> sweet!
> 
> Such a change may create a *lot* of rebaselines. Perhaps we can opt-in to that cleanup/change per platform, so as not to force it for platforms that don't want it (or don't care). I can post such a CL, unless David wants to.

Please go ahead and create such a CL if you like.

[David Reveman]

*** This bug has been marked as a duplicate of bug 89888 ***