Bug 45249 – Add cubic curve classifier

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Kenneth Russell]

As part of the GPU accelerated path rendering algorithm being added in https://bugs.webkit.org/show_bug.cgi?id=44729 , the code which classifies cubic curves needs to be integrated.

[Kenneth Russell]

Created attachment 66608 [details]
Patch

From the ChangeLog:

Adding the cubic curve classification algorithm per the GPU Gems 3 chapter. No tests yet; will be tested in conjunction with later code.

[James Robinson]

Comment on attachment 66608 [details]
Patch

Looks good overall, r- for the nits below.

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

> WebCore/platform/graphics/gpu/LoopBlinnClassifier.cpp:42
> +LoopBlinnClassifier::Result LoopBlinnClassifier::classify(const FloatPoint& c0,
> +                                                          const FloatPoint& c1,
> +                                                          const FloatPoint& c2,
> +                                                          const FloatPoint& c3)
> +{
> +    // Consult the chapter for the definitions of the following
> +    // (terse) variable names.
Could we add a link to the algorithm itself somewhere?

> WebCore/platform/graphics/gpu/LoopBlinnClassifier.cpp:74
> +    float discr = d1 * d1 * term0;
This should be discriminant.

> WebCore/platform/graphics/gpu/LoopBlinnClassifier.cpp:84
> +    d1 = roundToZero(d1);
> +    d2 = roundToZero(d2);
> +    d3 = roundToZero(d3);
> +    discr = roundToZero(discr);
Have these declare the namespace explicitly.

> WebCore/platform/graphics/gpu/LoopBlinnClassifier.h:63
> +        float d1() const { return m_d1; }
> +        float d2() const { return m_d2; }
> +        float d3() const { return m_d3; }
Please add a comment indicating what these mean.

[Kenneth Russell]

Created attachment 66984 [details]
Revised patch

Addressed code review feedback above.

[Kenneth Russell]

Comment on attachment 66608 [details]
Patch

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

> WebCore/platform/graphics/gpu/LoopBlinnClassifier.cpp:42
> +    // (terse) variable names.
Added link in the header.

> WebCore/platform/graphics/gpu/LoopBlinnClassifier.cpp:74
> +    float discr = d1 * d1 * term0;
Done.

> WebCore/platform/graphics/gpu/LoopBlinnClassifier.cpp:84
> +    discr = roundToZero(discr);
Now explicitly using the functions from LoopBlinnMathUtils.

> WebCore/platform/graphics/gpu/LoopBlinnClassifier.h:63
> +        float d3() const { return m_d3; }
Done.

[James Robinson]

Comment on attachment 66984 [details]
Revised patch

Cool!  R=me

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

> WebCore/platform/graphics/gpu/LoopBlinnClassifier.cpp:90
> +    if (approxEqual(b0, b1)
> +        && approxEqual(b0, b2)
> +        && approxEqual(b0, b3)) {
nit: this could be on one line

[Kenneth Russell]

Created attachment 67074 [details]
Revised patch

It turns out that while converting this code to use FloatPoint3D I introduced a regression; the 2D points are considered to be homogeneous, meaning that during conversion to 3D their "z" coordinate (actually the "w" coordinate in an (x, y, w) triplet) needs to be set to 1.0. Please re-review. Also fixed formatting nit above.

[WebKit Review Bot]

Attachment 67074 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/gpu/LoopBlinnClassifier.cpp:92:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Robinson]

Comment on attachment 67074 [details]
Revised patch

The style error is legit, please fix.

Otherwise R=me

[Kenneth Russell]

(In reply to comment #8)
> (From update of attachment 67074 [details])
> The style error is legit, please fix.

Will fix upon landing.

[Kenneth Russell]

Committed r67103: <http://trac.webkit.org/changeset/67103>