Make debug bounds checking for FilterNodeSoftware thread safe (bug 1425056, r=mstange) draft
authorRyan Hunt <rhunt@eqrion.net>
Mon, 18 Dec 2017 13:06:30 -0600
changeset 713691 0f82c1eeef3f7aa64c07d2125c153c37a4a7a004
parent 713690 9550bb5ccfef0429a1fe8ab93709fe82358216cd
child 713692 c856b693abc3196e3e7e771af6d6447e621d6d7c
push id93735
push userbmo:rhunt@eqrion.net
push dateWed, 20 Dec 2017 22:39:00 +0000
reviewersmstange
bugs1425056
milestone59.0a1
Make debug bounds checking for FilterNodeSoftware thread safe (bug 1425056, r=mstange) This debug only bounds checking is not thread safe to any filter nodes drawing at the same time. I believe it makes sense to just manually calculate the bounds and pass them along in the functions that need them. MozReview-Commit-ID: 9GiYRbWuVF6
gfx/2d/FilterNodeSoftware.cpp
--- a/gfx/2d/FilterNodeSoftware.cpp
+++ b/gfx/2d/FilterNodeSoftware.cpp
@@ -2090,95 +2090,73 @@ void
 FilterNodeConvolveMatrixSoftware::SetAttribute(uint32_t aIndex,
                                                bool aPreserveAlpha)
 {
   MOZ_ASSERT(aIndex == ATT_CONVOLVE_MATRIX_PRESERVE_ALPHA);
   mPreserveAlpha = aPreserveAlpha;
 }
 
 #ifdef DEBUG
-static bool sColorSamplingAccessControlEnabled = false;
-static uint8_t* sColorSamplingAccessControlStart = nullptr;
-static uint8_t* sColorSamplingAccessControlEnd = nullptr;
-
-struct DebugOnlyAutoColorSamplingAccessControl
+static inline void
+DebugOnlyCheckColorSamplingAccess(const uint8_t* aSampleAddress, const uint8_t* aBoundsBegin, const uint8_t* aBoundsEnd)
 {
-  explicit DebugOnlyAutoColorSamplingAccessControl(DataSourceSurface* aSurface)
-  {
-    sColorSamplingAccessControlStart = aSurface->GetData();
-    sColorSamplingAccessControlEnd = sColorSamplingAccessControlStart +
-      aSurface->Stride() * aSurface->GetSize().height;
-    sColorSamplingAccessControlEnabled = true;
-  }
-
-  ~DebugOnlyAutoColorSamplingAccessControl()
-  {
-    sColorSamplingAccessControlEnabled = false;
-  }
-};
-
-static inline void
-DebugOnlyCheckColorSamplingAccess(const uint8_t* aSampleAddress)
-{
-  if (sColorSamplingAccessControlEnabled) {
-    MOZ_ASSERT(aSampleAddress >= sColorSamplingAccessControlStart, "accessing before start");
-    MOZ_ASSERT(aSampleAddress < sColorSamplingAccessControlEnd, "accessing after end");
-  }
+  MOZ_ASSERT(aSampleAddress >= aBoundsBegin, "accessing before start");
+  MOZ_ASSERT(aSampleAddress < aBoundsEnd, "accessing after end");
 }
 #else
-typedef DebugOnly<DataSourceSurface*> DebugOnlyAutoColorSamplingAccessControl;
-#define DebugOnlyCheckColorSamplingAccess(address)
+#define DebugOnlyCheckColorSamplingAccess(address, boundsBegin, boundsEnd)
 #endif
 
 static inline uint8_t
-ColorComponentAtPoint(const uint8_t *aData, int32_t aStride, int32_t x, int32_t y, size_t bpp, ptrdiff_t c)
+ColorComponentAtPoint(const uint8_t *aData, int32_t aStride, const uint8_t *aBoundsBegin, const uint8_t *aBoundsEnd, int32_t x, int32_t y, size_t bpp, ptrdiff_t c)
 {
-  DebugOnlyCheckColorSamplingAccess(&aData[y * aStride + bpp * x + c]);
+  DebugOnlyCheckColorSamplingAccess(&aData[y * aStride + bpp * x + c], aBoundsBegin, aBoundsEnd);
   return aData[y * aStride + bpp * x + c];
 }
 
 static inline int32_t
-ColorAtPoint(const uint8_t *aData, int32_t aStride, int32_t x, int32_t y)
+ColorAtPoint(const uint8_t *aData, int32_t aStride, const uint8_t *aBoundsBegin, const uint8_t *aBoundsEnd, int32_t x, int32_t y)
 {
-  DebugOnlyCheckColorSamplingAccess(aData + y * aStride + 4 * x);
+  DebugOnlyCheckColorSamplingAccess(aData + y * aStride + 4 * x, aBoundsBegin, aBoundsEnd);
   return *(uint32_t*)(aData + y * aStride + 4 * x);
 }
 
 // Accepts fractional x & y and does bilinear interpolation.
 // Only call this if the pixel (floor(x)+1, floor(y)+1) is accessible.
 static inline uint8_t
-ColorComponentAtPoint(const uint8_t *aData, int32_t aStride, Float x, Float y, size_t bpp, ptrdiff_t c)
+ColorComponentAtPoint(const uint8_t *aData, int32_t aStride, const uint8_t *aBoundsBegin, const uint8_t *aBoundsEnd, Float x, Float y, size_t bpp, ptrdiff_t c)
 {
   const uint32_t f = 256;
   const int32_t lx = floor(x);
   const int32_t ly = floor(y);
   const int32_t tux = uint32_t((x - lx) * f);
   const int32_t tlx = f - tux;
   const int32_t tuy = uint32_t((y - ly) * f);
   const int32_t tly = f - tuy;
-  const uint8_t &cll = ColorComponentAtPoint(aData, aStride, lx,     ly,     bpp, c);
-  const uint8_t &cul = ColorComponentAtPoint(aData, aStride, lx + 1, ly,     bpp, c);
-  const uint8_t &clu = ColorComponentAtPoint(aData, aStride, lx,     ly + 1, bpp, c);
-  const uint8_t &cuu = ColorComponentAtPoint(aData, aStride, lx + 1, ly + 1, bpp, c);
+  const uint8_t &cll = ColorComponentAtPoint(aData, aStride, aBoundsBegin, aBoundsEnd, lx,     ly,     bpp, c);
+  const uint8_t &cul = ColorComponentAtPoint(aData, aStride, aBoundsBegin, aBoundsEnd, lx + 1, ly,     bpp, c);
+  const uint8_t &clu = ColorComponentAtPoint(aData, aStride, aBoundsBegin, aBoundsEnd, lx,     ly + 1, bpp, c);
+  const uint8_t &cuu = ColorComponentAtPoint(aData, aStride, aBoundsBegin, aBoundsEnd, lx + 1, ly + 1, bpp, c);
   return ((cll * tlx + cul * tux) * tly +
           (clu * tlx + cuu * tux) * tuy + f * f / 2) / (f * f);
 }
 
 static int32_t
 ClampToNonZero(int32_t a)
 {
   return a * (a >= 0);
 }
 
 template<typename CoordType>
 static void
 ConvolvePixel(const uint8_t *aSourceData,
               uint8_t *aTargetData,
               int32_t aWidth, int32_t aHeight,
               int32_t aSourceStride, int32_t aTargetStride,
+              const uint8_t* aSourceBegin, const uint8_t* aSourceEnd,
               int32_t aX, int32_t aY,
               const int32_t *aKernel,
               int32_t aBias, int32_t shiftL, int32_t shiftR,
               bool aPreserveAlpha,
               int32_t aOrderX, int32_t aOrderY,
               int32_t aTargetX, int32_t aTargetY,
               CoordType aKernelUnitLengthX,
               CoordType aKernelUnitLengthY)
@@ -2192,17 +2170,17 @@ ConvolvePixel(const uint8_t *aSourceData
   int32_t roundingAddition = shiftL == 0 ? 0 : 1 << (shiftL - 1);
 
   for (int32_t y = 0; y < aOrderY; y++) {
     CoordType sampleY = aY + (y - aTargetY) * aKernelUnitLengthY;
     for (int32_t x = 0; x < aOrderX; x++) {
       CoordType sampleX = aX + (x - aTargetX) * aKernelUnitLengthX;
       for (int32_t i = 0; i < channels; i++) {
         sum[i] += aKernel[aOrderX * y + x] *
-          ColorComponentAtPoint(aSourceData, aSourceStride,
+          ColorComponentAtPoint(aSourceData, aSourceStride, aSourceBegin, aSourceEnd,
                                 sampleX, sampleY, 4, offsets[i]);
       }
     }
   }
   for (int32_t i = 0; i < channels; i++) {
     int32_t clamped = umin(ClampToNonZero(sum[i] + aBias), 255 << shiftL >> shiftR);
     aTargetData[aY * aTargetStride + 4 * aX + offsets[i]] =
       (clamped + roundingAddition) << shiftR >> shiftL;
@@ -2300,34 +2278,34 @@ FilterNodeConvolveMatrixSoftware::DoRend
 
   RefPtr<DataSourceSurface> input =
     GetInputDataSourceSurface(IN_CONVOLVE_MATRIX_IN, srcRect, NEED_COLOR_CHANNELS, mEdgeMode, &mSourceRect);
 
   if (!input) {
     return nullptr;
   }
 
-  DebugOnlyAutoColorSamplingAccessControl accessControl(input);
-
   RefPtr<DataSourceSurface> target =
     Factory::CreateDataSourceSurface(aRect.Size(), SurfaceFormat::B8G8R8A8, true);
   if (MOZ2D_WARN_IF(!target)) {
     return nullptr;
   }
 
   IntPoint offset = aRect.TopLeft() - srcRect.TopLeft();
 
   DataSourceSurface::ScopedMap sourceMap(input, DataSourceSurface::READ);
   DataSourceSurface::ScopedMap targetMap(target, DataSourceSurface::WRITE);
   if (MOZ2D_WARN_IF(!sourceMap.IsMapped() || !targetMap.IsMapped())) {
     return nullptr;
   }
 
   uint8_t* sourceData = DataAtOffset(input, sourceMap.GetMappedSurface(), offset);
   int32_t sourceStride = sourceMap.GetStride();
+  uint8_t* sourceBegin = sourceMap.GetData();
+  uint8_t* sourceEnd = sourceBegin + sourceStride * input->GetSize().height;
   uint8_t* targetData = targetMap.GetData();
   int32_t targetStride = targetMap.GetStride();
 
   // Why exactly are we reversing the kernel?
   std::vector<Float> kernel = ReversedVector(mKernelMatrix);
   kernel = ScaledVector(kernel, mDivisor);
   Float maxResultAbs = std::max(MaxVectorSum(kernel) + mBias,
                                 MaxVectorSum(ScaledVector(kernel, -1)) - mBias);
@@ -2344,17 +2322,17 @@ FilterNodeConvolveMatrixSoftware::DoRend
   for (size_t i = 0; i < kernel.size(); i++) {
     intKernel[i] = NS_lround(kernel[i] * factorFromShifts);
   }
   int32_t bias = NS_lround(mBias * 255 * factorFromShifts);
 
   for (int32_t y = 0; y < aRect.Height(); y++) {
     for (int32_t x = 0; x < aRect.Width(); x++) {
       ConvolvePixel(sourceData, targetData,
-                    aRect.Width(), aRect.Height(), sourceStride, targetStride,
+                    aRect.Width(), aRect.Height(), sourceStride, targetStride, sourceBegin, sourceEnd,
                     x, y, intKernel, bias, shiftL, shiftR, mPreserveAlpha,
                     mKernelSize.width, mKernelSize.height, mTarget.x, mTarget.y,
                     aKernelUnitLengthX, aKernelUnitLengthY);
     }
   }
   delete[] intKernel;
 
   return target.forget();
@@ -2463,16 +2441,18 @@ FilterNodeDisplacementMapSoftware::Rende
   DataSourceSurface::ScopedMap mapMap(map, DataSourceSurface::READ);
   DataSourceSurface::ScopedMap targetMap(target, DataSourceSurface::WRITE);
   if (MOZ2D_WARN_IF(!(inputMap.IsMapped() && mapMap.IsMapped() && targetMap.IsMapped()))) {
     return nullptr;
   }
 
   uint8_t* sourceData = DataAtOffset(input, inputMap.GetMappedSurface(), offset);
   int32_t sourceStride = inputMap.GetStride();
+  uint8_t* sourceBegin = inputMap.GetData();
+  uint8_t* sourceEnd = sourceBegin + sourceStride * input->GetSize().height;
   uint8_t* mapData = mapMap.GetData();
   int32_t mapStride = mapMap.GetStride();
   uint8_t* targetData = targetMap.GetData();
   int32_t targetStride = targetMap.GetStride();
 
   static const ptrdiff_t channelMap[4] = {
                              B8G8R8A8_COMPONENT_BYTEOFFSET_R,
                              B8G8R8A8_COMPONENT_BYTEOFFSET_G,
@@ -2488,17 +2468,17 @@ FilterNodeDisplacementMapSoftware::Rende
     for (int32_t x = 0; x < aRect.Width(); x++) {
       uint32_t mapIndex = y * mapStride + 4 * x;
       uint32_t targIndex = y * targetStride + 4 * x;
       int32_t sourceX = x +
         scaleOver255 * mapData[mapIndex + xChannel] + scaleAdjustment;
       int32_t sourceY = y +
         scaleOver255 * mapData[mapIndex + yChannel] + scaleAdjustment;
       *(uint32_t*)(targetData + targIndex) =
-        ColorAtPoint(sourceData, sourceStride, sourceX, sourceY);
+        ColorAtPoint(sourceData, sourceStride, sourceBegin, sourceEnd, sourceX, sourceY);
     }
 
     // Keep valgrind happy.
     PodZero(&targetData[y * targetStride + 4 * aRect.Width()], targetStride - 4 * aRect.Width());
   }
 
   return target.forget();
 }
@@ -3213,41 +3193,41 @@ DistantLightSoftware::GetVectorToLight(c
 uint32_t
 DistantLightSoftware::GetColor(uint32_t aLightColor, const Point3D &aVectorToLight)
 {
   return aLightColor;
 }
 
 template<typename CoordType>
 static Point3D
-GenerateNormal(const uint8_t *data, int32_t stride,
+GenerateNormal(const uint8_t *data, int32_t stride, uint8_t* boundsBegin, uint8_t* boundsEnd,
                int32_t x, int32_t y, float surfaceScale,
                CoordType dx, CoordType dy)
 {
   const uint8_t *index = data + y * stride + x;
 
   CoordType zero = 0;
 
   // See this for source of constants:
   //   http://www.w3.org/TR/SVG11/filters.html#feDiffuseLightingElement
   int16_t normalX =
-    -1 * ColorComponentAtPoint(index, stride, -dx, -dy, 1, 0) +
-     1 * ColorComponentAtPoint(index, stride, dx, -dy, 1, 0) +
-    -2 * ColorComponentAtPoint(index, stride, -dx, zero, 1, 0) +
-     2 * ColorComponentAtPoint(index, stride, dx, zero, 1, 0) +
-    -1 * ColorComponentAtPoint(index, stride, -dx, dy, 1, 0) +
-     1 * ColorComponentAtPoint(index, stride, dx, dy, 1, 0);
+    -1 * ColorComponentAtPoint(index, stride, boundsBegin, boundsEnd, -dx, -dy, 1, 0) +
+     1 * ColorComponentAtPoint(index, stride, boundsBegin, boundsEnd, dx, -dy, 1, 0) +
+    -2 * ColorComponentAtPoint(index, stride, boundsBegin, boundsEnd, -dx, zero, 1, 0) +
+     2 * ColorComponentAtPoint(index, stride, boundsBegin, boundsEnd, dx, zero, 1, 0) +
+    -1 * ColorComponentAtPoint(index, stride, boundsBegin, boundsEnd, -dx, dy, 1, 0) +
+     1 * ColorComponentAtPoint(index, stride, boundsBegin, boundsEnd, dx, dy, 1, 0);
 
   int16_t normalY =
-    -1 * ColorComponentAtPoint(index, stride, -dx, -dy, 1, 0) +
-    -2 * ColorComponentAtPoint(index, stride, zero, -dy, 1, 0) +
-    -1 * ColorComponentAtPoint(index, stride, dx, -dy, 1, 0) +
-     1 * ColorComponentAtPoint(index, stride, -dx, dy, 1, 0) +
-     2 * ColorComponentAtPoint(index, stride, zero, dy, 1, 0) +
-     1 * ColorComponentAtPoint(index, stride, dx, dy, 1, 0);
+    -1 * ColorComponentAtPoint(index, stride, boundsBegin, boundsEnd, -dx, -dy, 1, 0) +
+    -2 * ColorComponentAtPoint(index, stride, boundsBegin, boundsEnd, zero, -dy, 1, 0) +
+    -1 * ColorComponentAtPoint(index, stride, boundsBegin, boundsEnd, dx, -dy, 1, 0) +
+     1 * ColorComponentAtPoint(index, stride, boundsBegin, boundsEnd, -dx, dy, 1, 0) +
+     2 * ColorComponentAtPoint(index, stride, boundsBegin, boundsEnd, zero, dy, 1, 0) +
+     1 * ColorComponentAtPoint(index, stride, boundsBegin, boundsEnd, dx, dy, 1, 0);
 
   Point3D normal;
   normal.x = -surfaceScale * normalX / 4.0f;
   normal.y = -surfaceScale * normalY / 4.0f;
   normal.z = 255;
   return Normalized(normal);
 }
 
@@ -3287,18 +3267,16 @@ FilterNodeLightingSoftware<LightType, Li
   if (!input) {
     return nullptr;
   }
 
   if (input->GetFormat() != SurfaceFormat::A8) {
     input = FilterProcessing::ExtractAlpha(input);
   }
 
-  DebugOnlyAutoColorSamplingAccessControl accessControl(input);
-
   RefPtr<DataSourceSurface> target =
     Factory::CreateDataSourceSurface(size, SurfaceFormat::B8G8R8A8);
   if (MOZ2D_WARN_IF(!target)) {
     return nullptr;
   }
 
   IntPoint offset = aRect.TopLeft() - srcRect.TopLeft();
 
@@ -3306,31 +3284,33 @@ FilterNodeLightingSoftware<LightType, Li
   DataSourceSurface::ScopedMap sourceMap(input, DataSourceSurface::READ);
   DataSourceSurface::ScopedMap targetMap(target, DataSourceSurface::WRITE);
   if (MOZ2D_WARN_IF(!(sourceMap.IsMapped() && targetMap.IsMapped()))) {
     return nullptr;
   }
 
   uint8_t* sourceData = DataAtOffset(input, sourceMap.GetMappedSurface(), offset);
   int32_t sourceStride = sourceMap.GetStride();
+  uint8_t* sourceBegin = sourceMap.GetData();
+  uint8_t* sourceEnd = sourceBegin + sourceStride * input->GetSize().height;
   uint8_t* targetData = targetMap.GetData();
   int32_t targetStride = targetMap.GetStride();
 
   MutexAutoLock lock(mLock);
 
   uint32_t lightColor = ColorToBGRA(mColor);
   mLight.Prepare();
   mLighting.Prepare();
 
   for (int32_t y = 0; y < size.height; y++) {
     for (int32_t x = 0; x < size.width; x++) {
       int32_t sourceIndex = y * sourceStride + x;
       int32_t targetIndex = y * targetStride + 4 * x;
 
-      Point3D normal = GenerateNormal(sourceData, sourceStride,
+      Point3D normal = GenerateNormal(sourceData, sourceStride, sourceBegin, sourceEnd,
                                       x, y, mSurfaceScale,
                                       aKernelUnitLengthX, aKernelUnitLengthY);
 
       IntPoint pointInFilterSpace(aRect.x + x, aRect.y + y);
       Float Z = mSurfaceScale * sourceData[sourceIndex] / 255.0f;
       Point3D pt(pointInFilterSpace.x, pointInFilterSpace.y, Z);
       Point3D rayDir = mLight.GetVectorToLight(pt);
       uint32_t color = mLight.GetColor(lightColor, rayDir);