Closed Bug 389746 Opened 13 years ago Closed 13 years ago

Implement feDisplacementMap

Categories

(Core :: SVG, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: tor, Assigned: tor)

References

(Blocks 1 open bug, )

Details

Attachments

(3 files, 7 obsolete files)

No description provided.
Blocks: 311029
Attachment #274063 - Attachment is obsolete: true
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.

Problems with testcase caused by bug 390193.
Attachment #274182 - Attachment is obsolete: true
Attachment #274820 - Flags: review?(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.
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.
(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.
(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'.
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.
One other thing. If scale = 0 then we can skip the filter and just do fr.CopySourceImage(); instead.
Attached patch add shortcut for scale=0 (obsolete) — Splinter Review
Attachment #274820 - Attachment is obsolete: true
Attachment #275028 - Flags: review?(longsonr)
Attachment #274820 - Flags: review?(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.
Attachment #275028 - Flags: review?(longsonr) → review-
Attachment #275028 - Attachment is obsolete: true
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);
>+}
>+
(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.
(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.

Attached patch adjust per comments (obsolete) — Splinter Review
Attachment #284234 - Attachment is obsolete: true
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 
Given the state of things now, I think we should take this bug.   What's the current status?
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.
Assignee: nobody → tor
Ok.  Can we make this happen?  
Attached patch address comment 20 (obsolete) — Splinter Review
Attachment #284373 - Attachment is obsolete: true
Attachment #293833 - Flags: superreview?(roc)
Attachment #293833 - Flags: review+
+      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.
Attachment #293833 - Attachment is obsolete: true
Attachment #294033 - Flags: superreview?(roc)
Attachment #293833 - Flags: superreview?(roc)
Attachment #294033 - Flags: superreview?(roc) → superreview+
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.
Attachment #294033 - Flags: approval1.9?
Comment on attachment 294033 [details] [diff] [review]
address sr comments

Yep, we should take this now.  Thanks for all the hard work guys.
Attachment #294033 - Flags: approval1.9? → approval1.9+
Checked in.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.