This is a meta bug for implementing the buffered rendering hint from SVG2: https://svgwg.org/svg2-draft/single-page.html#painting-BufferedRendering The basic idea here is to cache static content. Feedback from many teams has indicated this is important for performance.
We should be really careful with that. It has a high risk of regressions and the checking (if an update is needed), might make the implementation difficult to understand. This is not a concern against a feature like that (I really believe we need something like background caching). But we should not be shy to propose another approach to the SVG WG if it makes more sense.
We just discussed it on IRC. Once we have layers in SVG, this property could create a new layer. We would need to cache the layers, which is just possible with HW acceleration at the moment IIRC. Leaving the note here for future brain storming,
My example implementation of SVG using RenderLayers make use off that: an off-screen buffer associated with a RenderLayer if it's transformed (eg. translate animation). Once we fully integrate within the RenderLayer framework, we get this virtually "for free" (no need to take any special care outside of rendering/ for that).
Created attachment 193982 [details] Work in progress: add the buffered-rendering SVG2 property. After the SVG image cache was removed, some users were saddened to learn that their scaled images would not be cached. I have been looking at implementing the SVG2 buffered-rendering property for a subset of the SVG renderers. The attached patch adds the property, but I need to decide what, where, and if we should actually cache. For concreteness, lets talk about an svg image of a snowflake used many times under translation and rotation to create a snowing effect. Even with layers, the snowing usecase will benefit from a hint that the snowflake image/subtree should be cached. For example: if the snowflake is complex, the rasterization time may be more expensive than simply stamping out a saved rasterized imagebuffer. This hint gives users the power to trade off memory vs performance even in purely declarative SVG files. Does this sound like a reasonable argument for adding paint-time caching of images as a first-pass?
Comment on attachment 193982 [details] Work in progress: add the buffered-rendering SVG2 property. View in context: https://bugs.webkit.org/attachment.cgi?id=193982&action=review > Source/WebCore/rendering/style/SVGRenderStyle.h:420 > + // 19 bits unused 18 bits unused, because you're using 2.
Created attachment 194768 [details] [SVG2] Add support for the buffered-rendering property Unfortunately this patch will fail the style checker due to the enum values in SVGRenderStyleDefs.h. I think these failures can be ignored because the enum format in SVGRenderStyleDefs does not follow the WebKit code format. Otherwise, this is ready for review! I put up a simple demo to show off our speedy cached rendering: http://philbit.com/bouncingTigers.html
Attachment 194768 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/svg/css/buffered-rendering-expected.txt', u'LayoutTests/svg/css/buffered-rendering.html', u'LayoutTests/svg/repaint/buffered-rendering-image-expected.html', u'LayoutTests/svg/repaint/buffered-rendering-image.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSPrimitiveValueMappings.h', u'Source/WebCore/css/CSSProperty.cpp', u'Source/WebCore/css/SVGCSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/SVGCSSParser.cpp', u'Source/WebCore/css/SVGCSSPropertyNames.in', u'Source/WebCore/css/SVGCSSStyleSelector.cpp', u'Source/WebCore/css/SVGCSSValueKeywords.in', u'Source/WebCore/rendering/style/SVGRenderStyle.cpp', u'Source/WebCore/rendering/style/SVGRenderStyle.h', u'Source/WebCore/rendering/style/SVGRenderStyleDefs.h', u'Source/WebCore/rendering/svg/RenderSVGImage.cpp', u'Source/WebCore/rendering/svg/RenderSVGImage.h', u'Source/WebCore/rendering/svg/SVGRenderingContext.cpp', u'Source/WebCore/rendering/svg/SVGRenderingContext.h', u'Source/WebCore/svg/SVGStyledElement.cpp', u'Source/WebCore/svg/svgattrs.in']" exit_code: 1 Source/WebCore/rendering/style/SVGRenderStyleDefs.h:87: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Source/WebCore/rendering/style/SVGRenderStyleDefs.h:88: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Source/WebCore/rendering/style/SVGRenderStyleDefs.h:89: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 3 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 194768 [details] [SVG2] Add support for the buffered-rendering property Attachment 194768 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17144674
Comment on attachment 194768 [details] [SVG2] Add support for the buffered-rendering property Attachment 194768 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17317054
Comment on attachment 194768 [details] [SVG2] Add support for the buffered-rendering property Attachment 194768 [details] did not pass gtk-ews (gtk): Output: http://webkit-commit-queue.appspot.com/results/17313047
Comment on attachment 194768 [details] [SVG2] Add support for the buffered-rendering property Attachment 194768 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17288348
Comment on attachment 194768 [details] [SVG2] Add support for the buffered-rendering property Attachment 194768 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17289447
Comment on attachment 194768 [details] [SVG2] Add support for the buffered-rendering property Attachment 194768 [details] did not pass cr-android-ews (chromium-android): Output: http://webkit-commit-queue.appspot.com/results/17220899
Comment on attachment 194768 [details] [SVG2] Add support for the buffered-rendering property Attachment 194768 [details] did not pass win-ews (win): Output: http://webkit-commit-queue.appspot.com/results/17183984
Comment on attachment 194768 [details] [SVG2] Add support for the buffered-rendering property Attachment 194768 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17323057
Created attachment 194875 [details] [SVG2] Add support for the buffered-rendering property Updated to actually compile on platforms other than mac. Note that this is expected to still fail the style checker.
Attachment 194875 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/svg/css/buffered-rendering-expected.txt', u'LayoutTests/svg/css/buffered-rendering.html', u'LayoutTests/svg/repaint/buffered-rendering-image-expected.html', u'LayoutTests/svg/repaint/buffered-rendering-image.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSPrimitiveValueMappings.h', u'Source/WebCore/css/CSSProperty.cpp', u'Source/WebCore/css/SVGCSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/SVGCSSParser.cpp', u'Source/WebCore/css/SVGCSSPropertyNames.in', u'Source/WebCore/css/SVGCSSStyleSelector.cpp', u'Source/WebCore/css/SVGCSSValueKeywords.in', u'Source/WebCore/rendering/style/SVGRenderStyle.cpp', u'Source/WebCore/rendering/style/SVGRenderStyle.h', u'Source/WebCore/rendering/style/SVGRenderStyleDefs.h', u'Source/WebCore/rendering/svg/RenderSVGImage.cpp', u'Source/WebCore/rendering/svg/RenderSVGImage.h', u'Source/WebCore/rendering/svg/SVGRenderingContext.cpp', u'Source/WebCore/rendering/svg/SVGRenderingContext.h', u'Source/WebCore/svg/SVGStyledElement.cpp', u'Source/WebCore/svg/svgattrs.in']" exit_code: 1 Source/WebCore/rendering/style/SVGRenderStyleDefs.h:87: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Source/WebCore/rendering/style/SVGRenderStyleDefs.h:88: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Source/WebCore/rendering/style/SVGRenderStyleDefs.h:89: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 3 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 194875 [details] [SVG2] Add support for the buffered-rendering property View in context: https://bugs.webkit.org/attachment.cgi?id=194875&action=review Minor stuff, really. Please create two tests. Use "none" as the expected result setting for both (i.e. both tests have the same expected result). Then have one test for dynamic and one for static. > Source/WebCore/ChangeLog:26 > + (WebCore): ap has been telling people (and I agree) to remove the empty WebCore line, so do that everywhere. Also, add something along the lines of "All these files changed to support the new property definition and handling". > Source/WebCore/rendering/svg/SVGRenderingContext.cpp:328 > +bool SVGRenderingContext::bufferForeground(OwnPtr<ImageBuffer>& imageBuffer) I would be happier if this were renamed "bufferImageElementForeground" to make it clear it only works on images, to prevent confusion with later code that enables it for more content. > Source/WebCore/rendering/svg/SVGRenderingContext.cpp:346 > + Close this if here. > Source/WebCore/rendering/svg/SVGRenderingContext.cpp:347 > + if (imageBuffer) { Move this out. > Source/WebCore/rendering/svg/SVGRenderingContext.cpp:357 > + if (!imageBuffer) this is now an else clause. > LayoutTests/svg/css/buffered-rendering.html:11 > +description('Test that image accept different buffered rendering values'); "... that SVG Image accepts all buffered rendering values"
Comment on attachment 194875 [details] [SVG2] Add support for the buffered-rendering property View in context: https://bugs.webkit.org/attachment.cgi?id=194875&action=review Naming nits :P > Source/WebCore/ChangeLog:19 > + http://philbit.com/bouncingTigers.html Would it be possible to add this as a manual test? I'm always wary of publishing links to personal content - they usually go stale sooner or later. > Source/WebCore/rendering/svg/RenderSVGImage.cpp:153 > + SVGImageElement* imageElement = static_cast<SVGImageElement*>(node()); I assume it's safe in this case, but I still cringe on static casts :) > Source/WebCore/rendering/svg/RenderSVGImage.h:53 > + void paintForeground(PaintInfo&); "Foreground" seems too narrow as we're painting the full content (filters, markers, outline, etc). "paintImage"/"paintImageContent" maybe? > Source/WebCore/rendering/svg/RenderSVGImage.h:73 > + void invalidateBufferedForeground(); I don't feel too strongly about this since it's related to the attribute name, but technically we're "caching" not "buffering". Plus the "foreground" comment above :) > Source/WebCore/rendering/svg/RenderSVGImage.h:88 > + OwnPtr<ImageBuffer> m_bufferedForeground; Ditto. >> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:357 >> + if (!imageBuffer) > > this is now an else clause. I would move this up, combine it with the top-level test and then you can just drop the previous conditional/else branch: if (!imageBuffer && !(imageBuffer = m_paintInfo->context->createCompatibleBuffer(expandedIntSize(boundingBox.size()), true))) return false; GraphicsContext* bufferedRenderingContext = imageBuffer->context(); ...
Created attachment 194942 [details] Update per reviewer comments
Comment on attachment 194875 [details] [SVG2] Add support for the buffered-rendering property View in context: https://bugs.webkit.org/attachment.cgi?id=194875&action=review >> Source/WebCore/ChangeLog:19 >> + http://philbit.com/bouncingTigers.html > > Would it be possible to add this as a manual test? I'm always wary of publishing links to personal content - they usually go stale sooner or later. I agree this link will go stale at some point. This example is just a public version of an internal benchmark created by the Swiffy team that shows this patch makes a real improvement. I'm not sure this should be checked into the manualtest directory, and sadly this won't run on our perftest infrastructure :( Do you have another suggestion for where we could put it? >> Source/WebCore/ChangeLog:26 >> + (WebCore): > > ap has been telling people (and I agree) to remove the empty WebCore line, so do that everywhere. > > Also, add something along the lines of "All these files changed to support the new property definition and handling". Done, and I've filed a bug about the extra WebCore line: https://bugs.webkit.org/show_bug.cgi?id=113221 >> Source/WebCore/rendering/style/SVGRenderStyleDefs.h:87 >> + BR_AUTO, > > enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] @StyleQueue: it's like you don't even read my comments >:( >> Source/WebCore/rendering/style/SVGRenderStyleDefs.h:88 >> + BR_DYNAMIC, > > enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Ditto! >> Source/WebCore/rendering/style/SVGRenderStyleDefs.h:89 >> + BR_STATIC > > enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Ditto!!1 >> Source/WebCore/rendering/svg/RenderSVGImage.cpp:153 >> + SVGImageElement* imageElement = static_cast<SVGImageElement*>(node()); > > I assume it's safe in this case, but I still cringe on static casts :) I agree. Abhishek is making an automated pass through these soon. To assuage your fears, I did check that RenderSVGImage can only be a renderer for SVGImageElements. >> Source/WebCore/rendering/svg/RenderSVGImage.h:53 >> + void paintForeground(PaintInfo&); > > "Foreground" seems too narrow as we're painting the full content (filters, markers, outline, etc). "paintImage"/"paintImageContent" maybe? All of these (other than outline) are considered "foreground" as far as the paint phases are concerned. See PaintPhase.h for the list of phases. This function is only intended for the foreground phase. I would like to stick with this naming scheme because in a future patch I would like to add a similarly-named function to RenderSVGTransformableContainer to support buffered-rendering on <g>. >> Source/WebCore/rendering/svg/RenderSVGImage.h:73 >> + void invalidateBufferedForeground(); > > I don't feel too strongly about this since it's related to the attribute name, but technically we're "caching" not "buffering". Plus the "foreground" comment above :) I think following the attribute naming scheme is best for readability, but, like you, I wish it was named cached-rendering. >> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:328 >> +bool SVGRenderingContext::bufferForeground(OwnPtr<ImageBuffer>& imageBuffer) > > I would be happier if this were renamed "bufferImageElementForeground" to make it clear it only works on images, to prevent confusion with later code that enables it for more content. I would like to call this function from RenderSVGTransformableContainer in the future in order to cache the foreground rendering of <g> elements. This function was written to be agnostic of the renderer, with the exception of the paintForeground(), which will require us to use something like (line 353): if (m_object->isSVGImage()) toRenderSVGImage(m_object)->paintForeground(bufferedInfo); else if (m_object->isSVGContainer()) toRenderSVGContainer(m_object)->paintForeground(bufferedInfo); Does this context change your mind? If not, I'm happy to switch it. >> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:346 >> + > > Close this if here. You and Florin were suggesting slightly different things here. I've cleaned this up, but could both of you please make sure you're happy here? >> LayoutTests/svg/css/buffered-rendering.html:11 >> +description('Test that image accept different buffered rendering values'); > > "... that SVG Image accepts all buffered rendering values" Done.
Attachment 194942 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/svg/css/buffered-rendering-expected.txt', u'LayoutTests/svg/css/buffered-rendering.html', u'LayoutTests/svg/repaint/buffered-rendering-dynamic-image-expected.html', u'LayoutTests/svg/repaint/buffered-rendering-dynamic-image.html', u'LayoutTests/svg/repaint/buffered-rendering-static-image-expected.html', u'LayoutTests/svg/repaint/buffered-rendering-static-image.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSPrimitiveValueMappings.h', u'Source/WebCore/css/CSSProperty.cpp', u'Source/WebCore/css/SVGCSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/SVGCSSParser.cpp', u'Source/WebCore/css/SVGCSSPropertyNames.in', u'Source/WebCore/css/SVGCSSStyleSelector.cpp', u'Source/WebCore/css/SVGCSSValueKeywords.in', u'Source/WebCore/rendering/style/SVGRenderStyle.cpp', u'Source/WebCore/rendering/style/SVGRenderStyle.h', u'Source/WebCore/rendering/style/SVGRenderStyleDefs.h', u'Source/WebCore/rendering/svg/RenderSVGImage.cpp', u'Source/WebCore/rendering/svg/RenderSVGImage.h', u'Source/WebCore/rendering/svg/SVGRenderingContext.cpp', u'Source/WebCore/rendering/svg/SVGRenderingContext.h', u'Source/WebCore/svg/SVGStyledElement.cpp', u'Source/WebCore/svg/svgattrs.in']" exit_code: 1 Source/WebCore/rendering/style/SVGRenderStyleDefs.h:87: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Source/WebCore/rendering/style/SVGRenderStyleDefs.h:88: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Source/WebCore/rendering/style/SVGRenderStyleDefs.h:89: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 3 in 25 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 194942 [details] Update per reviewer comments Attachment 194942 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17305197
(In reply to comment #23) > (From update of attachment 194942 [details]) > Attachment 194942 [details] did not pass mac-wk2-ews (mac-wk2): > Output: http://webkit-commit-queue.appspot.com/results/17305197 This is a real failure, I need to wrap the assignment in an additional set of parenthesis.
Comment on attachment 194942 [details] Update per reviewer comments Attachment 194942 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17232784
Comment on attachment 194942 [details] Update per reviewer comments Attachment 194942 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17241414
Comment on attachment 194942 [details] Update per reviewer comments View in context: https://bugs.webkit.org/attachment.cgi?id=194942&action=review Looks like I missed an issue earlier. Another pass required. > Source/WebCore/rendering/svg/SVGRenderingContext.cpp:339 > + if (bufferSize != imageBuffer->internalSize()) Below you use expandedIntSize(boundingBox.size()), but here you compute something else to check the size against. This seems like a problem, as you'll get a different image size depending on which code path you take. > Source/WebCore/rendering/svg/SVGRenderingContext.cpp:345 > + if (imageBuffer = m_paintInfo->context->createCompatibleBuffer(expandedIntSize(boundingBox.size()), true)) { Above you include scale information in computing the size. > LayoutTests/ChangeLog:58 > + * platform/qt/css3/selectors3/xhtml/css3-modsel-14a7-expected.png: This seems rather unexpected. Find/Replace error?
(In reply to comment #21) > (From update of attachment 194875 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=194875&action=review > >> Source/WebCore/rendering/svg/RenderSVGImage.h:53 > >> + void paintForeground(PaintInfo&); > > > > "Foreground" seems too narrow as we're painting the full content (filters, markers, outline, etc). "paintImage"/"paintImageContent" maybe? > > All of these (other than outline) are considered "foreground" as far as the paint phases are concerned. See PaintPhase.h for the list of phases. This function is only intended for the foreground phase. The SVG layer patch introduces dedicated phases for masking and clipping also. I was interpreting the name wrong though: this is not "we're only painting the foreground of the SVG content" but "we're painting the foreground phase of RenderSVGImage", which makes sense. > >> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:346 > >> + > > > > Close this if here. > > You and Florin were suggesting slightly different things here. I've cleaned this up, but could both of you please make sure you're happy here? I think we're both suggesting to do away with nested conditional blocks :) Can't find it in the style guide, but I seem to remember Dirk or Niko mentioned early returns are preferred: if (!ok) return false; <ok block> instead of if (ok) { <ok block> } else return false;
Comment on attachment 194942 [details] Update per reviewer comments View in context: https://bugs.webkit.org/attachment.cgi?id=194942&action=review >> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:339 >> + if (bufferSize != imageBuffer->internalSize()) > > Below you use expandedIntSize(boundingBox.size()), but here you compute something else to check the size against. This seems like a problem, as you'll get a different image size depending on which code path you take. These implementations end up being the same but certainly do look quite different. Above, I've extracted out the exact sizing code used by createCompatibleBuffer. >> LayoutTests/ChangeLog:58 >> + * platform/qt/css3/selectors3/xhtml/css3-modsel-14a7-expected.png: > > This seems rather unexpected. Find/Replace error? patch gremlins.
Created attachment 195698 [details] Updated per reviewer comments
Attachment 195698 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/svg/css/buffered-rendering-expected.txt', u'LayoutTests/svg/css/buffered-rendering.html', u'LayoutTests/svg/repaint/buffered-rendering-dynamic-image-expected.html', u'LayoutTests/svg/repaint/buffered-rendering-dynamic-image.html', u'LayoutTests/svg/repaint/buffered-rendering-static-image-expected.html', u'LayoutTests/svg/repaint/buffered-rendering-static-image.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSPrimitiveValueMappings.h', u'Source/WebCore/css/CSSProperty.cpp', u'Source/WebCore/css/SVGCSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/SVGCSSParser.cpp', u'Source/WebCore/css/SVGCSSPropertyNames.in', u'Source/WebCore/css/SVGCSSStyleSelector.cpp', u'Source/WebCore/css/SVGCSSValueKeywords.in', u'Source/WebCore/rendering/style/SVGRenderStyle.cpp', u'Source/WebCore/rendering/style/SVGRenderStyle.h', u'Source/WebCore/rendering/style/SVGRenderStyleDefs.h', u'Source/WebCore/rendering/svg/RenderSVGImage.cpp', u'Source/WebCore/rendering/svg/RenderSVGImage.h', u'Source/WebCore/rendering/svg/SVGRenderingContext.cpp', u'Source/WebCore/rendering/svg/SVGRenderingContext.h', u'Source/WebCore/svg/SVGStyledElement.cpp', u'Source/WebCore/svg/svgattrs.in']" exit_code: 1 Source/WebCore/rendering/style/SVGRenderStyleDefs.h:87: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Source/WebCore/rendering/style/SVGRenderStyleDefs.h:88: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Source/WebCore/rendering/style/SVGRenderStyleDefs.h:89: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 3 in 25 files If any of these errors are false positives, please file a bug against check-webkit-style.
Landed manually with http://trac.webkit.org/changeset/147348