Bug 47901 – fast/canvas/canvas-getImageData-negative-source.html fails on Mac

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

[Andreas Kling]

fast/canvas/canvas-getImageData-negative-source.html fails on Mac.
It was added in <http://trac.webkit.org/changeset/70050>

[James Robinson]

Fails on chromium mac as well (but passes on other chromium platforms).  Looks like a CoreGraphics issue.

[Mihai Parparita]

Created attachment 75998 [details]
Patch

[Darin Adler]

Comment on attachment 75998 [details]
Patch

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

Looks fine.

> WebCore/html/HTMLCanvasElement.cpp:349
>  #if PLATFORM(SKIA)
> -    if (wf > MaxSkiaDim || hf > MaxSkiaDim)
> +    if (width > MaxSkiaDim || height > MaxSkiaDim)
>          return IntSize();
>  #endif

Really? Why does Skia need this at this level? Why doesn’t any other graphics system need this?

[Mihai Parparita]

(In reply to comment #3)
> Really? Why does Skia need this at this level? Why doesn’t any other graphics system need this?

This was done in http://trac.webkit.org/changeset/63219 because of a security bug (bug 41962) that I don't have permission to see, perhaps one of the people involved with that patch can say for sure.

[James Robinson]

(In reply to comment #3)
> (From update of attachment 75998 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75998&action=review
> 
> Looks fine.
> 
> > WebCore/html/HTMLCanvasElement.cpp:349
> >  #if PLATFORM(SKIA)
> > -    if (wf > MaxSkiaDim || hf > MaxSkiaDim)
> > +    if (width > MaxSkiaDim || height > MaxSkiaDim)
> >          return IntSize();
> >  #endif
> 
> Really? Why does Skia need this at this level? Why doesn’t any other graphics system need this?

The short story here is that Skia's limitations are more strict than that of other graphics systems - every graphics system has a max area cap, but Skia also has a maximum value it can handle in any one dimension.  I think the decision was made to enforce the Skia limits in the same places that the limitations of other graphics systems are enforced rather than reducing everyone's limits to that of skia.  I agree it's kind of weird.

At this point, I'm not sure if the limits for Skia are even necessary any more (we've switched to using floating point math instead of fixed point math in some of Skia's internals) - so it's possible that we can just drop all of the Skia guards and stick the cross-platform limits.  Stephen or Adam would probably know if I'm just imagining things here.

[Stephen White]

Comment on attachment 75998 [details]
Patch

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

>>> WebCore/html/HTMLCanvasElement.cpp:349
>>>  #endif
>> 
>> Really? Why does Skia need this at this level? Why doesn’t any other graphics system need this?
> 
> The short story here is that Skia's limitations are more strict than that of other graphics systems - every graphics system has a max area cap, but Skia also has a maximum value it can handle in any one dimension.  I think the decision was made to enforce the Skia limits in the same places that the limitations of other graphics systems are enforced rather than reducing everyone's limits to that of skia.  I agree it's kind of weird.
> 
> At this point, I'm not sure if the limits for Skia are even necessary any more (we've switched to using floating point math instead of fixed point math in some of Skia's internals) - so it's possible that we can just drop all of the Skia guards and stick the cross-platform limits.  Stephen or Adam would probably know if I'm just imagining things here.

No, unfortunately we still need this.  See r63219 where this was added.

One way to make it less ugly would be to have a function or enum on GraphicsContext.h to retrieve the (platform-specific) maximum dimension, rather than having an #if PLATFORM here.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 75998 [details]:

http/tests/appcache/foreign-fallback.html
fast/css/font-face-download-error.html

Please file bugs against the tests.  These tests were authored by [e-mail] and [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 75998 [details]
Patch

Rejecting patch 75998 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'apply-attachment', '--force-clean', '--non-interactive', 75998]" exit_code: 2
Last 500 characters of output:
s.txt
Hunk #1 FAILED at 2412.
Hunk #2 succeeded at 2919 (offset -15 lines).
Hunk #3 FAILED at 2991.
2 out of 3 hunks FAILED -- saving rejects to file LayoutTests/platform/chromium/test_expectations.txt.rej
patching file WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebCore/html/HTMLCanvasElement.cpp
patching file WebCore/html/HTMLCanvasElement.h

Failed to run "[u'/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Darin Adler', u'--force']" exit_code: 1

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

[Mihai Parparita]

Committed r73744: <http://trac.webkit.org/changeset/73744>

[Matthew Delaney]

This test appears to be still on the platform/mac/Skipped list. If it's fixed for all/mac, it should be taken off all/mac skipped lists.