Bug 37986 – SVGPaintServer needs to be converted to the new RenderSVGResource* system

• Component: SVG
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

SVGPaintServer needs to be converted to the new RenderSVGResource* system.
The big patch removing SVGPaintServer, and designing it just like the other resources (clipper/filter/masker/marker) is done. It's a huge speed-up, patches following soon.

[Nikolas Zimmermann]

Created attachment 54052 [details]
File moves / build system changes

[Dirk Schulze]

(In reply to comment #1)
> Created an attachment (id=54052) [details]
> Patch

What for a garbage ;-)

[Nikolas Zimmermann]

Hrmpf, just because SVGResourceListener.h is empty the ews has troubles, more specifically svn-apply.
I don't really get it, as I created the patch using "webkit-patch post"....

[Dirk Schulze]

Comment on attachment 54052 [details]
File moves / build system changes

r=me.

[Nikolas Zimmermann]

Committed r58093: <http://trac.webkit.org/changeset/58093>

[Nikolas Zimmermann]

Heh, forgot to tell webkit-patch not to close this bug, it's our master bug for the transition.

[Nikolas Zimmermann]

Comment on attachment 54052 [details]
File moves / build system changes

Marking first patch as obsolete, it has been landed and was just a preparation.

[Nikolas Zimmermann]

Created attachment 54062 [details]
LayoutTests changes

[Nikolas Zimmermann]

Created attachment 54063 [details]
WebCore changes

Huuuuuge speedup, try zooming/panning around pattern/gradient tests :-)

[Dirk Schulze]

(In reply to comment #9)
> Created an attachment (id=54063) [details]
> WebCore changes
> 
> Huuuuuge speedup, try zooming/panning around pattern/gradient tests :-)

In RenderSVGresource.cpp

 41     ASSERT(svgElement && svgElement->isStyled());

 two asserts please.

 I dislike the general changes on applyResource(), Can't we set FillMode or StrokeMode as a locale variable during the fillPaintingResource/strokePaintingResources calls?
 Solid-, Pattern-, GradientResources wold ask for it in applyResource. It's not quite understandable to have this in Clipper, Masker, Marker and Filter, without needing it as well. The same for postApplyResource.

 What is anyQName() in RenderSVGResourceGradient and Pattern? Can we get a better name for it?


RenderSVGResourcePattern:

 245     OwnPtr<ImageBuffer> tileImage = ImageBuffer::create(imageSize);

 295     OwnPtr<ImageBuffer> newTileImage = ImageBuffer::create(IntSize(tileWidth, tileHeight));

 Can you add check's right before creating the ImageBuffers, if both sides will be >= 1?


  63     PassOwnPtr<ImageBuffer> createTileImage(FloatRect& patternBoundaries, AffineTransform& patternTransform, const SVGPatternElement*, RenderObject*) const;

Why do you pass the PatternElement, do you think it's a speed win?

 85 #if PLATFORM(SKIA)
 86     // FIXME: Move this into the GraphicsContext
 87     // WebKit implicitly expects us to reset the path.
 88     // For example in fillAndStrokePath() of RenderPath.cpp the path is 
 89     // added back to the context after filling. This is because internally it
 90     // calls CGContextFillPath() which closes the path.
 91     context->beginPath();
 92     context->platformContext()->setFillShader(0);
 93     context->platformContext()->setStrokeShader(0);
 94 #endif

 I think thats a general issue with Skia and mainly affects Patterns and Gradients. This code might be placed in GraphicsContextSkia directly.


  149         // FIXME: Why doesn't ColorStop take a Color and an offset??

You wrote this line in SVGGradientElement.cpp, but 4 lines later you just do it :-)

Realy great patch!!!

[Dirk Schulze]

(In reply to comment #8)
> Created an attachment (id=54062) [details]
> LayoutTests changes

The new DRT results look realy great!! Can you explain why pservers-grad-19-b
looks different? It was broken, but the new reult doesn't look better!

[Nikolas Zimmermann]

(In reply to comment #10)
> (In reply to comment #9)
> > Created an attachment (id=54063) [details] [details]
> > WebCore changes
> > 
> > Huuuuuge speedup, try zooming/panning around pattern/gradient tests :-)
> 
> In RenderSVGresource.cpp
> 
>  41     ASSERT(svgElement && svgElement->isStyled());
> 
>  two asserts please.

Done.

> 
>  I dislike the general changes on applyResource(), Can't we set FillMode or
> StrokeMode as a locale variable during the
> fillPaintingResource/strokePaintingResources calls?
>  Solid-, Pattern-, GradientResources wold ask for it in applyResource. It's not
> quite understandable to have this in Clipper, Masker, Marker and Filter,
> without needing it as well. The same for postApplyResource.

Remember the first version of my patch? It contains the same concept that you're demanding.
I had "resourceMode()" and "setResourceMode()" methods in RenderSVGResource. For paths everything worked well, though in text rendering the concept has a problem. Let me explain:

When rendering text SVGRootInlineBox sets up the painting resources (setupFill/setupStroke) etc, and then SVGInlineTextBox is used  to actually draw the text (drawText() on GraphicsContext). For regular text this works fine. When the rendering happens through SVGFont, paths have to be filled/stroked.
The problem is that SVGRootInlineBox sets setResourceMode(ApplyToFillMode | ApplyToTextMode) (to indicate text rendering) and SVGFont calls setResourceMode(ApplyToFillMode). The information that we're actually rendering text is lost, so we'd have to preserve the resourceMode, _before_ calling applyResource, and restoring it after calling postApplyResource. I would have done that though the problem is a bit more complex, as it affects more places than just SVGFont.

The problem is completely gone when _not_ storing the resourceMode anywhere, but explicitely passing it in applyResource. So I think in the end this solution is better. I've added ASSERTS in RenderSVGResourceClipper/Filter/Masker, to make sure no one ever passes something else then ApplyToDefaultMode there. Hope this solution is acceptable.
 
>  What is anyQName() in RenderSVGResourceGradient and Pattern? Can we get a
> better name for it?
anyQName() == any QualifiedName - it's defined in QualifiedName.h, I think it's a good name :-)

> RenderSVGResourcePattern:
> 
>  245     OwnPtr<ImageBuffer> tileImage = ImageBuffer::create(imageSize);
> 
>  295     OwnPtr<ImageBuffer> newTileImage =
> ImageBuffer::create(IntSize(tileWidth, tileHeight));
> 
>  Can you add check's right before creating the ImageBuffers, if both sides will
> be >= 1?
Ok will do that.

>   63     PassOwnPtr<ImageBuffer> createTileImage(FloatRect& patternBoundaries,
> AffineTransform& patternTransform, const SVGPatternElement*, RenderObject*)
> const;
> 
> Why do you pass the PatternElement, do you think it's a speed win?
Sure, we save doing the same node() -> SVGPatternElement* cast twice.
 
>  85 #if PLATFORM(SKIA)
>  86     // FIXME: Move this into the GraphicsContext
>  87     // WebKit implicitly expects us to reset the path.
>  88     // For example in fillAndStrokePath() of RenderPath.cpp the path is 
>  89     // added back to the context after filling. This is because internally
> it
>  90     // calls CGContextFillPath() which closes the path.
>  91     context->beginPath();
>  92     context->platformContext()->setFillShader(0);
>  93     context->platformContext()->setStrokeShader(0);
>  94 #endif
> 
>  I think thats a general issue with Skia and mainly affects Patterns and
> Gradients. This code might be placed in GraphicsContextSkia directly.
I have no clue about the Skia code, and would rather prefer this to be done by some Chromium folks. I am just preserving existing behasviour.

> 
> 
>   149         // FIXME: Why doesn't ColorStop take a Color and an offset??
> 
> You wrote this line in SVGGradientElement.cpp, but 4 lines later you just do it
> :-)
No. ColorStop takes an offset and four floats - see platform/graphics/Gradient.h

> Realy great patch!!!

Thanks a lot, will tidy it up and write a detailed ChangeLog.

[Nikolas Zimmermann]

(In reply to comment #11)
> (In reply to comment #8)
> > Created an attachment (id=54062) [details] [details]
> > LayoutTests changes
> 
> The new DRT results look realy great!! Can you explain why pservers-grad-19-b
> looks different? It was broken, but the new reult doesn't look better!

Oh, pservsers-grad-19-b is a progression. The animateColor animation is now actually working (for the bottom-right rectangle). This is because we're now wrapping <g display="none"> in RenderSVGHiddenContainers instead of not creating renderers. This also resolves the problem that SVGGradientElement::buildStop() had to do manual style resolving if a <stop> element renderer was not existant. This is not the case anymore.

If you wonder why it's yellow now. This is the expected _beginning_ of the animation. If you wait the 5 seconds, it will turn into solid green color, of course the layout tests don't test that, as you know.

[Dirk Schulze]

Comment on attachment 54062 [details]
LayoutTests changes

Ok great!

[Dirk Schulze]

> >  85 #if PLATFORM(SKIA)
> >  86     // FIXME: Move this into the GraphicsContext
> >  87     // WebKit implicitly expects us to reset the path.
> >  88     // For example in fillAndStrokePath() of RenderPath.cpp the path is 
> >  89     // added back to the context after filling. This is because internally
> > it
> >  90     // calls CGContextFillPath() which closes the path.
> >  91     context->beginPath();
> >  92     context->platformContext()->setFillShader(0);
> >  93     context->platformContext()->setStrokeShader(0);
> >  94 #endif
> > 
> >  I think thats a general issue with Skia and mainly affects Patterns and
> > Gradients. This code might be placed in GraphicsContextSkia directly.
> I have no clue about the Skia code, and would rather prefer this to be done by
> some Chromium folks. I am just preserving existing behasviour.

OK, but make sure someone of the Skia/Chrome-guys is online to fix this issue on landing please.


> 
> > 
> > 
> >   149         // FIXME: Why doesn't ColorStop take a Color and an offset??
> > 
> > You wrote this line in SVGGradientElement.cpp, but 4 lines later you just do it
> > :-)
> No. ColorStop takes an offset and four floats - see
> platform/graphics/Gradient.h
Ah you mean Color()? ok. I missunderstood it.

[Nikolas Zimmermann]

Created attachment 54145 [details]
WebCore changes - v2

[WebKit Review Bot]

Attachment 54145 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/rendering/RenderSVGResourceGradient.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/rendering/RenderSVGResource.h:44:  One space before end of line comments  [whitespace/comments] [5]
WebCore/rendering/RenderSVGResource.cpp:137:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/svg/SVGDocumentExtensions.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/svg/GradientAttributes.h:49:  More than one command on the same line  [whitespace/newline] [4]
WebCore/svg/PatternAttributes.h:28:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 6 in 66 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nikolas Zimmermann]

Created attachment 54147 [details]
WebCore changes - v3

Hopefully w/o any more style issues now.

[WebKit Review Bot]

Attachment 54145 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1854060

[WebKit Review Bot]

Attachment 54145 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/1886011

[Nikolas Zimmermann]

Created attachment 54150 [details]
WebCore changes - v4

I love EWS! :-) Forgot to include a header for the Skia port, and missed to modify GNUmakefile.am... hopefully it's working now!

[WebKit Review Bot]

Attachment 54147 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1862043

[WebKit Review Bot]

Attachment 54147 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/1849063

[Nikolas Zimmermann]

Created attachment 54151 [details]
WebCore changes - v5

Hm, must have uploaded the wrong patch :( Retrying...
Weird though that gtk doesn't build as even the patch v4, included the GNUmakefile.am changes, hm.

[Dirk Schulze]

Comment on attachment 54151 [details]
WebCore changes - v5

Looks like the other bots don't want to test this monster ;-)

Patch looks great! Good job.

r=me

[Nikolas Zimmermann]

Committed r58212: <http://trac.webkit.org/changeset/58212>

[WebKit Review Bot]

http://trac.webkit.org/changeset/58212 might have broken Qt Linux Release

[Nikolas Zimmermann]

Landed fix for release builds in r58213.

[Nikolas Zimmermann]

Landed linking fix in r58214.

[Nikolas Zimmermann]

Landed new qt results in r58215.

[Jeremy Orlow]

Committed r58524: <http://trac.webkit.org/changeset/58524>