Bug 100026 – inconsistency in drawImage with target rect negative dimensions.

• Component: Canvas
• Status: RESOLVED FIXED

[arno.]

Hi,
when calling canvasContext.drawImage with either dw either dh negative, the behaviour is inconsistent:

If dw is -1, it is considered to be canvas.width but if it is -2 or below, the image is drawn from dx,dy but on the opposite side (flipped).

There are two ways to fix the inconsistency: either draw image flipped with -1 dimension, either forbids negative dimensions in destination rectangle (I tested opera and firefox, they both throw a INDEX_SIZE_ERR on that case).

[arno.]

Created attachment 169978 [details]
testcase

[arno.]

From bug #20468 and philip.html5.org tests, it looks like it's valid to call canvas with destination with negative dimension. So, the solution is to handle -1 dimension correctly.

[arno.]

Created attachment 170489 [details]
Patch

patch proposal

[Build Bot]

Comment on attachment 170489 [details]
Patch

Attachment 170489 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14559545

New failing tests:
css3/filters/null-effect-check.html

[Adam Barth]

Comment on attachment 170489 [details]
Patch

This appears to be causing many failures for the EWS bot.

[Dirk Schulze]

Comment on attachment 170489 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Remove -1, -1 special case in drawImage and drawImageBuffer. Instead,
> +        always call those functions with the correct rectangle dimensions.

If you look at the code, this special casing is done to indicate that it should use the intrinsic dimensions of the image. You can't just remove this check, since it will break half of the drawImage content in the web :)

The question stays if it should be allowed to have negative values for dh and dw or not. You say that Firefox throws an exception? What do IE and Opera do?

The specification does not disallow negative values. Should all other implementations throw, then this indicates a bug in the specification IMO.

[Dirk Schulze]

Adding Rik and Ian, the editors, to the bug. Maybe they have more input to drawImage and negative dw/dh.

[Rik Cabanier]

(In reply to comment #7)
> Adding Rik and Ian, the editors, to the bug. Maybe they have more input to drawImage and negative dw/dh.

It's obvious from the IDL and the prose that negative values should be allowed.
There should be no special behavior depending on <-1 or > -1...

[Ian 'Hixie' Hickson]

Indeed. The spec also says the image shouldn't be flipped ("The image data must be processed in the original direction, even if the dimensions given are negative.").

http://whatwg.org/html/#dom-context-2d-drawimage

[Dirk Schulze]

(In reply to comment #9)
> Indeed. The spec also says the image shouldn't be flipped ("The image data must be processed in the original direction, even if the dimensions given are negative.").
> 
> http://whatwg.org/html/#dom-context-2d-drawimage

Ah! Indeed a useful clarification. (even if I would disagree :))

[Rik Cabanier]

(In reply to comment #10)
> (In reply to comment #9)
> > Indeed. The spec also says the image shouldn't be flipped ("The image data must be processed in the original direction, even if the dimensions given are negative.").
> > 
> > http://whatwg.org/html/#dom-context-2d-drawimage
> 
> Ah! Indeed a useful clarification. (even if I would disagree :))

yes, this function is a little odd in that it create a source rectangle and maps it to a destination rectangle. Any scaling that happens is because the rectangles are difference size.

There might be a bug here though. What happens if the current transformation matrix has scaling or skewing?

[Dirk Schulze]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > Indeed. The spec also says the image shouldn't be flipped ("The image data must be processed in the original direction, even if the dimensions given are negative.").
> > > 
> > > http://whatwg.org/html/#dom-context-2d-drawimage
> > 
> > Ah! Indeed a useful clarification. (even if I would disagree :))
> 
> yes, this function is a little odd in that it create a source rectangle and maps it to a destination rectangle. Any scaling that happens is because the rectangles are difference size.
> 
> There might be a bug here though. What happens if the current transformation matrix has scaling or skewing?

You draw it into the canvas with its current CTM. That doesn't seem problematic?

[Rik Cabanier]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > (In reply to comment #9)
> > > > Indeed. The spec also says the image shouldn't be flipped ("The image data must be processed in the original direction, even if the dimensions given are negative.").
> > > > 
> > > > http://whatwg.org/html/#dom-context-2d-drawimage
> > > 
> > > Ah! Indeed a useful clarification. (even if I would disagree :))
> > 
> > yes, this function is a little odd in that it create a source rectangle and maps it to a destination rectangle. Any scaling that happens is because the rectangles are difference size.
> > 
> > There might be a bug here though. What happens if the current transformation matrix has scaling or skewing?
> 
> You draw it into the canvas with its current CTM. That doesn't seem problematic?

This sentence is problematic:
When drawImage() is invoked, the region of the image specified by the source rectangle must be painted on the region of the canvas specified by the destination rectangle, after applying the current transformation matrix to the points of the destination rectangle.

It doesn't say that the CTM is applied to the scaled bitmap; it says it's applied to the points.

[Dirk Schulze]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > (In reply to comment #10)
> > > > (In reply to comment #9)
> > > > > Indeed. The spec also says the image shouldn't be flipped ("The image data must be processed in the original direction, even if the dimensions given are negative.").
> > > > > 
> > > > > http://whatwg.org/html/#dom-context-2d-drawimage
> > > > 
> > > > Ah! Indeed a useful clarification. (even if I would disagree :))
> > > 
> > > yes, this function is a little odd in that it create a source rectangle and maps it to a destination rectangle. Any scaling that happens is because the rectangles are difference size.
> > > 
> > > There might be a bug here though. What happens if the current transformation matrix has scaling or skewing?
> > 
> > You draw it into the canvas with its current CTM. That doesn't seem problematic?
> 
> This sentence is problematic:
> When drawImage() is invoked, the region of the image specified by the source rectangle must be painted on the region of the canvas specified by the destination rectangle, after applying the current transformation matrix to the points of the destination rectangle.
> 
> It doesn't say that the CTM is applied to the scaled bitmap; it says it's applied to the points.
This is more of a spec discussion and does not really belong here. But the canvas should already be transformed when drawing. So any note about "applying a transform to something before drawing the image" is unnecessary IMO.

[arno.]

Created attachment 171001 [details]
patch v2

[arno.]

(In reply to comment #6)
> (From update of attachment 170489 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170489&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Remove -1, -1 special case in drawImage and drawImageBuffer. Instead,
> > +        always call those functions with the correct rectangle dimensions.
> 
> If you look at the code, this special casing is done to indicate that it should use the intrinsic dimensions of the image. You can't just remove this check, since it will break half of the drawImage content in the web :)

Actually, I do not just remove the test, I also make sure to call drawImage with intrincis dimensions of the image.

(In reply to comment #5)
> (From update of attachment 170489 [details])
> This appears to be causing many failures for the EWS bot.

But I had indeed forgotten some places where drawImage is called with -1 -1

The updated patch  only address the -1 -1 inconsistency, not the fact that rectangle should not be flipped.

[Ian 'Hixie' Hickson]

Re comment 13, please file a bug at http://whatwg.org/newbug — there's a bunch of places in the spec where it talks about transforming points rather than shapes and I am not really sure what the right wording really should be, but we should fix them, so please do report them as you find them.

[WebKit Review Bot]

Comment on attachment 171001 [details]
patch v2

Attachment 171001 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14617166

New failing tests:
svg/filters/feComposite.svg
svg/repaint/filter-repaint.svg
svg/W3C-SVG-1.1/filters-composite-02-b.svg

[Rik Cabanier]

Comment on attachment 171001 [details]
patch v2

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

> Source/WebCore/platform/graphics/GraphicsContext.cpp:442
> +    if (!image)

Why is this check here?

> Source/WebCore/platform/graphics/GraphicsContext.cpp:449
> +    if (!image)

ditto

> Source/WebCore/platform/graphics/GraphicsContext.cpp:466
> +    if (!image)

ditto

> Source/WebCore/platform/graphics/GraphicsContext.cpp:-478
> -        th = image->height();

This is a huge change.
Are you sure that other parts of the WebKit code don't rely on this behavior (ie CSS Filters)?

> Source/WebCore/platform/graphics/GraphicsContext.cpp:531
> +    if (!image)

Why this check?

[Dirk Schulze]

Comment on attachment 171001 [details]
patch v2

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

Looks quite good already. I am afraid, but you need to check the caller site as well. There are quite some places that assume that we have the -1, -1 checks.

>> Source/WebCore/platform/graphics/GraphicsContext.cpp:442
>> +    if (!image)
> 
> Why is this check here?

This is from the last overload function. There you have !image check. And the image is accessed now, tho check will be necessary. So I assume we can't be sure that image is not null? I would rather replace the if(!image) conditions with assertions. Should be wrong in the first place to call the function with null. Can you investigate?

> Source/WebCore/platform/graphics/GraphicsContext.cpp:444
> +    drawImage(image, styleColorSpace, p, IntRect(IntPoint(0, 0), image->size()), op, shouldRespectImageOrientation);

IntPoint() is enough. Please fix this on all functions.

>> Source/WebCore/platform/graphics/GraphicsContext.cpp:-478
>> -        th = image->height();
> 
> This is a huge change.
> Are you sure that other parts of the WebKit code don't rely on this behavior (ie CSS Filters)?

This actually looks correct.

[Dirk Schulze]

(In reply to comment #20)
> (From update of attachment 171001 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171001&action=review
> 
> Looks quite good already. I am afraid, but you need to check the caller site as well. There are quite some places that assume that we have the -1, -1 checks.
> 
> >> Source/WebCore/platform/graphics/GraphicsContext.cpp:442
> >> +    if (!image)
> > 
> > Why is this check here?
> 
> This is from the last overload function. There you have !image check. And the image is accessed now, tho check will be necessary. So I assume we can't be sure that image is not null? I would rather replace the if(!image) conditions with assertions. Should be wrong in the first place to call the function with null. Can you investigate?
> 
> > Source/WebCore/platform/graphics/GraphicsContext.cpp:444
> > +    drawImage(image, styleColorSpace, p, IntRect(IntPoint(0, 0), image->size()), op, shouldRespectImageOrientation);
> 
> IntPoint() is enough. Please fix this on all functions.
> 
> >> Source/WebCore/platform/graphics/GraphicsContext.cpp:-478
> >> -        th = image->height();
> > 
> > This is a huge change.
> > Are you sure that other parts of the WebKit code don't rely on this behavior (ie CSS Filters)?
> 
> This actually looks correct.

FEComposite.cpp is just one class that assumes the old behavior for drawImage with -1, -1.

[arno.]

(In reply to comment #19)

> Are you sure that other parts of the WebKit code don't rely on this behavior (ie CSS Filters)?

There is indeed FEComposite, which was trapped by chromium tests. So, I've looked in all files containing -1 and drawImage, and I've found ImageGStreamer::crop also returns a -1 -1 rectangle. In theory, I may now miss some calls if: the -1 -1 argument is not set in the same file as the drawImage call AND it is not trapped in a test. I don't known how likely it is.(In reply to comment #20)

(In reply to comment #20)

> This is from the last overload function. There you have !image check. And the image is accessed now, tho check will be necessary. So I assume we can't be sure that image is not null? I would rather replace the if(!image) conditions with assertions. Should be wrong in the first place to call the function with null. Can you investigate?

Some places do not check the image argument, and may then pass a null image or imagebuffer.

[arno.]

Created attachment 171309 [details]
fix calls in FEComposite and in Gstreamer; replace IntPoint(0, 0) with IntPoint()

updated patch

[Dirk Schulze]

Comment on attachment 171309 [details]
fix calls in FEComposite and in Gstreamer; replace IntPoint(0, 0) with IntPoint()

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

> Source/WebCore/platform/graphics/GraphicsContext.cpp:443
> +    if (!image)
> +        return;

I would really like ti have all these functions have ASSERT(!image) instead. Can you check if it is possible to check for the image before calling this functions?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1626
> -    context->drawImage(reinterpret_cast<Image*>(gstImage->image().get()), ColorSpaceSRGB,
> -                       rect, gstImage->rect(), CompositeCopy, DoNotRespectImageOrientation, false);
> +    Image* image = reinterpret_cast<Image*>(gstImage->image().get());
> +    if (!image)
> +        return;
> +
> +    FloatRect srcRect = gstImage->cropRect();
> +    if (srcRect.isEmpty())
> +        srcRect  = FloatRect(IntRect(IntPoint(), image->size()));

I assume that gstImage->rect() implies that -1, -1 means undetermined intrinsic lines. Does the GStreamer code make the same assumption somewhere else? Why did you change to gstImage->cropRect()? Can we may fix gstImage->rect() instead?

[arno.]

Created attachment 171482 [details]
Patch

updated patch to address reviewer comments

[Build Bot]

Comment on attachment 171482 [details]
Patch

Attachment 171482 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14643213

New failing tests:
css3/filters/huge-region.html
fast/gradients/gradient-on-pseudoelement-crash.html
fast/gradients/crash-on-1px-border.html

[Dirk Schulze]

(In reply to comment #26)
> (From update of attachment 171482 [details])
> Attachment 171482 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/14643213
> 
> New failing tests:
> css3/filters/huge-region.html
> fast/gradients/gradient-on-pseudoelement-crash.html
> fast/gradients/crash-on-1px-border.html

Looks like the CSS Filters code and the CSS or Canvas gradients have problems?

[arno.]

Created attachment 171558 [details]
updated patch

(In reply to comment #27)
> (In reply to comment #26)
> > (From update of attachment 171482 [details] [details])
> > Attachment 171482 [details] [details] did not pass mac-ews (mac):
> > Output: http://queues.webkit.org/results/14643213
> > 
> > New failing tests:
> > css3/filters/huge-region.html
> > fast/gradients/gradient-on-pseudoelement-crash.html
> > fast/gradients/crash-on-1px-border.html
> 
> Looks like the CSS Filters code and the CSS or Canvas gradients have problems?

Indeed.
I think this patch should pass the tests. But I'm concerned about this patch: drawImage/drawImageBuffer is called more than 100 times in many different times. I investigating them, but in some of the calls, I cannot do much more than "guess" wether argument can be one day null (sometimes, the check is made in the caller caller function. Sometimes, I checked that the argument cannot be logically false). But for example, in the filters, I don't known if  FilterEffect->asImageBuffer() can under some circumstances be null.

[arno.]

Created attachment 173412 [details]
check image pointer in DrawImage(Buffer?) methods

here is a patch checking image pointer in the DrawImage methods because it was too cumbersome and fragile to make that check in the callers; I am not marking it as neeeding review now because I did not manage to run the performance tests and get meaningful results

[arno.]

I've run performance tests, and they did not report performance issues with anything related to graphics/canvas

[Dirk Schulze]

(In reply to comment #30)
> I've run performance tests, and they did not report performance issues with anything related to graphics/canvas

Just asked and there don't seem to be performance tests for Canvas at the moment. Especially not for drawImage, that may explain that you didn't see performance issues. Can you try writing an performance test for drawImage please? See in the folder PerformanceTests for examples.

[arno.]

Created attachment 174523 [details]
The performance test show no performance regression (and no improvement also) with the patch

updated patch: add a performance test for drawImage

[Philip Rogers]

Comment on attachment 174523 [details]
The performance test show no performance regression (and no improvement also) with the patch

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

Drive-by review by a non-reviewer.

I like how this gets rid of the inconsistent -1 cases but I'm not a fan of the redundant (!image) checks it spawns. An idea: could we add IntRect::nullRect() and IntRect::isNull() to handle the special-case, similar to the use of -1 before? We could also extract PaintInfo::infiniteRect() for this purpose.

Also a couple nits on the performance test.

> PerformanceTests/Canvas/drawimage.html:1
> +<!DOCTYPE html>

Please add <html> and </html> as well

> PerformanceTests/ChangeLog:8
> +        Create a drawImage performance test

Can you add sample output showing before and after performance?

[arno.]

(In reply to comment #33)
> (From update of attachment 174523 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174523&action=review
> 
> Drive-by review by a non-reviewer.
> 
> I like how this gets rid of the inconsistent -1 cases but I'm not a fan of the redundant (!image) checks it spawns. An idea: could we add IntRect::nullRect() and IntRect::isNull() to handle the special-case, similar to the use of -1 before? 

We would have to do the same for FloatRect also. We don't need LayoutRect now. But should we also do the same for LayoutRect. But anyway, I find the null rectangle confusing. For example:

void GraphicsContext::drawImageBuffer(ImageBuffer* image, ColorSpace styleColorSpace, const IntPoint& dest, const IntRect& srcRect, CompositeOperator op)
{
    drawImageBuffer(image, styleColorSpace, IntRect(dest, srcRect.size()), srcRect, op);
}


This is sometimes called with a specified dest, and an unspecified (-1 -1) srcRect. So, the resulting intRect should be: location specified, size unspecified. Is that a null rectangle with a location. Currently, I think the opposite case does not exist (rectangle created from an unspecified location, and a specified location). So, should we assume it won't happen, and consider that null rectangle means: unspecified size but location may be specified ? or is there a better way ?

[Darin Adler]

Comment on attachment 174523 [details]
The performance test show no performance regression (and no improvement also) with the patch

Why all the null checks? Did the drawImage function work properly with null images before?

[arno.]

(In reply to comment #35)
> (From update of attachment 174523 [details])
> Why all the null checks? Did the drawImage function work properly with null images before?

The null check is needed for the functions when methods are called on that image object. So, it prevents null pointer dereferencing. Before, the null check is only performed in the "final" drawImage function.

[Eric Seidel (no email)]

Comment on attachment 174523 [details]
The performance test show no performance regression (and no improvement also) with the patch

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

> Source/WebCore/platform/graphics/GraphicsContext.cpp:474
>      if (paintingDisabled() || !image)
>          return;

It looks like we still do the check here?  So why would we want the additional null checks darin highlighted?

[arno.]

(In reply to comment #37)
> (From update of attachment 174523 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174523&action=review
> 
> > Source/WebCore/platform/graphics/GraphicsContext.cpp:474
> >      if (paintingDisabled() || !image)
> >          return;
> 
> It looks like we still do the check here?  So why would we want the additional null checks darin highlighted?

for example here:
+    if (!image)
+        return;
+    drawImage(image, styleColorSpace, dest, FloatRect(IntRect(IntPoint(), image->size())));

to avoid crashing in image->size() if image is null

[Dirk Schulze]

(In reply to comment #38)
> (In reply to comment #37)
> > (From update of attachment 174523 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=174523&action=review
> > 
> > > Source/WebCore/platform/graphics/GraphicsContext.cpp:474
> > >      if (paintingDisabled() || !image)
> > >          return;
> > 
> > It looks like we still do the check here?  So why would we want the additional null checks darin highlighted?
> 
> for example here:
> +    if (!image)
> +        return;
> +    drawImage(image, styleColorSpace, dest, FloatRect(IntRect(IntPoint(), image->size())));
> 
> to avoid crashing in image->size() if image is null

Arno is right, the checks are necessary now. schenney opened bug 104367 for fixing the multiple drawImage calls. Any objections to land this patch?

[Dirk Schulze]

arno, can you elaborate in pdr's feedback on comment 33? The patch looks good to me otherwise.

[arno.]

(In reply to comment #33)
> (From update of attachment 174523 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174523&action=review
> 
> Drive-by review by a non-reviewer.
> 
> I like how this gets rid of the inconsistent -1 cases but I'm not a fan of the redundant (!image) checks it spawns. An idea: could we add IntRect::nullRect() and IntRect::isNull() to handle the special-case, similar to the use of -1 before?

Then first problem is that while it would help us get rid of the (!image) checks, it would add many isNull() checks (in the drawImages, when FloatRect are created from IntRect or when IntRect are created from IntPoint dest, IntRect srcRect.

The second problem is that "nullRect" may be confusing: it looks like the rectangle is undetermined. But actually, rectangles would not be totally undefined. Only their size would be.

> 
> Also a couple nits on the performance test.
> 
> > PerformanceTests/Canvas/drawimage.html:1
> > +<!DOCTYPE html>
> 
> Please add <html> and </html> as well
> 
> > PerformanceTests/ChangeLog:8
> > +        Create a drawImage performance test
> 
> Can you add sample output showing before and after performance?

Thanks, I'll provide an update patch

[arno.]

Created attachment 178893 [details]
Patch

updated patch

[Philip Rogers]

> 
> Thanks, I'll provide an update patch

I like the updated patch (once the merge failure is resolved). There's already a followup queued up that will further reduce the chained drawImage calls (wkbug.com/104367).

[Dirk Schulze]

Comment on attachment 178893 [details]
Patch

There was a blending patch that introduced new arguments. You need to merge your patch with trunk. Other then that, I agree with pdr.

[arno.]

Created attachment 179544 [details]
Patch

patch updated against tot

[Ryosuke Niwa]

Comment on attachment 179544 [details]
Patch

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

> PerformanceTests/ChangeLog:10
> +        Create a drawImage performance test. There is no significative change
> +        in performance: 27144.6851528 runs/s without the patch; 27153.517612
> +        runs/s with the patch.

Please add this to PerformanceTests/Skipped. Until we implement a whitelist for performance tests, we’re going to blacklist all performance tests by default.

[arno.]

Created attachment 179563 [details]
Patch

updated patch: skip performance test

[Darin Adler]

Comment on attachment 179563 [details]
Patch

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

> Source/WebCore/platform/graphics/filters/FEComposite.cpp:278
> +    ImageBuffer* imgBuffer = in->asImageBuffer();
> +    ImageBuffer* imgBuffer2 = in2->asImageBuffer();

We normally prefer not to abbreviate words like “image” as “img”. These should be named imageBuffer and imageBuffer2.

[arno.]

Created attachment 182865 [details]
same patch except s/imgBuffer/imageBuffer/ and s/imgBuffer2/imageBuffer2/

[Dean Jackson]

Comment on attachment 182865 [details]
same patch except s/imgBuffer/imageBuffer/ and s/imgBuffer2/imageBuffer2/

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

Setting to r+ cq+ since Darin already gave r+. I checked that his requested changes were made.

> PerformanceTests/Canvas/drawimage.html:9
> +var source = document.createElement("canvas");
> +source.width = 300;
> +source.height = 150;

For future reference, these are the default values and thus not necessary.

> PerformanceTests/Canvas/drawimage.html:15
> +var target = document.createElement("canvas");
> +target.width = source.width;
> +target.height = source.height;

Same.

[WebKit Review Bot]

Comment on attachment 182865 [details]
same patch except s/imgBuffer/imageBuffer/ and s/imgBuffer2/imageBuffer2/

Clearing flags on attachment: 182865

Committed r139911: <http://trac.webkit.org/changeset/139911>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.