Bug 37986

Summary: SVGPaintServer needs to be converted to the new RenderSVGResource* system
Product: WebKit Reporter: Nikolas Zimmermann <zimmermann>
Component: SVGAssignee: Nikolas Zimmermann <zimmermann>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, bdakin, dglazkov, eric, gns, jorlow, krit, staikos, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 38106    
Attachments:
Description Flags
File moves / build system changes
krit: review+
LayoutTests changes
krit: review+, krit: commit-queue-
WebCore changes
none
WebCore changes - v2
none
WebCore changes - v3
none
WebCore changes - v4
none
WebCore changes - v5 krit: review+

Description Nikolas Zimmermann 2010-04-22 05:02:53 PDT
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.
Comment 1 Nikolas Zimmermann 2010-04-22 05:28:48 PDT
Created attachment 54052 [details]
File moves / build system changes
Comment 2 Dirk Schulze 2010-04-22 05:44:59 PDT
(In reply to comment #1)
> Created an attachment (id=54052) [details]
> Patch

What for a garbage ;-)
Comment 3 Nikolas Zimmermann 2010-04-22 06:13:35 PDT
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"....
Comment 4 Dirk Schulze 2010-04-22 06:32:48 PDT
Comment on attachment 54052 [details]
File moves / build system changes

r=me.
Comment 5 Nikolas Zimmermann 2010-04-22 06:51:21 PDT
Committed r58093: <http://trac.webkit.org/changeset/58093>
Comment 6 Nikolas Zimmermann 2010-04-22 07:42:10 PDT
Heh, forgot to tell webkit-patch not to close this bug, it's our master bug for the transition.
Comment 7 Nikolas Zimmermann 2010-04-22 08:53:53 PDT
Comment on attachment 54052 [details]
File moves / build system changes

Marking first patch as obsolete, it has been landed and was just a preparation.
Comment 8 Nikolas Zimmermann 2010-04-22 08:55:54 PDT
Created attachment 54062 [details]
LayoutTests changes
Comment 9 Nikolas Zimmermann 2010-04-22 08:57:18 PDT
Created attachment 54063 [details]
WebCore changes

Huuuuuge speedup, try zooming/panning around pattern/gradient tests :-)
Comment 10 Dirk Schulze 2010-04-22 11:01:23 PDT
(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!!!
Comment 11 Dirk Schulze 2010-04-22 11:17:41 PDT
(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!
Comment 12 Nikolas Zimmermann 2010-04-23 00:42:16 PDT
(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.
Comment 13 Nikolas Zimmermann 2010-04-23 00:44:33 PDT
(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.
Comment 14 Dirk Schulze 2010-04-23 00:52:29 PDT
Comment on attachment 54062 [details]
LayoutTests changes

Ok great!
Comment 15 Dirk Schulze 2010-04-23 00:55:17 PDT
> >  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.
Comment 16 Nikolas Zimmermann 2010-04-23 03:38:10 PDT
Created attachment 54145 [details]
WebCore changes - v2
Comment 17 WebKit Review Bot 2010-04-23 03:43:36 PDT
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.
Comment 18 Nikolas Zimmermann 2010-04-23 04:08:43 PDT
Created attachment 54147 [details]
WebCore changes - v3

Hopefully w/o any more style issues now.
Comment 19 WebKit Review Bot 2010-04-23 04:34:10 PDT
Attachment 54145 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1854060
Comment 20 WebKit Review Bot 2010-04-23 04:35:49 PDT
Attachment 54145 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/1886011
Comment 21 Nikolas Zimmermann 2010-04-23 04:58:50 PDT
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!
Comment 22 WebKit Review Bot 2010-04-23 05:26:54 PDT
Attachment 54147 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1862043
Comment 23 WebKit Review Bot 2010-04-23 05:31:31 PDT
Attachment 54147 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/1849063
Comment 24 Nikolas Zimmermann 2010-04-23 05:39:54 PDT
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.
Comment 25 Dirk Schulze 2010-04-23 09:09:12 PDT
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
Comment 26 Nikolas Zimmermann 2010-04-24 04:43:39 PDT
Committed r58212: <http://trac.webkit.org/changeset/58212>
Comment 27 WebKit Review Bot 2010-04-24 05:06:32 PDT
http://trac.webkit.org/changeset/58212 might have broken Qt Linux Release
Comment 28 Nikolas Zimmermann 2010-04-24 05:36:58 PDT
Landed fix for release builds in r58213.
Comment 29 Nikolas Zimmermann 2010-04-24 05:54:46 PDT
Landed linking fix in r58214.
Comment 30 Nikolas Zimmermann 2010-04-24 06:03:03 PDT
Landed new qt results in r58215.
Comment 31 Jeremy Orlow 2010-04-29 10:42:22 PDT
Committed r58524: <http://trac.webkit.org/changeset/58524>