Bug 54491 – [cairo][canvas] Drawing from/into float rectangles with width or height in range 0 to 1 fails

• Component: Canvas
• Status: RESOLVED FIXED

[Zan Dobersek]

This bug covers the failure and fix for test canvas/philip/tests/2d.drawImage.floatsource.html.

Failure occurs when context draws image from source float rectangle (x:10.1, y:10.1, width:0.1, height:0.1).
The x and y coordinates both properly round to 10, but width and height are both rounded to 0, which results in this test not passing.

Rounding algorithms should be adjusted to correctly round width and height in such cases.

[Zan Dobersek]

Created attachment 82515 [details]
Adjust rounding steps to ensure minimal width or height in such cases

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:768:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:769:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2 in 3 files


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

[Zan Dobersek]

Created attachment 82631 [details]
Adjust rounding steps to ensure minimal width or height in such cases

Updated patch that no longer fails style checks.

[Martin Robinson]

Comment on attachment 82631 [details]
Adjust rounding steps to ensure minimal width or height in such cases

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

Great stuff. Just a bit more cleanup and I think this is a definite r+.

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:755
> -    double x = frect.x();
> -    double y = frect.y();
> +    double x, y;
> +
>      cairo_t* cr = m_data->cr;
> +
> +    x = frect.x();
> +    y = frect.y();

I think you don't need to make this change.

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:766
>      x = frect.width();
>      y = frect.height();

Please create two more variables here width and height. This code needs a little cleanup and the memory savings are not worth the rampant chaos. :)

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:769
> +    x = !round(x) ? (!x ? 0.0f : (x > 0 ? 1.0f : -1.0f)) : round(x);
> +    y = !round(y) ? (!y ? 0.0f : (y > 0 ? 1.0f : -1.0f)) : round(y);

Instead of rounding twice, I'd rather this be cached.

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:772
>      result.setWidth(static_cast<float>(x));
>      result.setHeight(static_cast<float>(y));

It might be more appropriate to do narrowToFloatPrecision here. I'm not sure.

[Zan Dobersek]

Created attachment 82971 [details]
Updated patch

Updated patch.

Uses narrowFloatToPrecision and makes variables declared where assigned.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:770:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:771:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2 in 3 files


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

[Zan Dobersek]

Created attachment 82972 [details]
Style-compliant patch

The same patch as before, but now style-compliant!

[Martin Robinson]

Comment on attachment 82972 [details]
Style-compliant patch

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

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:771
> +    double rwidth = round(width);
> +    double rheight = round(height);
> +    width = !rwidth ? (!width ? 0 : (width > 0 ? 1 : -1)) : rwidth;
> +    height = !rheight ? (!height ? 0 : (height > 0 ? 1 : -1)) : rheight;

Is it okay to simply write this as:
if (width < 0 && width > -1)
    width = -1;
if (width > 0 && width < 1)
    width = 1;
if (height < 0 && height > -1)
    height = -1;
if (height > 0 && height < 1)
    height = 1;

?

[Zan Dobersek]

Comment on attachment 82972 [details]
Style-compliant patch

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

>> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:771
>> +    height = !rheight ? (!height ? 0 : (height > 0 ? 1 : -1)) : rheight;
> 
> Is it okay to simply write this as:
> if (width < 0 && width > -1)
>     width = -1;
> if (width > 0 && width < 1)
>     width = 1;
> if (height < 0 && height > -1)
>     height = -1;
> if (height > 0 && height < 1)
>     height = 1;
> 
> ?

A shorter hack that should work goes like this, only shown on width:

width = frect.width();
...
rwidth = round(frect.width());
if (!rwidth && width) // width rounds to zero, but is not zero itself
    rwidth = width/fabs(width) // This returns either -1 or 1, depends on the sign of the number value
...
result.setWidth(narrowPrecisionToFloat(rwidth));

It's shorter, and it works also.
So, there are a few options, what to go with?

[Martin Robinson]

(In reply to comment #9)
> (From update of attachment 82972 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82972&action=review
> It's shorter, and it works also.
> So, there are a few options, what to go with?

I think we should go for two things in this section: execution speed and clarity. If this is not a particularly hot function, we should go mostly for clarity. If this is a hot function we should maximize execution speed and if there are some particularly low hanging speedups we can apply, we should be sure to comment them thorougly.

[Zan Dobersek]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 82972 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=82972&action=review
> > It's shorter, and it works also.
> > So, there are a few options, what to go with?
> 
> I think we should go for two things in this section: execution speed and clarity. If this is not a particularly hot function, we should go mostly for clarity. If this is a hot function we should maximize execution speed and if there are some particularly low hanging speedups we can apply, we should be sure to comment them thorougly.

I've ran the three options through a benchmark. The four if statements proved to be the fastest, executing in half the time than the other two options.

They also offer the most clarity, so I'll be updating the patch to use this option and uploading it tomorrow.

[Martin Robinson]

(In reply to comment #11)
> I've ran the three options through a benchmark. The four if statements proved to be the fastest, executing in half the time than the other two options.
> They also offer the most clarity, so I'll be updating the patch to use this option and uploading it 
tomorrow.

Great! Thanks for your work here.

[Zan Dobersek]

Created attachment 83060 [details]
Updated patch

Updated patch, now if statements are used, faster and more clearer.

[WebKit Commit Bot]

Comment on attachment 83060 [details]
Updated patch

Clearing flags on attachment: 83060

Committed r79129: <http://trac.webkit.org/changeset/79129>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.