Bug 67988 – Large canvas fills should not crash or create unnecessarily large image buffers

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Ben Wells]

Large canvas fills should not crash or create unnecessarily large image buffers

[Ben Wells]

Created attachment 107148 [details]
Patch

[Stephen White]

Comment on attachment 107148 [details]
Patch

Looks good.  r=me

[Jeff Timanus]

Comment on attachment 107148 [details]
Patch

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

> LayoutTests/fast/canvas/canvas-large-fills.html:5
> +<p>Tests that using reasonably large values for canvas.height and canvas.height don't cause a crash</p>

Type-o:  canvas.width

[Ben Wells]

Created attachment 107253 [details]
Patch

[Darin Adler]

Comment on attachment 107253 [details]
Patch

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

I’m concerned about the change from FloatRect to IntRect. Otherwise, the patch looks good.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1513
> +    IntRect canvasRect(0, 0, canvas()->width(), canvas()->height());
> +    canvasRect = canvas()->baseTransform().mapRect(canvasRect);

Why is it OK to change this from FloatRect to IntRect? Does the code below always work properly with an integer-rounded rect?

[Ben Wells]

> Why is it OK to change this from FloatRect to IntRect? Does the code below always work properly with an integer-rounded rect?

This rect starts in canvas space and is transformed to device space. The canvas space is integral. The coordinates in device space will also be integral, I think, but if it isn't in some cases it shouldn't be a problem - internally the transform is done with a FloatRect and then EnclosingIntRect is used to expand this if needed.

The canvasRect is used in one call later on, clearRect, which takes a FloatRect. The IntRect will be implicitly cast to a FloatRect.

If the canvasRect ends up being a little bigger than it needs to be through rounding in the transform it won't be a problem. This is used now in two ways - to limit the size of the buffer used and to clear the original canvas. If our rect is a little bigger than needed this will all still work.

[Darin Adler]

(In reply to comment #6)
> The canvas space is integral. The coordinates in device space will also be integral, I think

Could you explain why? I know that doesn’t matter for this patch, but I’m curious why you think that.

> If the canvasRect ends up being a little bigger than it needs to be through rounding in the transform it won't be a problem.

OK.

[Ben Wells]

(In reply to comment #7)
> (In reply to comment #6)
> > The canvas space is integral. The coordinates in device space will also be integral, I think
> 
> Could you explain why? I know that doesn’t matter for this patch, but I’m curious why you think that.
> 

It could be a case of a little knowledge being a dangerous thing. 

I've seen two instances of the base transform - in skia where it is the identity transform, and with core graphics where it seemed to be integral (something like flipping y and an offset if I remember correctly).

I think I have made an assumption that in general the underlying canvas image buffer will always be of integral bounds in device space (i.e. the transformed canvasRect will be integral).

Thinking about it some more for some devices with low resolution, and if the whole space is zoomed (e.g. at a browser level) the bounds not be integral but I don't know whether this is applied at this level or at a higher level (like RenderLayer).

Please, feel free to correct any misunderstandings I have just demonstrated!

[WebKit Review Bot]

Comment on attachment 107253 [details]
Patch

Rejecting attachment 107253 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ernational/danda-space.html = IMAGE
  fast/text/international/thai-baht-space.html = IMAGE
  fast/text/international/thai-line-breaks.html = IMAGE
  platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html = IMAGE

Regressions: Unexpected DumpRenderTree crashes : (1)
  css3/calc/regression-62276.html = CRASH

Regressions: Unexpected image and text mismatch : (2)
  fast/borders/border-image-scrambled.html = IMAGE+TEXT
  svg/custom/svg-fonts-word-spacing.html = IMAGE+TEXT



Full output: http://queues.webkit.org/results/9657356

[WebKit Review Bot]

Comment on attachment 107253 [details]
Patch

Rejecting attachment 107253 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Mike Lawther]

Comment on attachment 107253 [details]
Patch

setting cq+ after Darin's r+

[Ben Wells]

Created attachment 107282 [details]
Patch

[Ben Wells]

Comment on attachment 107282 [details]
Patch

Fixed test, I had not updated the expectations after updating the comment in the html.

[noel gordon]

Ok, lets try it again.

[Darin Adler]

(In reply to comment #8)
> I've seen two instances of the base transform - in skia where it is the identity transform, and with core graphics where it seemed to be integral (something like flipping y and an offset if I remember correctly).

What you’ve described is probably right for the baseTransform of the image buffer. 

But if you look at the implementation of HTMLCanvasElement::baseTransform function you’ll see that the transform can easily be non-integral if there is a non-integral ratio between the size of the canvas (determined by the height and width attributes) and the size of the place where the canvas element is laid out on the page (determined by CSS  height and width).

> I think I have made an assumption that in general the underlying canvas image buffer will always be of integral bounds in device space (i.e. the transformed canvasRect will be integral).

It's an incorrect assumption.

[Ben Wells]

(In reply to comment #15)
> (In reply to comment #8)
> > I've seen two instances of the base transform - in skia where it is the identity transform, and with core graphics where it seemed to be integral (something like flipping y and an offset if I remember correctly).
> 
> What you’ve described is probably right for the baseTransform of the image buffer. 
> 
> But if you look at the implementation of HTMLCanvasElement::baseTransform function you’ll see that the transform can easily be non-integral if there is a non-integral ratio between the size of the canvas (determined by the height and width attributes) and the size of the place where the canvas element is laid out on the page (determined by CSS  height and width).
> 
> > I think I have made an assumption that in general the underlying canvas image buffer will always be of integral bounds in device space (i.e. the transformed canvasRect will be integral).
> 
> It's an incorrect assumption.

Thanks for clarifying. I can see now there are four coordinate spaces in action: transformed canvas, untransformed canvas, page (or css) and device. I wasn't considering the page / css space.

Based on your explanation I can see some areas of the code for me to understand better, and some cases to test to make sure these fill modes are working as expected - in particular when the canvas space and page / css spaces aren't 1:1.

[WebKit Review Bot]

Comment on attachment 107282 [details]
Patch

Clearing flags on attachment: 107282

Committed r95080: <http://trac.webkit.org/changeset/95080>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Gabor Rapcsanyi]

Reopen, because it was rolled out by http://trac.webkit.org/changeset/95084

See https://bugs.webkit.org/show_bug.cgi?id=68070 for details.

[Ben Wells]

Created attachment 107443 [details]
Patch

[Ben Wells]

Comment on attachment 107443 [details]
Patch

The previous patch caused some tests to fail on CG. Specifically when the region being filled did not intersect with the canvas.

I believe this is because clipOut in the CG graphics context clips out the entire context, if called with an empty rect.

This version of the patch optimizes out this case as we just have to clear the canvas.

[Ben Wells]

As I wrote that last comment it sounded implausible. I'm doing more testing to verify this or if its something else figure out what is happening.

[Ben Wells]

Created attachment 107445 [details]
Patch

[Ben Wells]

Comment on attachment 107445 [details]
Patch

The earlier statement was incorrect, it is not clipOut acting differently but ImageBuffer::create. On CG this returns null if the requested buffer size is empty.

Change log has been updated to reflect this.

[Ben Wells]

Created attachment 107448 [details]
Patch

[Ben Wells]

Comment on attachment 107448 [details]
Patch

Change log updated and test formatting changed based on feedback.

[Darin Adler]

Comment on attachment 107448 [details]
Patch

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

I’m going to say review+, but I think it would be much better to post the early return version.

> LayoutTests/fast/canvas/canvas-large-fills-expected.txt:7
> +source-over
> +PASS Actual: 0 Expected: 0
> +PASS Actual: 0 Expected: 0
> +PASS Actual: 255 Expected: 255

This test output is pretty good. I think it would be even better to check these as a single value; less verbose and easier to understand. The test can convert them to hex form or some other easy to understand form. More like:

    PASS: source-over: #0000FF

That would cut the number of lines of output by 4 and make the test clearer.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1518
> +    if (!bufferRect.isEmpty()) {

In the WebKit project our usual style is to use “early return”. If we do that here, a side benefit is that the patch will be smaller. Like this:

    if (bufferRect.isEmpty()) {
        clearCanvas();
        return;
    }

This makes it so we don’t have the whole function indented.

[Matthew Delaney]

Comment on attachment 107448 [details]
Patch

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

> LayoutTests/fast/canvas/canvas-large-fills-expected.txt:8
> +source-in

Totally agree with Darin. The output text should optimize the time it takes to find out exactly what the failures are - if there are any. So, I'd say one step further would be to only output failures and if there are none, simply have one "PASS" line and that's it. This minimizes the burden on someone who finds the test failing on the bots or when run locally.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1516
> +    bufferRect.intersect(canvasRect);

Do we need to normalize the bufferRect here (since intersect assumes it is)?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1521
> +        RenderingMode renderMode = canvas()->buffer()->isAccelerated() ? Accelerated : Unaccelerated;

FYI, this line changed yesterday.

[Darin Adler]

(In reply to comment #28)
> Totally agree with Darin. The output text should optimize the time it takes to find out exactly what the failures are - if there are any.

Glad we agree on this.

> So, I'd say one step further would be to only output failures and if there are none, simply have one "PASS" line and that's it.

I don’t agree with this. I do not like tests that just say PASS.

[Ben Wells]

Created attachment 107593 [details]
Patch

[Ben Wells]

Comment on attachment 107593 [details]
Patch

This patch has added the early exit. This makes the code much nicer and also makes the diff cleaner.

The test output has been trimmed, and now doesn't need scrolling when showing up in test results. I've compromised with the test output: passing lines are all shown, but failure lines have extra text (...EXPECTED x, GOT y versus just ...x) so they stand out. The javascript for the test has also been cleaned up based on offline feedback to be more readable.

About the normalised rectangles: as long as Path.boundingRect returns a normalised rect this code should be fine. Looking at the skia implementation this will be the case; I cannot tell for sure about CG. Is this a safe assumption to make? From my reading of the SVG path code (rendering/svg/RenderSVGPath) it also makes this assumption.

[noel gordon]

cq+ following Darin's r+

[Ben Wells]

Comment on attachment 107593 [details]
Patch

Taking of cq+ as the queue has been stuck for hours. Will try putting through the queue again tomorrow.

[WebKit Review Bot]

Comment on attachment 107593 [details]
Patch

Clearing flags on attachment: 107593

Committed r95346: <http://trac.webkit.org/changeset/95346>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.