Harmonize HTML & SVG rendering

[Nikolas Zimmermann]

This is the master bug tracking HTML & SVG harmonization.

In particular:
- RenderLayer & co should work for SVG
- z-index support for SVG
- 3D Transformations on any SVG element
- Move SVG rendering tree to use layers to handle transform/clipping/masking etc.
- SVG masking/clipper/filters on any RenderBox
- clipping via compositing operations (aka. reuse RenderBox code)
- paving the way for h/w acceleration

[Nikolas Zimmermann]

Created attachment 151165 [details]
Prototype patch

This patch is a first prototype which implements the aforementioned features and integrates SVG rendering within HTML in a much more tight way.
All except 56 tests pass in svg/W3C-SVG-1.1-SE svg/W3C-SVG-1.2-Tiny svg/W3C-SVG-1.1 svg/W3C-I18N  svg/animations svg/as-background-image svg/as-border-image svg/as-image svg/as-object  svg/custom, which are the most important ones.

I'll tell more about this patch soon -- for tonight I just want to share it, so the others can have a look.
NOTE: This is still work in progress (with the patch as-is, 3d transforms don't work yet if a transform attr is set as well, stuff like that, but in general it does)

[Nikolas Zimmermann]

(In reply to comment #0)
> In particular:
...
- Remove ALL of SVG specific transform support (aka. localToParentTransform(), custom mapLocalToContainer/pushMappingToAncestor/computeRectForRepaint methods)
- Make all renders inherit from RenderBox -- RenderSVGModelObject is a dead-end, and removed.
- Use CSS relative positioning internally to position shapes within containers (see moveChildren of RenderSVGResourceContainer)
- ... lots more I forgot during the work of the last weeks

The patch is not intended for any review yet, I'm rather looking for general comments if the direction is good at this point.

[Rob Buis]

Comment on attachment 151165 [details]
Prototype patch

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

Looks great! I posted some questions, and I guess I did do a mini-review after all :)

> Source/WebCore/css/StyleResolver.cpp:2159
> +        bool isOuterSVGElement = e->hasTagName(SVGNames::svgTag) && e->parentNode() && !e->parentNode()->isSVGElement();

There may be an opportunity to share code, aka a free function, since I think SVGSVGElement needs the same thing(likely we have a method for this on SVGSVGElement).

> Source/WebCore/loader/cache/CachedImage.cpp:156
> +        return 0;

Is this a related or separate fix?

> Source/WebCore/rendering/RenderLayer.cpp:547
> +static inline bool isLayerAwareSVGRenderer(RenderObject* renderer)

Is it possible to use requiresLayer instead?

> Source/WebCore/rendering/RenderLayer.cpp:3005
> +            if (!useElement->hasAttribute(SVGNames::clip_ruleAttr))

Is this correct? Can it be that it is not there, but inherited from parent? (I forgot how that exactly should work).

> Source/WebCore/rendering/RenderLayer.cpp:3300
> +        }

Possibly can share more code here.

> Source/WebCore/rendering/RenderLayer.h:1000
> +    bool m_isPaintingSVGResourceLayer;

This will need to move into the bitfield above ultimately, since quite a few RenderLayers will be created in general :)

> Source/WebCore/rendering/RenderObject.cpp:756
> +            if (0 && o->isSVGForeignObject()) //foreignObject is the containing block for contents inside it

Ok <foreignObject> support is still TODO it seems :)

> Source/WebCore/rendering/RenderObject.cpp:2237
> +            if (0 && o->isSVGForeignObject()) // foreignObject is the containing block for contents inside it

Ditto.

> Source/WebCore/rendering/RenderObject.h:-469
> -    virtual const AffineTransform& localToParentTransform() const;

Great to see so much (SVG) stuff going from this class.

> Source/WebCore/rendering/svg/RenderSVGContainer.cpp:139
> +    if (paintInfo.phase == PaintPhaseSVGClip) {

I think clipping is more common than applying masks.

> Source/WebCore/rendering/svg/RenderSVGResourceMarker.cpp:157
> +void RenderSVGResourceMarker::calculateViewport()

Maybe such changes should be in one commit "renaming abbrev methods to unabbrev".

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:176
> +        return resolveLengthAttributeForSVG(svg->intrinsicWidth(SVGSVGElement::IgnoreCSSProperties), 1, /*style()->effectiveZoom(),*/ containingBlock()->availableLogicalWidth(), view());

Is 1 temporary or the correct value here?

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:210
> +        return resolveLengthAttributeForSVG(height, 1/*style()->effectiveZoom()*/, containingBlock()->availableLogicalHeight(), view());

Ditto.

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:319
> +    }

What is the role of m_viewportContainer?

> Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:176
> +    // - When an outermost svg element is embedded inline within a parent XML grammar which uses CSS layout ([CSS2], chapter 9) or XSL formatting [XSL], if the âoverflowâ property

Unicode issue?

> Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:179
> +    // - When an outermost svg element is stand-alone or embedded inline within a parent XML grammar which does not use CSS layout or XSL formatting, the âoverflowâ property on

Ditto.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:287
> +/*

May need a clearer FIXME?

> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:119
> +            if (0 && !masker->applyResource(m_object, style, m_context, ApplyToDefaultMode))

I was thinking, maybe #if 0 is clearer?

> Source/WebCore/svg/SVGTextContentElement.cpp:298
> +    return false;

Why does this change?

[Simon Fraser (smfr)]

(In reply to comment #2)
> (In reply to comment #0)
> > In particular:
> ...
> - Remove ALL of SVG specific transform support (aka. localToParentTransform(), custom mapLocalToContainer/pushMappingToAncestor/computeRectForRepaint methods)

Sounds good.

> - Make all renders inherit from RenderBox -- RenderSVGModelObject is a dead-end, and removed.

I don't think this is right. RenderBox is geared towards rectangular CSS-rendered things. I think we need a common base class (bug 86022).

> - Use CSS relative positioning internally to position shapes within containers (see moveChildren of RenderSVGResourceContainer)
> - ... lots more I forgot during the work of the last weeks

Hmm. I think offsets should be in renderer x(), y(), not relative position offsets.

[Nikolas Zimmermann]

Thanks for the first comments!

> > - Make all renders inherit from RenderBox -- RenderSVGModelObject is a dead-end, and removed.
> 
> I don't think this is right. RenderBox is geared towards rectangular CSS-rendered things. I think we need a common base class (bug 86022).
Fully agreed. I chose this approach to proof first that using layers & co for SVG works nicely.
I wanted to avoid refactoring RenderLayers off RenderBox as first step, until it's really clear that this works.

> > - Use CSS relative positioning internally to position shapes within containers (see moveChildren of RenderSVGResourceContainer)
> > - ... lots more I forgot during the work of the last weeks
> 
> Hmm. I think offsets should be in renderer x(), y(), not relative position offsets.
I tried both approaches. For constructions like <g> <rect> </g> I need the <g> to be a stacking-context, and I did so by forcing position: relative to the <g>, so that the <rect> will be rendered in the right coordinates space, if <g> has a transform. As the CSS left/top properties are passed through relativePositionOffset() when position: relative is set, I thought I'd do it the same.

<div style="position: relative; left: 100px; top: 75px; width: 100px; height: 200px"> yields a frameRect==borderBoxRect==(0,0,100,200), and all positioning is in relativePositionOffset() which returns (100,75).

I'm open to all suggestions, if you prefer the other approach I can easily change it -- I'd like to hear more about it, why this is better though.

[Nikolas Zimmermann]

Created attachment 151910 [details]
Prototype patch v2

Updated against ToT. Individual changes:
- 3D Transforms on SVG elements work (as in my demo video)
- getScreenCTM() is fixed (all tests pass again)
- RenderSVGResourceGradient now longer manages a HashMap<RenderObject*,GradientData*> instead it holds a single RefPtr<Gradient>. We just have to update the gradientSpaceTransform for each client that wants to draw, instead of holding separated Gradient objects in a cache.
- RenderSVGShape is refactored based upons Philips work

Now looking forward to see the RenderLayer refactoring patch land, so i can base the SVG renders upon the new RenderBaseObject (or however it will be called).

[Florin Malita]

I've got a patched CR build, and AFAICT FO layers are working fine now! All the outstanding issues from https://bugs.webkit.org/show_bug.cgi?id=23113 (SVG vs foreign content stacking, inheriting masks, etc.) seem to addressed. Nice work Niko!

Are you planning to set up a dedicated branch at some point?

[Nikolas Zimmermann]

In reply to comment #7)
> Are you planning to set up a dedicated branch at some point?
Yes. Is anyone familiar with the process?

(In reply to comment #4)
> Hmm. I think offsets should be in renderer x(), y(), not relative position offsets.
Fixed. I reworked it to avoid position: relative - and the relativePositionOffset() overrides in rendering/svg.

You're right there was no need to go for the relative positioning. I'll post an updated patch now.

[Nikolas Zimmermann]

Created attachment 154921 [details]
Prototype patch v3

Marking for r? to get builds results from other platforms as well. I again only included Source/WebCore, no LayoutTests/ changes, which are massive (all txt files changed containing layers).

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/Configurations/FeatureDefin..." exit_code: 1
Source/WebCore/rendering/svg/SVGRenderSupport.cpp:151:  Missing spaces around /  [whitespace/operators] [3]
Source/WebCore/rendering/RenderLayerBacking.cpp:904:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/rendering/RenderLayerBacking.cpp:905:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/rendering/RenderLayerBacking.cpp:906:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/rendering/RenderLayerBacking.cpp:907:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/rendering/RenderLayer.cpp:409:  Extra space before ) in if  [whitespace/parens] [5]
Source/WebCore/rendering/svg/RenderSVGRoot.cpp:42:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/rendering/RenderObject.cpp:747:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/rendering/svg/SVGRenderingContext.cpp:94:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/rendering/svg/SVGRenderingContext.cpp:94:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 10 in 72 files


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

[Gyuyoung Kim]

Comment on attachment 154921 [details]
Prototype patch v3

Attachment 154921 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13377315

[Early Warning System Bot]

Comment on attachment 154921 [details]
Prototype patch v3

Attachment 154921 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13362661

[Early Warning System Bot]

Comment on attachment 154921 [details]
Prototype patch v3

Attachment 154921 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13372367

[Build Bot]

Comment on attachment 154921 [details]
Prototype patch v3

Attachment 154921 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13374361

[WebKit Review Bot]

Comment on attachment 154921 [details]
Prototype patch v3

Attachment 154921 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13375389

[Simon Fraser (smfr)]

Why is RenderBox painting anything SVG-related?

[Dirk Schulze]

(In reply to comment #16)
> Why is RenderBox painting anything SVG-related?

Since HTML elements will render in SVG content without foreignObject in the future (and hopefully vice versa), it might not be the worst idea.

[Build Bot]

Comment on attachment 154921 [details]
Prototype patch v3

Attachment 154921 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13376699

[Dirk Schulze]

Comment on attachment 154921 [details]
Prototype patch v3

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

Awesome code. Now I understand your problem with animated transform. Still, can we get this code into trunk this weekend? :P I still couldn't "review" the whole patch. It is a lot of stuff. But agree with some comments from Rob.

Looking at the changes again, I am not sure if we really need a new base class for SVG elements. If we have RenderBox as a common class, it seems easier to share code between HTML and SVG. Filters and Masks are just one part. Paint servers in general are the next step (SVG gradients and patterns on HTML content and CSS <image> on SVG). Even if the elements don't share the CSS Layout boxes with the HTML world. Or we can refactor the code more later.

> Source/WebCore/rendering/PaintPhase.h:54
> +    PaintPhaseMask,
> +    PaintPhaseSVGClip,
> +    PaintPhaseSVGMask

We may not need to differ between PaintPhaseMask and PaintPhaseSVGMask with CSS Masking [1]. Hopefully the same for clip-path :P.

[1] http://dvcs.w3.org/hg/FXTF/raw-file/tip/masking/index.html

But maybe it is good to have it as intermediate step.

> Source/WebCore/rendering/RenderLayer.cpp:3199
> +#if ENABLE(SVG) && ENABLE(FILTERS)
> +    GraphicsContext* svgSavedContext = 0;
> +    SVGResources* resources = SVGResourcesCache::cachedResourcesForRenderObject(renderer());
> +    RenderSVGResourceFilter* svgFilter = resources ? resources->filter() : 0;
> +    if (svgFilter) {
> +        svgSavedContext = context;
> +        if (!svgFilter->applyResource(renderer(), renderer()->style(), context, ApplyToDefaultMode))
> +            return;
> +        // FIXME: We can optimize this further for the specific SVG filter operations.
> +        // We should either introduce a SVGFilterEffectRendererHelper class or makw FilterEffectRendererHelper aware of SVG. To be discussed.
> +        useClipRect = false;
> +    } else if (renderer()->hasSVGFilter())
> +        return;
> +#endif

Would this still be needed? We have SVG Filters on HTML content. We just need to make sure to create a layer. (Which is done in HTML right now anyway). Just want to say, no special handling of SVG Filters needed.

> Source/WebCore/rendering/RenderLayer.cpp:3318
> +#if ENABLE(SVG)
> +    if (!renderer()->isSVGResourceContainer()) {
> +        if ((localPaintFlags & PaintLayerPaintingCompositingSVGMaskPhase) && shouldPaintContent && renderer()->hasSVGMask() && !selectionOnly) {
> +            if (useClipRect)
> +                clipToRect(rootLayer, context, paintDirtyRect, damageRect, DoNotIncludeSelfForBorderRadius); // Mask painting will handle clipping to self.
> +
> +            // Paint the svg mask.
> +            PaintInfo paintInfo(context, pixelSnappedIntRect(damageRect.rect()), PaintPhaseSVGMask, false, paintingRootForRenderer, region, 0);
> +            renderer()->paint(paintInfo, paintOffset);

This can/should be combined with 'mask-image' later. All that is needed is the correct calculation of the mask size + luminance mask.

[Dirk Schulze]

Comment on attachment 154921 [details]
Prototype patch v3

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

Awesome code. Now I understand your problem with animated transform. Still, can we get this code into trunk this weekend? :P I still couldn't "review" the whole patch. It is a lot of stuff. But agree with some comments from Rob.

Looking at the changes again, I am not sure if we really need a new base class for SVG elements. If we have RenderBox as a common class, it seems easier to share code between HTML and SVG. Filters and Masks are just one part. Paint servers in general are the next step (SVG gradients and patterns on HTML content and CSS <image> on SVG). Even if the elements don't share the CSS Layout boxes with the HTML world. Or we can refactor the code more later.

> Source/WebCore/rendering/PaintPhase.h:54
> +    PaintPhaseMask,
> +    PaintPhaseSVGClip,
> +    PaintPhaseSVGMask

We may not need to differ between PaintPhaseMask and PaintPhaseSVGMask with CSS Masking [1]. Hopefully the same for clip-path :P.

[1] http://dvcs.w3.org/hg/FXTF/raw-file/tip/masking/index.html

But maybe it is good to have it as intermediate step.

> Source/WebCore/rendering/RenderLayer.cpp:3199
> +#if ENABLE(SVG) && ENABLE(FILTERS)
> +    GraphicsContext* svgSavedContext = 0;
> +    SVGResources* resources = SVGResourcesCache::cachedResourcesForRenderObject(renderer());
> +    RenderSVGResourceFilter* svgFilter = resources ? resources->filter() : 0;
> +    if (svgFilter) {
> +        svgSavedContext = context;
> +        if (!svgFilter->applyResource(renderer(), renderer()->style(), context, ApplyToDefaultMode))
> +            return;
> +        // FIXME: We can optimize this further for the specific SVG filter operations.
> +        // We should either introduce a SVGFilterEffectRendererHelper class or makw FilterEffectRendererHelper aware of SVG. To be discussed.
> +        useClipRect = false;
> +    } else if (renderer()->hasSVGFilter())
> +        return;
> +#endif

Would this still be needed? We have SVG Filters on HTML content. We just need to make sure to create a layer. (Which is done in HTML right now anyway). Just want to say, no special handling of SVG Filters needed.

> Source/WebCore/rendering/RenderLayer.cpp:3318
> +#if ENABLE(SVG)
> +    if (!renderer()->isSVGResourceContainer()) {
> +        if ((localPaintFlags & PaintLayerPaintingCompositingSVGMaskPhase) && shouldPaintContent && renderer()->hasSVGMask() && !selectionOnly) {
> +            if (useClipRect)
> +                clipToRect(rootLayer, context, paintDirtyRect, damageRect, DoNotIncludeSelfForBorderRadius); // Mask painting will handle clipping to self.
> +
> +            // Paint the svg mask.
> +            PaintInfo paintInfo(context, pixelSnappedIntRect(damageRect.rect()), PaintPhaseSVGMask, false, paintingRootForRenderer, region, 0);
> +            renderer()->paint(paintInfo, paintOffset);

This can/should be combined with 'mask-image' later. All that is needed is the correct calculation of the mask size + luminance mask.

[Dirk Schulze]

Comment on attachment 154921 [details]
Prototype patch v3

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

Awesome code. Now I understand your problem with animated transform. Still, can we get this code into trunk this weekend? :P I still couldn't "review" the whole patch. It is a lot of stuff. But agree with some comments from Rob.

Looking at the changes again, I am not sure if we really need a new base class for SVG elements. If we have RenderBox as a common class, it seems easier to share code between HTML and SVG. Filters and Masks are just one part. Paint servers in general are the next step (SVG gradients and patterns on HTML content and CSS <image> on SVG). Even if the elements don't share the CSS Layout boxes with the HTML world. Or we can refactor the code more later.

> Source/WebCore/rendering/PaintPhase.h:54
> +    PaintPhaseMask,
> +    PaintPhaseSVGClip,
> +    PaintPhaseSVGMask

We may not need to differ between PaintPhaseMask and PaintPhaseSVGMask with CSS Masking [1]. Hopefully the same for clip-path :P.

[1] http://dvcs.w3.org/hg/FXTF/raw-file/tip/masking/index.html

But maybe it is good to have it as intermediate step.

> Source/WebCore/rendering/RenderLayer.cpp:3199
> +#if ENABLE(SVG) && ENABLE(FILTERS)
> +    GraphicsContext* svgSavedContext = 0;
> +    SVGResources* resources = SVGResourcesCache::cachedResourcesForRenderObject(renderer());
> +    RenderSVGResourceFilter* svgFilter = resources ? resources->filter() : 0;
> +    if (svgFilter) {
> +        svgSavedContext = context;
> +        if (!svgFilter->applyResource(renderer(), renderer()->style(), context, ApplyToDefaultMode))
> +            return;
> +        // FIXME: We can optimize this further for the specific SVG filter operations.
> +        // We should either introduce a SVGFilterEffectRendererHelper class or makw FilterEffectRendererHelper aware of SVG. To be discussed.
> +        useClipRect = false;
> +    } else if (renderer()->hasSVGFilter())
> +        return;
> +#endif

Would this still be needed? We have SVG Filters on HTML content. We just need to make sure to create a layer. (Which is done in HTML right now anyway). Just want to say, no special handling of SVG Filters needed.

> Source/WebCore/rendering/RenderLayer.cpp:3318
> +#if ENABLE(SVG)
> +    if (!renderer()->isSVGResourceContainer()) {
> +        if ((localPaintFlags & PaintLayerPaintingCompositingSVGMaskPhase) && shouldPaintContent && renderer()->hasSVGMask() && !selectionOnly) {
> +            if (useClipRect)
> +                clipToRect(rootLayer, context, paintDirtyRect, damageRect, DoNotIncludeSelfForBorderRadius); // Mask painting will handle clipping to self.
> +
> +            // Paint the svg mask.
> +            PaintInfo paintInfo(context, pixelSnappedIntRect(damageRect.rect()), PaintPhaseSVGMask, false, paintingRootForRenderer, region, 0);
> +            renderer()->paint(paintInfo, paintOffset);

This can/should be combined with 'mask-image' later. All that is needed is the correct calculation of the mask size + luminance mask.

[Dirk Schulze]

Sorry for spamming. The submit button did not react at all.

[Nikolas Zimmermann]

Thanks for your review comments and apologizes for not replying to them. I was very busy with non-WebKit stuff the last weeks. Anyhow, going to rebase first to produce a new version w/o adressing your review comments, will do that afterwards.

[Nikolas Zimmermann]

Created attachment 160166 [details]
Prototype patch v4

Patch v4 builds against ToT again, there are some new crashes that I couldn't yet fix (in svg/batik/text). I'm gone for the weekend, I hope to have those crashes resolved next week again.

[Florin Malita]

Created attachment 166976 [details]
Updated against ToT.

Builds & seems to mostly work, but I noticed some positioning issues (for example the triangles in https://bugs.webkit.org/attachment.cgi?id=166247 are not perfectly aligned).

[Nikolas Zimmermann]

(In reply to comment #25)
> Created an attachment (id=166976) [details]
> Updated against ToT.
> 
> Builds & seems to mostly work, but I noticed some positioning issues (for example the triangles in https://bugs.webkit.org/attachment.cgi?id=166247 are not perfectly aligned).

Phew, good to hear! Thanks Florin! I was planning to give the patch another shot on the weekend, as your RenderLayerModelObject patch has landed.
Some questions:

a) Did you do any changes except making it build again?
b) Did you run all pixel tests with it?
c) Shall we finally setup a branch for us?

[Florin Malita]

(In reply to comment #26) 
> Phew, good to hear! Thanks Florin! I was planning to give the patch another shot on the weekend, as your RenderLayerModelObject patch has landed.

No problem, I figured that with RLMO in place we can start pushing this harder :) 

I'm still trying to grok the new approach, but let me know if you think of specific tasks I can help with. Some things I was planning to look at:

* refactoring stuff on top of RLMO
* failing tests/crashes
* performance


> a) Did you do any changes except making it build again?

No changes - just merging/api updates & Chromium build fixes.

> b) Did you run all pixel tests with it?

Not yet, planning to do that today.

> c) Shall we finally setup a branch for us?

Yes, please :) Can you do that, or do you know the right person to handle it?

[Dirk Schulze]

Some more questions. Does it make sense to wait with the branch for the -WebKit-mask and -WebKit-clip-path patches? I would like to make them short hands for mask and clip-path properties until we unprefix. The second question is about the place where to apply masking and clip-path. At the moment the prefixed version require a new stacking context and The creation of a RenderLayer. Therefore, masking and clipping are done on the RenderLayer. In Niko's code masking and clipping are handled in RenderBox. Would it be ok to create a layer for SVG as well and continue with moving the code to RenderLayer?

[Florin Malita]

(In reply to comment #27)
> > b) Did you run all pixel tests with it?
> 
> Not yet, planning to do that today.

To follow up on this:

* 47 crashes, most of them in the batik tests, mostly asserts
* ~500 image diffs - this sounds worse than it is: the majority of these are caused either by repaint rect changes or the small positioning variance (rounding/precision issues?) that I mentioned earlier

The are a few areas with consistent pixel failures that seem to indicate real problems:

- clipping and masking
- shadows
- dynamic updates
- zooming

(In reply to comment #28)
> Some more questions. Does it make sense to wait with the branch for the -WebKit-mask and -WebKit-clip-path patches?

We need to track ToT in this branch anyway, so your patches would be picked up regardless. And at some point someone still has to update/merge Niko's patch even if we defer branching - might as well do it on the branch where it can be reviewed :)

[Dirk Schulze]

Comment on attachment 166976 [details]
Updated against ToT.

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

> Source/WebCore/css/StyleResolver.cpp:2225
> +            TransformOperations transform = cssTransform;
> +            transform.operations().append(MatrixTransformOperation::create(localTransform.toTransformationMatrix()));
> +            style->setTransform(transform);
> +            style->setUnique();

Oh, never looked at this part before. This does not sound like a good idea. We loose all information about the individual operations on the transform attribute (at least in CSSOM). And does it mean that it overrides the property values?

CSS Transforms wants the transform attribute to be a presentation attribute. Therefore, the CSSOM is aware of all operations on the transform attribute. There are some small differences between the syntax on SVG and CSS. rotate() can have 3 arguments on SVG. There can be spaces between operation name and the opening function brace. I did some preparation on the CSS Parser that allows us to differ between CSS and SVG parsing mode inside of the CSS Parser. I also implemented rotate() with three arguments for CSS in a local build (just HW composited rendering does not work yet). Another Adobe employee investigated on the space issue between function names and opening braces.

I am not sure how to proceed with these patches yet, since they require transform to be a presentation attribute. Unless it is not a presentation attribute, it can't be tested and we don't land source code that is not testable.

On the other hand I did not investigate how the CSSOM information can be exposed to SVGDOM (I did the opposite from what you did). So a lot of JS scripts won't work after these changes. I would like to continue with the patch on the new branch and would welcome help on the SVGDOM side.

[Nikolas Zimmermann]

(In reply to comment #30)
> > +            style->setTransform(transform);
> > +            style->setUnique();
> 
> Oh, never looked at this part before. This does not sound like a good idea. We loose all information about the individual operations on the transform attribute (at least in CSSOM). And does it mean that it overrides the property values?
Sure this needs to be fixed, I've done this to simplify things in the first patch - it doesn't matter to rendering.

[Florin Malita]

Resurrecting this fine idea...

I've been keeping an up-to date version of Niko's patch here: https://bugs.webkit.org/show_bug.cgi?id=90738. There was only one messy merge IIRC where I had to disable thorton's svg shadow fix, but other than that it should be pretty close to the original.

I've also tried with moderate success to re-implement just the SVG layers piece from scratch. In order to make it tractable, I think we need to split this into several focus areas:

* SVG layer support
* SVG resources applied as layers (depends on the above)
* various clean-ups (unrelated to any of the above)

If everyone still agrees that this is a good direction, I'll try pushing small incremental patches on these fronts.

[Florin Malita]

(In reply to comment #32)
> Resurrecting this fine idea...
> 
> I've been keeping an up-to date version of Niko's patch here: https://bugs.webkit.org/show_bug.cgi?id=90738. 

Err, that would be https://github.com/fmalita/webkit/compare/master...svg-refactor-niko-merge

[Nikolas Zimmermann]

I am looking again into this topic, just wanted to let you know, in case someone is interested in this work.

IMHO this is one of the most important things to solve (besides text rendering), since we have many foreignObject related WPT bugs, z-index issues, etc. 

Since 2012 many things happened which will ease the job: subpixel support in layout, the RenderLayerModelObject introduction, etc.

[Simon Fraser (smfr)]

A good start to fix foreignObject RenderLayer stuff would be to make a RenderFoo class that is shared between RenderView and RenderForeinObject that knows how to act as the root of a layer tree.

We might also use this for <dialog> overlay stuff.

[Nikolas Zimmermann]

I have a working PoC patch, that implements RenderSVGModelObject on top of RenderLayerModelObject -- it greatly simplifies the whole SVG rendering code.

Just a few highlights:
- The SVG 'transform' attribute is now a CSS property, simplifying the whole renderer invalidation logic.
No more setNeedsShapeUpdate/setNeedsBoundariesUpdate/setNeedsTransformUpdate, etc.

Whenever a transform property is set on a SVG renderer a RenderLayer is created.

- SVG <foreignObject> is now trivial, since the coordinate system is identical between HTML / SVG.

- SMIL <animateMotion> is implemented as an internal CSS property (-internal-svg-supplemental-transform) and is treated just like the 'transform' property and applied in RenderLayer::updateTransform. This removes a few special cases, and simplifies the SMIL code.

I am currently working on RenderSVGTransformable/ViewportContainer - probably the transformations will be mapped to an internal CSS property, but I am not decided yet - still prototyping.

Also SVG markers/clippers/filters are currently disabled in my PoC patch - they need to be implemented from scratch (see the "Prototype patch v4" from 2012 - RenderSVGResourceLayer).

Stay tuned!

[Dirk Schulze]

The syntax between transform attribute and transform property differs and needs to continue to do so. Just as an awareness note.

[Dirk Schulze]

As another comment, that would need a lot of test cases with fo element, z-index (which should not be supported yet but maybe in the future), 3D transform, getScreebCTM, blend modes (which may break if you also create graphics layers since they would start isolating) and a lot more.

[Nikolas Zimmermann]

(In reply to Dirk Schulze from comment #37)
> The syntax between transform attribute and transform property differs and
> needs to continue to do so. Just as an awareness note.

Fully aware of it, the intention of my current work is to outline the path to a modern SVG rendering engine, properly integrating with HTML.

That's why I called it a "Proof Of Concept" patch. It will also never be landable as-is, given the size, the amount of changes etc.

What I do not want to do is: introduce hacks to support e.g. a separated SVG transform attribute in the new concept, I want everything properly integrated with CSS -- we can then see what things need to change, in order to be SVG2 compilant, not break existing content, etc. But let's get rid of two decades old SVG specific concepts, that are not future proof :-)

[Nikolas Zimmermann]

(In reply to Dirk Schulze from comment #38)
> As another comment, that would need a lot of test cases with fo element,
> z-index (which should not be supported yet but maybe in the future), 3D
> transform, getScreebCTM, blend modes (which may break if you also create
> graphics layers since they would start isolating) and a lot more.

Oh yes, and I will call for help in a while, to collect testcases that are specs compilant, and broken with our current implementation.

But first all basic features need to work again in the PoC branch, once that is done, I am happy to look at new testcases/features!

I will also think about a good approach to share the current work with others - in form of a git branch hosted somewhere...

Thanks for answering Dirk, highly appreciated that you look into this issue, too.

[Nikolas Zimmermann]

Most svg/ layout tests now pass using the new svg_rewrite branch. Just wanted to leave a message that I am still working on this.

[Diego Pino]

After r260062 the two remaining TestExpectations failures pointing to this bug were passing. I removed those entries from TestExpectations in https://trac.webkit.org/changeset/260097/webkit.

Since there are no more failures pointing to this bug now, maybe it can be closed.

[Nikos]

What about the issues that are depending on this bug? Any plans for it to beAny plans for it to be fixed?

[Nikos]

my pain: https://stackoverflow.com/questions/63690664/safari-macos-foreign-object-not-scaled-properly-inside-svg?noredirect=1#comment112632074_63690664

[Nikolas Zimmermann]

Created attachment 429285 [details]
Patch v1, full prototype (including layout test changes)

[Nikolas Zimmermann]

Comment on attachment 429285 [details]
Patch v1, full prototype (including layout test changes)

Patch v1, full prototype (including layout test changes):

This contains the complete patch, porting SVG to use RenderLayer.
It is huge, but I'd love to get EWS results -- let's see if EWS can cope with a 17 MB patch :-(

On my Apple M1 machine pixel tests results for the svg/ subdirectory are identical, and all tests outside of svg/ pass. There are a handful known regressions, marked as such in TestExpectations, that I'm still working on.

This is obviously not intended for a full review, but I want to share the complete patch so that you can see what approaches I've used for integrating HTML/CSS rendering.

A design document / blog post will follow about this, but not in the next weeks - holidays are approaching for me.

I hope this will serve as discussion starting point, to see how we can split this into manageable pieces for review. I'm very open for input :-)

[Nikolas Zimmermann]

Created attachment 429316 [details]
Patch v2, full prototype (including layout test changes)

[Nikolas Zimmermann]

Created attachment 429352 [details]
Patch v3, full prototype (including layout test changes)

[Simon Fraser (smfr)]

How does this impact MotionMark? The "Suits" test is an SVG test.

[Nikolas Zimmermann]

(In reply to Simon Fraser (smfr) from comment #49)
> How does this impact MotionMark? The "Suits" test is an SVG test.

Ough, I missed that comment - excuse the late reply:

I've worked on Suits extensively last year, and got down to a <5% regression, on WPE port using a different version of this patch (different kind of optimizations + a few real hacks, that need to be reworked).

This will all developed in the context of Gtk/WPE ports, and I only moved onward to test on macOS in January. As first step the RenderLayerScrollableArea changes were upstreamed.

Since then I rebased my SVG PoC branch on top of latest WebKit (moving 6+ months forward), and gave up on all the perf optimization commits/hacks in my local branch to ease the rebasing, since they were sprinkled over many places - with Cairo specific patches, and a few real hacks that need to be reworked.

I instead focused on correctness first, to really get everything to pass on macOS where the test coverage is larger than on Gtk/WPE --> this is the patch that I've shared here.

I am currently repeating the performance work done last year, now using macOS, to bring us as close as possible to the vanilla performance (which does not have the RenderLayer overhead, but of course at the same time performs badly in cases where e.g. transformations are animated, whereas the new SVG PoC branch now shines due to the accelerated compositing...).

Here are some concrete numbers obtained today on a release M1 build:

A = WebKit + SVG patch
B = WebKit vanilla 

Test name                    | Score A | Score B | Performance A / B |
-----------------------------|---------|---------|-------------------|
Suits - total                |    1875 |    2415 |  ~ 78% of vanilla |
Suits: clip only             |    1946 |    2549 |  ~ 76% of vanilla |
Suits: shape only            |    2744 |    3479 |  ~ 79% of vanilla |
Suits: clip, shape, rotation |    1847 |    2181 |  ~ 85% of vanilla |
Suits: clip, shape, gradient |    1587 |    2164 |  ~ 73% of vanilla |
Suits: static                |    1480 |    1890 |  ~ 78% of vanilla |

Take the numbers with a grain of salt, that's not the end of the story.

Still I thought it's time to share the patch, given the large amount of test progressions (<foreignObject> really works nicely now that the layer hierarchy is aware of SVG: all the positioning: absolute / z-index / ... issues are gone), the nice new features such as 3D transforms on SVG and the fluent animations when e.g. transform/opacity is animated.

Simon, Rob commented that he'd find a design document helpful that describes the overall design, similar to his document describing the CSS containment implementation. Would you agree on that?

[Nikolas Zimmermann]

Created attachment 429865 [details]
Patch v4, full prototype (including layout test changes)

[Simon Fraser (smfr)]

A design document would be good.

[Nikolas Zimmermann]

(In reply to Simon Fraser (smfr) from comment #52)
> A design document would be good.

Will do, but with a delay, I'm on holidays for ~ 3 weeks.
I'll try to get the patch in a good shape though, besides working on the document.

[Nikolas Zimmermann]

Created attachment 430151 [details]
Patch v5, full prototype (including layout test changes)

[Nikolas Zimmermann]

Created attachment 433027 [details]
Patch v6, including layout test changes

Rebased on ToT + performance fixes -- will discuss this later, once the design document is ready.

[Nikolas Zimmermann]

The patch from July revealed unfixable issues, when I started to look into enabling accelerating transforms everywhere. This all became apparent when I worked on the design document, questioning some decisions that I made along the way.

One of them was that it's possible to construct the SVG transformation matrices in such a way that we can leave the way RenderLayer handles transformations largely unchanged: first translate, then apply transform. This works in 2D, but is not generalizeable to 3D in a performant way....

Speaking math: Constructing a matrix T' such that A * T' = T * A. Pen & paper exercise in 2D, for 3D and no other conditions (no vanishing terms, etc.) each component of the new 4x4 matrix gets lengthy... not impossible, but hard to compute.

Anyhow, that is not solveable and in fact the whole assumption (don't change how RenderLayer applies transformations) is simply not practiable for SVG -- it involves unnaturual coordinate system origin translations in many places (see patch v6 from July) that are ugly, and good news: all of them can be avoided.

The new patch (last rebased on Sep 11) was tested on macOS Desktop / WPE Desktop / WPE embedded, still has a few minor regressions (IIRC 7 in total; one is mix-blend-mode support is producing black canvas, and a few more subpixel-positioning issues when composited layers are active).

But still it works much nicer and transitions between composited / non-composited of individual SVG elements, all work as expected now -- this was all broken before, where it only worked for simple cases. Accelerated transforms (GraphicsLayer not asking RenderLayer to repaint) are now working as intended.

I'm delaying to upload this new version, since I want to get the design document in a good shape, covering the new patch, so I can release them at the sime time.

In the WebKit contributors meeting you can see a demo of the patch in action.

[Nikolas Zimmermann]

Created attachment 441646 [details]
Patch v7, including layout test changes

Next iteration of the "Layer based SVG engine", as demoed on the WebKit contributors meeting.
This is the same version that was used for the demos, but rebased to Oct, 18th 2021.

The technical document is almost ready that explains & motivates this patch. It will briefly
discuss the upstreaming strategy that we discussed during and after the contributors meeting.

This patch isn't intended for review, but rather helpful for me to receive EWS feedback - sorry for the mail spam due to that :-)

[Nikolas Zimmermann]

Comment on attachment 441646 [details]
Patch v7, including layout test changes

I marked the patch as obsolete, easier to look at https://github.com/nikolaszimmermann/WebKitIgalia/commits/main ("Integrate new layer based SVG engine").

An overview of the all-in-one-patch is given here https://blogs.igalia.com/nzimmermann/posts/2021-10-29-layer-based-svg-engine/page/8/#svg-poc-code-changes.

Bug 233211 is the first patch, adding a new runtime flag for the layer based SVG engine.

[Brent Fulgham]

I don't think we need to keep this 10-year old bug open, especially since it seems to be complete.

[Simon Fraser (smfr)]

This is ongoing work.

[Radar WebKit Bug Importer]

<rdar://problem/97121598>