Bug 47901 - fast/canvas/canvas-getImageData-negative-source.html fails on Mac
Summary: fast/canvas/canvas-getImageData-negative-source.html fails on Mac
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Mihai Parparita
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-19 06:40 PDT by Andreas Kling
Modified: 2011-04-19 13:51 PDT (History)
8 users (show)

See Also:


Attachments
Patch (9.71 KB, patch)
2010-12-08 18:23 PST, Mihai Parparita
darin: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2010-10-19 06:40:27 PDT
fast/canvas/canvas-getImageData-negative-source.html fails on Mac.
It was added in <http://trac.webkit.org/changeset/70050>
Comment 1 James Robinson 2010-10-19 13:47:42 PDT
Fails on chromium mac as well (but passes on other chromium platforms).  Looks like a CoreGraphics issue.
Comment 2 Mihai Parparita 2010-12-08 18:23:06 PST
Created attachment 75998 [details]
Patch
Comment 3 Darin Adler 2010-12-08 18:25:49 PST
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?
Comment 4 Mihai Parparita 2010-12-09 15:46:31 PST
(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.
Comment 5 James Robinson 2010-12-09 16:26:59 PST
(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.
Comment 6 Stephen White 2010-12-09 17:00:51 PST
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.
Comment 7 WebKit Commit Bot 2010-12-09 19:08:18 PST
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 ap@webkit.org and yuzo@google.com.  The commit-queue is continuing to process your patch.
Comment 8 WebKit Commit Bot 2010-12-09 19:09:31 PST
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
Comment 9 Mihai Parparita 2010-12-10 10:48:05 PST
Committed r73744: <http://trac.webkit.org/changeset/73744>
Comment 10 Matthew Delaney 2011-04-19 13:51:05 PDT
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.