Bug 31507 - REGRESSION: [CAIRO] wrong drawing of Gradients and Patterns on texts
: REGRESSION: [CAIRO] wrong drawing of Gradients and Patterns on texts
Status: RESOLVED FIXED
: WebKit
WebKit Gtk
: 525.x (Safari 3.1)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-11-14 02:57 PST by
Modified: 2011-06-20 16:52 PST (History)


Attachments
Test for text with Gradient. The gradient should hav same dimension on the rect as well as the text (673 bytes, image/svg+xml)
2009-11-14 02:57 PST, Dirk Schulze
no flags Details
Patch (1.91 MB, patch)
2011-02-26 12:42 PST, Martin Robinson
no flags Review Patch | Details | Formatted Diff | Diff
Patch with Benjamin's suggestions (1.90 MB, patch)
2011-02-27 12:41 PST, Martin Robinson
no flags Review Patch | Details | Formatted Diff | Diff
Patch updated to ToT and using PlatformContextCairo (1.72 MB, patch)
2011-04-11 13:45 PST, Martin Robinson
no flags Review Patch | Details | Formatted Diff | Diff
Patch (1.34 MB, patch)
2011-05-03 22:21 PST, Martin Robinson
no flags Review Patch | Details | Formatted Diff | Diff
Patch with Dirk's suggestion (1.34 MB, patch)
2011-05-04 08:39 PST, Martin Robinson
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-11-14 02:57:33 PST
Created an attachment (id=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.
------- Comment #1 From 2009-11-22 12:46:38 PST -------
Added Alexander, since it is his patch, that breaks the rendering.
------- Comment #2 From 2009-12-02 16:20:30 PST -------
Note: This renders properly in WinCairo.
------- Comment #3 From 2009-12-03 14:44:59 PST -------
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?
------- Comment #4 From 2011-02-25 18:33:47 PST -------
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.
------- Comment #5 From 2011-02-26 12:42:31 PST -------
Created an attachment (id=83948) [details]
Patch
------- Comment #6 From 2011-02-26 14:51:03 PST -------
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?
------- Comment #7 From 2011-02-27 12:41:20 PST -------
Created an attachment (id=83986) [details]
Patch with Benjamin's suggestions
------- Comment #8 From 2011-02-27 12:41:49 PST -------
(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.
------- Comment #9 From 2011-04-10 22:31:30 PST -------
(From update of attachment 83986 [details])
I like this change a lot.  It simplifies a number of rather confusing routines.
------- Comment #10 From 2011-04-11 13:45:55 PST -------
Created an attachment (id=89073) [details]
Patch updated to ToT and using PlatformContextCairo
------- Comment #11 From 2011-04-26 16:41:12 PST -------
(From update of attachment 89073 [details])
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
------- Comment #12 From 2011-04-26 16:51:32 PST -------
(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.
------- Comment #13 From 2011-04-26 17:02:06 PST -------
(In reply to comment #12)
> (From update of attachment 89073 [details] [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.
------- Comment #14 From 2011-05-03 21:46:46 PST -------
(From update of attachment 89073 [details])
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.
------- Comment #15 From 2011-05-03 22:18:06 PST -------
(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...

> 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?
------- Comment #16 From 2011-05-03 22:21:22 PST -------
Created an attachment (id=92192) [details]
Patch
------- Comment #17 From 2011-05-03 22:25:43 PST -------
(In reply to comment #15)
> (From update of attachment 89073 [details] [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.
------- Comment #18 From 2011-05-04 00:27:45 PST -------
(From update of attachment 92192 [details])
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.
------- Comment #19 From 2011-05-04 08:24:17 PST -------
(From update of attachment 92192 [details])
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. :)
------- Comment #20 From 2011-05-04 08:39:01 PST -------
Created an attachment (id=92248) [details]
Patch with Dirk's suggestion
------- Comment #21 From 2011-06-18 22:39:16 PST -------
(From update of attachment 92248 [details])
r=me
------- Comment #22 From 2011-06-20 16:52:02 PST -------
Committed r89314: <http://trac.webkit.org/changeset/89314>
------- Comment #23 From 2011-06-20 16:52:33 PST -------
(From update of attachment 92248 [details])
Clearing flags after landing. Thanks for the review Dirk!