While implementing https://bugs.webkit.org/show_bug.cgi?id=41308 I found that portions of the new code expect that GraphicsContext::addPath should not affect subsequent drawing. Furthermore, the path added to the GC via addPath should be unaffected by any drawn done that does not use expect to use the current GC path directly. For instance, one should be able to call: 1. GraphicsContext::addPath 2. GraphicsContext::drawConvexPolygon 3. GraphicsContext::strokePath In this scenario, the call to drawConvexPolygon should not modify the path added via addPath. Likewise, the polygon drawn in step two should not be affected by the path on the GC from addPath.
Created attachment 61084 [details] Patch for this issue
Comment on attachment 61084 [details] Patch for this issue > diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog > index 4c7f4a6e441aa83fac483dda6abedbffabe6a225..baef5072848f85bfbffb6df354cc3832cdd85b05 100644 > --- a/WebCore/ChangeLog > +++ b/WebCore/ChangeLog > @@ -1,3 +1,46 @@ > +2010-07-09 Martin Robinson <mrobinson@igalia.com> > + > + Reviewed by NOBODY (OOPS!). > + > + [GTK] Bring behavior of paths on the Cairo GraphicsContext into line with the CoreGraphics port > + https://bugs.webkit.org/show_bug.cgi?id=41732 > + > + Make drawing routines on the Cairo graphics context independent from path manipulation. > + This means that drawing shapes does not affect in any way paths added to the context via > + addPath. Furthermore, paths added to the context do not affect shapes drawn via methods > + such as drawEllipse. cairo_save() and cairo_restore() do not affect any path that is on > + the cairo context, so we do this manually. > + > + This behavior is necessary to close bug https://bugs.webkit.org/show_bug.cgi?id=41308 > + so tests for that issue will test this fix. > + > + * platform/graphics/cairo/GraphicsContextCairo.cpp: > + (WebCore::GraphicsContextPlatformPrivate::saveCairoPath): Added. This method > + saves the current cairo path to a member variable and begins a new path on > + the context. > + (WebCore::GraphicsContextPlatformPrivate::restoreCairoPath): Added. Restores the > + saved cairo path to the context if one exists. > + (WebCore::GraphicsContext::drawRect): Guard the previous path on the cairo context by > + calling the saveCairoPath/restoreCairoPath pair. > + (WebCore::GraphicsContext::drawLine): Ditto. > + (WebCore::GraphicsContext::drawEllipse): Ditto > + (WebCore::GraphicsContext::strokeArc): Ditto > + (WebCore::GraphicsContext::drawConvexPolygon): Ditto. > + (WebCore::GraphicsContext::fillRect): Ditto. > + (WebCore::GraphicsContext::clip): Ditto. > + (WebCore::GraphicsContext::clipPath): Ditto. > + (WebCore::GraphicsContext::drawFocusRing): Ditto. > + (WebCore::GraphicsContext::drawLineForMisspellingOrBadGrammar): Ditto > + (WebCore::GraphicsContext::addInnerRoundedRectClip): Ditto. > + (WebCore::GraphicsContext::clearRect): Ditto. > + (WebCore::GraphicsContext::strokeRect): Ditto. > + (WebCore::GraphicsContext::clipOut): Ditto. > + (WebCore::GraphicsContext::fillRoundedRect): Ditto. > + * platform/graphics/cairo/GraphicsContextPlatformPrivateCairo.h: Added a member variable > + for caching a cairo path so that it can be saved and restored while doing other operations. > + (WebCore::GraphicsContextPlatformPrivate::GraphicsContextPlatformPrivate): Intialize the > + saved path to 0. > + > 2010-07-08 Jon Honeycutt <jhoneycutt@apple.com> > > Missing plug-ins may cause an assertion failure. > diff --git a/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp b/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp > index 8b62267b365e959e67727e92f7fff614a0d1f5a7..5931039787085b71387e8e4cfa3358642d489428 100644 > --- a/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp > +++ b/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp > @@ -143,6 +143,25 @@ static inline void copyContextProperties(cairo_t* srcCr, cairo_t* dstCr) > cairo_set_fill_rule(dstCr, cairo_get_fill_rule(srcCr)); > } > > +void GraphicsContextPlatformPrivate::saveCairoPath() > +{ > + if (m_savedCairoPath) > + cairo_path_destroy(m_savedCairoPath); > + m_savedCairoPath = cairo_copy_path(cr); > + cairo_new_path(cr); > +} > + > +void GraphicsContextPlatformPrivate::restoreCairoPath() > +{ > + if (!m_savedCairoPath) > + return; > + > + cairo_new_path(cr); > + cairo_append_path(cr, m_savedCairoPath); > + cairo_path_destroy(m_savedCairoPath); > + m_savedCairoPath = 0; > +} > + > void GraphicsContext::calculateShadowBufferDimensions(IntSize& shadowBufferSize, FloatRect& shadowRect, float& kernelSize, const FloatRect& sourceRect, const FloatSize& shadowSize, float shadowBlur) > { > #if ENABLE(FILTERS) > @@ -249,6 +268,7 @@ void GraphicsContext::drawRect(const IntRect& rect) > > cairo_t* cr = m_data->cr; > cairo_save(cr); > + m_data->saveCairoPath(); > > if (fillColor().alpha()) > fillRectSourceOver(cr, rect, fillColor()); > @@ -263,6 +283,7 @@ void GraphicsContext::drawRect(const IntRect& rect) > } > > cairo_restore(cr); > + m_data->restoreCairoPath(); > } > > // This is only used to draw borders. > @@ -277,6 +298,7 @@ void GraphicsContext::drawLine(const IntPoint& point1, const IntPoint& point2) > > cairo_t* cr = m_data->cr; > cairo_save(cr); > + m_data->saveCairoPath(); > > float width = strokeThickness(); > if (width < 1) > @@ -354,6 +376,7 @@ void GraphicsContext::drawLine(const IntPoint& point1, const IntPoint& point2) > > cairo_stroke(cr); > cairo_restore(cr); > + m_data->restoreCairoPath(); > } > > // This method is only used to draw the little circles used in lists. > @@ -364,6 +387,7 @@ void GraphicsContext::drawEllipse(const IntRect& rect) > > cairo_t* cr = m_data->cr; > cairo_save(cr); > + m_data->saveCairoPath(); > float yRadius = .5 * rect.height(); > float xRadius = .5 * rect.width(); > cairo_translate(cr, rect.x() + xRadius, rect.y() + yRadius); > @@ -382,7 +406,7 @@ void GraphicsContext::drawEllipse(const IntRect& rect) > cairo_stroke(cr); > } > > - cairo_new_path(cr); > + m_data->restoreCairoPath(); > } > > void GraphicsContext::strokeArc(const IntRect& rect, int startAngle, int angleSpan) > @@ -404,6 +428,7 @@ void GraphicsContext::strokeArc(const IntRect& rect, int startAngle, int angleSp > > cairo_t* cr = m_data->cr; > cairo_save(cr); > + m_data->saveCairoPath(); > > if (w != h) > cairo_scale(cr, 1., scaleFactor); > @@ -469,6 +494,7 @@ void GraphicsContext::strokeArc(const IntRect& rect, int startAngle, int angleSp > > cairo_stroke(cr); > cairo_restore(cr); > + m_data->restoreCairoPath(); > } > > void GraphicsContext::drawConvexPolygon(size_t npoints, const FloatPoint* points, bool shouldAntialias) > @@ -480,8 +506,9 @@ void GraphicsContext::drawConvexPolygon(size_t npoints, const FloatPoint* points > return; > > cairo_t* cr = m_data->cr; > - > cairo_save(cr); > + m_data->saveCairoPath(); > + > cairo_set_antialias(cr, shouldAntialias ? CAIRO_ANTIALIAS_DEFAULT : CAIRO_ANTIALIAS_NONE); > cairo_move_to(cr, points[0].x(), points[0].y()); > for (size_t i = 1; i < npoints; i++) > @@ -502,6 +529,7 @@ void GraphicsContext::drawConvexPolygon(size_t npoints, const FloatPoint* points > > cairo_new_path(cr); > cairo_restore(cr); > + m_data->restoreCairoPath(); > } > > void GraphicsContext::clipConvexPolygon(size_t numPoints, const FloatPoint* points) > @@ -563,8 +591,14 @@ void GraphicsContext::fillRect(const FloatRect& rect) > return; > > cairo_t* cr = m_data->cr; > + cairo_save(cr); > + m_data->saveCairoPath(); > + > cairo_rectangle(cr, rect.x(), rect.y(), rect.width(), rect.height()); > fillPath(); > + > + cairo_restore(cr); > + m_data->restoreCairoPath(); > } > > static void drawBorderlessRectShadow(GraphicsContext* context, const FloatRect& rect, const Color& rectColor) > @@ -596,9 +630,16 @@ void GraphicsContext::fillRect(const FloatRect& rect, const Color& color, ColorS > if (paintingDisabled()) > return; > > + cairo_t* cr = m_data->cr; > + cairo_save(cr); > + m_data->saveCairoPath(); > + > drawBorderlessRectShadow(this, rect, color); > if (color.alpha()) > fillRectSourceOver(m_data->cr, rect, color); > + > + cairo_restore(cr); > + m_data->restoreCairoPath(); > } > > void GraphicsContext::clip(const FloatRect& rect) > @@ -607,12 +648,16 @@ void GraphicsContext::clip(const FloatRect& rect) > return; > > cairo_t* cr = m_data->cr; > + m_data->saveCairoPath(); > + > cairo_rectangle(cr, rect.x(), rect.y(), rect.width(), rect.height()); > cairo_fill_rule_t savedFillRule = cairo_get_fill_rule(cr); > cairo_set_fill_rule(cr, CAIRO_FILL_RULE_WINDING); > cairo_clip(cr); > cairo_set_fill_rule(cr, savedFillRule); > m_data->clip(rect); > + > + m_data->restoreCairoPath(); > } > > void GraphicsContext::clipPath(WindRule clipRule) > @@ -623,6 +668,7 @@ void GraphicsContext::clipPath(WindRule clipRule) > cairo_t* cr = m_data->cr; > cairo_set_fill_rule(cr, clipRule == RULE_EVENODD ? CAIRO_FILL_RULE_EVEN_ODD : CAIRO_FILL_RULE_WINDING); > cairo_clip(cr); > + cairo_new_path(cr); > } > > void GraphicsContext::drawFocusRing(const Vector<Path>& paths, int width, int offset, const Color& color) > @@ -639,6 +685,7 @@ void GraphicsContext::drawFocusRing(const Vector<IntRect>& rects, int width, int > > cairo_t* cr = m_data->cr; > cairo_save(cr); > + m_data->saveCairoPath(); > cairo_push_group(cr); > cairo_new_path(cr); > > @@ -691,6 +738,7 @@ void GraphicsContext::drawFocusRing(const Vector<IntRect>& rects, int width, int > cairo_set_operator(cr, CAIRO_OPERATOR_OVER); > cairo_paint(cr); > cairo_restore(cr); > + m_data->restoreCairoPath(); > } > > void GraphicsContext::drawLineForText(const IntPoint& origin, int width, bool printing) > @@ -719,6 +767,7 @@ void GraphicsContext::drawLineForMisspellingOrBadGrammar(const IntPoint& origin, > > cairo_t* cr = m_data->cr; > cairo_save(cr); > + m_data->saveCairoPath(); > > // Convention is green for grammar, red for spelling > // These need to become configurable > @@ -735,6 +784,7 @@ void GraphicsContext::drawLineForMisspellingOrBadGrammar(const IntPoint& origin, > #endif > > cairo_restore(cr); > + m_data->restoreCairoPath(); > } > > FloatRect GraphicsContext::roundToDevicePixels(const FloatRect& frect) > @@ -844,6 +894,8 @@ void GraphicsContext::addInnerRoundedRectClip(const IntRect& rect, int thickness > if (paintingDisabled()) > return; > > + m_data->saveCairoPath(); > + > clip(rect); > > Path p; > @@ -860,6 +912,8 @@ void GraphicsContext::addInnerRoundedRectClip(const IntRect& rect, int thickness > cairo_set_fill_rule(cr, CAIRO_FILL_RULE_EVEN_ODD); > cairo_clip(cr); > cairo_set_fill_rule(cr, savedFillRule); > + > + m_data->restoreCairoPath(); > } > > void GraphicsContext::clipToImageBuffer(const FloatRect& rect, const ImageBuffer* imageBuffer) > @@ -949,12 +1003,15 @@ void GraphicsContext::clearRect(const FloatRect& rect) > return; > > cairo_t* cr = m_data->cr; > - > cairo_save(cr); > + m_data->saveCairoPath(); > + > cairo_rectangle(cr, rect.x(), rect.y(), rect.width(), rect.height()); > cairo_set_operator(cr, CAIRO_OPERATOR_CLEAR); > cairo_fill(cr); > + > cairo_restore(cr); > + m_data->restoreCairoPath(); > } > > void GraphicsContext::strokeRect(const FloatRect& rect, float width) > @@ -964,10 +1021,14 @@ void GraphicsContext::strokeRect(const FloatRect& rect, float width) > > cairo_t* cr = m_data->cr; > cairo_save(cr); > + m_data->saveCairoPath(); > + > cairo_rectangle(cr, rect.x(), rect.y(), rect.width(), rect.height()); > cairo_set_line_width(cr, width); > strokePath(); > + > cairo_restore(cr); > + m_data->restoreCairoPath(); > } > > void GraphicsContext::setLineCap(LineCap lineCap) > @@ -1104,6 +1165,8 @@ void GraphicsContext::clip(const Path& path) > return; > > cairo_t* cr = m_data->cr; > + m_data->saveCairoPath(); > + > cairo_path_t* p = cairo_copy_path(path.platformPath()->m_cr); > cairo_append_path(cr, p); > cairo_path_destroy(p); > @@ -1112,6 +1175,8 @@ void GraphicsContext::clip(const Path& path) > cairo_clip(cr); > cairo_set_fill_rule(cr, savedFillRule); > m_data->clip(path); > + > + m_data->restoreCairoPath(); > } > > void GraphicsContext::canvasClip(const Path& path) > @@ -1125,15 +1190,18 @@ void GraphicsContext::clipOut(const Path& path) > return; > > cairo_t* cr = m_data->cr; > + m_data->saveCairoPath(); > + cairo_fill_rule_t savedFillRule = cairo_get_fill_rule(cr); > + > double x1, y1, x2, y2; > cairo_clip_extents(cr, &x1, &y1, &x2, &y2); > cairo_rectangle(cr, x1, y1, x2 - x1, y2 - y1); > addPath(path); > - > - cairo_fill_rule_t savedFillRule = cairo_get_fill_rule(cr); > cairo_set_fill_rule(cr, CAIRO_FILL_RULE_EVEN_ODD); > cairo_clip(cr); > + > cairo_set_fill_rule(cr, savedFillRule); > + m_data->restoreCairoPath(); > } > > void GraphicsContext::rotate(float radians) > @@ -1160,14 +1228,18 @@ void GraphicsContext::clipOut(const IntRect& r) > return; > > cairo_t* cr = m_data->cr; > + m_data->saveCairoPath(); > + cairo_fill_rule_t savedFillRule = cairo_get_fill_rule(cr); > + > double x1, y1, x2, y2; > cairo_clip_extents(cr, &x1, &y1, &x2, &y2); > cairo_rectangle(cr, x1, y1, x2 - x1, y2 - y1); > cairo_rectangle(cr, r.x(), r.y(), r.width(), r.height()); > - cairo_fill_rule_t savedFillRule = cairo_get_fill_rule(cr); > cairo_set_fill_rule(cr, CAIRO_FILL_RULE_EVEN_ODD); > cairo_clip(cr); > + > cairo_set_fill_rule(cr, savedFillRule); > + m_data->restoreCairoPath(); > } > > void GraphicsContext::clipOutEllipseInRect(const IntRect& r) > @@ -1186,13 +1258,17 @@ void GraphicsContext::fillRoundedRect(const IntRect& r, const IntSize& topLeft, > return; > > cairo_t* cr = m_data->cr; > + m_data->saveCairoPath(); > + > cairo_save(cr); > beginPath(); > addPath(Path::createRoundedRectangle(r, topLeft, topRight, bottomLeft, bottomRight)); > setColor(cr, color); > drawPathShadow(this, m_common, true, false); > cairo_fill(cr); > + > cairo_restore(cr); > + m_data->restoreCairoPath(); > } > > #if PLATFORM(GTK) > diff --git a/WebCore/platform/graphics/cairo/GraphicsContextPlatformPrivateCairo.h b/WebCore/platform/graphics/cairo/GraphicsContextPlatformPrivateCairo.h > index 5e388324dcd6fb1e71db1ea582be69743a0b8eb3..b6c07e7ffba71291cf7279f2bb22b89c9ea31d85 100644 > --- a/WebCore/platform/graphics/cairo/GraphicsContextPlatformPrivateCairo.h > +++ b/WebCore/platform/graphics/cairo/GraphicsContextPlatformPrivateCairo.h > @@ -45,6 +45,7 @@ class GraphicsContextPlatformPrivate { > public: > GraphicsContextPlatformPrivate() > : cr(0) > + , m_savedCairoPath(0) > #if PLATFORM(GTK) > , expose(0) > #elif PLATFORM(WIN) > @@ -52,6 +53,7 @@ public: > , m_hdc(0) > , m_transparencyCount(0) > , m_shouldIncludeChildWindows(false) > + , m_shouldIncludeChildWindows(false) > #endif > { > } > @@ -61,6 +63,9 @@ public: > cairo_destroy(cr); > } > > + void saveCairoPath(); > + void restoreCairoPath(); > + > #if PLATFORM(WIN) > // On Windows, we need to update the HDC for form controls to draw in the right place. > void save(); > @@ -95,6 +100,7 @@ public: > > cairo_t* cr; > Vector<float> layers; > + cairo_path_t* m_savedCairoPath; > > #if PLATFORM(GTK) > GdkEventExpose* expose; WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:149 + cairo_path_destroy(m_savedCairoPath); Do we know that we will only ever be one level deep? Could the 'saveCairoPath' be hit during a recursive call to the graphic context?
Comment on attachment 61084 [details] Patch for this issue Krit has suggested that I could do this in a way that didn't affect the performance of some of the hotter parts of the GraphicsContext (drawRect, etc).
Created attachment 62904 [details] Patch with fewer performance implications
Comment on attachment 62904 [details] Patch with fewer performance implications You forgot some context transformations like scale and tranlate. They need to do path transformation similar to ctm transformation. Also did you check, if save and restore infulence the path transformations as well? You can test this with HTML Canvas: context->translate(10,10); context->moveTo(0,0); context->lineTo(100,0); context->save(); context->scale(0,5); context->lineTo(100,100); context->restore(); context->lineTo(0,100); context->closePath; context->stroke(); Compare the result with firefox and you'll see if you need extra logic for save/restore.
(In reply to comment #5) > (From update of attachment 62904 [details]) > You forgot some context transformations like scale and tranlate. They need to do path transformation similar to ctm transformation. > > Also did you check, if save and restore infulence the path transformations as well? You can test this with HTML Canvas: > context->translate(10,10); > context->moveTo(0,0); > context->lineTo(100,0); > context->save(); > context->scale(0,5); > context->lineTo(100,100); > context->restore(); > context->lineTo(0,100); > context->closePath; > context->stroke(); > > Compare the result with firefox and you'll see if you need extra logic for save/restore. haha, the scale should be 0.5 not 0,5 :-)
Created attachment 62982 [details] Patch which instead handles the path transformation matrix in addPath
Per our discussion on IRC, we should only need to update the transformation right before adding the path to the m_pendingPath context. This means there is a little bit of overhead to calling addPath, but the benefit is that there is no extra overhead for changing the transformation matrix at other points in the class. This seems like an acceptable trade-off.
Comment on attachment 62982 [details] Patch which instead handles the path transformation matrix in addPath Great patch. Just one note: > + cairo_t* pathContext = m_data->m_pendingPath.m_cr; > + cairo_path_t* cairoPath = cairo_copy_path(pathContext); > + cairo_new_path(pathContext); > + cairo_append_path(cr, cairoPath); > + cairo_path_destroy(cairoPath); Cany ou move this into a static function and add a comment about the intention?
Created attachment 62995 [details] Updated patch with Dirk's suggestions
Comment on attachment 62995 [details] Updated patch with Dirk's suggestions You don't need to write the comment three times. It's ok to write it once right before +static void setPathOnCairoContext(cairo_t* to, cairo_t* from) :-) I give r+ but please change the style to the suggestion befor landing.
Committed r64318: <http://trac.webkit.org/changeset/64318>
Reverted r64318 for reason: This change broke many tests. Committed r64322: <http://trac.webkit.org/changeset/64322>
It turns out that fillRect/strokeRect relied on fillPath/strokeRect and should not have. I've moved the common logic to helper functions fill/strokeCurrentCairoPath and now fillRect, strokeRect, fillPath and strokePath all call those helpers.
Created attachment 63018 [details] Patch which also removes invalid calls to strokePath and fillPath
(In reply to comment #15) > Created an attachment (id=63018) [details] > Patch which also removes invalid calls to strokePath and fillPath Are you sure that they are invalid? IIRC they are just used from HTML Canvas and should be handled similar like Paths, means replace the current Path. Doesn't Cg replace the current path with this rect? You can test it again with HTML Canvas: canvas->moveTo(10,10); canvas->lineTo(110,10); canvas->lineTo(110,110); canvas->lineTo(10,110); canvas->closePath(); canvas->strokeRect(120,10,100,100); canvas->stroke(); If just see the right rect, Safari replaces the currenPath with the rect.
Comment on attachment 63018 [details] Patch which also removes invalid calls to strokePath and fillPath Ah forgot that Canvas handles it's path itself :-/ So your way is possibly faster for strokeRect/fillRect.
Comment on attachment 63018 [details] Patch which also removes invalid calls to strokePath and fillPath Landed http://trac.webkit.org/changeset/64318
Not usre if this patch might have introduce a regression. Some strokes on paths (like in http://dev.w3.org/SVG/profiles/1.1F2/test/harness/htmlObject/paths-data-01-t.html) don't get antialiased. E.g at the black stroke of the yellow circle, or the blue half-circle. Martin, can you investigate on this?
(In reply to comment #19) > E.g at the black stroke of the yellow circle, or the blue half-circle. Hrm. These still look anti-aliased to me on a recent GTK+ build. Do you have any further tips for reproducing this issue?
Created attachment 64019 [details] Screenshot A screenshot of the rendering. On the left hand side an older build, on the right hand side trunk. Compare the borders. You can see it best on the shape with the red stroke and on the blue filled stroke. The anti-aliasing is off. But it doesn't seem the case for the two unfilled shapes. Maybe a problem with shapes, that get filled and stroked?!? Not sure.