361070 - Implement feBlend and feComposite

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Alex Menzies]

Upcoming patch implements feBlend and feComposite.  Currently, using feComposite with the arithmetic operator produces results that are inconsistent with the behavior of Batik and the Adobe plugin.  Perhaps this is due to linear vs. nonlinear colorspaces?

Please note that the result is consistent with Opera's behavior except that opera renders in1 and in2 in a different order than all the other viewers.

Comment 1

[Alex Menzies]

Attachment: Blend testcase

Comment 2

[Alex Menzies]

Attachment: Composite Testcase

Comment 3

[Alex Menzies]

Attachment: Patch A

Comment 4

[tor]

For feBlend, it seems it would be fairly straightfoward to keep everything in integer calculations.  Make {qc}{ab} be PRUint32 so that you have enough bits for the calcuation, replace 1 with 255 (or 255*255 for alpha) in the calculations, and use FAST_DIVIDE_BY_255 for the scaling to [0-255] for the final assignment.

feComposite could probably be done likewise by scaling up k1-4 by 255, though you'd need to carefully use signed quantities in that method.

Comment 5

[Alex Menzies]

Attachment: Patch using Integer Arithmetic

This patch uses integer arithmetic for both the blend and composite filters.  However, given the complexity of the overflow detection code, it is probably best to implement the composite filter using floats instead.  I have posted this code because it contains an example of how integer overflow can be detected using bitwise operations which others may find useful.

Comment 6

[Alex Menzies]

Attachment: Patch B

Comment 7

[tor]

Comment on attachment 246704 [details] [diff] [review]
Patch B

Wrong bug...

Comment 8

[tor]

Comment on attachment 246704 [details] [diff] [review]
Patch B

>+NS_IMETHODIMP
>+nsSVGFEBlendElement::Filter(nsSVGFilterInstance *instance)
>+{
...
>+#ifdef DEBUG_tor
>+  fprintf(stderr, "FILTER GAUSS rect: %d,%d  %dx%d\n",
>+          rect.x, rect.y, rect.width, rect.height);
>+#endif

s/GAUSS/BLEND/

>+  for (PRInt32 x = rect.x; x < rect.x + rect.width; x++) {
>+    for (PRInt32 y = rect.y; y < rect.y + rect.height; y++) {
>+      PRUint32 targIndex = y * stride + 4 * x;
>+      PRUint32 qa = targetData[targIndex + 3];
>+      PRUint32 qb = sourceData[targIndex + 3];
>+      for (PRInt32 i = 0; i < 3; i++) {
>+        PRUint32 ca = targetData[targIndex + i];
>+        PRUint32 cb = sourceData[targIndex + i];
>+        PRUint32 val;
>+        switch (mode) {
>+          case nsSVGFEBlendElement::SVG_MODE_NORMAL:
>+            val = (255 - qa) * cb / 255 + ca;

FAST_DIVIDE_BY_255(val, (255 - qa) * cb);
val += ca;

>+            break;
>+          case nsSVGFEBlendElement::SVG_MODE_MULTIPLY:
>+            val = ((255 - qa) * cb + (255 - qb + cb) * ca);
>+            FAST_DIVIDE_BY_255(val, val);

Strike FAST_DIVIDE_BY_255

>+            break;
>+          case nsSVGFEBlendElement::SVG_MODE_SCREEN:
>+            val = cb + ca - ca * cb / 255;

val = 255 * (cb + ca) + ca * cb;

>+            break;
>+          case nsSVGFEBlendElement::SVG_MODE_DARKEN:
>+            val = PR_MIN((255 - qa) * cb / 255 + ca,
>+                         (255 - qb) * ca / 255 + cb);

val = PR_MIN((255 - qa) * cb + 255 * ca,
             (255 - qb) * ca + 255 * cb);

>+            break;
>+          case nsSVGFEBlendElement::SVG_MODE_LIGHTEN:
>+            val = PR_MAX((255 - qa) * cb / 255 + ca,
>+                         (255 - qb) * ca / 255 + cb);

val = PR_MAX((255 - qa) * cb + 255 * ca,
             (255 - qb) * ca + 255 * cb);

>+            break;
>+          default:
>+            return NS_ERROR_FAILURE;
>+            break;
>+        }
>+        targetData[targIndex + i] = NS_STATIC_CAST(PRUint8, val);

FAST_DIVIDE_BY_255(targetData[targIndex + i], val)

>+      }
>+      PRUint32 alpha = 255 * 255 - (255 - qa) * (255 - qb);
>+      FAST_DIVIDE_BY_255(alpha, alpha);
>+      targetData[targIndex + 3] = NS_STATIC_CAST(PRUint8, alpha);

Combine last two lines into one step?

>+    }
>+  }
>+  return NS_OK;
>+}

>+NS_IMETHODIMP
>+nsSVGFECompositeElement::Filter(nsSVGFilterInstance *instance)
>+{
...
>+  // CAIRO_OPERATOR_OVER is used by default
>+  if (op == nsSVGFECompositeElement::SVG_OPERATOR_IN) {
>+    cairo_set_operator(cr, CAIRO_OPERATOR_IN);
>+  } else if (op == nsSVGFECompositeElement::SVG_OPERATOR_OUT) {
>+    cairo_set_operator(cr, CAIRO_OPERATOR_OUT);
>+  } else if (op == nsSVGFECompositeElement::SVG_OPERATOR_ATOP) {
>+    cairo_set_operator(cr, CAIRO_OPERATOR_ATOP);
>+  } else if (op == nsSVGFECompositeElement::SVG_OPERATOR_XOR) {
>+    cairo_set_operator(cr, CAIRO_OPERATOR_XOR);
>+  } else if (op != nsSVGFECompositeElement::SVG_OPERATOR_OVER) {
>+    cairo_destroy(cr);
>+    return NS_ERROR_FAILURE;
>+  }

Maybe do this with a lookup table?

cairo_operator_t opMap[] =
{ CAIRO_OPERATOR_DEST, CAIRO_OPERATOR_OVER, CAIRO_OPERATOR_IN,
  CAIRO_OPERATOR_OUT, CAIRO_OPERATOR_ATOP, CAIRO_OPERATOR_XOR };
cairo_set_operator(opMap[op]);

Comment 9

[Alex Menzies]

Attachment: Patch C

Comment 10

[tor]

Comment on attachment 246987 [details] [diff] [review]
Patch C

>+nsresult
>+nsSVGFEBlendElement::Init()
>+{
...
>+    nsCOMPtr<nsISVGEnum> modes;
>+    rv = NS_NewSVGEnum(getter_AddRefs(modes),
>+                       nsSVGFEBlendElement::SVG_MODE_LIGHTEN,
>+                       gModeTypes);

Default should be NORMAL.

>+NS_IMETHODIMP
>+nsSVGFEBlendElement::Filter(nsSVGFilterInstance *instance)
>+{
...
>+      PRUint32 alpha = 255 * 255 - (255 - qa) * (255 - qb);
>+      targetData[targIndex + 3] = NS_STATIC_CAST(PRUint8, alpha / 255);

The alpha calculation appears to be within the range of FAST_DIVIDE, so this should be changed to use that.

With those changes, r=tor.

Comment 11

[Alex Menzies]

Attachment: Patch D

Comment 12

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

Comment on attachment 247002 [details] [diff] [review]
Patch D

+  for (PRInt32 x = rect.x; x < rect.x + rect.width; x++) {
+    for (PRInt32 y = rect.y; y < rect.y + rect.height; y++) {

Use XMost() and YMost()

+        targetData[targIndex + i] =  NS_STATIC_CAST(PRUint8, val / 255);

Use FAST_DIVIDE_BY_255 here

+//----------------------------------------------------------------------
+// nsIDOMSVGFEGaussianBlurElement methods
+
+/* readonly attribute nsIDOMSVGAnimatedString in1; */
+NS_IMETHODIMP nsSVGFECompositeElement::GetIn1(nsIDOMSVGAnimatedString * *aIn)

Bad comment in the second line?

+    for (PRInt32 x = rect.x; x < rect.x + rect.width; x++) {
+      for (PRInt32 y = rect.y; y < rect.y + rect.height; y++) {

XMost()/YMost() again

Comment 13

[Alex Menzies]

Attachment: Patch E

Patch implements roc's changes with one exception.  I have not used FAST_DIVIDE_BY_255 in the line:

targetData[targIndex + i] =  NS_STATIC_CAST(PRUint8, val / 255);

because I have found experimentally that FAST_DIVIDE_BY_255 is only accurate when v < 255*256.  When v >= 255*256, FAST_DIVIDE_BY_255 sometimes returns a value that is 1 less than the correct result.

Comment 14

[tor]

(In reply to comment #13)
> Created an attachment (id=247433) [edit]
> Patch E
> 
> Patch implements roc's changes with one exception.  I have not used
> FAST_DIVIDE_BY_255 in the line:
> 
> targetData[targIndex + i] =  NS_STATIC_CAST(PRUint8, val / 255);
> 
> because I have found experimentally that FAST_DIVIDE_BY_255 is only accurate
> when v < 255*256.  When v >= 255*256, FAST_DIVIDE_BY_255 sometimes returns a
> value that is 1 less than the correct result.  

If val can exceed 255*255, you need to use PR_MAX (and PR_MIN?) appropriately to clamp the result.
 

Comment 15

[Alex Menzies]

Attachment: Patch F

Patch clamps val to an upper bound of 255.  None of the blend equations can produce a negative number so clamping to 0 is unnecessary.

Comment 16

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

Comment on attachment 247457 [details] [diff] [review]
Patch F

+          float result = k1*i1*i2/255.0f + k2*i1 + k3*i2 + k4*255.0f;

Does the compiler implement /255.0 using a division, or by multiplying by 1/255.0? The latter would be a lot faster. You might want to move 1/255.0 into a constant outside the loop to make sure it does the fast thing.

Comment 17

[Alex Menzies]

Attachment: Patch for checkin

Comment 18

[Alex Menzies]

Attachment: Updated Patch for checkin

Fixed xor_ atom definition

Comment 19

[tor]

For the blend testcase attached to this bug, the second column doesn't appear to be correct - all the images look the same.

Comment 20

[tor]

(In reply to comment #19)
> For the blend testcase attached to this bug, the second column doesn't appear
> to be correct - all the images look the same.

Ok, so "the same" is exaggerating a bit.  They look quite similar, much more so than the same testcase in opera.

Comment 21

[tor]

(In reply to comment #20)
> (In reply to comment #19)
> > For the blend testcase attached to this bug, the second column doesn't appear
> > to be correct - all the images look the same.
> 
> Ok, so "the same" is exaggerating a bit.  They look quite similar, much more so
> than the same testcase in opera.

Though it does look the same as batik - hmm...

Comment 22

[tor]

Webkit also agrees with us, so either opera is wrong or there's a common misunderstanding of the specification.

Checked in for Alex.

Comment 23

[Mathieu Parent]

update of http://www.mozilla.org/projects/svg/status.html needed

Comment 24

[Jonathan Watt [:jwatt]]

Wow! Fixing this bug "regressed" rendering time massively for http://upload.wikimedia.org/wikipedia/en/c/ca/Video_Standards.svg :-P