REGRESSION: [CAIRO] wrong drawing of Gradients and Patterns on texts

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Dirk Schulze]

Created attachment 43226 [details]
Test for text with Gradient. The gradient should hav same dimension on the rect as well as the text

The rendering of texts (filled/stroked) with gradients or patterns is wrong. This is broken since r44268 http://trac.webkit.org/changeset/44268/trunk/WebCore/platform/graphics/cairo/FontCairo.cpp. Maybe the changes are to Windows specific and should be embraced by if PLATFORM(WIN)??? I got it working after undoing the changes.
There is also a LayoutTest or bug number missing on r44268 for a easy check.

[Dirk Schulze]

Added Alexander, since it is his patch, that breaks the rendering.

[Brent Fulgham]

Note: This renders properly in WinCairo.

[Dirk Schulze]

Brent gave me the link of the original bug. It's bug 21492. I unapplied the changes of FontCairo.cpp and everything works. Should we role out the patch?

[Martin Robinson]

The issue here seems the be that we are setting the Cairo source with the transformation matrix at the time of the fill, rather than with the transformation matrix at the time the fill was set on the WebCore::GraphicsContext. I should have a patch for this posted within the next couple of days that fixes quite a few SVG test cases.

[Martin Robinson]

Created attachment 83948 [details]
Patch

[Benjamin Otte]

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

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:146
> +    cairo_push_group(cr);

please add:
cairo_set_operator(cr, CAIRO_OPERATOR_SOURCE);

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:173
> +            reduceSourceByAlpha(cr, globalAlpha);

Maybe add a FIXME here to add a function for reducing color of gradients manually? As I said yesterday, you can create a new gradient pattern with the same colorstops reduced by global alpha.

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:180
> +    }

I'd factor out lines 163-180 into a common function to be called from here and prepareForStroking.

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:226
> +    cairo_restore(cr);

Is this correct now that we do global alpha in setPlatformFill()?
This should just be a cairo_fill(), no?

[Martin Robinson]

Created attachment 83986 [details]
Patch with Benjamin's suggestions

[Martin Robinson]

(In reply to comment #6)
> View in context: https://bugs.webkit.org/attachment.cgi?id=83948&action=review

Thanks for the suggestions.

> > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:146
> > +    cairo_push_group(cr);
> 
> please add:
> cairo_set_operator(cr, CAIRO_OPERATOR_SOURCE);

Done!

> 
> > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:173
> > +            reduceSourceByAlpha(cr, globalAlpha);
> 
> Maybe add a FIXME here to add a function for reducing color of gradients manually? As I said yesterday, you can create a new gradient pattern with the same colorstops reduced by global alpha.

Done!

> 
> > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:180
> > +    }
> 
> I'd factor out lines 163-180 into a common function to be called from here and prepareForStroking.

Done!

> > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:226
> > +    cairo_restore(cr);
> 
> Is this correct now that we do global alpha in setPlatformFill()?
> This should just be a cairo_fill(), no?

Since I passed in the NoAdjustment argument here, prepareForFilling didn't adjust the source patterns for global alpha. Since we can clip here, I think that cairo_paint_with_alpha might be faster here.

[Brent Fulgham]

Comment on attachment 83986 [details]
Patch with Benjamin's suggestions

I like this change a lot.  It simplifies a number of rather confusing routines.

[Martin Robinson]

Created attachment 89073 [details]
Patch updated to ToT and using PlatformContextCairo

[Dirk Schulze]

Comment on attachment 89073 [details]
Patch updated to ToT and using PlatformContextCairo

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

Great patch. r=me

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:135
> +    cairo_set_source_rgb(shadowContext, 1.0, 0.0, 0.0);

use 1 and 0

[Eric Seidel (no email)]

Comment on attachment 89073 [details]
Patch updated to ToT and using PlatformContextCairo

I feel like this patch may be wrong.  Because you should probably be more like CG, and not need to use setPlatformPattern, instead just use the ctm at draw time.

But, this passes the test.  This stuff is hard, and requires iteration.  I think we should land this patch and work from here.  Talk to krit.

[Dirk Schulze]

(In reply to comment #12)
> (From update of attachment 89073 [details])
> I feel like this patch may be wrong.  Because you should probably be more like CG, and not need to use setPlatformPattern, instead just use the ctm at draw time.
> 
> But, this passes the test.  This stuff is hard, and requires iteration.  I think we should land this patch and work from here.  Talk to krit.

The reason why we do it the way in CG is different to Cairo. With CG you can transform patterns, gradients and the context idependent of each other. Cairo is able to do so.

[Martin Robinson]

Comment on attachment 89073 [details]
Patch updated to ToT and using PlatformContextCairo

It seems that I misunderstood the root cause of this issue. It turns out that that we just need to ensure that the pattern/gradient is set on the context before transforming for the final text render. I've uploaded a greatly simplified patch for this issue, which is also based on ToT. Sorry for the patch spam.

[Eric Seidel (no email)]

Comment on attachment 89073 [details]
Patch updated to ToT and using PlatformContextCairo

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

Hmm... I predict you're gonna rewrite this again... I rewrote the gradient stuff for CG at least 3 times before it got to its current state...

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:786
> +    platformContext()->updateStrokeTransform();

I'm still not sure why you need the transform saved at this time and can't just use the CTM when the drawing is done?

> Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:39
> +    , m_strokeTransform(static_cast<cairo_matrix_t*>(fastMalloc(sizeof(cairo_matrix_t))))

Wow.  Really?  They don't provide any create function?

> Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:48
> +    fastFree(m_strokeTransform);

Why not just use an OwnPtr if we're just gonna malloc and free?  I guess ownptr calls delete... which would be bad... or?

> Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:101
> +    if (alpha >= 1)

What would this even mean?

[Martin Robinson]

Created attachment 92192 [details]
Patch

[Martin Robinson]

(In reply to comment #15)
> (From update of attachment 89073 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89073&action=review
> Hmm... I predict you're gonna rewrite this again... I rewrote the gradient stuff for CG at least 3 times before it got to its current state...

It's surprisingly tricky!

> 
> > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:786
> > +    platformContext()->updateStrokeTransform();
> 
> I'm still not sure why you need the transform saved at this time and can't just use the CTM when the drawing is done?
> 
> > Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:39
> > +    , m_strokeTransform(static_cast<cairo_matrix_t*>(fastMalloc(sizeof(cairo_matrix_t))))
> 
> Wow.  Really?  They don't provide any create function?
> 
> > Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:48
> > +    fastFree(m_strokeTransform);
> 
> Why not just use an OwnPtr if we're just gonna malloc and free?  I guess ownptr calls delete... which would be bad... or?

Apolgies! There was a delay in uploading the new patch because of a scrollbar race condition. I'm concurrently working on that as well. The code above no longer exists.
> 
> > Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:101
> > +    if (alpha >= 1)
> What would this even mean?

It seemed safer given the range of floats. I can use an ASSERT here instead, if you think that would be better.

[Dirk Schulze]

Comment on attachment 92192 [details]
Patch

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

> Source/WebCore/platform/graphics/cairo/FontCairo.cpp:51
> +        static const float syntheticObliqueSkew = -tanf(14 * acosf(0) / 90);

I'd move this to the top of the file. This is common style in webkit for static consts.

> Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:102
> +        RefPtr<cairo_pattern_t> cairoPattern(adoptRef(pattern->createPlatformPattern(AffineTransform())));
> +        cairo_set_source(cr, cairoPattern.get());

Again, it looks like you change the CTM of the context to match the pattern space instead of doing it the other way around. We have to d it in CG, because CG doesn't provide a pattern transform. But cairo has a second transform. Just unapply every change you made to the context to get italic fonts working from the pattern and you're done. No extra changes to the CTM please.

[Martin Robinson]

Comment on attachment 92192 [details]
Patch

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

>> Source/WebCore/platform/graphics/cairo/FontCairo.cpp:51
>> +        static const float syntheticObliqueSkew = -tanf(14 * acosf(0) / 90);
> 
> I'd move this to the top of the file. This is common style in webkit for static consts.

Sure thing. I'll upload a new patch.

>> Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:102
>> +        cairo_set_source(cr, cairoPattern.get());
> 
> Again, it looks like you change the CTM of the context to match the pattern space instead of doing it the other way around. We have to d it in CG, because CG doesn't provide a pattern transform. But cairo has a second transform. Just unapply every change you made to the context to get italic fonts working from the pattern and you're done. No extra changes to the CTM please.

I'm not sure I understand what you mean here. The pattern is transformed with cairo_pattern_set_matrix in Pattern::createPlatformPattern, but not here. If I understand what you're saying correctly, I'm doing what you suggest already. :)

[Martin Robinson]

Created attachment 92248 [details]
Patch with Dirk's suggestion

[Dirk Schulze]

Comment on attachment 92248 [details]
Patch with Dirk's suggestion

r=me

[Martin Robinson]

Committed r89314: <http://trac.webkit.org/changeset/89314>

[Martin Robinson]

Comment on attachment 92248 [details]
Patch with Dirk's suggestion

Clearing flags after landing. Thanks for the review Dirk!