Bug 42273 – Convolution computation causes bad alpha channel values

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

[Alex Nicolaou]

First pointed out in http://code.google.com/p/chromium/issues/detail?id=48498 there is a problem with convolution of alpha values for SVG.

The SVG specification is wrong in my opinion. However, what it says in http://www.w3.org/TR/SVG/filters.html#feConvolveMatrixElement is that if preserveAlpha is true, then unpremultiply, convolve, and multiply again (sounds good to me); while if it's false, just apply the convolution to every channel (easy to create invalid results). I think the two should have the same semantics; unpremultiply, convolve either just the color channels or color+alpha, then premultiply again. 

Ignoring my opinion, the bottom line is that the spec implies that each channel should be transformed independently. This can produce an alpha channel that is lower than the premultiplied R, G, or B channels, which is invalid. The result is either assertion failures or crashes in the skia rendering path for chromium.

[Alex Nicolaou]

Created attachment 61543 [details]
Patch

[Adam Langley]

There's a comment in the layout test HTML saying that this test simply shouldn't crash. However, I think that comment should exist in the output, otherwise the expected values are pretty mysterious.

Secondly, if it still crashes as a text-only layout test, that would be a plus. (I could well believe that it doesn't, but it would be nice to check.)

The fix for the crash looks good.

I'm afraid that I don't know enough to really comment on the change to the convolution. I don't think values should be un-premultiplied before convolving (which I don't think this patch does): It seems right that adding $x times an almost transparent pixel doesn't do a lot.

So this code looks sane, but I could imagine other sane solutions.

[Eric Seidel (no email)]

Comment on attachment 61543 [details]
Patch

LayoutTests/ChangeLog:14
 +          * svg/custom/sigfpe.svg: Added.
This doesn't seem like a very useful name for a layout test.

LayoutTests/svg/custom/sigfpe.svg:5
 +  <rect x="10" y="10" width="20" height="20" fill="green"/>
historicaly 100x100 green rect has meant success in graphical tests.  I'm not sure this test even needs pixel results though.  <text> which said PASS would be sufficient.  Then you could mark it as layoutTestController.dumpAsText().

WebCore/platform/graphics/skia/SkiaUtils.cpp:89
 +          SkASSERT(false); // invalid colour channels; r,g,b should be 0 if a=0
Please use full english sentences, beginning with a capital and ending in a period.

WebCore/platform/graphics/skia/SkiaUtils.cpp:90
 +          // protect against sigfpe for production
divide by zero might be a more common name for SIGFPE.

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:251
 +                  paintingData.dstPixelArray->set(pixel++, std::min(clampRGBAValue(totals[0] / m_divisor + paintingData.bias), alpha));
Bah.  This should be a function of some form.  Copy/pasting 4 lines in a row is silly.

[Eric Seidel (no email)]

I don't disagree with the fix.  I just think the code quality of the patch could be much better/more readable.

[Alex Nicolaou]

@ Adam

For the moment, let's ignore my comment on whether the SVG spec is right or not.

I am not clear if your'e not happy with the clamping of the values in the convolve filter (necessary to avoid creating bogus premultiplied rgba values) or if you're OK on the whole patch.

I'll check whether it crashes without the pixel test, I think it will actually. Sorry for not doing that in the first place (Eric points this out too).

[Alex Nicolaou]

@Eric

All your comments are trivial to apply except this one:


| WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:251
|  +                  paintingData.dstPixelArray->set(pixel++, std::min(clampRGBAValue(totals[0] / m_divisor + paintingData.bias), alpha));
| Bah.  This should be a function of some form.  Copy/pasting 4 lines in a row is silly.

This just follows the pattern that was already in the code, including a side effect on pixel and the only change between lines being totals[0] -> 1, 2. I could make it prettier but it's not germane to the fix. Would you prefer:

paintingData.dstPixelArray->setPixel(pixel++, computeChannel(totals[0], alpha));
paintingData.dstPixelArray->setPixel(pixel++, computeChannel(totals[1], alpha));
paintingData.dstPixelArray->setPixel(pixel++, computeChannel(totals[2], alpha));

If that looks better it's trivial enough for me to put into this patch. If you'd like a bigger rearrange of the code can we put it on a follow up bug? Thanks for your feedback!

[Eric Seidel (no email)]

(In reply to comment #6)
> | WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:251
> |  +                  paintingData.dstPixelArray->set(pixel++, std::min(clampRGBAValue(totals[0] / m_divisor + paintingData.bias), alpha));
> | Bah.  This should be a function of some form.  Copy/pasting 4 lines in a row is silly.
> 
> This just follows the pattern that was already in the code, including a side effect on pixel and the only change between lines being totals[0] -> 1, 2. I could make it prettier but it's not germane to the fix.

Agreed.  The problem is you're adding more bad copy/paste-ness.

Would you prefer:
> 
> paintingData.dstPixelArray->setPixel(pixel++, computeChannel(totals[0], alpha));
> paintingData.dstPixelArray->setPixel(pixel++, computeChannel(totals[1], alpha));
> paintingData.dstPixelArray->setPixel(pixel++, computeChannel(totals[2], alpha));
> 
> If that looks better it's trivial enough for me to put into this patch. If you'd like a bigger rearrange of the code can we put it on a follow up bug? Thanks for your feedback!

compute channel, or simply a inlline like:

doTheSetPixelStuff(paintingData, totals, alpha);

I don't know exactly what parts I woudl throw into an inline, but there is clearly code to share here.

Part of the goal of using inlines for things like this is to elmiinate the possibliy of copy/paste errors.

[Adam Langley]

(In reply to comment #5)
> I am not clear if your'e not happy with the clamping of the values in the convolve filter (necessary to avoid creating bogus premultiplied rgba values) or if you're OK on the whole patch.

I'm not sure what should happen. However, I agree that this patch improves matters, so LGTM once Eric is happy with the form of the patch itself.

[Eric Seidel (no email)]

I'm not trying to get in the way of a nice crash fix. :)  But I do think code readablity is super-important to the long-term health of the project, and so it's importnat to maintain it.  Part of that is making sure we have no copy paste code. :)

[Alex Nicolaou]

OK I'll take another run at this and put up a new patch.

[Dirk Schulze]

Adding the original author of our feConvoloveMatrix implementation.
To the code: normaly we use wtf/MathExtra.h instead of math.h.
You mentioned a problem in the spec, is this related to http://www.w3.org/Graphics/SVG/WG/track/products/24 ?

[Alex Nicolaou]

Created attachment 61613 [details]
Patch

[Alex Nicolaou]

Created attachment 61614 [details]
Patch

[Zoltan Herczeg]

(In reply to comment #11)
> Adding the original author of our feConvoloveMatrix implementation.

Thanks Dirk.

I like this patch. Just one more thing: wouldn't it be better to add an extra argument to clampRGBAValue, since it actually a min/max function?

So changing: min(min(max(value, 0), 255), alpha) to min(max(value, 0), alpha) ? It would also remove the <math.h> dependency, which is not preferred in WebKit.

clampRGBAValue is inline, so it would not cause slowdown on the unchanged case.

[Alex Nicolaou]

OK I think this last patch satisfies all comments so far on the bug. Please take another look.

@Dirk - no, the particular complaint I have isn't yet raised against the specification. When I become 100% sure about it I'll try to figure out how to ask someone to add it as an issue. Put as simply as I can phrase it, premultiplied alpha is an optimization, and whether we store the bytes of an image premultiplied or not should not affect the output of a filter like a convolution. However, as the specification currently stands, the same image premultiplied will produce a different output than the non-premultipled version after applying certain convolution filters...it seems to me that if I apply the same filter, and if the image looked the same to begin with, the output should be the same, whether the user agent works with premultiplied image data or not. I am, separately, working on an example to demonstrate the issue.

@Zoltan - yes, I had the same idea as I tried to clean up the code per Eric's request. Have a look at the latest patch.

[Zoltan Herczeg]

(In reply to comment #13)
> Created an attachment (id=61614) [details]
> Patch

Mid air collision :)

You have already noticed what I suggested. The patch is ok for me.

[Eric Seidel (no email)]

Comment on attachment 61614 [details]
Patch

Way better.  Thank you.

[Nikolas Zimmermann]

Comment on attachment 61614 [details]
Patch

LayoutTests/ChangeLog:5
 +          Bug https://bugs.webkit.org/show_bug.cgi?id=42273
Please rerun, prepare-ChangeLog --bug=42273. It generates the correct template to be used in WebKit ChangeLogs.
It will list the bug title, that's currently missing here, and leads to confusion.

WebCore/ChangeLog:5
 +          Bug https://bugs.webkit.org/show_bug.cgi?id=42273
Ditto.

WebCore/platform/graphics/skia/SkiaUtils.cpp:89
 +          // A 0 alpha when there are non-zero R, G, or B channels is an 
I'd use "A zero alpha value" instead of "A 0 alpha".

WebCore/platform/graphics/skia/SkiaUtils.cpp:90
 +          // invalid premultiplid color (since all channels should have been
Typo, premultiplied.

WebCore/platform/graphics/skia/SkiaUtils.cpp:91
 +          // multiplied by 0 if a=0). Assert(false) to catch in debug mode.
I don't think you need to mention that you're asserting, pretty clear from the line below :-)

WebCore/platform/graphics/skia/SkiaUtils.cpp:93
 +          // In production, return 0 to protect against divide by 0.
rephrase, to protect against division by zero.

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:207
 +  ALWAYS_INLINE void setDestinationPixels(CanvasPixelArray *image, int *pixel, float *totals, float divisor, float bias, CanvasPixelArray *src)
Style issues: the star * goes next to the type. I'd suggest to use int& pixel, to avoid, using (*pixel)++, see the next comments.

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:209
 +      for (int i = 0; i < 3 + !preserveAlphaValues; ++i)
I hope this compiles on Windows, it's sometimes picky about these contructs, to be safe, I'd use: "3 + (preserveAlphaValues ? 0 : 1)". That's also used in other parts of the code.


WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:214
 +          image->set((*pixel)++, clampRGBAValue(totals[0], alpha));
Okay, this should really read: image->set(pixel++, clampRGBAValue(....)), just pass the pixel int by reference, that's faster and avoids the unnecessary dereference operator usage.

I still think the function could be more readable, suggestion:

template<bool preserveAlphaValues>
ALWAYS_INLINE void setDestinationPixels(CanvasPixelArray* image, int& pixel, float* totals, float divisor, float bias, CanvasPixelArray* src)
{
    unsigned char maxAlpha = 255;
    if (!preserveAlphaValues)
        maxAlpha = clampRGBAValue(totals[3]);

    for (int i = 0; i < 3; ++i)
        image->set(pixel++, clampRGBAValue(totals[i], maxAlpha);

    image->set(pixel + 1, preserveAlphaValues ? src->get(pixel) : alpha);
    ++pixel;
}

Note that I've decided to not use the "image->set(pixel++ ? src->get(pixel) : alpha)" approach, as I think it's hard to read, better be explicit than confusing.
What do you think?

Other than that, it's a nice patch! Looking forward to the next one.

[Eric Seidel (no email)]

Wow, I'm an awful reviewer.  Thanks Niko!

[Nikolas Zimmermann]

Comment on attachment 61614 [details]
Patch

Resetting, cq to - as well.

[Zoltan Herczeg]

>     image->set(pixel + 1, preserveAlphaValues ? src->get(pixel) : alpha);
>     ++pixel;

Are you sure it is "pixel + 1"? Isn't it just "pixel"? And "pixel++" even shorter.

[Zoltan Herczeg]

    unsigned char maxAlpha = 255;
    if (!preserveAlphaValues)
        maxAlpha = clampRGBAValue(totals[3]);

Maybe:

    unsigned char maxAlpha = preserveAlphaValues ? 255 : clampRGBAValue(totals[3]);

Not sure this is preferred.

[Nikolas Zimmermann]

(In reply to comment #22)
>     unsigned char maxAlpha = 255;
>     if (!preserveAlphaValues)
>         maxAlpha = clampRGBAValue(totals[3]);
> 
> Maybe:
> 
>     unsigned char maxAlpha = preserveAlphaValues ? 255 : clampRGBAValue(totals[3]);
> 
> Not sure this is preferred.

Definately, good suggestion.

(In reply to comment #21)
> >     image->set(pixel + 1, preserveAlphaValues ? src->get(pixel) : alpha);
> >     ++pixel;
> 
> Are you sure it is "pixel + 1"? Isn't it just "pixel"? And "pixel++" even shorter.

I think both versions are wrong, this should be correct (please correct me if I'm wrong):

if (preserveAlphaValues) {
     image->set(pixel, src->get(pixel));
     ++pixel;
} else
    image->set(pixel++, alpha);

I find "image->set(pixel++, src->get(pixel));" hard to read, that's why I was suggesting the change.

[Alex Nicolaou]

Created attachment 61667 [details]
Patch

[Alex Nicolaou]

OK I've taken another run at this.  Please take another look.

<aside>
My personal preference would be to submit simple patches that fix bugs, like the very first patch that I had for this bug; and only then make a new bug that has a patch for the pretty code whose only purpose is to clean it up. I feel like the final patch has really obscured what was once a simple fix that avoided a crash; while if we commit two patches there'll be the obvious bugfix and then the code cleanup one, and later analysis of further bugs would be easier. Given that we've gone down this road, let's just get this committed (or fix it up further as needed), but also it would be useful to me to set my expectation on separation of form and function for future changes. Thanks!
</aside>

[Brett Wilson (Google)]

This code looks good to me, as long as the compiler will actually unroll your loops.

[Eric Seidel (no email)]

I'm sorry to have distracted you with Nikolas and my code readability preferences.  I'm going to let Niko (or one of the active SVG folks) complete this review, as I clearly missed some things in my last go at it.

[Eric Seidel (no email)]

Comment on attachment 61667 [details]
Patch

Personally I liked v2 better.  But it doesn't matter.  We can go more iterations of cleanup on a second bug as you suggest.

Thank you for attempting to get rid of the copy/paste code which someone left there before.

[WebKit Commit Bot]

Comment on attachment 61667 [details]
Patch

Clearing flags on attachment: 61667

Committed r63485: <http://trac.webkit.org/changeset/63485>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Nikolas Zimmermann]

Hm, this broke svg/W3C-SVG-1.1/filters-conv-01-f.svg	 on Mac/CG.
It's all grey, the color is gone. Can someone investigate, please?

[Nikolas Zimmermann]

Issue 1:
float* totals is passed from fastSetInteriorPixels, and the new helper function setDestinationPixels is called multiple times. That means you're doing the divisior division multiple times, accumulated!

for (int i = 0; i < numTotals; ++i)
    totals[i] = totals[i] / divisor + bias;

Issue 2:
for (int i = 0; i < 3; ++i)
    image->set(pixel++, clampRGBAValue(totals[0], maxAlpha));

You're always clamping totals[0], not totals[i].

Very evil!

[Nikolas Zimmermann]

Reopend bug.

[Nikolas Zimmermann]

Created attachment 61786 [details]
Fixup patch

Unbreak feConvolveMatrix.

[Dirk Schulze]

Comment on attachment 61786 [details]
Fixup patch

r=me

[Nikolas Zimmermann]

Landed in r63535. Please always run tests, when doing last minute modifications :-)

[Alex Nicolaou]

Oh how embarrassing :-(

Unfortunately I'm on my way to a funeral so it'll be hours before I can fix. I'll get on it as soon as I'm at a computer. 

Re totals if memory serves they are reset to 0 each time around the loop so that should be ok. But I could be misremembering.

[Alex Nicolaou]

I'm back from the funeral, sorry for the huge delay.

I think splitting out the divisor+bias computation in the last CL is a readability mistake. The totals values are never reused and the original code was correct. In each case, the setDestinationPixels occurs as the last thing in the loop that accumulates the totals and the totals are cleared at the top of the loop. Should we clean it up or leave it well enough alone?

Once again, I'm sorry I didn't catch the hideous error introduced by the last style rewrite.

[Nikolas Zimmermann]

(In reply to comment #38)
> I'm back from the funeral, sorry for the huge delay.
No worries, there are more important things than WebKit.
 
> I think splitting out the divisor+bias computation in the last CL is a readability mistake. The totals values are never reused and the original code was correct. In each case, the setDestinationPixels occurs as the last thing in the loop that accumulates the totals and the totals are cleared at the top of the loop. Should we clean it up or leave it well enough alone?
Oh yes, I misread the loop. If you feel it should be cleaned up, post a patch.
I think it's not hurting readability, and saves a local variable, but if you have strong feelings don't hesitate to write a patch :-)

> Once again, I'm sorry I didn't catch the hideous error introduced by the last style rewrite.
No problem, just run tests before comitting as general rule, that helps to avoid these things :-)