924103 - Use Moz2D for SVG filter rendering

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Markus Stange [:mstange]]

This bug will switch our existing SVG filter implementation to use the new Moz2D filter capabilities from bug 924102 instead.

Comment 1

[Markus Stange [:mstange]]

Attachment: part1: Add infrastructure

This part adds FilterPrimitiveDescription and FilterDescription classes and ways to render them by creating appropriate Moz2D FilterNodes.

Comment 2

[Markus Stange [:mstange]]

Attachment: part2: Use FilterSupport for SVG filters

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 814113 [details] [diff] [review]
part1: Add infrastructure

Review of attachment 814113 [details] [diff] [review]:
-----------------------------------------------------------------

This is only a partial review, I haven't finished reading yet.

::: gfx/src/FilterSupport.cpp
@@ +43,5 @@
> +  0.930f, 0.931f, 0.933f, 0.935f, 0.937f, 0.939f, 0.941f, 0.943f,
> +  0.945f, 0.946f, 0.948f, 0.950f, 0.952f, 0.954f, 0.956f, 0.957f,
> +  0.959f, 0.961f, 0.963f, 0.965f, 0.967f, 0.968f, 0.970f, 0.972f,
> +  0.974f, 0.975f, 0.977f, 0.979f, 0.981f, 0.983f, 0.984f, 0.986f,
> +  0.988f, 0.990f, 0.991f, 0.993f, 0.995f, 0.997f, 0.998f, 1.000f

I suggest instead of including this table verbatim we dynamically allocate and lazily initialize the table in LinearRGBToSRGB the first time we need it.

@@ +80,5 @@
> +  0.687f, 0.694f, 0.701f, 0.708f, 0.716f, 0.723f, 0.730f, 0.738f,
> +  0.745f, 0.753f, 0.761f, 0.768f, 0.776f, 0.784f, 0.791f, 0.799f,
> +  0.807f, 0.815f, 0.823f, 0.831f, 0.839f, 0.847f, 0.855f, 0.863f,
> +  0.871f, 0.880f, 0.888f, 0.896f, 0.905f, 0.913f, 0.922f, 0.930f,
> +  0.939f, 0.947f, 0.956f, 0.965f, 0.973f, 0.982f, 0.991f, 1.000f

This too.

@@ +94,5 @@
> +  static TemporaryRef<FilterNode>
> +  LinearRGBToSRGB(DrawTarget* aDT, FilterNode* aInput)
> +  {
> +    RefPtr<FilterNode> transfer = aDT->CreateFilter(FILTER_DISCRETE_TRANSFER);
> +    transfer->SetAttribute(ATT_DISCRETE_TRANSFER_DISABLE_R, false);

Can't these DISABLE values default to false so we don't have to set them here?

@@ +119,5 @@
> +    return transfer;
> +  }
> +
> +  static TemporaryRef<FilterNode>
> +  UnPremultiply(DrawTarget* aDT, FilterNode* aInput)

"Un" is not a word so "P" should be lowercase.

@@ +200,5 @@
> +  TemporaryRef<FilterNode> WrapForColorModel(ColorModel aColorModel);
> +
> +  RefPtr<DrawTarget> mDT;
> +  ColorModel mOriginalColorModel;
> +  RefPtr<FilterNode> mFilterForColorModel[4];

Comment that this array is indexed by ColorModel::ToIndex.

@@ +277,5 @@
> +      0,       0,       0,       0, 0,
> +      0,       0,       0,       0, 0,
> +      0.2125f, 0.7154f, 0.0721f, 0, 0 };
> +
> +  float s, c;

Declare these where you define them below.

@@ +288,5 @@
> +        return NS_ERROR_FAILURE;
> +
> +      for(uint32_t j = 0; j < aValues.Length(); j++) {
> +        aOutMatrix[j] = aValues[j];
> +      }

Use PodCopy with aValues.Elements()

@@ +302,5 @@
> +
> +      if (s > 1 || s < 0)
> +        return NS_ERROR_FAILURE;
> +
> +      memcpy(aOutMatrix, identityMatrix, sizeof(float) * 20);

Use PodCopy (and below).

::: gfx/src/FilterSupport.h
@@ +5,5 @@
> +
> +#ifndef __FilterSupport_h
> +#define __FilterSupport_h
> +
> +#include "nsMathUtils.h"

Why do you need to include this in this file?

@@ +17,5 @@
> +
> +class gfxASurface;
> +
> +// Morphology Operators
> +static const unsigned short SVG_OPERATOR_UNKNOWN = 0;

Put all the new stuff in a namespace. "mozilla::gfx" I guess.

@@ +19,5 @@
> +
> +// Morphology Operators
> +static const unsigned short SVG_OPERATOR_UNKNOWN = 0;
> +static const unsigned short SVG_OPERATOR_ERODE = 1;
> +static const unsigned short SVG_OPERATOR_DILATE = 2;

Should not be static.

@@ +146,5 @@
> +class FilterNode;
> +
> +namespace filterattribute {
> +struct Attribute;
> +}

Instead of using a namespace, maybe we could just call it FilterAttribute.

@@ +204,5 @@
> +  bool operator==(const ColorModel& aOther) const {
> +    return mColorSpace == aOther.mColorSpace &&
> +           mAlphaModel == aOther.mAlphaModel;
> +  }
> +  operator uint8_t() const { return (mColorSpace << 1) + mAlphaModel; }

Explain why this is needed. Also, instead of making this an implicit conversion, make it a method called ToIndex or something like that.

@@ +209,5 @@
> +  ColorSpace mColorSpace;
> +  AlphaModel mAlphaModel;
> +};
> +
> +class FilterPrimitiveDescription MOZ_FINAL {

Add a comment explaining what this class means.

@@ +251,5 @@
> +  size_t                NumberOfInputs() const;
> +  int32_t               InputPrimitiveIndex(size_t aInputIndex) const;
> +
> +  ColorSpace            InputColorSpace(size_t aInputIndex) const;
> +  ColorSpace            OutputColorSpace() const;

I don't think we need to indent the method names.

@@ +267,5 @@
> +  nsTArray<ColorSpace> mInputColorSpaces;
> +  ColorSpace mOutputColorSpace;
> +};
> +
> +struct FilterDescription MOZ_FINAL {

Add a comment explaining what this class is for. Also explain that this will be serializable via IPDL so must remain a simple data structure without complex functionality.

@@ +279,5 @@
> +  nsTArray<FilterPrimitiveDescription> mPrimitives;
> +  IntRect mFilterSpaceBounds;
> +};
> +
> +class FilterSupport {

Add a comment explaining why these methods aren't on FilterDescription.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 814113 [details] [diff] [review]
part1: Add infrastructure

Review of attachment 814113 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/src/FilterSupport.cpp
@@ +365,5 @@
> +
> +// Called for one channel at a time.
> +// This function creates the required FilterNodes on demand and tries to
> +// merge conversions of different channels into the same FilterNode if
> +// possible.

You need to say more about what the parameters mean.

@@ +492,5 @@
> +    float exponent = aFunctionAttributes.GetFloat(eComponentTransferFunctionExponent);
> +    float offset = aFunctionAttributes.GetFloat(eComponentTransferFunctionOffset);
> +    filter->SetAttribute(amplitudeAtt[aChannel], amplitude);
> +    filter->SetAttribute(exponentAtt[aChannel], exponent);
> +    filter->SetAttribute(offsetAtt[aChannel], offset);

Can we save some runtime work here by introducing static_asserts that Moz2D constant values match the SVG constant values?

@@ +663,5 @@
> +      static const uint32_t edgeModes[SVG_EDGEMODE_NONE+1] = {
> +        EDGE_MODE_NONE,      // SVG_EDGEMODE_UNKNOWN
> +        EDGE_MODE_DUPLICATE, // SVG_EDGEMODE_DUPLICATE
> +        EDGE_MODE_WRAP,      // SVG_EDGEMODE_WRAP
> +        EDGE_MODE_NONE       // SVG_EDGEMODE_NONE

Another place where we could probably static_assert that values match and avoid programmatic conversion.

@@ +690,5 @@
> +        COLOR_CHANNEL_R, // SVG_CHANNEL_UNKNOWN
> +        COLOR_CHANNEL_R, // SVG_CHANNEL_R
> +        COLOR_CHANNEL_G, // SVG_CHANNEL_G
> +        COLOR_CHANNEL_B, // SVG_CHANNEL_B
> +        COLOR_CHANNEL_A  // SVG_CHANNEL_A

And another...

@@ +715,5 @@
> +                           (uint32_t)atts.GetFloat(eTurbulenceSeed));
> +      static const uint32_t type[SVG_TURBULENCE_TYPE_TURBULENCE+1] = {
> +        TURBULENCE_TYPE_FRACTAL_NOISE, // SVG_TURBULENCE_TYPE_UNKNOWN
> +        TURBULENCE_TYPE_FRACTAL_NOISE, // SVG_TURBULENCE_TYPE_FRACTALNOISE
> +        TURBULENCE_TYPE_TURBULENCE     // SVG_TURBULENCE_TYPE_TURBULENCE

Etc...

@@ +942,5 @@
> +                T& aSourceGraphicElement,
> +                T& aSourceAlphaElement,
> +                T& aFillPaintElement,
> +                T& aStrokePaintElement)
> +{

Just call the const version and cast away const from the result, instead of duplicating code.

@@ +1006,5 @@
> +                               const IntRect& aSourceAlphaRect,
> +                               SourceSurface* aFillPaint,
> +                               const IntRect& aFillPaintRect,
> +                               SourceSurface* aStrokePaint,
> +                               const IntRect& aStrokePaintRect,

I think we should pass an array of structs, or two arrays, instead of explicit parameters, since we'll probably want to extend this list over time and this function already has too many parameters.

@@ +1014,5 @@
> +  const IntRect& filterSpaceBounds = aFilter.mFilterSpaceBounds;
> +
> +  nsIntRegion dummy;
> +  nsTArray<nsIntRegion> primitiveNeededRegions;
> +  FilterSupport::ComputeSourceNeededRegions(aFilter, nsIntRect(aSurfaceRect.x, aSurfaceRect.y, aSurfaceRect.width, aSurfaceRect.height), dummy, dummy, dummy, dummy, &primitiveNeededRegions);

We must have a better way to convert IntRect to nsIntRect!

@@ +1149,5 @@
> +  nsIntRegionRectIterator it(aRegion);
> +  while (const nsIntRect* sr = it.Next()) {
> +    nsIntRect r = *sr;
> +    r.Deflate(aMargin);
> +    result.Or(result, r);

This method doesn't make much sense to me. For a complex region it can create holes inside the region.

@@ +1297,5 @@
> +      inputChangeRegions.AppendElement(inputChangeRegion);
> +    }
> +    nsIntRegion changeRegion =
> +      ResultChangeRegionForPrimitive(descr, inputChangeRegions);
> +    IntRect ps = RoundedToInt(descr.PrimitiveSubregion());

We should be explicitly rounding out here and RoundedToInt doesn't do that AFAICT. Same for other callers of RoundedToInt below.

@@ +1329,5 @@
> +      int32_t ry = clamped(int32_t(ceil(radii.height)), 0, 100000);
> +      uint32_t op = atts.GetUint(eMorphologyOperator);
> +      if (op == SVG_OPERATOR_ERODE) {
> +        return DeflatedRegion(aInputExtents[0], nsIntSize(rx, ry));
> +      }

Let's avoid DeflatedRegion entirely and just return aInputExtents[0].

@@ +1547,5 @@
> +AttributeMap&
> +FilterPrimitiveDescription::Attributes()
> +{
> +  return mAttributes;
> +}

All these simple accessor functions should be inlined in the header file.

::: gfx/src/FilterSupport.h
@@ +313,5 @@
> +                             nsTArray<nsIntRegion>* aPrimitiveNeededRegions = nullptr);
> +
> +  static nsIntRegion
> +  ComputePostFilterExtents(const FilterDescription& aFilter,
> +                           const nsIntRegion& aSourceGraphicExtents);

The public methods in this class all need good documentation.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 814114 [details] [diff] [review]
part2: Use FilterSupport for SVG filters

Review of attachment 814114 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/svg/content/src/nsSVGFilters.h
@@ +29,5 @@
>  };
>  
>  typedef nsSVGElement nsSVGFEBase;
> +typedef mozilla::gfx::AttributeMap AttributeMap;
> +typedef mozilla::gfx::FilterPrimitiveDescription FilterPrimitiveDescription;

We shouldn't do these typedefs at the top level.

::: layout/svg/nsSVGFilterInstance.cpp
@@ +16,5 @@
>  #include "nsSVGUtils.h"
>  #include "SVGContentUtils.h"
> +#include "FilterSupport.h"
> +
> +using namespace mozilla;

add "using namespace mozilla::dom;"

@@ +26,5 @@
>    val.Init(aCtxType, 0xff, aValue,
>             nsIDOMSVGLength::SVG_LENGTHTYPE_NUMBER);
>  
>    float value;
> +  if (mPrimitiveUnits == dom::SVG_UNIT_TYPE_OBJECTBOUNDINGBOX) {

... so we can remove "dom::" here

@@ +142,5 @@
>    return NS_OK;
>  }
>  
>  nsresult
>  nsSVGFilterInstance::BuildPrimitives()

Why do we still need the mPrimitives array? Doesn't FilterSupport and FilterDescription contain everything we need now?

@@ +347,5 @@
> +      gfxPlatform::GetPlatform()->GetSourceSurfaceForSurface(aTargetDT, offscreenSurface);
> +  } else {
> +    aPrimitive->mSourceSurface = offscreenDT->Snapshot();
> +  }
> +  aPrimitive->mSurfaceRect = gfx::RoundedToInt(gfx::ToRect(neededRect));

Why are we converting from int to float to int here?

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

wrt bug 928985 - :mstange and :roc can comment more here, but a good path forward seems to be to take the large chunk of this patch that sets up the infrastructure (that we'd need to pass info to D2D/SkiaGL/etc.) anyway, and then just use Skia for the software rendering -- contributing :mstange's optimized implementations where appropriate to upstream Skia to fix any issues there.  That gets us a "use Skia" path that just works whether you're using GL or not.

Comment 8

[Andreas Gal :gal]

Yup, lets break this up in chunks and land this quickly and iteratively. This is really exciting stuff.

Comment 9

[Markus Stange [:mstange]]

Attachment: part 1, v2 (wip)

I haven't applied all review comments to this patch yet, so not requesting review.

Comment 10

[Markus Stange [:mstange]]

Attachment: part 2, v2

I think I addressed all review comments in this part. Except for adding comments in nsSVGFilterInstance.h... I'll do that tomorrow.

Comment 11

[Markus Stange [:mstange]]

Attachment: part 1, v3

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
> @@ +94,5 @@
> > +  static TemporaryRef<FilterNode>
> > +  LinearRGBToSRGB(DrawTarget* aDT, FilterNode* aInput)
> > +  {
> > +    RefPtr<FilterNode> transfer = aDT->CreateFilter(FILTER_DISCRETE_TRANSFER);
> > +    transfer->SetAttribute(ATT_DISCRETE_TRANSFER_DISABLE_R, false);
> 
> Can't these DISABLE values default to false so we don't have to set them
> here?

FilterNodes don't guarantee having the same defaults on different backends. The user is asked to always set all attributes. Maybe we should change that? However, I think defaulting DISABLE to true would make more sense because an enabled transfer needs the other attributes set in order to know what to do.

> ::: gfx/src/FilterSupport.h
> @@ +5,5 @@
> > +
> > +#ifndef __FilterSupport_h
> > +#define __FilterSupport_h
> > +
> > +#include "nsMathUtils.h"
> 
> Why do you need to include this in this file?

I don't, removed.

> @@ +146,5 @@
> > +class FilterNode;
> > +
> > +namespace filterattribute {
> > +struct Attribute;
> > +}
> 
> Instead of using a namespace, maybe we could just call it FilterAttribute.

FilterAttribute it is.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4)
> Comment on attachment 814113 [details] [diff] [review]
> part1: Add infrastructure
> 
> Review of attachment 814113 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/src/FilterSupport.cpp
> @@ +365,5 @@
> > +
> > +// Called for one channel at a time.
> > +// This function creates the required FilterNodes on demand and tries to
> > +// merge conversions of different channels into the same FilterNode if
> > +// possible.
> 
> You need to say more about what the parameters mean.

I've added some comments. Are those helpful enough or should I say more?

> @@ +492,5 @@
> > +    float exponent = aFunctionAttributes.GetFloat(eComponentTransferFunctionExponent);
> > +    float offset = aFunctionAttributes.GetFloat(eComponentTransferFunctionOffset);
> > +    filter->SetAttribute(amplitudeAtt[aChannel], amplitude);
> > +    filter->SetAttribute(exponentAtt[aChannel], exponent);
> > +    filter->SetAttribute(offsetAtt[aChannel], offset);
> 
> Can we save some runtime work here by introducing static_asserts that Moz2D
> constant values match the SVG constant values?

In this case we can't because the component transfer functions have different enum values depending on the color channel.

> 
> @@ +663,5 @@
> > +      static const uint32_t edgeModes[SVG_EDGEMODE_NONE+1] = {
> > +        EDGE_MODE_NONE,      // SVG_EDGEMODE_UNKNOWN
> > +        EDGE_MODE_DUPLICATE, // SVG_EDGEMODE_DUPLICATE
> > +        EDGE_MODE_WRAP,      // SVG_EDGEMODE_WRAP
> > +        EDGE_MODE_NONE       // SVG_EDGEMODE_NONE
> 
> Another place where we could probably static_assert that values match and
> avoid programmatic conversion.
> 
> @@ +690,5 @@
> > +        COLOR_CHANNEL_R, // SVG_CHANNEL_UNKNOWN
> > +        COLOR_CHANNEL_R, // SVG_CHANNEL_R
> > +        COLOR_CHANNEL_G, // SVG_CHANNEL_G
> > +        COLOR_CHANNEL_B, // SVG_CHANNEL_B
> > +        COLOR_CHANNEL_A  // SVG_CHANNEL_A
> 
> And another...
> 
> @@ +715,5 @@
> > +                           (uint32_t)atts.GetFloat(eTurbulenceSeed));
> > +      static const uint32_t type[SVG_TURBULENCE_TYPE_TURBULENCE+1] = {
> > +        TURBULENCE_TYPE_FRACTAL_NOISE, // SVG_TURBULENCE_TYPE_UNKNOWN
> > +        TURBULENCE_TYPE_FRACTAL_NOISE, // SVG_TURBULENCE_TYPE_FRACTALNOISE
> > +        TURBULENCE_TYPE_TURBULENCE     // SVG_TURBULENCE_TYPE_TURBULENCE
> 
> Etc...
> 

For these cases the "unknown" values make it tricky. I haven't changed it in this patch.

> @@ +1006,5 @@
> > +                               const IntRect& aSourceAlphaRect,
> > +                               SourceSurface* aFillPaint,
> > +                               const IntRect& aFillPaintRect,
> > +                               SourceSurface* aStrokePaint,
> > +                               const IntRect& aStrokePaintRect,
> 
> I think we should pass an array of structs, or two arrays, instead of
> explicit parameters, since we'll probably want to extend this list over time
> and this function already has too many parameters.

I got rid of the SourceAlpha arguments for all these methods, so that reduced the number of arguments a bit. And, as far as I'm aware, BackgroundImage is the only other input that we will want to add in the near future (is that correct?), so I think we can keep the arguments separate for now. I think it's simpler than using an array.

> @@ +1014,5 @@
> > +  const IntRect& filterSpaceBounds = aFilter.mFilterSpaceBounds;
> > +
> > +  nsIntRegion dummy;
> > +  nsTArray<nsIntRegion> primitiveNeededRegions;
> > +  FilterSupport::ComputeSourceNeededRegions(aFilter, nsIntRect(aSurfaceRect.x, aSurfaceRect.y, aSurfaceRect.width, aSurfaceRect.height), dummy, dummy, dummy, dummy, &primitiveNeededRegions);
> 
> We must have a better way to convert IntRect to nsIntRect!

Oops. This should not have been left in the patch. :)

> 
> @@ +1149,5 @@
> > +  nsIntRegionRectIterator it(aRegion);
> > +  while (const nsIntRect* sr = it.Next()) {
> > +    nsIntRect r = *sr;
> > +    r.Deflate(aMargin);
> > +    result.Or(result, r);
> 
> This method doesn't make much sense to me. For a complex region it can
> create holes inside the region.

Oh, right! I removed DeflatedRegion.

> @@ +1297,5 @@
> > +      inputChangeRegions.AppendElement(inputChangeRegion);
> > +    }
> > +    nsIntRegion changeRegion =
> > +      ResultChangeRegionForPrimitive(descr, inputChangeRegions);
> > +    IntRect ps = RoundedToInt(descr.PrimitiveSubregion());
> 
> We should be explicitly rounding out here and RoundedToInt doesn't do that
> AFAICT.

I added a RoundOut() call.

> Same for other callers of RoundedToInt below.

I haven't changed it for the primitive subregion because nsSVGFilterInstance already does it for us. Should I change PrimitiveSubregion to IntRect?

Comment 12

[Markus Stange [:mstange]]

Attachment: part 2, v3

This one should be much better regarding namespaces/typedefs and comments than yesterday's patch.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> @@ +142,5 @@
> >    return NS_OK;
> >  }
> >  
> >  nsresult
> >  nsSVGFilterInstance::BuildPrimitives()
> 
> Why do we still need the mPrimitives array? Doesn't FilterSupport and
> FilterDescription contain everything we need now?

It does. I've rewritten this part.

> 
> @@ +347,5 @@
> > +      gfxPlatform::GetPlatform()->GetSourceSurfaceForSurface(aTargetDT, offscreenSurface);
> > +  } else {
> > +    aPrimitive->mSourceSurface = offscreenDT->Snapshot();
> > +  }
> > +  aPrimitive->mSurfaceRect = gfx::RoundedToInt(gfx::ToRect(neededRect));
> 
> Why are we converting from int to float to int here?

I think when I started writing this code we didn't have ToIntRect yet. We do now, so all these cases should be fixed now.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Markus Stange [:mstange] from comment #11)
> FilterNodes don't guarantee having the same defaults on different backends.
> The user is asked to always set all attributes. Maybe we should change that?

Yes I think we probably should. It will save a little bit of work in some cases.

> However, I think defaulting DISABLE to true would make more sense because an
> enabled transfer needs the other attributes set in order to know what to do.

Sure.

> I got rid of the SourceAlpha arguments for all these methods, so that
> reduced the number of arguments a bit. And, as far as I'm aware,
> BackgroundImage is the only other input that we will want to add in the near
> future (is that correct?), so I think we can keep the arguments separate for
> now. I think it's simpler than using an array.

Mmm, OK.

> I haven't changed it for the primitive subregion because nsSVGFilterInstance
> already does it for us. Should I change PrimitiveSubregion to IntRect?

Yes I think so.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 827699 [details] [diff] [review]
part 1, v3

Review of attachment 827699 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/src/FilterSupport.cpp
@@ +48,5 @@
> +      for (int32_t i = 0; i < 256; i++) {
> +        float c = i / 255.0f;
> +        sMap[i] = c <= 0.0031308f ? c * 12.92f : 1.055f * powf(c, 1 / 2.4f) - 0.055f;
> +      }
> +      sCalculatedMap = true;

This isn't thread-safe, which is potentially a problem since we may end up using this code on both the content thread and compositor thread in the same process.

How about we go back to the constant table you previously had, but document there the formula that defines each value.

@@ +90,5 @@
> +  static TemporaryRef<FilterNode>
> +  Crop(DrawTarget* aDT, FilterNode* aInputFilter, const Rect& aRect)
> +  {
> +    // printf("NOT CROPPING\n");
> +    // return aInputFilter;

better remove this

@@ +256,5 @@
> +    {
> +      if (aValues.Length() != 20)
> +        return NS_ERROR_FAILURE;
> +
> +      PodCopy(aOutMatrix, aValues.Elements(), aValues.Length());

make the length 20

@@ +289,5 @@
> +    }
> +
> +    case SVG_FECOLORMATRIX_TYPE_HUE_ROTATE:
> +    {
> +      memcpy(aOutMatrix, identityMatrix, sizeof(float) * 20);

PodCopy. Move it below the aValues.Length check.

@@ +318,5 @@
> +    }
> +
> +    case SVG_FECOLORMATRIX_TYPE_LUMINANCE_TO_ALPHA:
> +    {
> +      memcpy(aOutMatrix, luminanceToAlphaMatrix, sizeof(float) * 20);

PodCopy

@@ +531,5 @@
> +        filter->SetInput(IN_COMPOSITE_IN_START, aSources[1]);
> +        filter->SetInput(IN_COMPOSITE_IN_START + 1, aSources[0]);
> +      } else {
> +        filter = aDT->CreateFilter(FILTER_BLEND);
> +        static const uint32_t blendModes[SVG_FEBLEND_MODE_LIGHTEN + 1] = {

How about static const uint8_t?

@@ +653,5 @@
> +                           atts.GetFloat(eConvolveMatrixBias));
> +      filter->SetAttribute(ATT_CONVOLVE_MATRIX_TARGET,
> +                           atts.GetIntPoint(eConvolveMatrixTarget));
> +      uint32_t edgeMode = atts.GetUint(eConvolveMatrixEdgeMode);
> +      static const uint32_t edgeModes[SVG_EDGEMODE_NONE+1] = {

How about static const uint8_t? Same for the other static const uint32_ts below.

@@ +871,5 @@
> +      // Create a new surface that will contain the transformed image.
> +      IntSize size(aTargetSurfaceRect.width, aTargetSurfaceRect.height);
> +      nsRefPtr<gfxASurface> targetImage =
> +        inputImage->CreateSimilarSurface(GFX_CONTENT_COLOR_ALPHA,
> +                                         gfxIntSize(size.width, size.height));

Actually I'm a little confused as to why we're making a copy of the surface here. Can't we use the transform on the gfxPattern instead?

@@ +1144,5 @@
> +{
> +  nsIntRegion result;
> +  result.And(aRegion1, aRegion2);
> +  return result;
> +}

Can you add all these region operations to nsIntRegion?

@@ +1730,5 @@
> +#undef MAKE_ATTRIBUTE_HANDLERS_BASIC
> +#undef MAKE_ATTRIBUTE_HANDLERS_CLASS
> +
> +void
> +AttributeMap::GetFloats(AttributeName aName, nsTArray<float>& aArray) const

Why can't GetFloats just return a const nsTArray&? I think that would save a lot of copying by the users of GetFloats.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 827700 [details] [diff] [review]
part 2, v3

Review of attachment 827700 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/svg/content/src/nsSVGFilters.cpp
@@ +145,5 @@
>  // nsSVGElement methods
>  
> +
> +bool
> +nsSVGFE::StyleIsSetToSRGB() {

{ on its own line

::: content/svg/content/src/nsSVGFilters.h
@@ +88,5 @@
> +
> +  virtual mozilla::gfx::FilterPrimitiveDescription
> +    GetPrimitiveDescription(nsSVGFilterInstance* aInstance,
> +                            const mozilla::gfx::Rect& aFilterSubregion,
> +                            nsTArray<nsRefPtr<gfxASurface> >& aInputImages) = 0;

Use typedefs to avoid prefixes.

::: layout/svg/nsSVGFilterInstance.cpp
@@ +481,3 @@
>  
> +  RefPtr<SourceSurface> resultImageSource;
> +  if (!aContext->IsCairo()) {

Mmm ... what if aContext->IsCairo()?

::: layout/svg/nsSVGFilterInstance.h
@@ +175,4 @@
>     * into filter space, depending on the value of mPrimitiveUnits. (For
>     * objectBoundingBoxUnits, the bounding box offset is applied to the point.)
>     */
> +  mozilla::gfx::Point3D ConvertLocation(const mozilla::gfx::Point3D& aPoint) const;

Use typedefs to avoid these prefixes, here and below.

Comment 16

[Andreas Gal :gal]

Comment on attachment 827699 [details] [diff] [review]
part 1, v3

Review of attachment 827699 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/src/FilterSupport.cpp
@@ +48,5 @@
> +      for (int32_t i = 0; i < 256; i++) {
> +        float c = i / 255.0f;
> +        sMap[i] = c <= 0.0031308f ? c * 12.92f : 1.055f * powf(c, 1 / 2.4f) - 0.055f;
> +      }
> +      sCalculatedMap = true;

Since the table is in the same place, and we are writing the same value at each location even when we race, and we are writing full 32-bit words, and we only set sCalculatedMap after we have calculated the entire table at least once, I think this is actually safe on every platform we run on.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 827699 [details] [diff] [review]
part 1, v3

Review of attachment 827699 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/src/FilterSupport.cpp
@@ +48,5 @@
> +      for (int32_t i = 0; i < 256; i++) {
> +        float c = i / 255.0f;
> +        sMap[i] = c <= 0.0031308f ? c * 12.92f : 1.055f * powf(c, 1 / 2.4f) - 0.055f;
> +      }
> +      sCalculatedMap = true;

I guess that's true in practice. In theory, in some memory models, there's a race where thread A observes thread B's write to sCalculatedMap before it observes some of B's write(s) to sMap, causing A to use a version of sMap that's not fully initialized. It's probably best just to use the original constant table (which also saves memory in a multiprocess situation).

Comment 18

[Markus Stange [:mstange]]

Attachment: part 1, v4

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14)
> Comment on attachment 827699 [details] [diff] [review]
> part 1, v3
> 
> Review of attachment 827699 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/src/FilterSupport.cpp
> @@ +48,5 @@
> > +      for (int32_t i = 0; i < 256; i++) {
> > +        float c = i / 255.0f;
> > +        sMap[i] = c <= 0.0031308f ? c * 12.92f : 1.055f * powf(c, 1 / 2.4f) - 0.055f;
> > +      }
> > +      sCalculatedMap = true;
> 
> This isn't thread-safe, which is potentially a problem since we may end up
> using this code on both the content thread and compositor thread in the same
> process.
> 
> How about we go back to the constant table you previously had, but document
> there the formula that defines each value.

Done.

> @@ +90,5 @@
> > +  static TemporaryRef<FilterNode>
> > +  Crop(DrawTarget* aDT, FilterNode* aInputFilter, const Rect& aRect)
> > +  {
> > +    // printf("NOT CROPPING\n");
> > +    // return aInputFilter;
> 
> better remove this

oops

> 
> @@ +256,5 @@
> > +    {
> > +      if (aValues.Length() != 20)
> > +        return NS_ERROR_FAILURE;
> > +
> > +      PodCopy(aOutMatrix, aValues.Elements(), aValues.Length());
> 
> make the length 20

Done.

> 
> @@ +289,5 @@
> > +    }
> > +
> > +    case SVG_FECOLORMATRIX_TYPE_HUE_ROTATE:
> > +    {
> > +      memcpy(aOutMatrix, identityMatrix, sizeof(float) * 20);
> 
> PodCopy. Move it below the aValues.Length check.

Done.

> @@ +318,5 @@
> > +    }
> > +
> > +    case SVG_FECOLORMATRIX_TYPE_LUMINANCE_TO_ALPHA:
> > +    {
> > +      memcpy(aOutMatrix, luminanceToAlphaMatrix, sizeof(float) * 20);
> 
> PodCopy

Done.

> @@ +531,5 @@
> > +        filter->SetInput(IN_COMPOSITE_IN_START, aSources[1]);
> > +        filter->SetInput(IN_COMPOSITE_IN_START + 1, aSources[0]);
> > +      } else {
> > +        filter = aDT->CreateFilter(FILTER_BLEND);
> > +        static const uint32_t blendModes[SVG_FEBLEND_MODE_LIGHTEN + 1] = {
> 
> How about static const uint8_t?

Done. I had to add casts to uint32_t in the calls to SetAttribute in order to resolve the ambiguity about which SetAttribute overload should be called.

> @@ +871,5 @@
> > +      // Create a new surface that will contain the transformed image.
> > +      IntSize size(aTargetSurfaceRect.width, aTargetSurfaceRect.height);
> > +      nsRefPtr<gfxASurface> targetImage =
> > +        inputImage->CreateSimilarSurface(GFX_CONTENT_COLOR_ALPHA,
> > +                                         gfxIntSize(size.width, size.height));
> 
> Actually I'm a little confused as to why we're making a copy of the surface
> here. Can't we use the transform on the gfxPattern instead?

I don't think we were making a copy; after the draw, targetImage contained the transformed inputImage. (And targetSurf was just the Moz2D-version of targetImage.)
However, I've rewritten this part to use the FILTER_TRANSFORM filter type instead, which is much simpler.

> 
> @@ +1144,5 @@
> > +{
> > +  nsIntRegion result;
> > +  result.And(aRegion1, aRegion2);
> > +  return result;
> > +}
> 
> Can you add all these region operations to nsIntRegion?

Can I postpone this until after bug 845874 lands? I don't want to rot that patch too much.
That said, only the Inflate method really adds new functionality. MovedRegion and IntersectRegions are just non-mutating versions of the already existing mutating "MoveBy" and "And" methods. And UnionOfRegions does not apply on a single region, but takes an array of regions.

> 
> @@ +1730,5 @@
> > +#undef MAKE_ATTRIBUTE_HANDLERS_BASIC
> > +#undef MAKE_ATTRIBUTE_HANDLERS_CLASS
> > +
> > +void
> > +AttributeMap::GetFloats(AttributeName aName, nsTArray<float>& aArray) const
> 
> Why can't GetFloats just return a const nsTArray&? I think that would save a
> lot of copying by the users of GetFloats.

Good point, that hadn't occurred to me. There's one problem: If the attribute does not yet exist in the attribute map, we now have to create it, because otherwise we have nothing that the returned reference can reference. And GetFloats should work on a const AttributeMap, too, so I had to make mMap mutable. I hope that's ok.

Comment 19

[Markus Stange [:mstange]]

(In reply to Markus Stange [:mstange] from comment #18)
> And GetFloats should work on a const AttributeMap, too, so I had to make
> mMap mutable. I hope that's ok.

It should be ok because adding an empty attribute to the map is not detectable for users of the AttributeMap object.

Comment 20

[Markus Stange [:mstange]]

Attachment: part 2, v4

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #15)
> Comment on attachment 827700 [details] [diff] [review]
> part 2, v3
> 
> Review of attachment 827700 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/svg/content/src/nsSVGFilters.cpp
> @@ +145,5 @@
> >  // nsSVGElement methods
> >  
> > +
> > +bool
> > +nsSVGFE::StyleIsSetToSRGB() {
> 
> { on its own line

Done.

> ::: content/svg/content/src/nsSVGFilters.h
> @@ +88,5 @@
> > +
> > +  virtual mozilla::gfx::FilterPrimitiveDescription
> > +    GetPrimitiveDescription(nsSVGFilterInstance* aInstance,
> > +                            const mozilla::gfx::Rect& aFilterSubregion,
> > +                            nsTArray<nsRefPtr<gfxASurface> >& aInputImages) = 0;
> 
> Use typedefs to avoid prefixes.

Done.

> ::: layout/svg/nsSVGFilterInstance.cpp
> @@ +481,3 @@
> >  
> > +  RefPtr<SourceSurface> resultImageSource;
> > +  if (!aContext->IsCairo()) {
> 
> Mmm ... what if aContext->IsCairo()?

Then we already have a non-null resultImage which we also pass to CompositeSurfaceMatrix.

> 
> ::: layout/svg/nsSVGFilterInstance.h
> @@ +175,4 @@
> >     * into filter space, depending on the value of mPrimitiveUnits. (For
> >     * objectBoundingBoxUnits, the bounding box offset is applied to the point.)
> >     */
> > +  mozilla::gfx::Point3D ConvertLocation(const mozilla::gfx::Point3D& aPoint) const;
> 
> Use typedefs to avoid these prefixes, here and below.

Done.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8334551 [details] [diff] [review]
part 1, v4

Review of attachment 8334551 [details] [diff] [review]:
-----------------------------------------------------------------

Just that region issue left :-)

::: gfx/src/FilterSupport.cpp
@@ +1169,5 @@
> +{
> +  nsIntRegion result;
> +  result.And(aRegion1, aRegion2);
> +  return result;
> +}

845874 has landed so these should move out to nsIntRegion. Yes, they're just sugar, but they belong there.

Comment 22

[Markus Stange [:mstange]]

Attachment: part 1, v5

With the added methods we now have
MoveBy  + MovedBy
Inflate + Inflated
And     + Intersect

I don't really like the name Inflated, but BaseRect::Inflate is a mutating method, too, and BaseRect::Intersect is non-mutating, so I just copied that inconsistency.

Comment 23

[Markus Stange [:mstange]]

Attachment: reftest changes

These are the reftest changes needed to get this green on try. There are three reasons for the changes: (1) legitimate intentional changes in behavior, (2) rounding issues and (3) real bugs.

Under (1) are:
 - feGaussianBlur and feMorphology now treat pixels outside the source image as transparent black, instead of repeating the pixel at the edges. For feGaussianBlur this was asked for in bug 776694 and for feMorphology Dirk confirmed it to me on IRC.
 - feDisplacementMap uses bilinear filtering on D2D 1.1, so the effective radius increases slightly.

(2) is mostly due to minor imprecisions in the fixed-point software filters. These are the tests with fuzzy(1,*) or fuzzy(2,*).

Under (3) are bugs with D2D that I don't want to debug now:
 - specular lighting seems to have a completely wrong light color
 - distant lighting has a slightly wrong light color
 - The D2D feImage-zoom failure is still a mystery to me. The transparent parts in the gfxASurface provided by the feImage element seem to contain junk. I will file a bug on this and ask Bas to debug it.

Comment 24

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3f360237e89
https://hg.mozilla.org/integration/mozilla-inbound/rev/ece82588eff5
https://hg.mozilla.org/integration/mozilla-inbound/rev/61ff58ee03e5

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b3f360237e89
https://hg.mozilla.org/mozilla-central/rev/ece82588eff5
https://hg.mozilla.org/mozilla-central/rev/61ff58ee03e5