FEMorphology::platformApplyGeneric() should bail out if the radius is less than or...
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 27 Mar 2015 17:02:46 +0000 (17:02 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 27 Mar 2015 17:02:46 +0000 (17:02 +0000)
https://bugs.webkit.org/show_bug.cgi?id=142885.

Patch by Said Abou-Hallawa <sabouhallawa@apple.com> on 2015-03-27
Reviewed by Dean Jackson.

Source/WebCore:

FEMorphology class implementation code clean up.

Tests: svg/filters/feMorphology-radius-cases.svg

* platform/graphics/filters/FEMorphology.cpp:
(WebCore::shouldSupersedeExtremum): Reuse code instead of repeating it and
use < and > instead of =< and >=.

(WebCore::pixelArrayIndex): Returns the array index of a pixel in an image
buffer, given: position(x, y), image width and the color channel.

(WebCore::columnExtremum): Returns the extremum of a column of pixels.

(WebCore::kernelExtremum): Returns the extremum of a filter kernel.

(WebCore::FEMorphology::platformApplyGeneric): Apply some code clean-up.
The kernel size should be equal to radius of the filter. The extra pixel
was causing the resulted image to be asymmetric in some cases.

(WebCore::FEMorphology::platformApplyDegenerate):
(WebCore::FEMorphology::platformApplySoftware): After applying scaling, we
still need to check the resulted radius is negative (overflow case) or less
than one (zero radius case) and treat these cases differently.

(WebCore::FEMorphology::morphologyOperator): Deleted.
(WebCore::FEMorphology::radiusX): Deleted.
(WebCore::FEMorphology::radiusY): Deleted.
* platform/graphics/filters/FEMorphology.h:
(WebCore::FEMorphology::morphologyOperator):
(WebCore::FEMorphology::radiusX):
(WebCore::FEMorphology::radiusY):
Move a single line functions from the source file to the header file.

LayoutTests:

* svg/filters/feMorphology-radius-cases-expected.svg: Added.
* svg/filters/feMorphology-radius-cases.svg: Added.
Test different cases for radius of the feMorphology filter. There are three
cases for the radius:
    1. radius < 0: This is an error case, the source image should not be rendered.
    2. radius = 0: This case is treated as if the filter never exists.
    3. radius > 0: If the scaled radius is > 0, the filter is applied.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@182067 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/svg/filters/feMorphology-radius-cases-expected.svg [new file with mode: 0644]
LayoutTests/svg/filters/feMorphology-radius-cases.svg [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/filters/FEMorphology.cpp
Source/WebCore/platform/graphics/filters/FEMorphology.h

index 10b1d04..e73893f 100644 (file)
@@ -1,3 +1,18 @@
+2015-03-27  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        FEMorphology::platformApplyGeneric() should bail out if the radius is less than or equal to zero.
+        https://bugs.webkit.org/show_bug.cgi?id=142885.
+
+        Reviewed by Dean Jackson.
+
+        * svg/filters/feMorphology-radius-cases-expected.svg: Added.
+        * svg/filters/feMorphology-radius-cases.svg: Added.
+        Test different cases for radius of the feMorphology filter. There are three 
+        cases for the radius:
+            1. radius < 0: This is an error case, the source image should not be rendered.
+            2. radius = 0: This case is treated as if the filter never exists.
+            3. radius > 0: If the scaled radius is > 0, the filter is applied.
+
 2015-03-26  Antti Koivisto  <antti@apple.com>
 
         Respect cache-control directives in request
diff --git a/LayoutTests/svg/filters/feMorphology-radius-cases-expected.svg b/LayoutTests/svg/filters/feMorphology-radius-cases-expected.svg
new file mode 100644 (file)
index 0000000..a8f2f78
--- /dev/null
@@ -0,0 +1,41 @@
+<svg xmlns="http://www.w3.org/2000/svg" version="1.1" width="1000" height="500">
+  <g stroke="black" stroke-width="10">
+    <rect x= "10" y="10" width="100" height="100" fill="lime"/>
+    <rect x="10" y="130" width="100" height="100" fill="lime"/>
+    <rect x="140" y="10" width="100" height="100" fill="lime"/>
+    <rect x="140" y="130" width="100" height="100" fill="lime"/>
+    <rect x= "270" y="10" width="100" height="100" fill="lime"/>
+    <rect x="270" y="130" width="100" height="100" fill="lime"/>
+  </g>
+
+  <svg x="400" y="10" width="100" height="100" viewBox="0 0 10000 10000">
+    <g stroke="black" stroke-width="1000">
+      <rect width="100%" height="100%" fill="lime"/>
+    </g>
+  </svg>
+  <svg x="400" y="130" width="100" height="100" viewBox="0 0 10000 10000">
+    <g stroke="black" stroke-width="1000">
+      <rect width="100%" height="100%" fill="lime"/>
+    </g>
+  </svg>
+
+  <svg x="535" y="15" width="90" height="90">
+    <g stroke="black" stroke-width="20">
+      <rect width="100%" height="100%" fill="lime"/>
+    </g>
+  </svg> 
+  <svg x="530" y="130" width="100" height="100">
+    <rect width="100%" height="100%" fill="lime"/>
+  </svg>
+
+  <svg x="665" y="15" width="90" height="90">
+    <g stroke="black" stroke-width="20">
+      <rect width="100%" height="100%" fill="lime"/>
+    </g>
+  </svg>
+  <svg x="650" y="120" width="120" height="120">
+    <g stroke="black" stroke-width="10">
+      <rect width="100%" height="100%" fill="lime"/>
+    </g>
+  </svg>
+</svg>
diff --git a/LayoutTests/svg/filters/feMorphology-radius-cases.svg b/LayoutTests/svg/filters/feMorphology-radius-cases.svg
new file mode 100644 (file)
index 0000000..f132128
--- /dev/null
@@ -0,0 +1,71 @@
+<svg xmlns="http://www.w3.org/2000/svg" version="1.1" width="1000" height="500">
+  <defs>
+    <filter id="Erode-1">
+      <feMorphology operator="erode" in="SourceGraphic" radius="-1" />
+    </filter>
+    <filter id="Erode0">
+      <feMorphology operator="erode" in="SourceGraphic" radius="0" />
+    </filter>
+    <filter id="Erode10">
+      <feMorphology operator="erode" in="SourceGraphic" radius="10" />
+    </filter>
+    <filter id="Dilate-1">
+      <feMorphology operator="dilate" in="SourceGraphic" radius="-1" />
+    </filter>
+    <filter id="Dilate0">
+      <feMorphology operator="dilate" in="SourceGraphic" radius="0" />
+    </filter>
+    <filter id="Dilate10">
+      <feMorphology operator="dilate" in="SourceGraphic" radius="10" />
+    </filter>
+  </defs>
+  <g stroke="black" stroke-width="10">
+    <rect x= "10" y="10" width="100" height="100" fill="lime"/>
+    <rect x="10" y="130" width="100" height="100" fill="lime"/>
+
+    <!-- negative radius case -->
+    <g>
+      <rect x="140" y="10" width="100" height="100" fill="lime"/>
+      <rect x="140" y="10" width="100" height="100" fill="red" filter="url(#Erode-1)"/>
+    </g>
+
+    <g>
+      <rect x="140" y="130" width="100" height="100" fill="lime"/>
+      <rect x="140" y="130" width="100" height="100" fill="red" filter="url(#Dilate-1)"/>
+    </g>
+    
+    <!-- zero radius case -->
+    <rect x= "270" y="10" width="100" height="100" fill="lime" filter="url(#Erode0)"/>
+    <rect x="270" y="130" width="100" height="100" fill="lime" filter="url(#Dilate0)"/>
+  </g>
+
+  <!-- positive radius case but scaled down to less than 1 (treated as zero radius case) -->
+  <svg x="400" y="10" width="100" height="100" viewBox="0 0 10000 10000">
+    <g stroke="black" stroke-width="1000">
+      <rect width="100%" height="100%" fill="lime" filter="url(#Erode10)"/>
+    </g>
+  </svg>
+  <svg x="400" y="130" width="100" height="100" viewBox="0 0 10000 10000">
+    <g stroke="black" stroke-width="1000">
+      <rect width="100%" height="100%" fill="lime" filter="url(#Dilate10)"/>
+    </g>
+  </svg>
+
+  <!-- positive radius case but inside a nested svg -->
+  <svg x="530" y="10" width="100" height="100">
+    <g stroke="black" stroke-width="10">
+      <rect width="100%" height="100%" fill="lime" filter="url(#Erode10)"/>
+    </g>
+  </svg>
+  <svg x="530" y="130" width="100" height="100" viewBox="0 0 100 100">
+    <g stroke="black" stroke-width="10">
+      <rect width="100%" height="100%" fill="lime" filter="url(#Dilate10)"/>
+    </g>
+  </svg>
+
+  <!-- positive radius case -->
+  <g stroke="black" stroke-width="10">
+    <rect x="660" y="10" width="100" height="100" fill="lime" filter="url(#Erode10)"/>
+    <rect x="660" y="130" width="100" height="100" fill="lime" filter="url(#Dilate10)"/>
+  </g>
+</svg>
index e1d87f8..ba363b8 100644 (file)
@@ -1,3 +1,43 @@
+2015-03-27  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        FEMorphology::platformApplyGeneric() should bail out if the radius is less than or equal to zero.
+        https://bugs.webkit.org/show_bug.cgi?id=142885.
+
+        Reviewed by Dean Jackson.
+
+        FEMorphology class implementation code clean up.
+        
+        Tests: svg/filters/feMorphology-radius-cases.svg
+
+        * platform/graphics/filters/FEMorphology.cpp:
+        (WebCore::shouldSupersedeExtremum): Reuse code instead of repeating it and
+        use < and > instead of =< and >=.
+        
+        (WebCore::pixelArrayIndex): Returns the array index of a pixel in an image
+        buffer, given: position(x, y), image width and the color channel.
+        
+        (WebCore::columnExtremum): Returns the extremum of a column of pixels.
+        
+        (WebCore::kernelExtremum): Returns the extremum of a filter kernel.
+        
+        (WebCore::FEMorphology::platformApplyGeneric): Apply some code clean-up.
+        The kernel size should be equal to radius of the filter. The extra pixel
+        was causing the resulted image to be asymmetric in some cases.
+        
+        (WebCore::FEMorphology::platformApplyDegenerate):
+        (WebCore::FEMorphology::platformApplySoftware): After applying scaling, we
+        still need to check the resulted radius is negative (overflow case) or less
+        than one (zero radius case) and treat these cases differently.
+        
+        (WebCore::FEMorphology::morphologyOperator): Deleted.
+        (WebCore::FEMorphology::radiusX): Deleted.
+        (WebCore::FEMorphology::radiusY): Deleted.
+        * platform/graphics/filters/FEMorphology.h:
+        (WebCore::FEMorphology::morphologyOperator):
+        (WebCore::FEMorphology::radiusX):
+        (WebCore::FEMorphology::radiusY):
+        Move a single line functions from the source file to the header file.
+
 2015-03-27  Antti Koivisto  <antti@apple.com>
 
         Move CacheValidation to platform
index a9aab0b..a345171 100644 (file)
@@ -46,11 +46,6 @@ Ref<FEMorphology> FEMorphology::create(Filter* filter, MorphologyOperatorType ty
     return adoptRef(*new FEMorphology(filter, type, radiusX, radiusY));
 }
 
-MorphologyOperatorType FEMorphology::morphologyOperator() const
-{
-    return m_type;
-}
-
 bool FEMorphology::setMorphologyOperator(MorphologyOperatorType type)
 {
     if (m_type == type)
@@ -59,11 +54,6 @@ bool FEMorphology::setMorphologyOperator(MorphologyOperatorType type)
     return true;
 }
 
-float FEMorphology::radiusX() const
-{
-    return m_radiusX;
-}
-
 bool FEMorphology::setRadiusX(float radiusX)
 {
     if (m_radiusX == radiusX)
@@ -72,9 +62,12 @@ bool FEMorphology::setRadiusX(float radiusX)
     return true;
 }
 
-float FEMorphology::radiusY() const
+bool FEMorphology::setRadiusY(float radiusY)
 {
-    return m_radiusY;
+    if (m_radiusY == radiusY)
+        return false;
+    m_radiusY = radiusY;
+    return true;
 }
 
 void FEMorphology::determineAbsolutePaintRect()
@@ -90,66 +83,77 @@ void FEMorphology::determineAbsolutePaintRect()
     setAbsolutePaintRect(enclosingIntRect(paintRect));
 }
 
-bool FEMorphology::setRadiusY(float radiusY)
+static inline bool shouldSupersedeExtremum(unsigned char newValue, unsigned char currentValue, MorphologyOperatorType type)
 {
-    if (m_radiusY == radiusY)
-        return false;
-    m_radiusY = radiusY;
-    return true;
+    return (type == FEMORPHOLOGY_OPERATOR_ERODE && newValue < currentValue)
+        || (type == FEMORPHOLOGY_OPERATOR_DILATE && newValue > currentValue);
+}
+
+static inline int pixelArrayIndex(int x, int y, int width, int colorChannel)
+{
+    return (y * width + x) * 4 + colorChannel;
+}
+
+static inline unsigned char columnExtremum(const Uint8ClampedArray* srcPixelArray, int x, int yStart, int yEnd, int width, unsigned colorChannel, MorphologyOperatorType type)
+{
+    unsigned char extremum = srcPixelArray->item(pixelArrayIndex(x, yStart, width, colorChannel));
+    for (int y = yStart + 1; y < yEnd; ++y) {
+        unsigned char pixel = srcPixelArray->item(pixelArrayIndex(x, y, width, colorChannel));
+        if (shouldSupersedeExtremum(pixel, extremum, type))
+            extremum = pixel;
+    }
+    return extremum;
+}
+
+static inline unsigned char kernelExtremum(const Vector<unsigned char>& kernel, MorphologyOperatorType type)
+{
+    Vector<unsigned char>::const_iterator iter = kernel.begin();
+    unsigned char extremum = *iter;
+    for (Vector<unsigned char>::const_iterator end = kernel.end(); ++iter != end; ) {
+        if (shouldSupersedeExtremum(*iter, extremum, type))
+            extremum = *iter;
+    }
+    return extremum;
 }
 
 void FEMorphology::platformApplyGeneric(PaintingData* paintingData, int yStart, int yEnd)
 {
     Uint8ClampedArray* srcPixelArray = paintingData->srcPixelArray;
     Uint8ClampedArray* dstPixelArray = paintingData->dstPixelArray;
-    const int width = paintingData->width;
-    const int height = paintingData->height;
-    const int effectWidth = width * 4;
     const int radiusX = paintingData->radiusX;
     const int radiusY = paintingData->radiusY;
+    const int width = paintingData->width;
+    const int height = paintingData->height;
+
+    ASSERT(radiusX <= width || radiusY <= height);
+    ASSERT(yStart >= 0 && yEnd <= height && yStart < yEnd);
 
     Vector<unsigned char> extrema;
     for (int y = yStart; y < yEnd; ++y) {
-        int extremaStartY = std::max(0, y - radiusY);
-        int extremaEndY = std::min(height - 1, y + radiusY);
-        for (unsigned int clrChannel = 0; clrChannel < 4; ++clrChannel) {
+        int yStartExtrema = std::max(0, y - radiusY);
+        int yEndExtrema = std::min(height - 1, y + radiusY);
+
+        for (unsigned colorChannel = 0; colorChannel < 4; ++colorChannel) {
             extrema.clear();
             // Compute extremas for each columns
-            for (int x = 0; x <= radiusX; ++x) {
-                unsigned char columnExtrema = srcPixelArray->item(extremaStartY * effectWidth + 4 * x + clrChannel);
-                for (int eY = extremaStartY + 1; eY < extremaEndY; ++eY) {
-                    unsigned char pixel = srcPixelArray->item(eY * effectWidth + 4 * x + clrChannel);
-                    if ((m_type == FEMORPHOLOGY_OPERATOR_ERODE && pixel <= columnExtrema)
-                        || (m_type == FEMORPHOLOGY_OPERATOR_DILATE && pixel >= columnExtrema)) {
-                        columnExtrema = pixel;
-                    }
-                }
-
-                extrema.append(columnExtrema);
-            }
+            for (int x = 0; x < radiusX; ++x)
+                extrema.append(columnExtremum(srcPixelArray, x, yStartExtrema, yEndExtrema, width, colorChannel, m_type));
 
             // Kernel is filled, get extrema of next column
             for (int x = 0; x < width; ++x) {
-                const int endX = std::min(x + radiusX, width - 1);
-                unsigned char columnExtrema = srcPixelArray->item(extremaStartY * effectWidth + endX * 4 + clrChannel);
-                for (int i = extremaStartY + 1; i <= extremaEndY; ++i) {
-                    unsigned char pixel = srcPixelArray->item(i * effectWidth + endX * 4 + clrChannel);
-                    if ((m_type == FEMORPHOLOGY_OPERATOR_ERODE && pixel <= columnExtrema)
-                        || (m_type == FEMORPHOLOGY_OPERATOR_DILATE && pixel >= columnExtrema))
-                        columnExtrema = pixel;
+                if (x < width - radiusX) {
+                    int xEnd = std::min(x + radiusX, width - 1);
+                    extrema.append(columnExtremum(srcPixelArray, xEnd, yStartExtrema, yEndExtrema + 1, width, colorChannel, m_type));
                 }
-                if (x - radiusX >= 0)
+
+                if (x > radiusX)
                     extrema.remove(0);
-                if (x + radiusX <= width)
-                    extrema.append(columnExtrema);
-
-                unsigned char entireExtrema = extrema[0];
-                for (unsigned kernelIndex = 1; kernelIndex < extrema.size(); ++kernelIndex) {
-                    if ((m_type == FEMORPHOLOGY_OPERATOR_ERODE && extrema[kernelIndex] <= entireExtrema)
-                        || (m_type == FEMORPHOLOGY_OPERATOR_DILATE && extrema[kernelIndex] >= entireExtrema))
-                        entireExtrema = extrema[kernelIndex];
-                }
-                dstPixelArray->set(y * effectWidth + 4 * x + clrChannel, entireExtrema);
+
+                // The extrema original size = radiusX.
+                // Number of new addition = width - radiusX.
+                // Number of removals = width - radiusX - 1.
+                ASSERT(extrema.size() >= static_cast<size_t>(radiusX + 1));
+                dstPixelArray->set(pixelArrayIndex(x, y, width, colorChannel), kernelExtremum(extrema, m_type));
             }
         }
     }
@@ -189,6 +193,23 @@ void FEMorphology::platformApply(PaintingData* paintingData)
     platformApplyGeneric(paintingData, 0, paintingData->height);
 }
 
+bool FEMorphology::platformApplyDegenerate(Uint8ClampedArray* dstPixelArray, const IntRect& imageRect, int radiusX, int radiusY)
+{
+    // Input radius is less than zero or an overflow happens when scaling it.
+    if (radiusX < 0 || radiusY < 0) {
+        dstPixelArray->zeroFill();
+        return true;
+    }
+
+    // Input radius is equal to zero or the scaled radius is less than one.
+    if (!m_radiusX || !m_radiusY) {
+        FilterEffect* in = inputEffect(0);
+        in->copyPremultipliedImage(dstPixelArray, imageRect);
+        return true;
+    }
+
+    return false;
+}
 
 void FEMorphology::platformApplySoftware()
 {
@@ -199,25 +220,28 @@ void FEMorphology::platformApplySoftware()
         return;
 
     setIsAlphaImage(in->isAlphaImage());
-    if (m_radiusX <= 0 || m_radiusY <= 0) {
-        dstPixelArray->zeroFill();
+
+    IntRect effectDrawingRect = requestedRegionOfInputImageData(in->absolutePaintRect());
+    if (platformApplyDegenerate(dstPixelArray, effectDrawingRect, m_radiusX, m_radiusY))
         return;
-    }
 
     Filter& filter = this->filter();
+    RefPtr<Uint8ClampedArray> srcPixelArray = in->asPremultipliedImage(effectDrawingRect);
     int radiusX = static_cast<int>(floorf(filter.applyHorizontalScale(m_radiusX)));
     int radiusY = static_cast<int>(floorf(filter.applyVerticalScale(m_radiusY)));
+    radiusX = std::min(effectDrawingRect.width() - 1, radiusX);
+    radiusY = std::min(effectDrawingRect.height() - 1, radiusY);
 
-    IntRect effectDrawingRect = requestedRegionOfInputImageData(in->absolutePaintRect());
-    RefPtr<Uint8ClampedArray> srcPixelArray = in->asPremultipliedImage(effectDrawingRect);
+    if (platformApplyDegenerate(dstPixelArray, effectDrawingRect, radiusX, radiusY))
+        return;
 
     PaintingData paintingData;
     paintingData.srcPixelArray = srcPixelArray.get();
     paintingData.dstPixelArray = dstPixelArray;
     paintingData.width = effectDrawingRect.width();
     paintingData.height = effectDrawingRect.height();
-    paintingData.radiusX = std::min(effectDrawingRect.width() - 1, radiusX);
-    paintingData.radiusY = std::min(effectDrawingRect.height() - 1, radiusY);
+    paintingData.radiusX = radiusX;
+    paintingData.radiusY = radiusY;
 
     platformApply(&paintingData);
 }
index 33a729a..cc93691 100644 (file)
@@ -36,13 +36,14 @@ enum MorphologyOperatorType {
 class FEMorphology : public FilterEffect {
 public:
     static Ref<FEMorphology> create(Filter*, MorphologyOperatorType, float radiusX, float radiusY);
-    MorphologyOperatorType morphologyOperator() const;
+
+    MorphologyOperatorType morphologyOperator() const { return m_type; }
     bool setMorphologyOperator(MorphologyOperatorType);
 
-    float radiusX() const;
+    float radiusX() const { return m_radiusX; }
     bool setRadiusX(float);
 
-    float radiusY() const;
+    float radiusY() const { return m_radiusY; }
     bool setRadiusY(float);
 
     virtual void platformApplySoftware();
@@ -76,6 +77,7 @@ public:
     inline void platformApplyGeneric(PaintingData*, const int yStart, const int yEnd);
 private:
     FEMorphology(Filter*, MorphologyOperatorType, float radiusX, float radiusY);
+    bool platformApplyDegenerate(Uint8ClampedArray* dstPixelArray, const IntRect& imageRect, int radiusX, int radiusY);
     
     MorphologyOperatorType m_type;
     float m_radiusX;