|Summary:||Crash on SVG feLigthing effects|
|Product:||WebKit||Reporter:||Dirk Schulze <krit>|
|Severity:||Normal||CC:||abarth, eric, webkit.review.bot, zherczeg, zimmermann|
|Version:||528+ (Nightly build)|
|OS:||OS X 10.5|
Description Dirk Schulze 2010-05-12 10:13:33 PDT
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.
Comment 1 Zoltan Herczeg 2010-05-12 10:48:03 PDT
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?
Comment 2 Dirk Schulze 2010-05-12 11:19:36 PDT
(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.
Comment 3 Dirk Schulze 2010-05-12 11:28:57 PDT
Look at other effects, I allways made sure that dst.location >= IntPoint()
Comment 5 Dirk Schulze 2010-05-18 08:08:13 PDT
(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.
Comment 6 Zoltan Herczeg 2010-05-19 02:53:26 PDT
Created attachment 56478 [details] fix This patch only fix the crash in the way Dirk suggested.
Comment 7 Dirk Schulze 2010-05-19 03:18:10 PDT
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.
Comment 8 Zoltan Herczeg 2010-05-19 09:02:40 PDT
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)
Comment 9 Dirk Schulze 2010-05-19 09:31:56 PDT
(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.
Comment 10 Zoltan Herczeg 2010-05-19 09:53:23 PDT
> 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.
Comment 11 Dirk Schulze 2010-05-20 01:14:02 PDT
(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.
Comment 12 Zoltan Herczeg 2010-05-20 05:00:07 PDT
Created attachment 56586 [details] no crash patch Well, ok. This patch nullifies the crash.
Comment 13 Zoltan Herczeg 2010-05-20 05:01:38 PDT
Oh, forgot to update the layout ChangeLog. A moment.
Comment 14 Zoltan Herczeg 2010-05-20 05:13:12 PDT
Created attachment 56588 [details] updated changelog
Comment 15 Zoltan Herczeg 2010-05-20 05:14:44 PDT
Hehe, this is not my day :)
Comment 16 Dirk Schulze 2010-05-20 06:39:25 PDT
Comment on attachment 56588 [details] updated changelog LGTM. r=me.
Comment 17 Zoltan Herczeg 2010-05-20 07:06:01 PDT
Landed in 59832: http://trac.webkit.org/changeset/59832 Closing bug.
Comment 18 WebKit Review Bot 2010-05-20 07:16:58 PDT
http://trac.webkit.org/changeset/59832 might have broken Qt Linux Release