Bug 39004 – Crash on SVG feLigthing effects

• Component: SVG
• Status: RESOLVED FIXED

[Dirk Schulze]

Went through the new test suite of SVG 1.1se and some test with feDiffuseLighting or feSpecularLighting crash. Try to post a crash-log later.

[Zoltan Herczeg]

Didn't know about this suite. On which platform is it happens? Debug or Release? Simple segmentation fault or invalid floating opeartion, or something else?

[Dirk Schulze]

(In reply to comment #1)
> Didn't know about this suite. On which platform is it happens? Debug or Release? Simple segmentation fault or invalid floating opeartion, or something else?

ASSERTION FAILED: destx >= 0
(../../WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:223 void WebCore::putImageData(WebCore::ImageData*&, const WebCore::IntRect&, const WebCore::IntPoint&, WebCore::ImageBufferData&, const WebCore::IntSize&) [with WebCore::Multiply multiplied = (WebCore::Multiply)1u])

Program received signal SIGSEGV, Segmentation fault.
0x00b0e35b in WebCore::putImageData<(WebCore::Multiply)1> (source=@0xbfffbd14, 
    sourceRect=..., destPoint=..., data=..., size=...)
    at ../../WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:223
223	    ASSERT(destx >= 0);
(gdb) bt
#0  0x00b0e35b in WebCore::putImageData<(WebCore::Multiply)1> (
    source=@0xbfffbd14, sourceRect=..., destPoint=..., data=..., size=...)
    at ../../WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:223
#1  0x00b0d6be in WebCore::ImageBuffer::putUnmultipliedImageData (
    this=0x843f4e0, source=0x843e558, sourceRect=..., destPoint=...)
    at ../../WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:271
#2  0x00ae1912 in WebCore::FELighting::apply (this=0x84421f0, filter=0x8442c70)
    at ../../WebCore/svg/graphics/filters/SVGFELighting.cpp:267
#3  0x009e0f35 in WebCore::FEComposite::apply (this=0x83ee0c0, 
    filter=0x8442c70)
    at ../../WebCore/platform/graphics/filters/FEComposite.cpp:122
#4  0x00a00914 in WebCore::RenderSVGResourceFilter::postApplyResource (
    this=0x844321c, object=0x821bc5c, context=@0xbfffc10c, resourceMode=1)
    at ../../WebCore/rendering/RenderSVGResourceFilter.cpp:268
#5  0x00a1fd99 in WebCore::SVGRenderBase::finishRenderSVGContent (
    object=0x821bc5c, paintInfo=..., filter=@0xbfffc168, 
    savedContext=0xbfffeb48)
    at ../../WebCore/rendering/SVGRenderSupport.cpp:171
#6  0x009f5fa6 in WebCore::RenderPath::paint (this=0x821bc5c, paintInfo=...)
    at ../../WebCore/rendering/RenderPath.cpp:178
#7  0x009f7903 in WebCore::RenderSVGContainer::paint (this=0x822591c, 
    paintInfo=...) at ../../WebCore/rendering/RenderSVGContainer.cpp:108
#8  0x009f7903 in WebCore::RenderSVGContainer::paint (this=0x8442b3c, 

Gtk, but I think we have this ASSERT on all platforms, since all platforms just copied the code from Mac.

[Dirk Schulze]

Look at other effects, I allways made sure that dst.location >= IntPoint()

[Zoltan Herczeg]

Created attachment 56367 [details]
patch

Opinion?

[Dirk Schulze]

(In reply to comment #4)
> Created an attachment (id=56367) [details]
> patch
> 
> Opinion?

What about the imageRect calculation of other effects? Isn't it possible to take the calculation of eg. GaussianBlur or ColorMatrix? :

    IntRect effectDrawingRect = calculateDrawingIntRect(m_in->scaledSubRegion());
    RefPtr<ImageData> srcImageData(m_in->resultImage()->getPremultipliedImageData(effectDrawingRect));
    CanvasPixelArray* srcPixelArray(srcImageData->data());

    IntRect imageRect(IntPoint(), resultImage()->size());
    PassRefPtr<ImageData> imageData(resultImage()->getUnmultipliedImageData(imageRect));
    PassRefPtr<CanvasPixelArray> srcPixelArray(imageData->data());

    ... <pixel manipulation> ...


    resultImage()->putPremultipliedImageData(srcImageData.get(), imageRect, IntPoint());

In my opinion all effects should use a similar calculation.

[Zoltan Herczeg]

Created attachment 56478 [details]
fix

This patch only fix the crash in the way Dirk suggested.

[Dirk Schulze]

Comment on attachment 56478 [details]
fix

You should mention, that you avoid negeative locations for the resulting image effect in combination with putUnmultipliedImageData.
This also needs a regression test, to avoid crashes in the feature.

The patch itself looks great. r- for the missing test and missing details on the Changelog.

[Zoltan Herczeg]

As I am thinking more and more about the whole I like less and less my solution.

Let the effects rect be (-2, -2, 6, 6), the input image has size (4, 4), and let it contains only black, opaque pixels. The alpha channel (0 - tranpsarent pixel, 1 - opaque pixel) of the destination will look like something like this:

0 0 0 0 0 0 0 0
0 0 0 0 0 0 0 0
0 0 1 1 1 1 0 0
0 0 1 1 1 1 0 0
0 0 1 1 1 1 0 0
0 0 1 1 1 1 0 0
0 0 0 0 0 0 0 0
0 0 0 0 0 0 0 0

(Only the alpha channel matters for lighting effects)

Normal vectors are calculated from the adjacent pixels (3x3 square, its center is the pixel). For example, the (-1, -1) pixel has the following adjacent pixels:

0 0 0
0 0 0
0 0 1

As for (0,0) pixel:

0 0 0
0 1 1
0 1 1

Hence, the normal vector will not point toward the z axis (with 0, 0, 1 normal vector), it will get a certain slope. The pixels look so bad near the edges.

I think it would be better to clacluate the lighting to the original (4,4) rectangle, and put the image later to the effects rect, and keep the outer pixels transparent.

I was also thinking about the light positions as well. If the light position is (2, 2, z) and the effects rect is (-2, -2, 6, 6), shouldn't it be translated by (2, 2) pixels? (If we calculate the light for the original rectangle, this transformation is not needed of course)

[Dirk Schulze]

(In reply to comment #8)
> As I am thinking more and more about the whole I like less and less my solution.
We have this problem on multiple effects, like GaussianBlur, ConvolveMatrix, Morphology and so on. You know the current position in the (4, 4) input, so if your matrix takes a point outside of this rect, just set the value to zero. I don't see the problem.

> I was also thinking about the light positions as well. If the light position is (2, 2, z) and the effects rect is (-2, -2, 6, 6), shouldn't it be translated by (2, 2) pixels? (If we calculate the light for the original rectangle, this transformation is not needed of course)

I don't understand the problem here either. You calculate the value of a certain point. And the matrix should calculate the value relative to this point. So it shouldn't matter how much pixel you use around the (4, 4) effect. The result shouldn't change.

[Zoltan Herczeg]

> You know the current position in the (4, 4) input, so if your matrix takes a point outside of this rect, just set the value to zero. I don't see the problem.

That is the problem :) It should have a 1.0 instead of 0.0, which badly affects the normal vector. There is no algorithm to guess the non-existing pixels. The input algorithm should only use the incoming pixels, and nothing more. The non-incoming pixels should be set to 0,0,0,0 RGBA value. I think.

> I don't understand the problem here either. You calculate the value of a certain point. And the matrix should calculate the value relative to this point. So it shouldn't matter how much pixel you use around the (4, 4) effect. The result shouldn't change.

It changes, unfortunately. The standard has different for algorithm for edge pixels. I.e: (0,0) point is the top-left corner, the normal vector is calculated from (0,0) (0,1) (1,0), (1,1) pixels. However, if (0,0) is an interior pixel, the 8 pixel around it should be used to determine the normal vector.

[Dirk Schulze]

(In reply to comment #10)
I suggest to fix the crash first. You can fix behavior issues later with another bug report. Please apply a regression test to your patch and we can land it.

[Zoltan Herczeg]

Created attachment 56586 [details]
no crash patch

Well, ok. This patch nullifies the crash.

[Zoltan Herczeg]

Oh, forgot to update the layout ChangeLog. A moment.

[Zoltan Herczeg]

Created attachment 56588 [details]
updated changelog

[Zoltan Herczeg]

Hehe, this is not my day :)

[Dirk Schulze]

Comment on attachment 56588 [details]
updated changelog

LGTM. r=me.

[Zoltan Herczeg]

Landed in 59832: http://trac.webkit.org/changeset/59832
Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/59832 might have broken Qt Linux Release

[Zoltan Herczeg]

(In reply to comment #18)
> http://trac.webkit.org/changeset/59832 might have broken Qt Linux Release

fixed http://trac.webkit.org/changeset/59834