369528 - SVG code not handing surface endianness properly

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[tor]

Attachment: respect endianness

SVG filters render oddly or not at all on OS-X/PPC.  Half of this is due to bug 367134, which affect Intel and PPC, the rest has to do with how we interpret the information in the image surfaces.  We currently access it as a byte array, with assumptions about the channel ordering, but cairo stores surfaces as 32-bit pixel values in native endianness.

The attached patch uses macros added to thebes to decompose/recompose cairo packed pixel values.  The operations could be sped up with custom versions for the various endianness, but lets get things working properly first.

Comment 1

[Robert Longson [:longsonr]]

Comment on attachment 254186 [details] [diff] [review]
respect endianness

>Index: layout/svg/base/src/nsSVGUtils.cpp
>===================================================================
> void
> nsSVGUtils::UnPremultiplyImageDataAlpha(PRUint8 *data, 
>                                         PRInt32 stride,
>                                         const nsRect &rect)
> {
>   for (PRInt32 y = rect.y; y < rect.YMost(); y++) {
>     for (PRInt32 x = rect.x; x < rect.XMost(); x++) {
>-      PRUint8 *pixel = data + stride * y + 4 * x;
>+      PRUint32 *pixel = (PRUint32 *)(data + stride * y + 
4 * x);
>+      nscolor color = *pixel;

How about this instead?

  nscolor *color = (color *)(data + stride * y + 4 * x);

Then work with *color. Avoids copying the data. After all nscolor is just a PRUint32 really.

>-        pixel[0] = (255 * pixel[0]) / a;
>-        pixel[1] = (255 * pixel[1]) / a;
>-        pixel[2] = (255 * pixel[2]) / a;
>+        *pixel = GFX_ARGB((255 * GFX_GET_R(color)) / a,
>+                          (255 * GFX_GET_G(color)) / a,
>+                          (255 * GFX_GET_B(color)) / a,
>+                          a);

Why are you using NS_GET_A for alpha but GFX_GET_R for red? I'm not sure where the GFX_GET or GFX_ARGB macros come from. Are they different to the NS_GET_ and NS_RGBA? If not can we use the NS_ versions as we do elsewhere?

> nsSVGUtils::PremultiplyImageDataAlpha(PRUint8 *data, 
>                                       PRInt32 stride,
>                                       const nsRect &rect)
> {
>   for (PRInt32 y = rect.y; y < rect.YMost(); y++) {
>     for (PRInt32 x = rect.x; x < rect.XMost(); x++) {

> 
>-      FAST_DIVIDE_BY_255(pixel[0], pixel[0] * a);
>-      FAST_DIVIDE_BY_255(pixel[1], pixel[1] * a);
>-      FAST_DIVIDE_BY_255(pixel[2], pixel[2] * a);
>+      PRUint8 r, g, b;
>+      FAST_DIVIDE_BY_255(r, GFX_GET_R(color) * a);
>+      FAST_DIVIDE_BY_255(g, GFX_GET_G(color) * a);
>+      FAST_DIVIDE_BY_255(b, GFX_GET_B(color) * a);

Perhaps we should use GFX_PREMULTIPLY here although it does the same thing. If we're not going to use the GFX_ macros at all though then it makes sense to leave this as FAST_DIVIDE...

Same comments apply to the rest. All looks fine from a functional POV.

Comment 2

[Robert Longson [:longsonr]]

Don't you need to do something in nsSVGFilterFrame::FilterPaint for the NS_FE_SOURCEALPHA case too?

Comment 3

[tor]

Attachment: update per comments

The NS_GET_* were left from when I was using those macros before reading the fine print and seeing that mozilla gfx and cairo ordered the channels differently within a work (ABGR vs ARGB).

Comment 4

[Robert Longson [:longsonr]]

Comment on attachment 254326 [details] [diff] [review]
update per comments

> NS_IMETHODIMP
> nsSVGFEBlendElement::GetRequirements(PRUint32 *aRequirements)
> {
>@@ -1455,20 +1461,22 @@ nsSVGFEComponentTransferElement::Filter(
>     if (elementA)
>       elementA->GenerateLookupTable(tableA);
>   }
> 
>   PRInt32 stride = fr.GetDataStride();
>   for (PRInt32 y = rect.y; y < rect.YMost(); y++)
>     for (PRInt32 x = rect.x; x < rect.XMost(); x++) {
>       PRInt32 targIndex = y * stride + x * 4;
>-      targetData[targIndex] = tableB[sourceData[targIndex]];
>-      targetData[targIndex + 1] = tableG[sourceData[targIndex + 1]];
>-      targetData[targIndex + 2] = tableR[sourceData[targIndex + 2]];
>-      targetData[targIndex + 3] = tableA[sourceData[targIndex + 3]];
>+      PRUint32 source = *(PRUint32 *)(sourceData + targIndex);

Make source a PRUInt32 * to avoid copying the data.

>+      *(PRUint32 *)(targetData + targIndex) =
>+        GFX_ARGB(tableA[GFX_GET_A(source)],
>+                 tableR[GFX_GET_R(source)],
>+                 tableG[GFX_GET_G(source)],
>+                 tableB[GFX_GET_B(source)]);
>     }
>   return NS_OK;
> }
> 

>+      PRInt32 col[4];
>       if (type == nsIDOMSVGFETurbulenceElement::SVG_TURBULENCE_TYPE_TURBULENCE) {
>         for (int i = 0; i < 4; i++)
>-          col[i] = Turbulence(i, point, fX, fY, octaves, PR_FALSE,
>-                              doStitch, filterX, filterY, filterWidth, filterHeight) * 255;
>+          col[i] = (PRInt32)(Turbulence(i, point, fX, fY, octaves, PR_FALSE,
>+                                        doStitch, filterX, filterY,
>+                                        filterWidth, filterHeight) * 255);

You could use NS_STATIC_CAST rather than (PRInt32). Various other opportunities to use NS_STATIC_CAST. I'd give it an r+ without this though.

nsSVGFilterFrame::FilterPaint(nsSVGRende
>     }
> 
>     PRUint8 *data = tmpSurface->Data();
>     PRUint8 *alphaData = cairo_image_surface_get_data(alpha);
>     PRUint32 stride = tmpSurface->Stride();
> 
>     for (PRUint32 yy=0; yy<filterResY; yy++)
>       for (PRUint32 xx=0; xx<filterResX; xx++) {
>-        alphaData[stride*yy + 4*xx]     = 0;
>-        alphaData[stride*yy + 4*xx + 1] = 0;
>-        alphaData[stride*yy + 4*xx + 2] = 0;
>-        alphaData[stride*yy + 4*xx + 3] = data[stride*yy + 4*xx + 3];
>+        *(PRUint32 *)(alphaData + stride * yy + 4 * xx) =
>+          GFX_ARGB(GFX_GET_A(*(PRUint32 *)(data + stride * yy + 4 * xx)),
>+                   0, 0, 0);

How about:

PRUint32 *pixel = (PRUint32 *)(alphaData + stride * yy + 4 * xx);
*pixel = GFX_ARGB(GFX_GET_A(*pixel), 0, 0, 0);

>Index: layout/svg/base/src/nsSVGMaskFrame.cpp
>===================================================================
>   for (PRUint32 y = 0; y < height; y++)
>     for (PRUint32 x = 0; x < width; x++) {
>-      PRUint8 *pixel = data + stride * y + 4 * x;
>+      PRUint32 *pixel = (PRUint32 *)(data + stride * y + 4 * x);
> 
>       /* linearRGB -> intensity */
>-      pixel[3] = (PRUint8)((pixel[2] * 0.2125 +
>-                            pixel[1] * 0.7154 +
>-                            pixel[0] * 0.0721) *
>-                            (pixel[3] / 255.0) * aOpacity);
>-      pixel[0] = 255;
>-      pixel[1] = 255;
>-      pixel[2] = 255;
>+      PRUint8 alpha = (PRUint8)((GFX_GET_R(*pixel) * 0.2125 +
>+                                 GFX_GET_G(*pixel) * 0.7154 +
>+                                 GFX_GET_B(*pixel) * 0.0721) *
>+                                (GFX_GET_A(*pixel) / 255.0) * aOpacity);
>+      *pixel = GFX_ARGB(alpha, alpha, alpha, alpha);

Now produces a premultiplied result. You did mean to make this change didn't you?

>     }

Comment 5

[tor]

> >     for (PRUint32 yy=0; yy<filterResY; yy++)
> >       for (PRUint32 xx=0; xx<filterResX; xx++) {
> >-        alphaData[stride*yy + 4*xx]     = 0;
> >-        alphaData[stride*yy + 4*xx + 1] = 0;
> >-        alphaData[stride*yy + 4*xx + 2] = 0;
> >-        alphaData[stride*yy + 4*xx + 3] = data[stride*yy + 4*xx + 3];
> >+        *(PRUint32 *)(alphaData + stride * yy + 4 * xx) =
> >+          GFX_ARGB(GFX_GET_A(*(PRUint32 *)(data + stride * yy + 4 * xx)),
> >+                   0, 0, 0);
> 
> How about:
> 
> PRUint32 *pixel = (PRUint32 *)(alphaData + stride * yy + 4 * xx);
> *pixel = GFX_ARGB(GFX_GET_A(*pixel), 0, 0, 0);

Not quite right; there's two bits of information we're looking up, from "alphaData" and "data".

> >+      PRUint8 alpha = (PRUint8)((GFX_GET_R(*pixel) * 0.2125 +
> >+                                 GFX_GET_G(*pixel) * 0.7154 +
> >+                                 GFX_GET_B(*pixel) * 0.0721) *
> >+                                (GFX_GET_A(*pixel) / 255.0) * aOpacity);
> >+      *pixel = GFX_ARGB(alpha, alpha, alpha, alpha);
> 
> Now produces a premultiplied result. You did mean to make this change didn't
> you?

Yes, having a invalid premultipied color value seemed incorrect when I was modifying the code.

Comment 6

[tor]

Attachment: update per comments

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Have you checked that this produces the same (or at least as good) code for little-endian machines?

Comment 8

[Robert Longson [:longsonr]]

A different approach (but more in keeping with the current implementation) might be to define some offsets e.g.

#if defined(IS_BIG_ENDIAN)
#define GFX_CAIRO_ARGB32_ALPHA 0
#define GFX_CAIRO_ARGB32_RED   1
#define GFX_CAIRO_ARGB32_GREEN 2
#define GFX_CAIRO_ARGB32_BLUE  3
#else
#define GFX_CAIRO_ARGB32_ALPHA 3
#define GFX_CAIRO_ARGB32_RED   2
#define GFX_CAIRO_ARGB32_GREEN 1
#define GFX_CAIRO_ARGB32_BLUE  0
#endif

Then use these defines to access the bytes

e.g. in nsSVGFETurbulenceElement::Filter

targetData[targIndex + GFX_CAIRO_SURFCE_BLUE] = b;
targetData[targIndex + GFX_CAIRO_SURFCE_GREEN] = g;
targetData[targIndex + GFX_CAIRO_SURFCE_RED] = r;
targetData[targIndex + GFX_CAIRO_SURFCE_ALPHA] = a;

In fact perhaps cairo itself should define something?

Comment 9

[tor]

Some performance numbers for the patch on a feComponentTransfer in linearRGB torture case, Hixie performance fraework, time in seconds, best of five runs:

             before      patched

linux/x86     17.52      17.82        101.7%

win32/x86     29.19      30.27        103.7%

os-x/ppc      79.51      75.48         94.9%

linux and os-x optimization option -Os, win32 default opt build.

Comment 10

[tor]

Attachment: use Robert's suggestion of offset macros

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 256529 [details] [diff] [review]
use Robert's suggestion of offset macros

+  const PRUint32 channels[] = { GFX_ARGB32_OFFSET_B,
+                                GFX_ARGB32_OFFSET_G,
+                                GFX_ARGB32_OFFSET_R };
 
   for (PRInt32 x = rect.x; x < rect.XMost(); x++) {
     for (PRInt32 y = rect.y; y < rect.YMost(); y++) {
       PRUint32 targIndex = y * stride + 4 * x;
-      PRUint32 qa = targetData[targIndex + 3];
-      PRUint32 qb = sourceData[targIndex + 3];
+      PRUint32 qa = targetData[targIndex + GFX_ARGB32_OFFSET_A];
+      PRUint32 qb = sourceData[targIndex + GFX_ARGB32_OFFSET_A];
       for (PRInt32 i = 0; i < 3; i++) {
-        PRUint32 ca = targetData[targIndex + i];
-        PRUint32 cb = sourceData[targIndex + i];
+        PRUint32 ca = targetData[targIndex + channels[i]];
+        PRUint32 cb = sourceData[targIndex + channels[i]];

Instead of using this channels array, you could write
  for (PRInt32 i = PR_MIN(GFX_ARGB32_OFFSET_B, GFX_ARGB32_OFFSET_R);
       i <= PR_MAX(GFX_ARGB32_OFFSET_B, GFX_ARGB32_OFFSET_R); ++) {

Comment 12

[tor]

Attachment: version for checkin, incorporates roc's suggestion

Comment 13

[tor]

Stuart isn't that interested in adding channel offset macros to gfxColor.h (see bug 369529 comment 3).  We could move the macros to nsSVGUtils.h and still take that direction with this patch.

Comment 14

[Robert Longson [:longsonr]]

(In reply to comment #13)
> Stuart isn't that interested in adding channel offset macros to gfxColor.h (see
> bug 369529 comment 3).  We could move the macros to nsSVGUtils.h and still take
> that direction with this patch.
> 

Would Carl be interested in putting something in cairo.h?

Comment 15

[tor]

(In reply to comment #14)
> (In reply to comment #13)
> > Stuart isn't that interested in adding channel offset macros to gfxColor.h (see
> > bug 369529 comment 3).  We could move the macros to nsSVGUtils.h and still take
> > that direction with this patch.
> 
> Would Carl be interested in putting something in cairo.h?

I asked, and he doesn't seem in favor of adding such items.

Comment 16

[Jonathan Watt [:jwatt]]

Did Carl say anything after "What values are you thinking of? I would imagine one would want masks and shift amounts. But, no, I don't think I'd ever considered trying to help the users here."? I initially incorrectly read that as "consider" not "considered". Don't know if you did the same?

Comment 17

[tor]

Attachment: move utility macros to nsSVGUtils.h

Comment 18

[tor]

Comment on attachment 255147 [details] [diff] [review]
update per comments

Removing old request.

Comment 19

[Robert Longson [:longsonr]]

(In reply to comment #17)

I know the ground changed under you but shouldn't there be nsSVGFEColorMatrixElement::Filter changes?

Comment 20

[tor]

(In reply to comment #16)
> Did Carl say anything after "What values are you thinking of? I would imagine
> one would want masks and shift amounts. But, no, I don't think I'd ever
> considered trying to help the users here."? I initially incorrectly read that
> as "consider" not "considered". Don't know if you did the same?

Yes, I did.  He clarified that he just hasn't considered this functionality - I'll write a proposal to the cairo list and see what feedback that gets.

Comment 21

[tor]

Attachment: fix feColorMatrix

Comment 22

[Jonathan Watt [:jwatt]]

> I'll write a proposal to the cairo list and see what feedback that gets.

Cool. That's great.

Comment 23

[tor]

(In reply to comment #20)
> I'll write a proposal to the cairo list and see what feedback that gets.

Short thread starts here:

  http://lists.freedesktop.org/archives/cairo/2007-March/009952.html

Comment 24

[tor]

Checked in - I'll revisit the issue if cairo adds one or both of the APIs.