Bug 27933 – SVG Filter premultiplied color support for getImageDate/putImageData

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

[Dirk Schulze]

Some Filters need premultiplied colors for the calculation. Thats why  premultiplied color support for getImageDate/putImageData is needed.

[Dirk Schulze]

Created attachment 34037 [details]
premultiplied color support for getImageData/putImageData

premultiplied color support for getImageData/putImageData. Still not sure if a simple boolean in getImageData/putImageData for premultiplied or not is enough.

[Eric Seidel (no email)]

Comment on attachment 34037 [details]
premultiplied color support for getImageData/putImageData

+== Rolled over to ChangeLog-2009-06-16 ==SVG Filter premultiplied color support for getImageDate/putImageData

??

is "NotPreMultiplied" the normal terminolgy?  "unmultiplied"?  Google seems to suggest that "straight" is the opposite of PreMultiplied  I'm not sure that would be more clear here though.  Unmultiplied might be clearer than NotPreMultiplied.  I'm not a graphics expert though.

No argument names:
         PassRefPtr<ImageData> getNotPreMultipliedImageData(const IntRect& rect) const;
 73         PassRefPtr<ImageData> getPreMultipliedImageData(const IntRect& rect) const;
 
they don't add anything.

Likewise here: ImageData* source

Seems we should add a "colorFrom..." for whatever type of pixel this is:
                 destRows[basex]     = (*pixel & 0x00FF0000) >> 16;
 171                 destRows[basex + 1] = (*pixel & 0x0000FF00) >> 8;
 172                 destRows[basex + 2] = (*pixel & 0x0000FF00);
 173                 destRows[basex + 3] = (*pixel & 0xFF000000) >> 24;

Then we just create the Color in an if and the pixel assignment is shared.

Same here:
*pixel = srcRows[basex + 3] << 24 | srcRows[basex] << 16 | srcRows[basex + 1] << 8 | srcRows[basex + 2];

Better to use inlines to share more code.  They also are are more self-documenting.

Seems the toImage() result could be in a local:
     if (multiplied == NotPreMultiplied)
 134         image = data.m_pixmap.toImage().convertToFormat(QImage::Format_ARGB32);
 135     else
 136         image = data.m_pixmap.toImage().convertToFormat(QImage::Format_ARGB32_Premultiplied)

How do we test this?

[Dirk Schulze]

(In reply to comment #2)
> is "NotPreMultiplied" the normal terminolgy?  "unmultiplied"?  Google seems to
> suggest that "straight" is the opposite of PreMultiplied  I'm not sure that
> would be more clear here though.  Unmultiplied might be clearer than
> NotPreMultiplied.  I'm not a graphics expert though.

The SVG specification call it not premultiplied, but I also found straight and unmultiplied. There is no consistent term. I'll use "Unmultiplied" and "Premultiplied" for the next patch.

[Dirk Schulze]

Created attachment 34365 [details]
premultiplied color support for getImageData/putImageData

Corrected patch.

Testing is a bit difficult since only SVG Filters make use of premultiplied colors, as far as I know (at least for geImageData/putImageData). SVG Filters are neither activated nor does a  effect exist, that makes already use of premultiplied colors.

[Dirk Schulze]

landed in r47099.

[Dirk Schulze]

Created attachment 34651 [details]
premultiplied color support for Skia

This is a untested suggestion for Skia.

[Dirk Schulze]

Comment on attachment 34365 [details]
premultiplied color support for getImageData/putImageData

clearing review flag.

[Eric Seidel (no email)]

I think this would need a chromium person to try this locally.  How would they test if this worked?

[Dirk Schulze]

(In reply to comment #8)
> I think this would need a chromium person to try this locally.  How would they
> test if this worked?
Hm... a posibility is to apply the last patch of https://bugs.webkit.org/show_bug.cgi?id=28133 replace getImageData with getPremultipliedImageData() and putImageData with putPremultipliedImageData(), build with --filter enabled and look at http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-filters-blend-01-b.html (ignore the first line, it's a bug in the filter implementation).

Sorry, I know that this is a bit complex just for testing, but it was a preperation for SVG filters.

[Eric Seidel (no email)]

Comment on attachment 34651 [details]
premultiplied color support for Skia

Seems this should be re-posted when it's easier to test.

[Peter Kasting]

Landed a build fix for Skia in r47126 (r47099 did not compile).

[Brett Wilson (Google)]

Comment on attachment 34651 [details]
premultiplied color support for Skia

> Index: WebCore/platform/graphics/skia/SkiaUtils.cpp
> ===================================================================
> --- WebCore/platform/graphics/skia/SkiaUtils.cpp	(revision 47098)
> +++ WebCore/platform/graphics/skia/SkiaUtils.cpp	(working copy)
> @@ -80,7 +80,7 @@ static U8CPU InvScaleByte(U8CPU componen
>      return (component * scale + 0x8000) >> 16;
>  }
>  
> -SkColor SkPMColorToColor(SkPMColor pm)
> +SkColor SkUMColorToColor(SkPMColor pm)
>  {
>      if (0 == pm)
>          return 0;
> @@ -94,6 +94,17 @@ SkColor SkPMColorToColor(SkPMColor pm)
>                            InvScaleByte(SkGetPackedB32(pm), scale));
>  }
>  
> +SkColor SkPMColorToColor(SkPMColor pm)
> +{
> +    if (0 == pm)
> +        return 0;
> +    
> +    return SkColorSetARGB(SkGetPackedA32(pm),
> +                          SkGetPackedR32(pm),
> +                          SkGetPackedG32(pm),
> +                          SkGetPackedB32(pm));
> +}


It seems like you have the name of this function backwards. UMColor->Color should just be re-ordering the bytes, PMColor->Color needs scaling.

Also, having SKUMColorToColor take a PMColor argument is a bit confusing. I actually think writing it out like this is easier to follow, even though it looks longer:

unsigned char* destPixel = &destRow[x * 4];
if (multiplied == Unmultiplied) {
    SkColor color = SkPMColorToColor(srcRow[x]);
    destPixel[0] = SkColorGetR(color);
    destPixel[1] = SkColorGetG(color);
    destPixel[2] = SkColorGetB(color);
    destPixel[3] = SkColorGetA(color);
} else {
    // Input and output are both pre-multiplied, we just need to re-arrange the bytes from the bitmap format to RGBA.
    destPixel[0] = SkGetPackedR32(srcRow[x]);
    destPixel[1] = SkGetPackedG32(srcRow[x]);
    destPixel[2] = SkGetPackedB32(srcRow[x]);
    destPixel[3] = SkGetPackedA32(srcRow[x]);
}

When we want pre-multiplied case, we just need to re-arrange the output. Having the extra function in there (especially one that takes the "wrong" type) makes this harder to see.

Brett

[Dirk Schulze]

Created attachment 35113 [details]
premultiplied color support for Skia

changed the patch according to brett's comment.

[Eric Seidel (no email)]

Comment on attachment 35113 [details]
premultiplied color support for Skia

Given no further commentary from Brett, this looks sane to me.  Dirk is a committer, so cq-.  He can change it to cq+ if he'd prefer the bot to commit this for him.

[Brett Wilson (Google)]

Looks good to me, sorry if you were waiting for me.

[Eric Seidel (no email)]

Comment on attachment 35113 [details]
premultiplied color support for Skia

Rejecting patch 35113 from commit-queue.  This patch will require manual commit.

WebKitTools/Scripts/build-webkit failed with exit code 1

[Eric Seidel (no email)]

Comment on attachment 35113 [details]
premultiplied color support for Skia

build-webkit was broken for a moment.  Not sure why the bots weren't showing it.

[Eric Seidel (no email)]

Comment on attachment 35113 [details]
premultiplied color support for Skia

Clearing flags on attachment: 35113

Committed r47991: <http://trac.webkit.org/changeset/47991>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.

[Dimitri Glazkov (Google)]

Broke chromium build. Needs SkColorPriv.h include. Fix coming up.

[Dimitri Glazkov (Google)]

Build fix committed as http://trac.webkit.org/changeset/47995.

[Dimitri Glazkov (Google)]

Argh. There's no such value -- "Unpremultiplied". Should we just roll out? Ping.

[Dimitri Glazkov (Google)]

Another fix committed http://trac.webkit.org/changeset/47996.