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.

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.

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 on attachment 246704 [details] [diff] [review] Patch B Wrong bug...

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 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 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

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.

(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.

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 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.

Fixed xor_ atom definition

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

(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.

(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...

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

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

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