Bug 27933 - SVG Filter premultiplied color support for getImageDate/putImageData
Summary: SVG Filter premultiplied color support for getImageDate/putImageData
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 68469 26389 28133
  Show dependency treegraph
 
Reported: 2009-08-02 03:37 PDT by Dirk Schulze
Modified: 2014-05-12 05:54 PDT (History)
6 users (show)

See Also:


Attachments
premultiplied color support for getImageData/putImageData (29.27 KB, patch)
2009-08-03 23:30 PDT, Dirk Schulze
eric: review-
Details | Formatted Diff | Diff
premultiplied color support for getImageData/putImageData (27.81 KB, patch)
2009-08-08 06:08 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
premultiplied color support for Skia (4.06 KB, patch)
2009-08-12 03:52 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
premultiplied color support for Skia (3.16 KB, patch)
2009-08-19 02:32 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Schulze 2009-08-02 03:37:12 PDT
Some Filters need premultiplied colors for the calculation. Thats why  premultiplied color support for getImageDate/putImageData is needed.
Comment 1 Dirk Schulze 2009-08-03 23:30:49 PDT
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.
Comment 2 Eric Seidel (no email) 2009-08-06 18:11:09 PDT
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?
Comment 3 Dirk Schulze 2009-08-07 23:11:45 PDT
(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.
Comment 4 Dirk Schulze 2009-08-08 06:08:54 PDT
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.
Comment 5 Dirk Schulze 2009-08-12 03:33:57 PDT
landed in r47099.
Comment 6 Dirk Schulze 2009-08-12 03:52:49 PDT
Created attachment 34651 [details]
premultiplied color support for Skia

This is a untested suggestion for Skia.
Comment 7 Dirk Schulze 2009-08-12 03:53:27 PDT
Comment on attachment 34365 [details]
premultiplied color support for getImageData/putImageData

clearing review flag.
Comment 8 Eric Seidel (no email) 2009-08-12 11:09:59 PDT
I think this would need a chromium person to try this locally.  How would they test if this worked?
Comment 9 Dirk Schulze 2009-08-12 11:21:13 PDT
(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.
Comment 10 Eric Seidel (no email) 2009-08-12 11:22:42 PDT
Comment on attachment 34651 [details]
premultiplied color support for Skia

Seems this should be re-posted when it's easier to test.
Comment 11 Peter Kasting 2009-08-12 11:24:00 PDT
Landed a build fix for Skia in r47126 (r47099 did not compile).
Comment 12 Brett Wilson (Google) 2009-08-12 12:59:18 PDT
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
Comment 13 Dirk Schulze 2009-08-19 02:32:06 PDT
Created attachment 35113 [details]
premultiplied color support for Skia

changed the patch according to brett's comment.
Comment 14 Eric Seidel (no email) 2009-09-02 03:25:08 PDT
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.
Comment 15 Brett Wilson (Google) 2009-09-02 09:11:27 PDT
Looks good to me, sorry if you were waiting for me.
Comment 16 Eric Seidel (no email) 2009-09-02 13:05:52 PDT
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
Comment 17 Eric Seidel (no email) 2009-09-02 13:21:19 PDT
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.
Comment 18 Eric Seidel (no email) 2009-09-02 13:38:58 PDT
Comment on attachment 35113 [details]
premultiplied color support for Skia

Clearing flags on attachment: 35113

Committed r47991: <http://trac.webkit.org/changeset/47991>
Comment 19 Eric Seidel (no email) 2009-09-02 13:39:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Dimitri Glazkov (Google) 2009-09-02 14:26:38 PDT
Broke chromium build. Needs SkColorPriv.h include. Fix coming up.
Comment 21 Dimitri Glazkov (Google) 2009-09-02 14:32:05 PDT
Build fix committed as http://trac.webkit.org/changeset/47995.
Comment 22 Dimitri Glazkov (Google) 2009-09-02 15:04:49 PDT
Argh. There's no such value -- "Unpremultiplied". Should we just roll out? Ping.
Comment 23 Dimitri Glazkov (Google) 2009-09-02 15:23:07 PDT
Another fix committed http://trac.webkit.org/changeset/47996.