Bug 19243 – Make SVG Masking platform aware

• Component: SVG
• Status: RESOLVED FIXED

[Dirk Schulze]

The masking-support is missing in Cairo/SVG. I made a basic implementation but run in to some problems.
The cairo-masking support ignores RGB and only looks to the alpha-channel. So you have to transform the mask-surface first.
After (or before) that you insert the masked object to the context and mask it with the transformed mask-surface.

Cg seems to do it a bit different because the masked object is drawn above the mask (the image seems to be given to context AFTER the mask is applied).

How can I get the masked object in SVGResourceMaskerCairo.cpp and avoid WebKit to draw the image after the mask was applied?

[Dirk Schulze]

Created attachment 21333 [details]
Masking Cairo/SVG

The basic implementation. Look at the TODOs for the problems described above.

[Dirk Schulze]

only a complement: The source pattern is masked in cairo. A group has to be on one pattern and this pattern have to be added to the source pattern.

An example:
in SVG
  <g id="group" x="20" y="260" width="320" height="250" mask="url(#m1)">
    <circle cx="80" cy="400" r="50" fill="blue" />
    <circle cx="180" cy="300" r="50" fill="green" />
    <circle cx="280" cy="400" r="50" fill="yellow" />
  </g>

in Cairo it could look like:
  cairo_surface_t* surface;
  cairo_surface_t* mask;
  cairo_surface_t* group;
  cairo_t* cr_group;

  // The masked group is added to a new context
  surface = cairo_image_surface_create(CAIRO_FORMAT_ARGB32, 360, 260);
  cr_group = cairo_create(surface);

  cairo_arc (cr_group, 100.0, 160.0, 50.0, 0, 2*3.14159265);
  cairo_set_source_rgb(cr_group, 0, 0, 1);
  cairo_fill(cr_group);

  cairo_arc (cr_group, 200.0, 60.0, 50.0, 0, 2*3.14159265);
  cairo_set_source_rgb(cr_group, 0, 1, 0);
  cairo_fill(cr_group);

  cairo_arc (cr_group, 300.0, 160.0, 50.0, 0, 2*3.14159265);
  cairo_set_source_rgb(cr_group, 1, 1, 0);
  cairo_fill(cr_group);

  // Add the group to the source
  group = cairo_get_target(cr_group);
  cairo_set_source_surface(cr, group, 0, 0);

  // Apply the mask to the source
  mask = cairo_image_surface_create_from_png ("image2.png");
  cairo_mask_surface(cr, mask, 0, 0);
  cairo_destroy(cr); 

this code demonstrates the procedure.

[Dirk Schulze]

(In reply to comment #2)
> only a complement: The source pattern is masked in cairo. A group has to be on
> one pattern and this pattern have to be added to the source pattern.

not necessarily a source pattern. The group can be a surface and this is added to the source (like in the example above).

[Eric Seidel (no email)]

Comment on attachment 21333 [details]
Masking Cairo/SVG

This could be cleaner.

For:
+    // TODO: check if there is a difference between little and big endian machines (for this code :-))

Just check the docs for CAIRO_FORMAT_ARGB32.  They should tell you what format you are to expect the pixels in.

+            int alpha = (*pixel & 0xff000000) >> 24;

Should really be encapsulated into static inline funtions (or use the existing ones in Cairo).

unsigned char alpha = alphaValue(pixel);

etc.

+            int luma = (int) (red * 0.3 + green * 0.59 + blue * 0.11);

Please add a comment to explain why you're choosing those multipliers.

Also, if you're going to be using alpha, red, green, etc. as floats, you might as well store them in floats to begin with (instead of the ints you're currently using, or the unsigned chars in my above example)

Again, something like this would be clearer:
*pixel = pixelFromValues(alpha, luma, luma, luma);

you could even have used something like this above:
float alpha, red, green, blue;
valuesFromPixel(alpha, red, green, blue);

I think some simple inlines like that would make this code much more readable (and less error prone).

m_mask.set(m_ownerElement->drawMaskerContent(boundingBox, m_maskRect).release())
drawMaskerContent returns a PassRefPtr, no need to call release() on it, as it will automatically release into the RefPtr (that's the point of a PRP).

This comment doesn't help:
+    // Transform the mask-surface for use of cairo_mask()


Otherwise looks OK.  I'd rather see the TODOs just fixed now, instead of landing this partial version.

r- due the fact that this code should be cleaner/more readable.

[Dirk Schulze]

Created attachment 27230 [details]
platform aware SVG Maskeing

This patch makes SVG Masking platform aware.

Every platform, that wants to use it needs an implementation for ImageBuffer::getImageData, ImageBuffer::putImageData and GraphicsContext::clipToImageBuffer.

It corrects some drawing issues with masking on CG.

We have to make sure that it is not excessive slower than the current implementation on CG, but it doesn't feel like that.

[Eric Seidel (no email)]

Comment on attachment 27230 [details]
platform aware SVG Maskeing

This looks fantastic!   I think it could use a little cleanup yet.

+    for (int i=0; i < imageSize.width() * imageSize.height(); i++)
+    {
does not match webkit style
i = 0
{ should be on the same line as the for

There has to be a nice way to get the dat than this:
+        sourceImageData->data()->get((i * 4), r);
+        sourceImageData->data()->get((i * 4 + 1), g);
+        sourceImageData->data()->get((i * 4 + 2), b);
+        sourceImageData->data()->get((i * 4 + 3), a);
don't we have a getPixel function or something that will fill an int or 4 chars or something?

Won't this doe the wrong thing for alpha = 0 ?

+        if (a != 0 && a != 255) {
+            double alpha = a;
+            luma = luma * (alpha / 255);
+        }

shouldn't that set luma to 0 when alpha is 0?

Also, I think the whole for contents shoudl be abstracted into a static function.  Something like:

int rgbaValue;
sourceImageData->data()->getPixel(i*4, rgbaValue);
applyLuminecenceFilterToPixel(destImageData, i);

Since destImageData is already a copy of source going into the for loop (which could possibly be avoided), no need to even pass the source into your inline.

Again, it looks fantastic.  Just a tiny amount of cleanup needed.

[Dirk Schulze]

(In reply to comment #6)
> (From update of attachment 27230 [details] [review])
> There has to be a nice way to get the dat than this:
> +        sourceImageData->data()->get((i * 4), r);
> +        sourceImageData->data()->get((i * 4 + 1), g);
> +        sourceImageData->data()->get((i * 4 + 2), b);
> +        sourceImageData->data()->get((i * 4 + 3), a);
> don't we have a getPixel function or something that will fill an int or 4 chars
> or something?
No, there isn't. If you give a wrong index, you'll get the colors of one and another pixel. e.g index=2 you'll get b,a of the first pixel and r,g of the next one. Sure, it can happen if you take every color apart too. But calling for rgba could give you a wrong feeling of safety.

> Won't this doe the wrong thing for alpha = 0 ?
> 
> +        if (a != 0 && a != 255) {
> +            double alpha = a;
> +            luma = luma * (alpha / 255);
> +        }
> 
> shouldn't that set luma to 0 when alpha is 0?
Well, it shoulf, normaly. But we only need the alpha for masking, and if alpha is already 0, we don't need further calculation.

> Also, I think the whole for contents shoudl be abstracted into a static
> function.  Something like:
> 
> int rgbaValue;
> sourceImageData->data()->getPixel(i*4, rgbaValue);
> applyLuminecenceFilterToPixel(destImageData, i);
> 
> Since destImageData is already a copy of source going into the for loop (which
> could possibly be avoided), no need to even pass the source into your inline.
I have a patch to use the pixelarray instead of calling srcImageData. I'll update it soon. But why abstracting this short peace of code?

[Dirk Schulze]

Created attachment 27614 [details]
platform aware SVG Masking

sorry eseidel. I was wrong with the alpha.
clean up and use pixelarray instead of calling imageData.

[Eric Seidel (no email)]

Comment on attachment 27614 [details]
platform aware SVG Masking

I think this ASSERT will fail with large masks.  You should write a test case with a super-large mask (or maybe we already have one?)

+void SVGResourceMasker::applyMask(GraphicsContext* context, const FloatRect& boundingBox)
+{
+    if (!m_mask)
+        m_mask.set(m_ownerElement->drawMaskerContent(boundingBox, m_maskRect).release());
+
+    ASSERT(m_mask);

All of these large mallocs could fail, we need to handle that.

basei would make more sense to me as pixelByteOffset or something. "i" could be renamed "pixelOffset"  I'm not sure those names are super-clear, but they're more clear than "i" and "basei" IMO.

I don't think you need a local for "alpha", (double)a should be sufficient.

Your' mixing float and double math here.  I would remove all of the f's from these.  And 255 should be 255.0

Otherwise this looks fine.

[Dirk Schulze]

Created attachment 27679 [details]
platform aware SVG Masking

The corrections of above. This patch will cause 3 pixel failures.
2 of them just need an update and a correction. But LayoutTests/svg/custom/grayscale-gradient-mask.svg is a bit tricky.
After this patch, the drawing looks like the one of opera, librsvg, inkscape and differs to current safari, firefox, batik. Not sure who is right currently. I would prefer Opera, librsvg, Inkscape ;-)

[Dirk Schulze]

I asked on the W3C-SVG mailing lsit. And it seems, that the current implementation, FF and Batik are wrong. That means, this patch fixes some more bugs.

[Dirk Schulze]

Created attachment 28015 [details]
platform aware SVG Masking

Corrected the tests, that have changed with this patch. The W3C-test looks correct now. I made the mask bigger for mask-excessive-malloc, since we now support bigger masks on Mac. grayscale-gradient-mask looks like Opera now. I'm pretty sure that it looks correct now. I asked on the W3C-SVG mailing list and they agreed with me.

[Eric Seidel (no email)]

Comment on attachment 28015 [details]
platform aware SVG Masking

Assuming this passes all the tests, then fantastic!

[Dirk Schulze]

landed in r41266.