Bug 90738

Summary: Harmonize HTML & SVG rendering
Product: WebKit Reporter: Nikolas Zimmermann <zimmermann>
Component: SVGAssignee: Nikolas Zimmermann <zimmermann>
Status: NEW ---    
Severity: Normal CC: aboxhall, alexander.stopher, andresg_22, aperez, apinheiro, benjamin, cdumez, cfleizach, changseok, clopez, cmarcelo, dbarton, dglazkov, dino, dmazzoni, dpino, eric, esprehn+autocc, ews-watchlist, fmalita, fred.wang, glenn, graouts, gustavo, gyuyoung.kim, hyatt, info, japhet, jchaffraix, jcraig, jdiggs, kangil.han, kondapallykalyan, krit, macpherson, menard, mifenton, mihnea, mmaxfield, pdr, peter, pnormand, rhodovan.u-szeged, rwlbuis, sabouhallawa, samuel_white, schenney, sergio, simon.fraser, staikos, syoichi, webkit.review.bot, xan.lopez, zherczeg, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 236190, 240864, 86022, 209136, 209137, 211787, 233211, 233666, 233863, 233870, 233871, 233872, 233873, 234235, 234524, 234632, 234877, 234878, 234954, 234992, 235099, 235100, 235101, 236077, 236184, 236185, 236186, 236187, 236188, 236189, 236191, 236192, 236193, 236194, 237023, 237024, 237553, 237554, 237590, 237701, 237711, 239717, 239743, 239764, 239877, 240793    
Bug Blocks: 23113, 110911, 227339    
Attachments:
Description Flags
Prototype patch
none
Prototype patch v2
none
Prototype patch v3
gyuyoung.kim: commit-queue-
Prototype patch v4
none
Updated against ToT.
none
Patch v1, full prototype (including layout test changes)
ews-feeder: commit-queue-
Patch v2, full prototype (including layout test changes)
ews-feeder: commit-queue-
Patch v3, full prototype (including layout test changes)
ews-feeder: commit-queue-
Patch v4, full prototype (including layout test changes)
ews-feeder: commit-queue-
Patch v5, full prototype (including layout test changes)
none
Patch v6, including layout test changes
ews-feeder: commit-queue-
Patch v7, including layout test changes ews-feeder: commit-queue-

Description Nikolas Zimmermann 2012-07-08 13:43:48 PDT
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
Comment 1 Nikolas Zimmermann 2012-07-08 14:33:08 PDT
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)
Comment 2 Nikolas Zimmermann 2012-07-08 14:36:13 PDT
(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.
Comment 3 Rob Buis 2012-07-08 16:10:05 PDT
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?
Comment 4 Simon Fraser (smfr) 2012-07-09 10:27:03 PDT
(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.
Comment 5 Nikolas Zimmermann 2012-07-10 05:26:19 PDT
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.
Comment 6 Nikolas Zimmermann 2012-07-12 04:55:40 PDT
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).
Comment 7 Florin Malita 2012-07-12 10:34:48 PDT
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?
Comment 8 Nikolas Zimmermann 2012-07-27 06:07:53 PDT
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.
Comment 9 Nikolas Zimmermann 2012-07-27 06:10:25 PDT
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).
Comment 10 WebKit Review Bot 2012-07-27 06:13:46 PDT
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.
Comment 11 Gyuyoung Kim 2012-07-27 06:15:11 PDT
Comment on attachment 154921 [details]
Prototype patch v3

Attachment 154921 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13377315
Comment 12 Early Warning System Bot 2012-07-27 06:19:01 PDT
Comment on attachment 154921 [details]
Prototype patch v3

Attachment 154921 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13362661
Comment 13 Early Warning System Bot 2012-07-27 06:28:59 PDT
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
Comment 14 Build Bot 2012-07-27 06:34:24 PDT
Comment on attachment 154921 [details]
Prototype patch v3

Attachment 154921 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13374361
Comment 15 WebKit Review Bot 2012-07-27 07:22:16 PDT
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
Comment 16 Simon Fraser (smfr) 2012-07-27 10:27:47 PDT
Why is RenderBox painting anything SVG-related?
Comment 17 Dirk Schulze 2012-07-27 10:38:25 PDT
(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.
Comment 18 Build Bot 2012-07-27 16:41:50 PDT
Comment on attachment 154921 [details]
Prototype patch v3

Attachment 154921 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13376699
Comment 19 Dirk Schulze 2012-08-17 13:40:37 PDT
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.
Comment 20 Dirk Schulze 2012-08-17 13:41:30 PDT
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.
Comment 21 Dirk Schulze 2012-08-17 13:41:44 PDT
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.
Comment 22 Dirk Schulze 2012-08-17 13:42:54 PDT
Sorry for spamming. The submit button did not react at all.
Comment 23 Nikolas Zimmermann 2012-08-22 06:41:23 PDT
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.
Comment 24 Nikolas Zimmermann 2012-08-23 08:12:21 PDT
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.
Comment 25 Florin Malita 2012-10-03 15:49:21 PDT
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).
Comment 26 Nikolas Zimmermann 2012-10-04 03:38:27 PDT
(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?
Comment 27 Florin Malita 2012-10-04 06:02:14 PDT
(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?
Comment 28 Dirk Schulze 2012-10-04 07:11:32 PDT
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?
Comment 29 Florin Malita 2012-10-04 10:36:18 PDT
(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 :)
Comment 30 Dirk Schulze 2012-10-07 14:07:18 PDT
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.
Comment 31 Nikolas Zimmermann 2012-10-21 07:56:46 PDT
(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.
Comment 32 Florin Malita 2013-02-26 14:29:32 PST
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.
Comment 33 Florin Malita 2013-02-26 14:30:22 PST
(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
Comment 34 Nikolas Zimmermann 2019-09-18 14:49:10 PDT
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.
Comment 35 Simon Fraser (smfr) 2019-09-19 02:59:48 PDT
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.
Comment 36 Nikolas Zimmermann 2019-10-01 14:03:58 PDT
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!
Comment 37 Dirk Schulze 2019-10-01 21:09:22 PDT
The syntax between transform attribute and transform property differs and needs to continue to do so. Just as an awareness note.
Comment 38 Dirk Schulze 2019-10-01 21:12:27 PDT
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.
Comment 39 Nikolas Zimmermann 2019-10-02 01:41:11 PDT
(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 :-)
Comment 40 Nikolas Zimmermann 2019-10-02 01:43:13 PDT
(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.
Comment 41 Nikolas Zimmermann 2020-03-16 04:02:39 PDT
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.
Comment 42 Diego Pino 2020-04-14 14:30:39 PDT
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.
Comment 43 Nikos 2020-09-01 12:46:59 PDT
What about the issues that are depending on this bug? Any plans for it to beAny plans for it to be fixed?
Comment 45 Nikolas Zimmermann 2021-05-21 04:56:36 PDT
Created attachment 429285 [details]
Patch v1, full prototype (including layout test changes)
Comment 46 Nikolas Zimmermann 2021-05-21 05:08:12 PDT
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 :-)
Comment 47 Nikolas Zimmermann 2021-05-21 11:39:48 PDT
Created attachment 429316 [details]
Patch v2, full prototype (including layout test changes)
Comment 48 Nikolas Zimmermann 2021-05-21 15:37:10 PDT
Created attachment 429352 [details]
Patch v3, full prototype (including layout test changes)
Comment 49 Simon Fraser (smfr) 2021-05-21 18:26:57 PDT
How does this impact MotionMark? The "Suits" test is an SVG test.
Comment 50 Nikolas Zimmermann 2021-05-27 03:14:26 PDT
(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?
Comment 51 Nikolas Zimmermann 2021-05-27 03:33:09 PDT
Created attachment 429865 [details]
Patch v4, full prototype (including layout test changes)
Comment 52 Simon Fraser (smfr) 2021-05-27 09:09:17 PDT
A design document would be good.
Comment 53 Nikolas Zimmermann 2021-05-30 15:03:29 PDT
(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.
Comment 54 Nikolas Zimmermann 2021-05-30 15:06:13 PDT
Created attachment 430151 [details]
Patch v5, full prototype (including layout test changes)
Comment 55 Nikolas Zimmermann 2021-07-07 07:14:27 PDT
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.
Comment 56 Nikolas Zimmermann 2021-09-26 15:26:03 PDT
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.
Comment 57 Nikolas Zimmermann 2021-10-18 15:27:21 PDT
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 :-)
Comment 58 Nikolas Zimmermann 2021-11-16 14:21:15 PST
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.