1366027 - setLineDash draws box taller than it should

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Ori Avtalion (salty-horse)]

Attachment: HTML file demonstrating the bug

The following code draws a red dashed stroke box (starting off screen), and a black filled box.

The two boxes should not touch. When setLineDash() isn't called, they do not touch.

On Firefox nightly, they do touch.
The same code on Chrome renders correctly.

The bug is also in Firefox 53.

The bug is sensitive to at least some of the values in the example.

If the call to setLineDash is changed from [8, 4] to [4, 2], the boxes are drawn apart.

If the red box's height becomes shorter, but so it will have the same bottom position (Y = -147 and height = 140 changed to Y = -137 and height = 130), the boxes are drawn apart.

Here's the code, also attached as an HTML file.

canvas = document.createElement('canvas');
document.body.appendChild(canvas);
ctx = canvas.getContext('2d');

canvas.width = 1000;
canvas.height = 1000;
ctx.translate(427.31465167323177, 41.531991340689785);
ctx.scale(3.332278481012659, 3.332278481012659);

ctx.strokeStyle = 'red';
ctx.lineWidth = 1;
ctx.setLineDash([8, 4]);
ctx.strokeRect(5, -147, 20, 140);
ctx.fillStyle = 'black';
ctx.fillRect(5, 5, 10, 10);

Comment 1

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Hi Kevin,
We probably have some problem to send the 2d draw command to the backend. Ethan has some experience on handling the wrong canvas drawing.

Comment 2

[Ori Avtalion (salty-horse)]

I first noticed the bug while programming panning and zooming with mouse controls.
It only happened at certain zoom levels, and certain pan values. If I panned by a few pixels, it flickered on and off.

Comment 3

[Kevin Chen[:kechen] (UTC + 8)]

This is a regression of Bug 1342571, the position of rect changed after ShrinkClippedStrokedRect[1].
This test case is displayed correctly in Windows with Direct2D1.1 backend.
Lee, do you have any thought to this bug?

[1] https://dxr.mozilla.org/mozilla-central/rev/74566d5345f4cab06c5683d4b620124104801e65/gfx/2d/DrawTargetSkia.cpp#887

Comment 4

[Kevin Chen[:kechen] (UTC + 8)]

It is a Floating-Point Precision problem in function ShrinkClippedStrokedRect, I will upload a patch to fix this.

Comment 5

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: Bug 1366027 - Calculate shrink clipped rect with double precision;

Review commit: https://reviewboard.mozilla.org/r/141722/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/141722/

Comment 6

[Kevin Chen[:kechen] (UTC + 8)]

Comment on attachment 8870266 [details]
Bug 1366027 - Calculate shrink clipped rect with double precision;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141722/diff/1-2/

Comment 7

[Lee Salzman [:lsalzman]]

Comment on attachment 8870266 [details]
Bug 1366027 - Calculate shrink clipped rect with double precision;

https://reviewboard.mozilla.org/r/141722/#review145608

::: gfx/2d/DrawTargetSkia.cpp:866
(Diff revision 2)
>    }
>  
>    // Reduce the rectangle side lengths in multiples of the dash period length
>    // so that the visible dashes stay in the same place.
> -  Margin insetBy = aStrokedRect - intersection;
> +  MarginDouble insetBy = strokedRectDouble - intersection;
>    insetBy.top = RoundDownToMultiple(insetBy.top, dashPeriodLength);

You need to fix RoundDownToMultiple to take double and floor instead of Float and floorf. Or otherwise using a MarginDouble becomes somewhat useless.

Comment 8

[Kevin Chen[:kechen] (UTC + 8)]

Comment on attachment 8870266 [details]
Bug 1366027 - Calculate shrink clipped rect with double precision;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141722/diff/2-3/

Comment 9

[Lee Salzman [:lsalzman]]

Comment on attachment 8870266 [details]
Bug 1366027 - Calculate shrink clipped rect with double precision;

https://reviewboard.mozilla.org/r/141722/#review146006

Comment 10

[Kevin Chen[:kechen] (UTC + 8)]

Here is the try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0f35e73543989dabaa2e0d8900d504819c2dbff

Comment 11

[Lee Salzman [:lsalzman]]

(In reply to Kevin Chen[:kechen] (UTC + 8) from comment #10)
> Here is the try result:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=b0f35e73543989dabaa2e0d8900d504819c2dbff

Just noticed something. "Double" is not actually a type defined anywhere in our codebase and is thus leaking in from somewhere accidentally. Please just use "double" instead.

Comment 12

[Lee Salzman [:lsalzman]]

(In reply to Lee Salzman [:lsalzman] from comment #11)
> (In reply to Kevin Chen[:kechen] (UTC + 8) from comment #10)
> > Here is the try result:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=b0f35e73543989dabaa2e0d8900d504819c2dbff
> 
> Just noticed something. "Double" is not actually a type defined anywhere in
> our codebase and is thus leaking in from somewhere accidentally. Please just
> use "double" instead.

Oops, long day, and my brain is dead. I did not see your typedef. Nevermind me. :(

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c8b6b986ca5
Calculate shrink clipped rect with double precision; r=lsalzman

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2c8b6b986ca5