Bug 23957 – SVG text gradients and patterns on chromium/skia don't work

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Stephen White]

SVG text gradients on chromium/skia don't work.

See layout tests:

svg/custom/js-late-gradient-and-object-creation
svg/custom/js-late-gradient-creation

[Stephen White]

Created attachment 27671 [details]
Fix for text gradients on chrome/skia.

[Dirk Schulze]

Don't know the Skia implementation, but what about patterns? In Cairo (and perhaps Qt too) would need the same for patterns. Is it possible to add patterns too? Or make a more general way, to give either a gradient, color, pattern back, or set the fill/stroke operation directly in GraphicsContext for platforms that need it?

[Stephen White]

(In reply to comment #2)
> Don't know the Skia implementation, but what about patterns? In Cairo (and
> perhaps Qt too) would need the same for patterns. Is it possible to add
> patterns too? Or make a more general way, to give either a gradient, color,
> pattern back, or set the fill/stroke operation directly in GraphicsContext for
> platforms that need it?

Not sure I fully understand your comment.  There are already functions for setting the color, pattern, etc.  Are you asking for GraphicsContext accessors for the color & pattern?  I could add them if you like, but noone will call them, currently.

The reason I needed these accessors is because the chromium implementation switches on the fly between GDI rendering (for speed) and skia rendering for functions that win32 doesn't support (such as gradients).  Normally, the platform layer will handle the gradient, pattern, etc when it goes to render.  However, chromium needs to know ahead of time, so it can decide which rendering path to use.

[Dirk Schulze]

I think we both mean the same but misunderstood us. You added this gradiend-calls to GraphicsContext, to have access to it in Font* and to fill/stroke texts with it (your gdi<->skia problem). But don't you have the same problem for patterns (eg. on http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-pservers-pattern-01-b.html)?
I just suggested to implement this calls in GraphicsContext for patterns too.

[Stephen White]

(In reply to comment #4)
> I think we both mean the same but misunderstood us. You added this
> gradiend-calls to GraphicsContext, to have access to it in Font* and to
> fill/stroke texts with it (your gdi<->skia problem). But don't you have the
> same problem for patterns (eg. on
> http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-pservers-pattern-01-b.html)?
> I just suggested to implement this calls in GraphicsContext for patterns too.
> 

You're absolutely right -- that test is broken too.  I have fixed it in the same  chrome build, but there are still some remaining issues:  it only works if the x scale is 1.00001f (not 1.0f).  Probably a skia issue.

Would you like me to submit this in the same patch, or do you want to let this one in as-is?

[Dimitri Glazkov (Google)]

It sounds like Brett should first look at this, as he is the supreme master Fu of the Skia clan.

[Brett Wilson (Google)]

Is it possible to encode the gradient in the SkPaint before you call the Skia paint function? This is the type of thing that the SkPaint should be for, and how its used for all other shading effects. This may make most of the other changes unnecessary.

I believe PassRefPtr takes the reference from the source and will pass it to the caller. The way you're using it is wrong, since the source is supposed to keep the reference. I think this should just return a Gradient* and the caller should AddRef if he wants to keep a reference.

If we have to keep the Gradient* to paintSkiaText, it should just be a Gradient* since you're not transferring ownership.

[Stephen White]

Thanks for your comments, Brett.

(In reply to comment #7)
> Is it possible to encode the gradient in the SkPaint before you call the Skia
> paint function? This is the type of thing that the SkPaint should be for, and
> how its used for all other shading effects. This may make most of the other
> changes unnecessary.

Yes, the gradient could be set in the SkPaint before the call (normally, this would be done for us by GraphicsContext[Skia]::fillPath(), but we call SkCanvas::drawPath() directly, not sure why), but I'd just have to retrieve the shader again in order to scale and offset the matrix.  This is actually the part of the fix I'm least happy with:  the problem is that the gradient is transformed with the canvas for each character.  If you can help me come up with an alternative, I'd appreciate it.  Another test that shows the same problem is at:

http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-pservers-grad-08-b.html

(Notice that each character has its own gradient.)  And in fact, my patch doesn't fix this test, since it uses custom SVG fonts which apparently have a different drawing path.  There may be some higher-level fix that will fix all these problems.  Perhaps a flag in the skia gradients which marks them as "absolute", rather than transformed with the canvas.  It would help to know how this works in CoreGraphics, since judging by the tests, it does work there.  

Another fix I tried was to stop translating the canvas entirely, and offset each glyph as it's drawn, but I still needed to scale the matrix in order to accommodate the whole text string.  It also required making a copy of each glyph, and I wasn't sure of the performance implications of that (although the culling code does the same thing later on).

> I believe PassRefPtr takes the reference from the source and will pass it to
> the caller. The way you're using it is wrong, since the source is supposed to
> keep the reference. I think this should just return a Gradient* and the caller
> should AddRef if he wants to keep a reference.
> 
> If we have to keep the Gradient* to paintSkiaText, it should just be a
> Gradient* since you're not transferring ownership.

Will fix.

[Dirk Schulze]

Don't worry about http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-pservers-grad-08-b.html
This is a "bug" in our SVG implementation. Safari goes around this by using masking instead of filling the text with the gradient. There is already a bug report about this https://bugs.webkit.org/show_bug.cgi?id=23875. It affects patterns too.

[Stephen White]

Created attachment 27898 [details]
Revised fix:  use bare pointers, don't offset canvas, fix patterns too

Ok, this patch addresses the comments from Dirk and Brett.  This should also fix these additional layout tests:

svg/custom/js-late-pattern-and-object-creation
svg/custom/js-late-pattern-creation

I also reverted the shader matrix computation, and am offsetting each glyph individually.  This is a lot cleaner and less error-prone, although potentially slower.  Given that the culling code is doing it anyway, I'm not too worried about speed.

[Brett Wilson (Google)]

+            // Don't give zero-width spaces a width.
+            if (!currentAdvance)
+                continue;

It's not clear what this has to do with this patch. I also don't entirely understand the comment. Can you elaborate on why this needs special handling (I assume we would otherwise remove extra letterspacing giving negative widths).

In static bool skiaDrawText(HFONT hfont... can you add a comment saying gradient and pattern may be null?

+    if (gradient)
+        shader->resetLocalMatrix();

It seems like if somebody had a shader matrix on here, you will overwrite it. When you create the shader, how about scaling the existing matrix rather than setting the scale on the shader, and then restoring the original when you're done.

This patch looks OK to me otherwise.

[Stephen White]

Created attachment 28037 [details]
Revised:  save & restore shader matrix, add comments, fix bad merge

[Stephen White]

(In reply to comment #11)
> +            // Don't give zero-width spaces a width.
> +            if (!currentAdvance)
> +                continue;
> 
> It's not clear what this has to do with this patch. I also don't entirely
> understand the comment. Can you elaborate on why this needs special handling (I
> assume we would otherwise remove extra letterspacing giving negative widths).

No idea where this came from.  Must've been a bad merge; removed.

> In static bool skiaDrawText(HFONT hfont... can you add a comment saying
> gradient and pattern may be null?

Done.

> 
> +    if (gradient)
> +        shader->resetLocalMatrix();
> 
> It seems like if somebody had a shader matrix on here, you will overwrite it.
> When you create the shader, how about scaling the existing matrix rather than
> setting the scale on the shader, and then restoring the original when you're
> done.

Done.

> This patch looks OK to me otherwise.
> 

[Eric Seidel (no email)]

Comment on attachment 28037 [details]
Revised:  save & restore shader matrix, add comments, fix bad merge

I don't think getters are named with "get" in WebKIt.  Also extra { } here:

5     } else if (pattern) {
 286         shader = pattern->createPlatformPattern(transformationMatrix);
 287     }


Yup:
Precede setters with the word "set". Use bare words for getters. Setter and getter names should match the names of the variables being set/gotten.
http://webkit.org/coding/coding-style.html

[Stephen White]

Created attachment 28039 [details]
Revised:  don't use "get" for getters; trim extraneous braces

[Stephen White]

(In reply to comment #14)
> I don't think getters are named with "get" in WebKIt.

Done (I was mimicing getShadow(), getAlpha(), getCTM() etc already in GraphicsContext).

> Also extra { } here:

Done.

[Eric Seidel (no email)]

Comment on attachment 28039 [details]
Revised:  don't use "get" for getters; trim extraneous braces

I'm not a huge fan of adding the accessors, but they don't really do any harm either.

Looks fine.  FYI, be sure to mark your patches r=? so that they get reviewed.  If you don't set review = ? then they just will get ignored. ;)

http://webkit.org/quality/lifecycle.html
has more information on how the process works.

[Darin Fisher (:fishd, Google)]

Landed as http://trac.webkit.org/changeset/41274