Implement feDisplacementMap

RESOLVED FIXED

Status

()

Core
SVG
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: tor, Assigned: tor)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 7 obsolete attachments)

(Assignee)

Description

10 years ago
Created attachment 274062 [details]
testcase that doesn't use feImage, from inkscape mailing list
(Assignee)

Comment 1

10 years ago
Created attachment 274063 [details] [diff] [review]
work in progress - horizontal displacements not correct

Updated

10 years ago
Blocks: 311029

Updated

10 years ago
(Assignee)

Comment 2

10 years ago
Created attachment 274182 [details] [diff] [review]
update to tip - horizontal issue still unsolved
Attachment #274063 - Attachment is obsolete: true

Comment 3

10 years ago
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.

(Assignee)

Comment 4

10 years ago
Problems with testcase caused by bug 390193.
(Assignee)

Comment 5

10 years ago
Created attachment 274820 [details] [diff] [review]
version with nearest neighbor sampling
Attachment #274182 - Attachment is obsolete: true
Attachment #274820 - Flags: review?(longsonr)

Comment 6

10 years ago
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.
(Assignee)

Comment 7

10 years ago
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

10 years ago
(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.
(Assignee)

Comment 9

10 years ago
(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

10 years ago
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

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

Comment 12

10 years ago
Created attachment 275028 [details] [diff] [review]
add shortcut for scale=0
Attachment #274820 - Attachment is obsolete: true
Attachment #275028 - Flags: review?(longsonr)
Attachment #274820 - Flags: review?(longsonr)

Comment 13

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

Comment 14

10 years ago
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-
(Assignee)

Comment 15

10 years ago
Created attachment 284234 [details] [diff] [review]
possibly spec-compliant feDisplacementMap
Attachment #275028 - Attachment is obsolete: true

Comment 16

10 years ago
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);
>+}
>+
(Assignee)

Comment 17

10 years ago
(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

10 years ago
(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.

(Assignee)

Comment 19

10 years ago
Created attachment 284373 [details] [diff] [review]
adjust per comments
Attachment #284234 - Attachment is obsolete: true

Comment 20

10 years ago
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?

Comment 22

10 years ago
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?  

Comment 24

10 years ago
Created attachment 293833 [details] [diff] [review]
address comment 20
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.

Comment 26

10 years ago
Created attachment 294033 [details] [diff] [review]
address sr comments
Attachment #293833 - Attachment is obsolete: true
Attachment #294033 - Flags: superreview?(roc)
Attachment #293833 - Flags: superreview?(roc)
Attachment #294033 - Flags: superreview?(roc) → superreview+

Comment 27

10 years ago
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+
(Assignee)

Comment 29

10 years ago
Created attachment 295275 [details] [diff] [review]
checkin version - update to tip
(Assignee)

Comment 30

10 years ago
Checked in.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.