Bug 32787 – SVG Mask result wrong, if two different objects call the same mask id

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Dirk Schulze]

Created attachment 45268 [details]
Mask called twice by different objects

If two different objects, with different properties call the same mask (the same mask id). The result of the second object is wrong. This is caused by the calculation of the mask image. This is done once for every mask (mask by id). So the mask image depends on the first object calling it.
I saw this bug during my work on SVG Filter. We had a similar problem with the filter size and it's effect. We build the filter on every drawing atm. That is a performance loss, but the only way to handle this problem atm.
We should identify masks, clipper, filters not only by their id, but also by the object they belong to.

I attached a simple exmaple. You should see two circles with different positions and dimensions.

[Nikolas Zimmermann]

We have to extend our SVGResource design, it has not been designed with the idea that a resource can NOT be shared between multiple clients.

We can only share SVGResources for gradients & clipper, at the time of writing. For filters, patterns, masks (anything that stores an ImageBuffer in a resource) we can't just hash the id of a resource against the SVGResource object itself, it needs to be dependant on the target element as well.

What we have now:
HashMap<String, SVGResource*>, identifying a SVGResource object by it's own id. (<clipPath id="foo" for example)

We could have a new HashMap<SVGElement*, SVGResource*> as our new main hash map, that maps _target_ elements to SVGResources. That would resolve the issue for filters, pattern, mask etc. Though for clipper/gradient there is no need to NOT share the resources. So we readd the existing hash map:
HashMap<String, SVGResource*> and use it to map resource-id to resource-objects, as it has been in trunk before.

Whenever we fail to lookup a resource in the HashMap<SVGElement*, SVGResource*> map, and if we're requesting gradients/clipper, then we can ask the id-map, if there's already a resource available for that id, if yes, we can just map the already-existing SVGResource* object to the SVGElement* that's just requesting it. This we we can still share resources for gradients/clipper, and resolve the bug for anything else.

....
This is rather simplified, as it would only allow us to hash _one_ resource per element. In real life we will need something like: HashMap<SVGElement*, HashSet<SVGResource*> >, as there may be multiple non-shared resources for a certain target SVGElement, but this is rather an implementation detail, and doesn't help to understand the problem, so I left it out in above analysis :-)

Hopefully anyone can start working on that soon..

[Dirk Schulze]

Created attachment 45519 [details]
Create more than one SVG Mask resource

This patch creates more than one SVG Mask resource, if more than one RenderObject call the same mask Id.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/svg/graphics/SVGResourceClipper.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/rendering/RenderSVGRoot.cpp:116:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 2

[Dirk Schulze]

Created attachment 45521 [details]
Create more than one SVG Mask resource

fixed style issues.

[Dirk Schulze]

Created attachment 45522 [details]
Create more than one SVG Mask resource

Some changes in RenderSVGRoot and SVGMaskElement after talking to Niko in IRC.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45522 [details] without any errors.

[Nikolas Zimmermann]

Comment on attachment 45522 [details]
Create more than one SVG Mask resource

LGTM, r=me.

Please remove trailing spaces, before landing below:

> Index: WebCore/svg/graphics/SVGResourceFilter.cpp
> ===================================================================
> --- WebCore/svg/graphics/SVGResourceFilter.cpp	(revision 52220)
> +++ WebCore/svg/graphics/SVGResourceFilter.cpp	(working copy)
> @@ -52,7 +52,7 @@ SVGResourceFilter::SVGResourceFilter(con
>      , m_savedContext(0)
>      , m_sourceGraphicBuffer(0)
>  {
> -    m_filterBuilder.set(new SVGFilterBuilder());
> +    m_filterBuilder.set(new SVGFilterBuilder());    
>  }

[Dirk Schulze]

Landed in r52580.