307708 - filters should operate in linearRGB color space

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Holger Will]

filter primitives currently operate in sRGB color space, but the default value
for color-interpolation-filters is linearRGB.

    <filter id="Table" filterUnits="objectBoundingBox" 
            x="0%" y="0%" width="100%" height="100%">
      <feComponentTransfer >
        <feFuncR type="discrete" tableValues="0.5 0.5 0.5"/>
      </feComponentTransfer>
    </filter>

in this example filter, the r component of the input color is maped to 0.5,
as this is interpreted as sRGB, the resulting r component is 

0.5 * 255 = 128

but the expected color component value converted to sRGB would be

0.5^(1/2.2) * 255 = 187

Comment 1

[Holger Will]

Attachment: testcase

the two rects should be colored in the same color, so you should only see one
rect.

Comment 2

[Robert Longson [:longsonr]]

Attachment: patch

Note that http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-filters-offset-01-b.html
comes out slightly too light. I think that is caused by rounding errors in the conversion to/from LinearRGB. Other filters in the svg test spect seem OK as does the test case.

Comment 3

[Robert Longson [:longsonr]]

Can correct the feFlood filter by ignoring the color-interpolation-filters flag as the flood colour is sRGB so there is no need to convert to/from linearRGB we can just work in sRGB always and get the same effect without rounding. 

I've broken this patch up into some smaller SVG only patches where I've made unrelated changes but the layout/style changes in the submitted patch are still required unchanged.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

I'm not a qualified reviewer for the SVG changes in this patch, so I'll just review the style system changes.  I'm adding a review request to tor for the SVG changes, but feel free to change that to another SVG peer if you want.  Whoever that is should also review the comment changes in nsStyleConsts.h to verify that those comments linking to IDL constants are no longer needed.  (Sorry, should have caught that sooner.)

>Index: layout/base/nsStyleConsts.h
>+// color-interpolation

The comment should have "and color-interpolation-filters"

>+#define NS_STYLE_COLOR_INTERPOLATION_AUTO           0
>+#define NS_STYLE_COLOR_INTERPOLATION_SRGB           1
>+#define NS_STYLE_COLOR_INTERPOLATION_LINEARRGB      2

>Index: layout/style/nsRuleNode.cpp

Would you mind checking for eCSSUnit_Initial as well, and setting to NS_STYLE_COLOR_INTERPOLATION_SRGB for color-interpolation and NS_STYLE_COLOR_INTERPOLATION_LINEARRGB for color-interpolation-filters.  (Our support for 'initial' is spotty, but we may as well work on it...)

nsStyleStruct.cpp:
>        mClipRule              != aOther.mClipRule              ||
>+       mColorInterpolation != aOther.mColorInterpolation       ||
>+       mColorInterpolationFilters != aOther.mColorInterpolationFilters ||

Could you at least line up the indent for mColorInterpolation, since it fits?

r=dbaron on the layout/style/* and nsStyleConsts.h changes with those fixes.

Comment 5

[Robert Longson [:longsonr]]

Attachment: updated patch

Addressed dbaron's review comments.
Removed extraneous changes into different bugs.
Made feMergeElement and feFloodElement always work in sRGB to avoid colour rounding errors from conversion to/from linearRGB.

Colours still exhibit rounding problems transforming sRGB image->linearRGB and then back again does not give you exactly the same colours. I suppose I could implement a float based (rather than UInt8 based) image store to avoid this if necessary but transformations would then be slower.

Comment 6

[tor]

Comment on attachment 252066 [details] [diff] [review]
updated patch

Don't you need to undo the premultiplication before you can do sRGB<->linearRGB conversions?

It seems that separately compiled methods called on each channel of each pixel would be slow - make them inline methods of nsSVGUtils or alternatively provide methods on SVGUtils to return the lookup tables?

Having a couple helper functions/methods for doing an image conversion would reduce the code somewhat.

Comment 7

[Robert Longson [:longsonr]]

Attachment: address review comments

(In reply to comment #6)
> (From update of attachment 252066 [details] [diff] [review])
> Don't you need to undo the premultiplication before you can do sRGB<->linearRGB
> conversions?
> 
> It seems that separately compiled methods called on each channel of each pixel
> would be slow - make them inline methods of nsSVGUtils or alternatively provide
> methods on SVGUtils to return the lookup tables?
> 
> Having a couple helper functions/methods for doing an image conversion would
> reduce the code somewhat.
> 

Implemented methods in SVGUtils to convert the entire image including premultiplication. Hopefully this addresses all the above issues.

Comment 8

[Robert Longson [:longsonr]]

(In reply to comment #7)

Colours seem better now.

One remaining niggle. 

Am I undoing the premultiplication twice in feComponentTransfer?

Comment 9

[tor]

Comment on attachment 252448 [details] [diff] [review]
address review comments

One thing I'm concerned about is that you're always converting the images back to sRGB.  If linearRGB was the exception, I'd say leave this to future optimization.  Unfortunately since linearRGB is the default we should probably tackle the question now.  Adding a mLinearRGB flag to nsSVGFilterInstance::ImageEntry and only doing the conversion when necessary would probably be the easiest thing.
 
>+void
>+nsSVGUtils::ConvertImageDataToLinearRGB(PRUint8 *data, 
>+                                        PRInt32 stride,
>+                                        const nsRect &rect)
>+{
>+  for (PRInt32 y = rect.y; y < rect.YMost(); y++) {
>+    for (PRInt32 x = rect.x; x < rect.XMost(); x++) {
>+      PRUint32 index = stride * y + 4 * x;

Micro-optimization which hopefully the compiler would do itself, but doing by hand neatens the code somewhat: "PRUint8 *pixel = data + stride * y + 4 * x;", then use pixel[0-3].

>+
>+      /* un-premultiply and sRGB -> linearRGB conversion */
>+      PRUint8 a = data[index + 3];
>+      if (a) {
>+        data[index] = gsRGBToLinearRGBMap[(255 * data[index]) / a];
>+        data[index + 1] = gsRGBToLinearRGBMap[(255 * data[index + 1]) / a];
>+        data[index + 2] = gsRGBToLinearRGBMap[(255 * data[index + 2]) / a];

I think you need to remultiply the alpha once you've done the mapping.

>+      } else {
>+        data[index] = 0;
>+        data[index + 1] = 0;
>+        data[index + 2] = 0;
>+      }
>+    }
>+  }
>+}

>+void
>+nsSVGUtils::ConvertImageDataFromLinearRGB(PRUint8 *data, 
>+                                          PRInt32 stride,
>+                                          const nsRect &rect)
>+{
>+  for (PRInt32 y = rect.y; y < rect.YMost(); y++) {
>+    for (PRInt32 x = rect.x; x < rect.XMost(); x++) {
>+      PRUint32 index = stride * y + 4 * x;

Ditto.

>+      /* linearRGB -> sRGB conversion and premultiply */
>+      PRUint8 a = data[index + 3];
>+      FAST_DIVIDE_BY_255(data[index],
>+                         glinearRGBTosRGBMap[data[index]] * a);
>+      FAST_DIVIDE_BY_255(data[index + 1],
>+                         glinearRGBTosRGBMap[data[index + 1]] * a);
>+      FAST_DIVIDE_BY_255(data[index + 2],
>+                         glinearRGBTosRGBMap[data[index + 2]] * a);

Need to un-premultiply first here, I think.

Don't you need a cast (or just specifying "a" as a PRUint32) to stop "glinearRGBTosRGBMap[...] * a" from wrapping?

>+    }
>+  }
>+}

Comment 10

[Robert Longson [:longsonr]]

Attachment: address additional review comments

(In reply to comment #9)
> (From update of attachment 252448 [details] [diff] [review])
> One thing I'm concerned about is that you're always converting the images back
> to sRGB.  If linearRGB was the exception, I'd say leave this to future
> optimization.  Unfortunately since linearRGB is the default we should probably
> tackle the question now.  Adding a mLinearRGB flag to
> nsSVGFilterInstance::ImageEntry and only doing the conversion when necessary
> would probably be the easiest thing.

Done.

> >+      PRUint32 index = stride * y + 4 * x;
> 
> Micro-optimization which hopefully the compiler would do itself, but doing by
> hand neatens the code somewhat: "PRUint8 *pixel = data + stride * y + 4 * x;",
> then use pixel[0-3].

Done.

> I think you need to remultiply the alpha once you've done the mapping.

Split into separate functions. I suppose for maximum efficienct we could do with implementing a function processes a rect within an image and which takes a converter function argument. Then pass in different converter functions. This could be more generally applied to make some/all of the filters just implement the converter function bit and then pass that to the processor function but I'm just trying to keep things simple for now.

> Don't you need a cast (or just specifying "a" as a PRUint32) to stop
> "glinearRGBTosRGBMap[...] * a" from wrapping?

I don't think the cast is necessary but I added it anyway.

Comment 11

[Robert Longson [:longsonr]]

Attachment: add simple premultiply optimisation

Comment 12

[tor]

Comment on attachment 252763 [details] [diff] [review]
add simple premultiply optimisation

Looks good.  A few minor items noted below.

> NS_IMETHODIMP_(PRBool)
> nsSVGFilterElement::IsAttributeMapped(const nsIAtom* name) const
> {
>   static const MappedAttributeEntry* const map[] = {
>     sFEFloodMap,
>+    sFiltersMap,
>     sFontSpecificationMap,
>     sGradientStopMap,
>     sMarkersMap,
>     sTextContentElementsMap,
>     sViewportsMap
>   };

sFiltersMap also need to be added to all the elements that make up %PresentationAttributes-All (Appendix M) - you've got some but not all (<use>, <g>, <marker> to name a few).  Here's where my presentation attribute consolidation patch would be handy.  :)

>+nsSVGFilterInstance::ColorModel
>+nsSVGFE::ColorModel(PRBool aAlphaPremultiplied)

Could you rename that to GetColorModel()?

>+  class ColorModel {
>+  public:
>+    enum ColorSpace { SRGB, LINEAR_RGB };
>+
>+    ColorModel(ColorSpace aColorSpace, PRBool aAlphaPremultiplied) :
>+      mColorSpace(aColorSpace), mAlphaPremultiplied(aAlphaPremultiplied) {}

Could you switch the second arg into another enum, to make code that uses ColorModel self-documenting?

>+void
>+nsSVGUtils::UnPremultiplyImageDataAlpha(PRUint8 *data, 
>+                                        PRInt32 stride,
>+                                        const nsRect &rect)
>+{
>+  for (PRInt32 y = rect.y; y < rect.YMost(); y++) {
>+    for (PRInt32 x = rect.x; x < rect.XMost(); x++) {
>+      PRUint8 *pixel = data + stride * y + 4 * x;
>+
>+      PRUint8 a = pixel[3];
>+      if (a == 0) {
>+        pixel[0] = 0;
>+        pixel[1] = 0;
>+        pixel[2] = 0;
>+      } else if (a != 255) {
>+        pixel[0] = (255 * (PRUint32)pixel[0]) / a;
>+        pixel[1] = (255 * (PRUint32)pixel[1]) / a;
>+        pixel[2] = (255 * (PRUint32)pixel[2]) / a;

Don't need to cast here, as constants are sized at the smallest an int (ansi-c spec).

>+void
>+nsSVGUtils::PremultiplyImageDataAlpha(PRUint8 *data, 
>+                                      PRInt32 stride,
>+                                      const nsRect &rect)
>+{
>+  for (PRInt32 y = rect.y; y < rect.YMost(); y++) {
>+    for (PRInt32 x = rect.x; x < rect.XMost(); x++) {
>+      PRUint8 *pixel = data + stride * y + 4 * x;
>+
>+      PRUint8 a = pixel[3];
>+      if (a != 255) {
>+        FAST_DIVIDE_BY_255(pixel[0], (PRUint32)pixel[0] * a);
>+        FAST_DIVIDE_BY_255(pixel[1], (PRUint32)pixel[1] * a);
>+        FAST_DIVIDE_BY_255(pixel[2], (PRUint32)pixel[2] * a);

Here we do need to worry about the size, but instead of casting it would look cleaner to just have "a" be of type PRUint32.

Comment 13

[Robert Longson [:longsonr]]

Attachment: address additional review comments

(In reply to comment #12)
> (From update of attachment 252763 [details] [diff] [review])

> sFiltersMap also need to be added to all the elements that make up
> %PresentationAttributes-All (Appendix M) - you've got some but not all (<use>,
> <g>, <marker> to name a few).  Here's where my presentation attribute
> consolidation patch would be handy.  :)

Your patch would make things much easier. I've added it some more classes in the mean time.

> 
> >+nsSVGFilterInstance::ColorModel
> >+nsSVGFE::ColorModel(PRBool aAlphaPremultiplied)
> 
> Could you rename that to GetColorModel()?

Done.

> 
> >+  class ColorModel {
> >+  public:
> >+    enum ColorSpace { SRGB, LINEAR_RGB };
> >+
> >+    ColorModel(ColorSpace aColorSpace, PRBool aAlphaPremultiplied) :
> >+      mColorSpace(aColorSpace), mAlphaPremultiplied(aAlphaPremultiplied) {}
> 
> Could you switch the second arg into another enum, to make code that uses
> ColorModel self-documenting?

Done.

> >+        pixel[0] = (255 * (PRUint32)pixel[0]) / a;
> >+        pixel[1] = (255 * (PRUint32)pixel[1]) / a;
> >+        pixel[2] = (255 * (PRUint32)pixel[2]) / a;
> 
> Don't need to cast here, as constants are sized at the smallest an int (ansi-c
> spec).

Done.

> 
> >+void
> >+nsSVGUtils::PremultiplyImageDataAlpha(PRUint8 *data, 
> >+                                      PRInt32 stride,
> >+                                      const nsRect &rect)
> >+{
> >+  for (PRInt32 y = rect.y; y < rect.YMost(); y++) {
> >+    for (PRInt32 x = rect.x; x < rect.XMost(); x++) {
> >+      PRUint8 *pixel = data + stride * y + 4 * x;
> >+
> >+      PRUint8 a = pixel[3];
> >+      if (a != 255) {
> >+        FAST_DIVIDE_BY_255(pixel[0], (PRUint32)pixel[0] * a);
> >+        FAST_DIVIDE_BY_255(pixel[1], (PRUint32)pixel[1] * a);
> >+        FAST_DIVIDE_BY_255(pixel[2], (PRUint32)pixel[2] * a);
> 
> Here we do need to worry about the size, but instead of casting it would look
> cleaner to just have "a" be of type PRUint32.
> 

I've changed it to PRUint32 but I don't see why a cast is required. Integral promotions should be performed on the operands as per usual arithmetic conversions in the C/C++ specs. FAST_DIVIDE_BY_255 stores its second argument in an unsigned temporary before manipulating it. See test attachment.

Comment 14

[Robert Longson [:longsonr]]

Attachment: testcase for lack of FAST_DIVIDE_BY_255 overflow

yields 200 as expected under Visual Studio.

Comment 15

[tor]

(In reply to comment #13)
> I've changed it to PRUint32 but I don't see why a cast is required. Integral
> promotions should be performed on the operands as per usual arithmetic
> conversions in the C/C++ specs. FAST_DIVIDE_BY_255 stores its second argument
> in an unsigned temporary before manipulating it. See test attachment.

All right, read the ansi/iso spec about arithmetic conversions.  I thought that the arithmetic happened at the precision of the lowest common denominator type, but the lowest type used is "int".
 

Comment 16

[Robert Longson [:longsonr]]

(In reply to comment #15)
> (In reply to comment #13)
> > I've changed it to PRUint32 but I don't see why a cast is required. Integral
> > promotions should be performed on the operands as per usual arithmetic
> > conversions in the C/C++ specs. FAST_DIVIDE_BY_255 stores its second argument
> > in an unsigned temporary before manipulating it. See test attachment.
> 
> All right, read the ansi/iso spec about arithmetic conversions.  I thought that
> the arithmetic happened at the precision of the lowest common denominator type,
> but the lowest type used is "int".
> 
> 

That's C for you, almost everything wants to be an int ;-). Do you want things as they are i.e. PRUint32 or changed to PRUint8, the current patch is PRUint32.

Comment 17

[tor]

Comment on attachment 253160 [details] [diff] [review]
address additional review comments

>+void
>+nsSVGUtils::PremultiplyImageDataAlpha(PRUint8 *data, 
>+                                      PRInt32 stride,
>+                                      const nsRect &rect)
>+{
>+  for (PRInt32 y = rect.y; y < rect.YMost(); y++) {
>+    for (PRInt32 x = rect.x; x < rect.XMost(); x++) {
>+      PRUint8 *pixel = data + stride * y + 4 * x;
>+
>+      PRUint32 a = pixel[3];

Switch to PRUint8. :)

r=tor

Comment 18

[Robert Longson [:longsonr]]

Attachment: address final review comment

Comment 19

[Robert Longson [:longsonr]]

checked in.