389746 - Implement feDisplacementMap

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[tor]

Attachment: testcase that doesn't use feImage, from inkscape mailing list

Comment 1

[tor]

Attachment: work in progress - horizontal displacements not correct

Comment 2

[tor]

Attachment: update to tip - horizontal issue still unsolved

Comment 3

[Robert Longson [:longsonr]]

You should use the nearest source pixel:

      PRInt32 sourceX =
        PRInt32(x + scale * (displacementData[targIndex + xChannel] / 255.0 - 0.5) + 0.5);
      PRInt32 sourceY =
        PRInt32(y + scale * (displacementData[targIndex + yChannel] / 255.0 - 0.5) + 0.5);

Not sure whether this solves anything though. I'll have another look later.

Comment 4

[tor]

Problems with testcase caused by bug 390193.

Comment 5

[tor]

Attachment: version with nearest neighbor sampling

Comment 6

[Robert Longson [:longsonr]]

color-interpolation-filters will affects the in input with this implementation.  
It should only affect the in2 input. Some options I can think of:

a) Pass in color maps to acquireSoureImage rather than the filterResource constructor.
b) fix it later as part of bug 389865 perhaps
c) land bug 389865 first and then fix it under the new architecture. Possibly by passing the address of inputs rather than inputs to acquireSourceImage then comparing addresses - which is what the bug currently implements incorrectly and what you commented on as why do that in that bug.

Comment 7

[tor]

It's curious that the specification talks about color-interpolation-filters not applying to 'in' in the paragraph right after it talks about using interpolation for high quality sampling of the input image.

Given that we're currently doing point sampling, having 'in' affected by that property will not produce any differences in the output, same for possible precision loss from roundtripping the colorspace conversion and the time taken for that operation.

Comment 8

[Robert Longson [:longsonr]]

(In reply to comment #7)
> It's curious that the specification talks about color-interpolation-filters not
> applying to 'in' in the paragraph right after it talks about using
> interpolation for high quality sampling of the input image.
> 

Isn't that two different things entirely? I took ignoring color-interpolation-filters to mean that you always convert in to linearRGB whereas how you interpret in2 depends on that property.

Comment 9

[tor]

(In reply to comment #8)
> (In reply to comment #7)
> > It's curious that the specification talks about color-interpolation-filters not
> > applying to 'in' in the paragraph right after it talks about using
> > interpolation for high quality sampling of the input image.
> 
> Isn't that two different things entirely? I took ignoring
> color-interpolation-filters to mean that you always convert in to linearRGB
> whereas how you interpret in2 depends on that property.

Seems somewhat ambiguous (big shock given the spec), so maybe color-interpolation should be used for 'in'.

Comment 10

[Robert Longson [:longsonr]]

I guess we should ask on the svg mailing list. I can think of other interpretations too. Meanwhile you could progress feImage, I don't think there are any contentious issues with that.

Comment 11

[Robert Longson [:longsonr]]

One other thing. If scale = 0 then we can skip the filter and just do fr.CopySourceImage(); instead.

Comment 12

[tor]

Attachment: add shortcut for scale=0

Comment 13

[Robert Longson [:longsonr]]

http://lists.w3.org/Archives/Public/www-svg/2007Aug/0010.html

Comment 14

[Robert Longson [:longsonr]]

Comment on attachment 275028 [details] [diff] [review]
add shortcut for scale=0

And now we know...

According to http://www.w3.org/Bugs/Public/show_bug.cgi?id=4919 you have to treat the inputs differently.

Comment 15

[tor]

Attachment: possibly spec-compliant feDisplacementMap

Comment 16

[Robert Longson [:longsonr]]

Comment on attachment 284234 [details] [diff] [review]
possibly spec-compliant feDisplacementMap

>+typedef nsSVGFE nsSVGFEDisplacementMapElementBase;
>+
>+class nsSVGFEDisplacementMapElement : public nsSVGFEDisplacementMapElementBase,
>+                                      public nsIDOMSVGFEDisplacementMapElement,
>+                                      public nsISVGFilter
>+{

>+
>+protected:
>+  virtual PRBool OperatesOnSRGB(nsIDOMSVGAnimatedString* aString) {
>+    if (aString != mIn1)
>+      return PR_TRUE;

I think you need to call the base class implementation rather than returning PR_TRUE;

>+
>+    return mSRGBInput;
>+  }
>+

> 
>+nsSVGFilterInstance::ColorModel
>+nsSVGFilterInstance::LookupImageColorModel(const nsAString &aName)
>+{
>+  ImageEntry *entry;
>+
>+  if (aName.IsEmpty())
>+    entry = mLastImage;
>+  else
>+    mImageDictionary.Get(aName, &entry);
>+
>+  if (entry)
>+    return entry->mColorModel;
>+
>+  // this return value won't be used

How about having an NS_NOTREACHED here then?

>+  return ColorModel(ColorModel::SRGB, ColorModel::PREMULTIPLIED);
>+}
>+

Comment 17

[tor]

(In reply to comment #16)
> >+nsSVGFilterInstance::ColorModel
> >+nsSVGFilterInstance::LookupImageColorModel(const nsAString &aName)
> >+{
> >+  ImageEntry *entry;
> >+
> >+  if (aName.IsEmpty())
> >+    entry = mLastImage;
> >+  else
> >+    mImageDictionary.Get(aName, &entry);
> >+
> >+  if (entry)
> >+    return entry->mColorModel;
> >+
> >+  // this return value won't be used
> 
> How about having an NS_NOTREACHED here then?

The code will be reached if someone specifies a nonexistent input for a filter, as we need to find the color model before calling AcquireSourceImage() which both uses that data and tells us if the input exists.

Comment 18

[Robert Longson [:longsonr]]

(In reply to comment #17)
> 
> The code will be reached if someone specifies a nonexistent input for a filter,
> as we need to find the color model before calling AcquireSourceImage() which
> both uses that data and tells us if the input exists.
> 

That sounds like better comment text.

Comment 19

[tor]

Attachment: adjust per comments

Comment 20

[Robert Longson [:longsonr]]

Why not get rid of the mSRGBInput and just have OperatesOnSRGB work it out itself. You save 4 bytes per filter instance at the cost of a couple of lines of code.

with that r=longsonr 

Comment 21

[Damon Sicore (:damons)]

Given the state of things now, I think we should take this bug.   What's the current status?

Comment 22

[Robert Longson [:longsonr]]

Ideally this needs comment 20 addressing and (assuming feImage is added) the SVG features list should be looked at as this would complete all filters so maybe we should change that, although changing the supported features could be done as a follow-up.

Comment 23

[Damon Sicore (:damons)]

Ok.  Can we make this happen?  

Comment 24

[Robert Longson [:longsonr]]

Attachment: address comment 20

Comment 25

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

+      PRInt32 sourceX =
+        PRInt32(x +
+                scale * (displacementData[targIndex + xChannel] / 255.0 - 0.5) +
+                0.5);
+      PRInt32 sourceY =
+        PRInt32(y +
+                scale * (displacementData[targIndex + yChannel] / 255.0 - 0.5) +
+                0.5);

Could you speed this up a bit by precalculating scale/255.0? Also pull x and y out of the PRInt32() since it's already an integer, that saves an int->double conversion. So you end up with code like

double scaleOver255 = scale/255.0;
double scaleAdjustment = 0.5 - 0.5*scale;

  PRInt32 sourceX =
    x + PRInt32(displacementData[targIndex + xChannel]*scaleOver255 + scaleAdjustment);

Otherwise looks fine.

Comment 26

[Robert Longson [:longsonr]]

Attachment: address sr comments

Comment 27

[Robert Longson [:longsonr]]

Comment on attachment 294033 [details] [diff] [review]
address sr comments

Asking for approval per comment 23. Note that this is new functionality although it is reasonably well contained as a new filter within the existing filter architecture.

Comment 28

[Damon Sicore (:damons)]

Comment on attachment 294033 [details] [diff] [review]
address sr comments

Yep, we should take this now.  Thanks for all the hard work guys.

Comment 29

[tor]

Attachment: checkin version - update to tip

Comment 30

[tor]

Checked in.