Bug 69416 – Prepare SVGImage intrinsic size negotiation: Introduce an IntSize <-> SVGImage cache in CachedImage

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Nikolas Zimmermann]

Prepare SVGImage intrinsic size negotiation: Make it possible to reuse the same SVGImage at different sizes.
The intrinsic width of a SVGImage is dependent on the place where it's embedded, soon.
To make this work, we have to associate Images with their owners, by passing a ImageUser pointer to the various GraphicsContext/Image draw() methods.
SVGImage::draw() and friends, will use that to lookup a size for a renderer from a HashMap. This will be done in a follow-up patch, covered by bug 47156.

[Nikolas Zimmermann]

Created attachment 109784 [details]
Patch v1

Adding draft patch, unlikely to build on non-Mac platforms yet, as it touches a lot of call sites in platform/.
Uploading early to receive EWS results...

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/loader/cache/CachedImage.h:64:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/loader/cache/CachedImage.h:65:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 2 in 77 files


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

[WebKit Review Bot]

Comment on attachment 109784 [details]
Patch v1

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

[Gyuyoung Kim]

Comment on attachment 109784 [details]
Patch v1

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

[Early Warning System Bot]

Comment on attachment 109784 [details]
Patch v1

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

[Nikolas Zimmermann]

Created attachment 109793 [details]
Patch v2

First stab at fixing non-mac platforms.

[WebKit Review Bot]

Comment on attachment 109793 [details]
Patch v2

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

[Early Warning System Bot]

Comment on attachment 109793 [details]
Patch v2

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

[Gyuyoung Kim]

Comment on attachment 109793 [details]
Patch v2

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

[Nikolas Zimmermann]

Created attachment 109805 [details]
Patch v3

Next round of fixes...

[Early Warning System Bot]

Comment on attachment 109805 [details]
Patch v3

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

[WebKit Review Bot]

Comment on attachment 109805 [details]
Patch v3

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

[Nikolas Zimmermann]

Created attachment 109844 [details]
Patch v4

Next round of fixes...

[James Robinson]

Comment on attachment 109844 [details]
Patch v4

I don't think it makes any sense to impose this upon the platform notion of Image.  Image::size() is already virtual so can't you just implement the behavior you want in SVGImage's override?

[Nikolas Zimmermann]

(In reply to comment #14)
> (From update of attachment 109844 [details])
> I don't think it makes any sense to impose this upon the platform notion of Image.  Image::size() is already virtual so can't you just implement the behavior you want in SVGImage's override?
You're right, now that I had some sleep I agree :-)

It was a lazy approach with high cost on platform layer, I already have a different, cleaner solution in mind.

[Nikolas Zimmermann]

Created attachment 110115 [details]
New approach - v1

A less intrusive approach, with minimal impact on the platform layer.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/rendering/style/StyleImage.h:110:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/rendering/style/StyleImage.h:111:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 17 files


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

[Darin Adler]

Comment on attachment 110115 [details]
New approach - v1

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

> Source/WebCore/ChangeLog:41
> +        ImageUser is just a simple typedef const void*, as we can't use RenderObject in platform/ as that would
> +        be a layering violation (and its also needed for non-RenderObject derived classes to use SVGImages).

What is done with the ImageUser in SVGImage? We should create an abstract base class for that which RenderObject can inherit from and not use a const void*.

[Nikolas Zimmermann]

(In reply to comment #18) 
> What is done with the ImageUser in SVGImage? We should create an abstract base class for that which RenderObject can inherit from and not use a const void*.

In a follow-up patch (see master bug 47156, whose patch makes us of this) SVGImage maintains a HashMap<ImageUser, IntSize> to return the correct override container sizes for a certain user of that Image. SVGImage::setImageContainerSize adds values to the HashMap, size() and paint() etc. query the HashMap.

Your suggestion to use a new abstract base class instead of a typdef to const void* would work as well, though it's not only RenderObject who would need to inherit from it, but also eg. SVGFEImage (non RenderObject derived but uses SVGImages).

Would you prefer if I'd introduce a "class ImageUser { }" and let RenderObject/SVGFEImage inherit from it?

[Darin Adler]

(In reply to comment #19)
> Would you prefer if I'd introduce a "class ImageUser { }" and let RenderObject/SVGFEImage inherit from it?

I do think there is value even if the class has no members, but the value is not as great was I though if an “image user” truly is only a hash table key.

[Antti Koivisto]

I wish you had tried my suggestion in https://bugs.webkit.org/show_bug.cgi?id=67087#c9 of going with multiple SVGImages per CachedImage, one per size the image is used in. I suspect (though have no proof, as this code is complex and I may be missing subtleties) that that would have led to a cleaner architecture without closing any avenues for future optimizations.

Having to use out-of-band information transfer tricks like ImageUser here is usually a sign of architecture heading to a wrong direction.

[Nikolas Zimmermann]

(In reply to comment #21)
> I wish you had tried my suggestion in https://bugs.webkit.org/show_bug.cgi?id=67087#c9 of going with multiple SVGImages per CachedImage, one per size the image is used in. I suspect (though have no proof, as this code is complex and I may be missing subtleties) that that would have led to a cleaner architecture without closing any avenues for future optimizations.
Well, I told you on IRC how heavy constructing SVGImages are. The better approach is to cache multiple rendered ImageBuffers per size, but that's a future optimization. Your suggested approach would work, but you're creating seperated dom/render trees/documents for each of the SVGImages. I don't think that's a good idea. I plan to add an additional caching layering in CachedImage in future, that handles caching rendered ImageBuffers, but that's not going to avoid ImageUser.

> Having to use out-of-band information transfer tricks like ImageUser here is usually a sign of architecture heading to a wrong direction.
I'd agree if I'd use the ImageUser for anything else than putting it into a HashMap as key to map an arbitrary user of the CachedImage (eg. RenderObject or SVGFEImage) to a certain IntSize.

[Nikolas Zimmermann]

Created attachment 110828 [details]
Next try

Awaiting EWS results...

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1

Source/WebCore/loader/cache/ImageBySizeCache.h:26:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/ChangeLog:76:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 2 in 65 files


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

[Early Warning System Bot]

Comment on attachment 110828 [details]
Next try

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

[Nikolas Zimmermann]

Created attachment 110834 [details]
Next try - v2

Fix qt build & style issues - should be done now.

[Early Warning System Bot]

Comment on attachment 110834 [details]
Next try - v2

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

[Antti Koivisto]

Comment on attachment 110834 [details]
Next try - v2

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

Looks generally good, my comments are mostly about naming and style.

> Source/WebCore/bindings/objc/DOM.mm:442
> -    return cachedImage->image()->getNSImage();
> +    return cachedImage->image(renderer)->getNSImage();

I think there should be a separate call for getting renderer specific version of the image, imageForRenderer() perhaps. CacheImage::image() (instead of image(0) as this patch does) should still return the generic image (or assert if that doesn't make sense).

> Source/WebCore/html/HTMLImageElement.cpp:260
> -            return m_imageLoader.image()->imageSize(1.0f).width();
> +            return m_imageLoader.image()->imageSize(renderer(), 1.0f).width();

Similarly imageSizeForRenderer.

> Source/WebCore/loader/cache/CachedImage.cpp:217
> -void CachedImage::setImageContainerSize(const IntSize& containerSize)
> +void CachedImage::setImageContainerSize(const RenderObject* renderer, const IntSize& containerSize)

setContainerSizeForRenderer perhaps?

> Source/WebCore/loader/cache/CachedImage.cpp:239
> +    // FIXME: Turn this on once, webkit.org/b/47156 lands.
> +#if ENABLE(SVG) && 0
> +    // SVGImages are independently cached per size in the ImageBySizeCache.
> +    RenderObjectSizeCountMap::const_iterator it = m_svgImageCache.clients().find(renderer);
> +    if (it != m_svgImageCache.clients().end()) {
> +        IntSize currentSize = it->second.first;
> +        ASSERT(!currentSize.isEmpty());
> +        if (currentSize == containerSize)
> +            return;
> +        m_svgImageCache.removeClient(renderer);
> +        if (!containerSize.isEmpty())
> +            m_svgImageCache.addClient(renderer, containerSize);
> +        return;
> +    }

There is quite of bit of caching logic added to CachedImage here and elsewhere. I wonder if more of it could live in the cache class.

> Source/WebCore/loader/cache/CachedImage.cpp:297
> -IntRect CachedImage::imageRect(float multiplier) const
> +IntRect CachedImage::imageRect(const RenderObject* renderer, float multiplier)

I can't find any clients for this call in this patch. Can the method be removed?

> Source/WebCore/loader/cache/ImageBySizeCache.cpp:22
> -#include "CSSImageGeneratorValue.h"
> +#include "ImageBySizeCache.h"

This shouldn't be under loader/cache, that is really for classes closer to networking. rendering/ would probably be appropriate as this deals with mappings from RenderObjects.

> Source/WebCore/loader/cache/ImageBySizeCache.cpp:66
> -Image* CSSImageGeneratorValue::getImage(RenderObject* renderer, const IntSize& size)
> +Image* ImageBySizeCache::getImage(const RenderObject* renderer, const IntSize& size)

Not a fan of the name "ImageBySizeCache". It doesn't really capture what this class does. From client perspective it really maps from RenderObject to Image. I couldn't came up with any great names though.

> Source/WebCore/loader/cache/ImageBySizeCache.h:35
> +typedef pair<IntSize, int> SizeCountPair;
> +typedef HashMap<const RenderObject*, SizeCountPair> RenderObjectSizeCountMap;

Copy code I know, but I would rather use a struct instead of pair<IntSize, int>. The resulting code reads much better.

> Source/WebCore/platform/win/ClipboardWin.cpp:242
> -    ASSERT(image->image()->data());
> +    ASSERT(image->image(0)->data());

These all go away if you just keep image() for non-renderer specific case.

[Nikolas Zimmermann]

Created attachment 110844 [details]
Next try - v3

Fixed Anttis comments. I hope merging worked fine.

[Antti Koivisto]

r=me

[Nikolas Zimmermann]

(In reply to comment #30)
> r=me

Thanks a lot, landed in r97448. Now continuing work on a new patch for bug 47156.

[Nikolas Zimmermann]

Comment on attachment 110844 [details]
Next try - v3

Fixed mac builds in r97452. On recreating the XCode additions on my other machine, I forgot to mark ImageBySizeCache.h private.