Bug 140812 – Apply feTurbulence spec change to fix zero length vector generation

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Nikos Andronikos]

A bug in the Filter Effects feTurbulence algorithm has recently been fixed in the spec.
Discussion:
https://lists.w3.org/Archives/Public/www-svg/2015Jan/0003.html
http://www.w3.org/2015/01/15-svg-minutes.html#item06

Updated spec:
http://dev.w3.org/fxtf/filters/#elementdef-feturbulence
Change is the addition of do while loop on these lines:
do
{ for (j = 0; j < 2; j++) fGradient[k][i][j] = (double)(((lSeed = random(lSeed)) % (BSize + BSize)) - BSize) / BSize; }

while(fGradient[k][i][0] == 0 && fGradient[k][i][1] == 0);

Test: http://jsfiddle.net/dodgeyhack/mo7x85zw/

This change is already landed in Chrome Canary and FireFox nightly.

[Nikos Andronikos]

Created attachment 245202 [details]
Patch

[Dirk Schulze]

Comment on attachment 245202 [details]
Patch

Looks good to me. I am not happy to submit it without a test. IIRC there were some test cases on public-fx or www-svg. Could you add one of these please? r- because of missing test.

[Nikos Andronikos]

Created attachment 245393 [details]
Patch

[Nikos Andronikos]

(In reply to comment #2)
> Comment on attachment 245202 [details]
> Patch
> 
> Looks good to me. I am not happy to submit it without a test. IIRC there
> were some test cases on public-fx or www-svg. Could you add one of these
> please? r- because of missing test.

Ok, done. I was a bit worried about adding a pixel test but managed to come up with a way to use a reftest for this.

[Darin Adler]

Comment on attachment 245393 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=245393&action=review

> Source/WebCore/platform/graphics/filters/FETurbulence.cpp:186
> +            do {
> +                gradient[0] = static_cast<float>((paintingData.random() % (2 * s_blockSize)) - s_blockSize) / s_blockSize;
> +                gradient[1] = static_cast<float>((paintingData.random() % (2 * s_blockSize)) - s_blockSize) / s_blockSize;
> +            } while (!gradient[0] && !gradient[1]);

Is a loop really the best way to avoid zeroes here?

[Darin Adler]

Comment on attachment 245393 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=245393&action=review

>> Source/WebCore/platform/graphics/filters/FETurbulence.cpp:186
>> +            } while (!gradient[0] && !gradient[1]);
> 
> Is a loop really the best way to avoid zeroes here?

I guess that’s what the specification calls for.

[Nikos Andronikos]

(In reply to comment #6)
> Comment on attachment 245393 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=245393&action=review
> 
> >> Source/WebCore/platform/graphics/filters/FETurbulence.cpp:186
> >> +            } while (!gradient[0] && !gradient[1]);
> > 
> > Is a loop really the best way to avoid zeroes here?
> 
> I guess that’s what the specification calls for.

It does =)
The idea is that the random distribution is maintained. Falling back to some known value would skew the result (but only very slightly in this case).

Discussion was here: https://lists.w3.org/Archives/Public/www-svg/2015Jan/0005.html

Thanks for the review.

[WebKit Commit Bot]

Comment on attachment 245393 [details]
Patch

Clearing flags on attachment: 245393

Committed r179171: <http://trac.webkit.org/changeset/179171>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.