Closed Bug 671892 Opened 13 years ago Closed 13 years ago

Simplify a common filter number conversion pattern

Tracking

()

Status:

RESOLVED FIXED

Milestone:

mozilla8

People

(Reporter: longsonr, Assigned: longsonr)

References

Details

Attachments

(1 file)

patch 13 years ago Robert Longson [:longsonr] 12.35 KB, patch	dholbert : review+	Details \| Diff \| Splinter Review

Robert Longson [:longsonr]

Assignee

Description

•

13 years ago

Attached patch patch — Details — Splinter Review

These are the non-functional code simplification parts of bug 619992 unbitrotted.

Robert Longson [:longsonr]

Assignee

Comment 1

•

13 years ago

Comment on attachment 546179 [details] [diff] [review]
patch

>diff --git a/content/svg/content/src/nsSVGFilters.cpp b/content/svg/content/src/nsSVGFilters.cpp
>--- a/content/svg/content/src/nsSVGFilters.cpp
>+++ b/content/svg/content/src/nsSVGFilters.cpp
>@@ -144,26 +144,24 @@ nsSVGFE::SetupScalingFilter(nsSVGFilterI
>   result.mRescaling = aKernelUnitLength->IsExplicitlySet();
>   if (!result.mRescaling) {
>     result.mSource = aSource->mImage;
>     result.mTarget = aTarget->mImage;
>     result.mDataRect = aDataRect;
>     return result;
>   }
> 
>-  float kernelX, kernelY;
>-  nsSVGLength2 val;
>-  val.Init(nsSVGUtils::X, 0xff,
>-           aKernelUnitLength->GetAnimValue(nsSVGNumberPair::eFirst),
>-           nsIDOMSVGLength::SVG_LENGTHTYPE_NUMBER);
>-  kernelX = aInstance->GetPrimitiveLength(&val);
>-  val.Init(nsSVGUtils::Y, 0xff,
>-           aKernelUnitLength->GetAnimValue(nsSVGNumberPair::eSecond),
>-           nsIDOMSVGLength::SVG_LENGTHTYPE_NUMBER);
>-  kernelY = aInstance->GetPrimitiveLength(&val);
>+  float kernelX =
>+    aInstance->GetPrimitiveNumber(
>+      nsSVGUtils::X,
>+      aKernelUnitLength, nsSVGNumberPair::eFirst);
>+  float kernelY =
>+    aInstance->GetPrimitiveNumber(
>+      nsSVGUtils::Y,
>+      aKernelUnitLength, nsSVGNumberPair::eSecond);
>   if (kernelX <= 0 || kernelY <= 0)
>     return result;
> 
>   PRBool overflow = PR_FALSE;
>   gfxIntSize scaledSize =
>     nsSVGUtils::ConvertToSurfaceSize(gfxSize(aTarget->mImage->Width() / kernelX,
>                                              aTarget->mImage->Height() / kernelY),
>                                      &overflow);
>@@ -646,25 +644,22 @@ GetBlurBoxSize(double aStdDev)
>     return max;
>   return PRUint32(floor(size + 0.5));
> }
> 
> nsresult
> nsSVGFEGaussianBlurElement::GetDXY(PRUint32 *aDX, PRUint32 *aDY,
>                                    const nsSVGFilterInstance& aInstance)
> {
>-  float stdX = mNumberPairAttributes[STD_DEV].GetAnimValue(nsSVGNumberPair::eFirst);
>-  float stdY = mNumberPairAttributes[STD_DEV].GetAnimValue(nsSVGNumberPair::eSecond);
>-
>-  nsSVGLength2 val;
>-  val.Init(nsSVGUtils::X, 0xff, stdX, nsIDOMSVGLength::SVG_LENGTHTYPE_NUMBER);
>-  stdX = aInstance.GetPrimitiveLength(&val);
>-
>-  val.Init(nsSVGUtils::Y, 0xff, stdY, nsIDOMSVGLength::SVG_LENGTHTYPE_NUMBER);
>-  stdY = aInstance.GetPrimitiveLength(&val);
>+  float stdX = aInstance.GetPrimitiveNumber(
>+    nsSVGUtils::X,
>+    &mNumberPairAttributes[STD_DEV], nsSVGNumberPair::eFirst);
>+  float stdY = aInstance.GetPrimitiveNumber(
>+    nsSVGUtils::Y,
>+    &mNumberPairAttributes[STD_DEV], nsSVGNumberPair::eSecond);
>   
>   if (stdX < 0 || stdY < 0)
>     return NS_ERROR_FAILURE;
> 
>   // If the box size is greater than twice the temporary surface size
>   // in an axis, then each pixel will be set to the average of all the
>   // other pixel values.
>   *aDX = GetBlurBoxSize(stdX);
>@@ -2561,28 +2556,20 @@ NS_IMETHODIMP nsSVGFEOffsetElement::GetD
> NS_IMETHODIMP nsSVGFEOffsetElement::GetDy(nsIDOMSVGAnimatedNumber * *aDy)
> {
>   return mNumberAttributes[DY].ToDOMAnimatedNumber(aDy, this);
> }
> 
> nsIntPoint
> nsSVGFEOffsetElement::GetOffset(const nsSVGFilterInstance& aInstance)
> {
>-  nsIntPoint offset;
>-  float fltX, fltY;
>-  nsSVGLength2 val;
>-
>-  GetAnimatedNumberValues(&fltX, &fltY, nsnull);
>-  val.Init(nsSVGUtils::X, 0xff, fltX, nsIDOMSVGLength::SVG_LENGTHTYPE_NUMBER);
>-  offset.x = PRInt32(aInstance.GetPrimitiveLength(&val));
>-
>-  val.Init(nsSVGUtils::Y, 0xff, fltY, nsIDOMSVGLength::SVG_LENGTHTYPE_NUMBER);
>-  offset.y = PRInt32(aInstance.GetPrimitiveLength(&val));
>-
>-  return offset;
>+  return nsIntPoint(PRInt32(aInstance.GetPrimitiveNumber(
>+                              nsSVGUtils::X, &mNumberAttributes[DX])),
>+                    PRInt32(aInstance.GetPrimitiveNumber(
>+                              nsSVGUtils::Y, &mNumberAttributes[DY])));
> }
> 
> nsresult
> nsSVGFEOffsetElement::Filter(nsSVGFilterInstance *instance,
>                              const nsTArray<const Image*>& aSources,
>                              const Image* aTarget,
>                              const nsIntRect& rect)
> {
>@@ -3693,29 +3680,30 @@ nsSVGFEMorphologyElement::ComputeChangeB
> {
>   return InflateRect(aSourceChangeBoxes[0], aInstance);
> }
> 
> #define MORPHOLOGY_EPSILON 0.0001
> 
> void
> nsSVGFEMorphologyElement::GetRXY(PRInt32 *aRX, PRInt32 *aRY,
>-        const nsSVGFilterInstance& aInstance)
>-{
>-  float rx = mNumberPairAttributes[RADIUS].GetAnimValue(nsSVGNumberPair::eFirst);
>-  float ry = mNumberPairAttributes[RADIUS].GetAnimValue(nsSVGNumberPair::eSecond);
>-  nsSVGLength2 val;
>-  val.Init(nsSVGUtils::X, 0xff, rx, nsIDOMSVGLength::SVG_LENGTHTYPE_NUMBER);
>+                                 const nsSVGFilterInstance& aInstance)
>+{
>   // Subtract an epsilon here because we don't want a value that's just
>   // slightly larger than an integer to round up to the next integer; it's
>   // probably meant to be the integer it's close to, modulo machine precision
>   // issues.
>-  *aRX = NSToIntCeil(aInstance.GetPrimitiveLength(&val) - MORPHOLOGY_EPSILON);
>-  val.Init(nsSVGUtils::Y, 0xff, ry, nsIDOMSVGLength::SVG_LENGTHTYPE_NUMBER);
>-  *aRY = NSToIntCeil(aInstance.GetPrimitiveLength(&val) - MORPHOLOGY_EPSILON);
>+  *aRX = NSToIntCeil(aInstance.GetPrimitiveNumber(
>+                       nsSVGUtils::X,
>+                       &mNumberPairAttributes[RADIUS], nsSVGNumberPair::eFirst) -
>+                         MORPHOLOGY_EPSILON);
>+  *aRY = NSToIntCeil(aInstance.GetPrimitiveNumber(
>+                       nsSVGUtils::Y,
>+                       &mNumberPairAttributes[RADIUS], nsSVGNumberPair::eSecond) -
>+                         MORPHOLOGY_EPSILON);
> }
> 
> nsresult
> nsSVGFEMorphologyElement::Filter(nsSVGFilterInstance *instance,
>                                  const nsTArray<const Image*>& aSources,
>                                  const Image* aTarget,
>                                  const nsIntRect& rect)
> {
>@@ -5852,34 +5840,31 @@ NS_IMETHODIMP nsSVGFEDisplacementMapElem
> }
> 
> nsresult
> nsSVGFEDisplacementMapElement::Filter(nsSVGFilterInstance *instance,
>                                       const nsTArray<const Image*>& aSources,
>                                       const Image* aTarget,
>                                       const nsIntRect& rect)
> {
>-  float scale = mNumberAttributes[SCALE].GetAnimValue();
>+  float scale = instance->GetPrimitiveNumber(
>+    nsSVGUtils::XY, &mNumberAttributes[SCALE]);
>   if (scale == 0.0f) {
>     CopyRect(aTarget, aSources[0], rect);
>     return NS_OK;
>   }
> 
>   PRInt32 width = instance->GetSurfaceWidth();
>   PRInt32 height = instance->GetSurfaceHeight();
> 
>   PRUint8* sourceData = aSources[0]->mImage->Data();
>   PRUint8* displacementData = aSources[1]->mImage->Data();
>   PRUint8* targetData = aTarget->mImage->Data();
>   PRUint32 stride = aTarget->mImage->Stride();
> 
>-  nsSVGLength2 val;
>-  val.Init(nsSVGUtils::XY, 0xff, scale, nsIDOMSVGLength::SVG_LENGTHTYPE_NUMBER);
>-  scale = instance->GetPrimitiveLength(&val);
>-
>   static const PRUint16 channelMap[5] = {
>                              0,
>                              GFX_ARGB32_OFFSET_R,
>                              GFX_ARGB32_OFFSET_G,
>                              GFX_ARGB32_OFFSET_B,
>                              GFX_ARGB32_OFFSET_A };
>   PRUint16 xChannel = channelMap[mEnumAttributes[CHANNEL_X].GetAnimValue()];
>   PRUint16 yChannel = channelMap[mEnumAttributes[CHANNEL_Y].GetAnimValue()];
>diff --git a/layout/svg/base/src/nsSVGFilterInstance.cpp b/layout/svg/base/src/nsSVGFilterInstance.cpp
>--- a/layout/svg/base/src/nsSVGFilterInstance.cpp
>+++ b/layout/svg/base/src/nsSVGFilterInstance.cpp
>@@ -44,26 +44,30 @@
> #include "gfxUtils.h"
> 
> static double Square(double aX)
> {
>   return aX*aX;
> }
> 
> float
>-nsSVGFilterInstance::GetPrimitiveLength(nsSVGLength2 *aLength) const
>+nsSVGFilterInstance::GetPrimitiveNumber(PRUint8 aCtxType, float aValue) const
> {
>+  nsSVGLength2 val;
>+  val.Init(aCtxType, 0xff, aValue,
>+           nsIDOMSVGLength::SVG_LENGTHTYPE_NUMBER);
>+
>   float value;
>   if (mPrimitiveUnits == nsIDOMSVGUnitTypes::SVG_UNIT_TYPE_OBJECTBOUNDINGBOX) {
>-    value = nsSVGUtils::ObjectSpace(mTargetBBox, aLength);
>+    value = nsSVGUtils::ObjectSpace(mTargetBBox, &val);
>   } else {
>-    value = nsSVGUtils::UserSpace(mTargetFrame, aLength);
>+    value = nsSVGUtils::UserSpace(mTargetFrame, &val);
>   }
> 
>-  switch (aLength->GetCtxType()) {
>+  switch (aCtxType) {
>   case nsSVGUtils::X:
>     return value * mFilterSpaceSize.width / mFilterRect.Width();
>   case nsSVGUtils::Y:
>     return value * mFilterSpaceSize.height / mFilterRect.Height();
>   case nsSVGUtils::XY:
>   default:
>     return value *
>       sqrt(Square(mFilterSpaceSize.width) + Square(mFilterSpaceSize.height)) /
>@@ -121,17 +125,17 @@ nsSVGFilterInstance::GetUserSpaceToFilte
> void
> nsSVGFilterInstance::ComputeFilterPrimitiveSubregion(PrimitiveInfo* aPrimitive)
> {
>   nsSVGFE* fE = aPrimitive->mFE;
> 
>   gfxRect defaultFilterSubregion(0,0,0,0);
>   if (fE->SubregionIsUnionOfRegions()) {
>     for (PRUint32 i = 0; i < aPrimitive->mInputs.Length(); ++i) {
>-      defaultFilterSubregion = 
>+      defaultFilterSubregion =
>           defaultFilterSubregion.Union(
>               aPrimitive->mInputs[i]->mImage.mFilterPrimitiveSubregion);
>     }
>   } else {
>     defaultFilterSubregion =
>       gfxRect(0, 0, mFilterSpaceSize.width, mFilterSpaceSize.height);
>   }
> 
>diff --git a/layout/svg/base/src/nsSVGFilterInstance.h b/layout/svg/base/src/nsSVGFilterInstance.h
>--- a/layout/svg/base/src/nsSVGFilterInstance.h
>+++ b/layout/svg/base/src/nsSVGFilterInstance.h
>@@ -38,38 +38,36 @@
> #define __NS_SVGFILTERINSTANCE_H__
> 
> #include "nsIDOMSVGLength.h"
> #include "nsIDOMSVGFilters.h"
> #include "nsRect.h"
> #include "nsIContent.h"
> #include "nsAutoPtr.h"
> #include "nsSVGFilters.h"
>-#include "nsISVGChildFrame.h"
>-#include "nsSVGString.h"
>+#include "nsSVGNumber2.h"
>+#include "nsSVGNumberPair.h"
> 
> #include "gfxImageSurface.h"
> 
>-class nsSVGLength2;
> class nsSVGElement;
> class nsSVGFilterElement;
> class nsSVGFilterPaintCallback;
> struct gfxRect;
> 
> /**
>  * This class performs all filter processing.
>  * 
>  * We build a graph of the filter image data flow, essentially
>  * converting the filter graph to SSA. This lets us easily propagate
>  * analysis data (such as bounding-boxes) over the filter primitive graph.
>  */
> class NS_STACK_CLASS nsSVGFilterInstance
> {
> public:
>-  float GetPrimitiveLength(nsSVGLength2 *aLength) const;
>   void ConvertLocation(float aValues[3]) const;
> 
>   nsSVGFilterInstance(nsIFrame *aTargetFrame,
>                       nsSVGFilterPaintCallback *aPaintCallback,
>                       nsSVGFilterElement *aFilterElement,
>                       const gfxRect &aTargetBBox,
>                       const gfxRect& aFilterRect,
>                       const nsIntSize& aFilterSpaceSize,
>@@ -103,16 +101,25 @@ public:
>   PRInt32 GetSurfaceWidth() const { return mSurfaceRect.width; }
>   PRInt32 GetSurfaceHeight() const { return mSurfaceRect.height; }
>   
>   nsresult Render(gfxASurface** aOutput);
>   nsresult ComputeOutputDirtyRect(nsIntRect* aDirty);
>   nsresult ComputeSourceNeededRect(nsIntRect* aDirty);
>   nsresult ComputeOutputBBox(nsIntRect* aBBox);
> 
>+  float GetPrimitiveNumber(PRUint8 aCtxType, const nsSVGNumber2 *aNumber) const
>+  {
>+    return GetPrimitiveNumber(aCtxType, aNumber->GetAnimValue());
>+  }
>+  float GetPrimitiveNumber(PRUint8 aCtxType, const nsSVGNumberPair *aNumberPair,
>+                           nsSVGNumberPair::PairIndex aIndex) const
>+  {
>+    return GetPrimitiveNumber(aCtxType, aNumberPair->GetAnimValue(aIndex));
>+  }
>   gfxMatrix GetUserSpaceToFilterSpaceTransform() const;
>   gfxMatrix GetFilterSpaceToDeviceSpaceTransform() const {
>     return mFilterSpaceToDeviceSpaceTransform;
>   }
> 
> private:
>   typedef nsSVGFE::Image Image;
>   typedef nsSVGFE::ColorModel ColorModel;
>@@ -169,16 +176,22 @@ private:
>   // when using cairo to draw into the surface). The surface is cleared
>   // to transparent black.
>   already_AddRefed<gfxImageSurface> CreateImage();
> 
>   void ComputeFilterPrimitiveSubregion(PrimitiveInfo* aInfo);
>   void EnsureColorModel(PrimitiveInfo* aPrimitive,
>                         ColorModel aColorModel);
> 
>+  /**
>+   * Scales a numeric filter primitive length in the X, Y or "XY" directions
>+   * into a length in filter space (no offset is applied).
>+   */
>+  float GetPrimitiveNumber(PRUint8 aCtxType, float aValue) const;
>+
>   gfxRect UserSpaceToFilterSpace(const gfxRect& aUserSpace) const;
>   void ClipToFilterSpace(nsIntRect* aRect) const
>   {
>     nsIntRect filterSpace(nsIntPoint(0, 0), mFilterSpaceSize);
>     aRect->IntersectRect(*aRect, filterSpace);
>   }
>   void ClipToGfxRect(nsIntRect* aRect, const gfxRect& aGfx) const;
>

Attachment #546179 - Attachment is patch: true

Attachment #546179 - Flags: review?(dholbert)

Robert Longson [:longsonr]

Assignee

Updated

•

13 years ago

Blocks: 619992

Daniel Holbert [:dholbert]

Comment 2

•

13 years ago

Comment on attachment 546179 [details] [diff] [review]
patch

Looks good! Just a few formatting nits:

>@@ -144,26 +144,24 @@ nsSVGFE::SetupScalingFilter(nsSVGFilterI
>+  float kernelX =
>+    aInstance->GetPrimitiveNumber(
>+      nsSVGUtils::X,
>+      aKernelUnitLength, nsSVGNumberPair::eFirst);
>+  float kernelY =
>+    aInstance->GetPrimitiveNumber(
>+      nsSVGUtils::Y,
>+      aKernelUnitLength, nsSVGNumberPair::eSecond);

Newline / indentation seems a bit odd here. It'd make more sense if we were in danger of breaking 80 chars, but we're not really. :)

This is easier to read (& fits better with Mozilla coding style):
    float kernelX = aInstance->GetPrimitiveNumber(nsSVGUtils::X,
                                                  aKernelUnitLength,
                                                  nsSVGNumberPair::eFirst);
    float kernelY = aInstance->GetPrimitiveNumber(nsSVGUtils::Y,
                                                  aKernelUnitLength,
                                                  nsSVGNumberPair::eSecond);

> nsresult
> nsSVGFEGaussianBlurElement::GetDXY(PRUint32 *aDX, PRUint32 *aDY,
>                                    const nsSVGFilterInstance& aInstance)
> {
>+  float stdX = aInstance.GetPrimitiveNumber(
>+    nsSVGUtils::X,
>+    &mNumberPairAttributes[STD_DEV], nsSVGNumberPair::eFirst);
>+  float stdY = aInstance.GetPrimitiveNumber(
>+    nsSVGUtils::Y,
>+    &mNumberPairAttributes[STD_DEV], nsSVGNumberPair::eSecond);

Same here -- we can format these lines similar to my suggested text above, without breaking 80 chars.

> nsSVGFEMorphologyElement::GetRXY(PRInt32 *aRX, PRInt32 *aRY,
[...]
>+  *aRX = NSToIntCeil(aInstance.GetPrimitiveNumber(
>+                       nsSVGUtils::X,
>+                       &mNumberPairAttributes[RADIUS], nsSVGNumberPair::eFirst) -
>+                         MORPHOLOGY_EPSILON);
>+  *aRY = NSToIntCeil(aInstance.GetPrimitiveNumber(
>+                       nsSVGUtils::Y,
>+                       &mNumberPairAttributes[RADIUS], nsSVGNumberPair::eSecond) -
>+                         MORPHOLOGY_EPSILON);

I think MORPHOLOGY EPSILON is indented 4 spaces too far here. (The current spacing implies that it's nested deeper than the 2 lines above it, when really it's nested less-deeply.)

> nsSVGFEDisplacementMapElement::Filter(nsSVGFilterInstance *instance,
>                                       const nsTArray<const Image*>& aSources,
>                                       const Image* aTarget,
>                                       const nsIntRect& rect)
> {
>-  float scale = mNumberAttributes[SCALE].GetAnimValue();
>+  float scale = instance->GetPrimitiveNumber(
>+    nsSVGUtils::XY, &mNumberAttributes[SCALE]);

As above, this would be better as:
    float scale = instance->GetPrimitiveNumber(nsSVGUtils::XY,
                                               &mNumberAttributes[SCALE]);

Attachment #546179 - Flags: review?(dholbert) → review+

Robert Longson [:longsonr]

Assignee

Updated

•

13 years ago

Whiteboard: [inbound]

Marco Bonardo [:mak] (Away Apr 25 - May 5)

Comment 3

•

13 years ago

http://hg.mozilla.org/mozilla-central/rev/d55a80aae5a8

Status: NEW → RESOLVED

Closed: 13 years ago

Resolution: --- → FIXED

Target Milestone: --- → mozilla8

Robert Longson [:longsonr]

Assignee

Updated

•

13 years ago

Assignee: nobody → longsonr

Whiteboard: [inbound]

You need to log in before you can comment on or make changes to this bug.

Bugzilla

Quick Search

Simplify a common filter number conversion pattern

Categories

(Core :: SVG, defect)

Tracking

()

People

(Reporter: longsonr, Assigned: longsonr)

References

Details

Crash Data

Security

(public)

User Story

Attachments

(1 file)

Description

Comment 1

Updated

Comment 2

Updated

Comment 3

Updated

Attachment

General

Description

File Name

Content Type