655328 - Canvas drawImage error on 20thingsilearned.com

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Alice0775 White]

Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/88fdbd974f82
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110506 Firefox/6.0a1 ID:20110506030557

Only entry page is registered in the popup, No entry is registered for each page.
Should I filed a bug?

Reproducible: Always

Steps to Reproduc:
1. Start Minefield with new profile
2. Open URL ( http://www.20thingsilearned.com/ ) in New Tab
3. Click forward button at right side of page
4. Click forward button at right side of page
5. Try, Right ckick on Back/Forward button

Actual Results:
 No entry in Back/Forward button

Expected Results:
 Each Page should be registered in the Back/Forward menu popup

Regression window:
Works, Entry of each page are registered in the back/Forward popup
http://hg.mozilla.org/mozilla-central/rev/6c1a5f4eb350
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.2a1pre) Gecko/20110329 Firefox/4.2a1pre ID:20110330085433
Fails, Only front page is registered in the back/Forward popup
http://hg.mozilla.org/mozilla-central/rev/422bbd8245a7
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.2a1pre) Gecko/20110331 Firefox/4.2a1pre ID:20110331030432
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6c1a5f4eb350&tochange=422bbd8245a7

Comment 1

[Justin Lebar (not reading bugmail)]

I'm having trouble reproducing, or understanding what's going wrong.

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a1) Gecko/20110506 Firefox/6.0a1

I click the page's forward button twice.  That takes me to [1].

Now when I right-click on the back button, it displays two entries, as expected.

[1] http://www.20thingsilearned.com/foreword/2

Comment 2

[Alice0775 White]

It happens on
http://hg.mozilla.org/mozilla-central/rev/88fdbd974f82
Mozilla/5.0 (X11; Linux i686; rv:6.0a1) Gecko/20110506 Firefox/6.0a1 ID:20110506030557

(In reply to comment #1)
> I'm having trouble reproducing, or understanding what's going wrong.
> 
> Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a1) Gecko/20110506
> Firefox/6.0a1
> 
> I click the page's forward button twice.  That takes me to [1].
> 
> Now when I right-click on the back button, it displays two entries, as
> expected.
> 
> [1] http://www.20thingsilearned.com/foreword/2

Page 1 is never registered.
And repeat step 2-5 few times(close resume popup, if any), the entry is never registered.

Comment 3

[Justin Lebar (not reading bugmail)]

So the problem is that you clicked forward twice but only got two history entries instead of three?

If so, I don't think that's a bug -- the page appears to be calling history.replaceState the first time you click forward.

Try it in Chrome; it does the same thing.

Comment 4

[Alice0775 White]

In Google Chrome 13.0.756.0 canary,
It works as I _expected_.
When I click foreward button of the page twice.
Then, 2 entries are registered.
Page 1
Front page

so I click Back button og the browser
Page 1 appears
and click Back button og the browser again
Front page appears

Comment 5

[Alice0775 White]

s/og/of/g

Comment 6

[Justin Lebar (not reading bugmail)]

Oh, huh, I can reproduce what you're seeing now.

That page is doing some strange things.  Sometimes when I visit it doesn't update the URI on every page change, but sometimes it does.

The problem is that the first time you click the right arrow, it doesn't register a new history entry, nor does it change the URI.  This leads me to suspect that it's calling replaceState or somesuch.  But maybe it's our bug!

Do you want to investigate and create a smaller testcase?

Comment 7

[Alice0775 White]

(In reply to comment #6)
> Oh, huh, I can reproduce what you're seeing now.
> 
> That page is doing some strange things.  Sometimes when I visit it doesn't
> update the URI on every page change, but sometimes it does.
> 
> The problem is that the first time you click the right arrow, it doesn't
> register a new history entry, nor does it change the URI.  This leads me to
> suspect that it's calling replaceState or somesuch.  But maybe it's our bug!
> 
> Do you want to investigate and create a smaller testcase?
Thanks,
I will discover regression bug by bisection.
However It is difficult to create test case because the site is very complicated for me.

Comment 8

[Justin Lebar (not reading bugmail)]

FWIW, I'm not convinced that this will show us something useful, since the page's behavior changes as the UA and capabilities of the browser changes.

Comment 9

[Justin Lebar (not reading bugmail)]

s/this/bisecting

Comment 10

[Alice0775 White]

In local build(from ceder repository)
build from cbb7ffa045d3 : fails
build from db8e95fbcd39 : fails
build from a3b18039e32f : fails
build from 4ede7b9b55bc : fails
build from e83560952624 : works
build from 8053d7723f6e : works
build from bc9f0b32325a : works
Triggered by:
4ede7b9b55bc	Yury — Bug 629875 - Handle negative arguments to drawImage; r=jmuizelaar

And I also confirm on m-c that cset 4ede7b9b55bc trigger the problem.
In local build(from m-c repository)
build from 62941612320d : fails
build from 62941612320d and backed out 4ede7b9b55bc : works

CC'ing
Ms2ger
jrmuizel
yury

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Thanks, Alice! CC'ing Paul Irish

Paul, I suspect someone is calling drawImage with zero sw or sh. Could you bug the right people?

Comment 12

[Yury Delendik (:yury)]

Attachment: Image resource for minimal test case

Comment 13

[Yury Delendik (:yury)]

Attachment: Minimal test case to replicate the issue

Comment 14

[Justin Lebar (not reading bugmail)]

Over to the right component.  Thanks for your work!

Comment 15

[Yury Delendik (:yury)]

The FF6 returns exception when drawImage is called with image 830x520 and the source region is (809.756104, 0.000000, 20.243902, 520.000000).  The 809.756104 + 20.243902 equals 830.000006, which is outside the image boundaries. 

Chrome, Opera and Safari are okay with that and do not raise any exception, which is wrong from the specification point of view IMHO

Comment 16

[Jeff Muizelaar [:jrmuizel]]

Sounds like we might need to fix the spec

Comment 17

[Boris Zbarsky [:bzbarsky]]

> 830.000006, which is outside the image boundaries. 

Yes, but I bet WebKit and Opera are doing their usual "round to integer" thing an then it works...

And yes, it sounds like this needs a spec fix.  It also sounds like we may need to back out bug 629875 on Aurora if we don't come up with an Aurora-appropriate fix for this....  We need to track this for Firefox 5.

Comment 18

[Yury Delendik (:yury)]

(In reply to comment #17)

> Yes, but I bet WebKit and Opera are doing their usual "round to integer"
> thing an then it works...

Not exactly. The mochitests (including http://w3c-test.org/html/tests/submission/PhilipTaylor/canvas/2d.drawImage.outsidesource.html) fail if the boundary components rounded to integer. However if those rounded to float, the mochitests and the attached test pass.
 
> 
> And yes, it sounds like this needs a spec fix. 

The spec (and the Philip Taylor's test) is clear about usage of the boundary components as double precision numbers. Currently, the Chrome passes the 2d.drawImage.outsidesource test.

Comment 19

[Yury Delendik (:yury)]

Attachment: "Round to float" image source boundaries for drawImage

Comment 20

[Boris Zbarsky [:bzbarsky]]

> fail if the boundary components rounded to integer

Hmm.  OK....

Then why does the minimal testcase here not fail in Chrome, exactly?

Comment 21

[Yury Delendik (:yury)]

Looking at http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp&q=drawImage&exact_package=chromium&l=1337 , Chrome is using float in the canvas context functions implementations:

void CanvasRenderingContext2D::drawImage(HTMLCanvasElement* canvas,
    float sx, float sy, float sw, float sh,
    float dx, float dy, float dw, float dh, ExceptionCode& ec);

Comment 22

[Boris Zbarsky [:bzbarsky]]

We use float there too.  That's just the API.  It doesn't say what happens to the numbers once they're passed in.  

Again, why is Chrome not throwing in the attached minimal testcase?

Comment 23

[Yury Delendik (:yury)]

Attachment: Changing doubles to floats inthe DrawImage and FloatValidate functions

(In reply to comment #22)
> We use float there too.  That's just the API.  It doesn't say what happens
> to the numbers once they're passed in.  

Good point. Changing doubles to floats in the DrawImage function allows mochitests and the attached minimal test case pass. 

The DrawImage was the last user of the FloatValidate with double arguments. Those functions were changed to have arguments as floats to avoid unnecessary implicit casts.

Comment 24

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 530942 [details] [diff] [review]
Changing doubles to floats inthe DrawImage and FloatValidate functions

Looks sane to me.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 530942 [details] [diff] [review]
Changing doubles to floats inthe DrawImage and FloatValidate functions

This seems ok to me as a quick-fix but:

1)  This needs to be escalated to the spec.
2)  There needs to be a less-fragile solution here, imo.

I'll do the spec escalation, I guess.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Actually... no.  I don't see where the spec says to throw if the source rect is not contained inside the image.  Instead it seems to say to fill the part outside the image with image edge pixels:

  If the original image data is a bitmap image, the value painted at a point in
  the destination rectangle is computed by filtering the original image data.
  The user agent may use any filtering algorithm (for example bilinear
  interpolation or nearest-neighbor). When the filtering algorithm requires a
  pixel value from outside the original image data, it must instead use the
  value from the nearest edge pixel. (That is, the filter uses 'clamp-to-edge'
  behavior.)

Am I just missing something here?

Comment 27

[Yury Delendik (:yury)]

Attachment: Changing doubles to floats in the DrawImage and FloatValidate functions, removing source boundary check

Looking at the historical specification ".. If the source rectangle is not entirely within the source image, or if one of the sw or sh arguments is zero, the implementation must raise an INDEX_SIZE_ERR exception..." and the code before bug 629875 :

-    // check args
-    if (sx < 0.0 || sy < 0.0 ||
-        sw < 0.0 || sw > (double) imgSize.width ||
-        sh < 0.0 || sh > (double) imgSize.height ||
-        dw < 0.0 || dh < 0.0)

So reverting back the rev 4ede7b9b55bc may not give the desired result for other web sites. Also, up-to-date specification makes Philip Taylor's 2d.drawImage.outsidesource incorrect.

The patch fixes the code to conform the new specification and it's keeping the double to float change from the previous patch. Also, it's marking as todo the 2d.drawImage.outsidesource section of the test_canvas.html.

Comment 28

[Boris Zbarsky [:bzbarsky]]

> and it's keeping the double to float change from the previous patch

Why do we want that change?  Does it have observable effects?

(I'm not trying to rag on you here; I really appreciate your looking into this.  I just want to understand the implications of the changes we're making, and I don't know this code that well.)

Comment 29

[Yury Delendik (:yury)]

Since the spec does not require check on the source boundary rectangle, so I remove the that check. (Which caused failures on 2d.drawImage.outsidesource tests, had to adjust test_canvas) The code below, after that check, uses translate/scale matrix for the gfxPattern, so I count on the Clip to not display stuff that off the source boundary rectangle.

Visible effects: attached test case and succeeded checks in the test_canvas. Do you think it worth to add pixel check tests?

Comment 30

[Boris Zbarsky [:bzbarsky]]

> so I remove the that check. 

Yes, that part sounds correct to me.  What I'm not sure is why we want the change from double to float.

Comment 31

[Yury Delendik (:yury)]

In the DrawImage body, the gfxMatrix and other surface methods use gfxRect type in arguments, so it does not make sense to keep the doubles:
- in DrawImage, to convert from float to double and then back to float just to perform simple arithmetic operations;
- in FloatValidate, after changing double to float in DrawImage, there is no users of FloatValidate with double arguments, but there are a lot of users of FloatValidate with float arguments. That removes unnecessary casts in other functions calls.

Comment 32

[Boris Zbarsky [:bzbarsky]]

OK.  So the change to double is just for internal code cleanliness and has no observable behavior effects on web content?

Comment 33

[Yury Delendik (:yury)]

Looks like it... just trying to avoid other double/float edge case.

Do you want to split "code cleanliness" part and actual fix into two different patches?

Comment 34

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #33)
> Looks like it... just trying to avoid other double/float edge case.
> 
> Do you want to split "code cleanliness" part and actual fix into two
> different patches?

Yes please.

Comment 35

[Yury Delendik (:yury)]

Attachment: Removing source boundary check plus tests

Comment 36

[Yury Delendik (:yury)]

Attachment: Changing doubles to floats in the DrawImage and FloatValidate functions

Comment 37

[Asa Dotzler [:asa]]

regression on the trunk after 4, we should try to get this in to 5 if it's not too scary.

Comment 38

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #35)
> Created attachment 531231 [details] [diff] [review] [review]
> Removing source boundary check plus tests

To confirm this reverts us to the behavior of Firefox 4 which matches what other browsers do?

Comment 39

[Yury Delendik (:yury)]

(In reply to comment #38)
> (In reply to comment #35)
> > Created attachment 531231 [details] [diff] [review] [review] [review]
> > Removing source boundary check plus tests
> 
> To confirm this reverts us to the behavior of Firefox 4 which matches what
> other browsers do?

Jeff,

Comment 27 shows behavior of the Firefox 4. That behavior is wrong per specification (see comment 26). 

Comment 23 fixes the behavior to match behavior of the Chrome (WebKit passes 2d.drawImage.outsidesource test), this test is invalid per spec as well.

The attachment 531231 [details] [diff] [review] removes the check that was introduced in rev 4ede7b9b55bc (similar and less restrictive check was in FF4). That check was introduces by older specification and does not matter to the code below it.

Comment 40

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 531231 [details] [diff] [review]
Removing source boundary check plus tests

I thought I had already responded to this but apparently not. Why are the tests being changed to todo? Aren't they wrong now?

Comment 41

[Yury Delendik (:yury)]

Jeff,

The test still present in Philip Taylor's tests. However, the specification does not require to perform the source boundary check (at was in older versions of the spec). Not sure what do with that test, so I mark it as to do: 1) in case in specification will be changed back, 2) have some "feedback" in case if the boundary check will be reverted back somehow.
 
Do you want to remove this test?

Comment 42

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #41)
> Jeff,
> 
> The test still present in Philip Taylor's tests. However, the specification
> does not require to perform the source boundary check (at was in older
> versions of the spec). Not sure what do with that test, so I mark it as to
> do: 1) in case in specification will be changed back, 2) have some
> "feedback" in case if the boundary check will be reverted back somehow.
>  
> Do you want to remove this test?

Why not have change them to 'fail' and add a comment about the situation? This way we still test the behavior and don't give the impression that the code is incomplete or unintentional.

Comment 43

[Yury Delendik (:yury)]

Attachment: Removing source boundary check plus tests (v2)

The test_2d.drawImage.outsidesource.html section was removed from the test_canvas.html. The new test was added to test the behavior described in the comment #26 (and the spec):

>   ... When the filtering algorithm requires a
>   pixel value from outside the original image data, it must instead use the
>   value from the nearest edge pixel. 
>   (That is, the filter uses 'clamp-to-edge' behavior.)

This test is a more common case of the "minimal test case to replicate the issue", and opposite to the obsolete 2d.drawImage.outsidesource.html test.

Comment 44

[Asa Dotzler [:asa]]

Time is running out for Firefox 5 and this regression is something we'd like to see fixed. Should we consider just backing out bug 629875 since we do not yet have a fully reviewed and tested on m-c patch for this regression?

Comment 45

[Jeff Muizelaar [:jrmuizel]]

I ran this patch against try server and the new test fails:
http://tbpl.mozilla.org/?tree=Try&rev=c6595914ccf9

e.g.
http://tinderbox.mozilla.org/showlog.cgi?log=Try/1305831631.1305832400.20048.gz

I think at this point we should seriously consider backing bug 629875 out.

Comment 46

[Jeff Muizelaar [:jrmuizel]]

This failure may come from the switch to using EXTEND_PAD in canvas that happened yesterday. EXTEND_PAD doesn't work properly on OS X which may be why the test passes on that platform.

Comment 47

[Mounir Lamouri (:mounir)]

Two patches are not obsolete. Which one should be pushed? Both?

Comment 48

[Yury Delendik (:yury)]

Attachment: Removing source boundary check plus tests (v3)

(In reply to comment #46)
> This failure may come from the switch to using EXTEND_PAD in canvas that
> happened yesterday. EXTEND_PAD doesn't work properly on OS X which may be
> why the test passes on that platform.

That's correct, the using EXTEND_PAD in canvas caused the failures. The test_bug655328.html was invalid, and the landing of the bug 620216 helped to find that out. 

The test was fixed. Also, the portion of the tests is marked as todo for the MacOS platform. Here is a try server run for this patch: http://tbpl.mozilla.org/?tree=Try&rev=841818ca41e5

Comment 49

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #48)
> Created attachment 533887 [details] [diff] [review] [review]
> Removing source boundary check plus tests (v3)
> 
> (In reply to comment #46)
> > This failure may come from the switch to using EXTEND_PAD in canvas that
> > happened yesterday. EXTEND_PAD doesn't work properly on OS X which may be
> > why the test passes on that platform.
> 
> That's correct, the using EXTEND_PAD in canvas caused the failures. The
> test_bug655328.html was invalid, and the landing of the bug 620216 helped to
> find that out. 
> 
> The test was fixed. Also, the portion of the tests is marked as todo for the
> MacOS platform. Here is a try server run for this patch:
> http://tbpl.mozilla.org/?tree=Try&rev=841818ca41e5

I'm concerned about this. From my understanding of canvas, working EXTEND_PAD shouldn't be need to implement canvas. drawImage shouldn't be drawing outside of the bounds of the image and it sounds like with this patch we are.

Comment 50

[Yury Delendik (:yury)]

IRC log from yesterday:

[15:34:20] <joe> clamp-to-edge == EXTEND_PAD
[15:35:06] <yury> ok
[15:35:21] <joe> so we should be testing that we use extend_pad
[15:36:28] <yury> joe, the test is http://hg.mozilla.org/try/diff/a7e871889f7c/content/canvas/test/test_bug655328.html
[15:38:53] <yury> probably, i'm not reading the spec right way: shall there, in the middle, be green?
[15:40:54] <joe> hm
[15:41:10] <joe> it looks like you draw over the same area in the canvas 4 times?
[15:43:25] <joe> did you reverse source and destination accidentally?
[15:44:01] <yury> it shall take data from outside of the image
[15:44:11] <joe> okay
[15:44:20] <joe> and draw it to the same position in the canvas?
[15:44:33] <yury> yep
[15:44:49] <joe> ok good
[15:44:57] <joe> in that case
[15:45:00] <yury> looks like I missunderstand EXTEND_PAD
[15:45:02] <joe> everything should be green
[15:45:09] <joe> since we're clamping everything to edge
[15:45:22] <joe> so you draw 4 100x50 green rectangles to (0, 0) in the canvas
[15:45:53] <yury> good, thanks
[15:46:00] <joe> however!!
[15:46:04] <joe> this test is going to fail on OS X
[15:46:13] <joe> because EXTEND_PAD doesn't work there properly
[15:47:33] -*- Philip agrees the spec says it should be drawing 100x50 green four times
[15:47:49] -*- Philip hopes that spec behaviour isn't causing compatibility problems
[15:48:00] <joe> yury: perhaps you should draw the rectangles to different positions on the canvas?
[15:48:07] <joe> to test all the different combinations
[15:48:34] -*- Philip really needs to get around to updating all his canvas tests, and then importing them back into Mozilla's test suite somehow...
[15:48:40] <yury> so i should remove pixel tests for outside source pixels
[15:48:45] <joe> no
[15:48:52] <joe> you should keep those pixel tests
[15:48:59] <joe> just ignore their results on OS X

It's consistent with the comment 26 statements.

Comment 51

[Boris Zbarsky [:bzbarsky]]

> drawImage shouldn't be drawing outside of the bounds of the image

Jeff, that's not what the current spec says...  Or more precisely the spec is unclear on what happens if the source rect extends outside the image (something we should bring up), but does have the bit from comment 26.

Comment 52

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #51)
> > drawImage shouldn't be drawing outside of the bounds of the image
> 
> Jeff, that's not what the current spec says...  Or more precisely the spec
> is unclear on what happens if the source rect extends outside the image
> (something we should bring up), but does have the bit from comment 26.

Chrome and Safari don't seem to follow that part of the spec and throw an exception on the proposed test case. I'm not really convinced that we want support drawing outside of the bounds of an image.

Comment 53

[Boris Zbarsky [:bzbarsky]]

Then that brings us back to the original question: why is Chrome, say, not throwing an exception on the original testcase for this bug?  If it's just a matter of intermediate computations on floats losing precision compared to those on doubles and hence things that shouldn't be equal becoming equal, then the spec needs to explicitly define all the floating-point operations here, which would be a huge PITA and very fragile for web authors, right?

Comment 54

[Yury Delendik (:yury)]

Also,

(In reply to comment #45)
> I think at this point we should seriously consider backing bug 629875 out.

(In reply to comment #52)
> I'm not really convinced that we want
> support drawing outside of the bounds of an image.

As indicated in comment #27, the code before landing of the bug 629875 looked like:
> -    // check args
> -    if (sx < 0.0 || sy < 0.0 ||
> -        sw < 0.0 || sw > (double) imgSize.width ||
> -        sh < 0.0 || sh > (double) imgSize.height ||
> -        dw < 0.0 || dh < 0.0)

By reverting rev 4ede7b9b55bc (the code above), we will still get some drawing outside of the bounds of an image (when sx + sw > height) and incorrect boundary check. Is the "regression" keyword good label for this issue? Neither the boundary check nor drawing outside of the bounds of an image were implemented property in FF4.

Comment 55

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #53)
> Then that brings us back to the original question: why is Chrome, say, not
> throwing an exception on the original testcase for this bug?  If it's just a
> matter of intermediate computations on floats losing precision compared to
> those on doubles and hence things that shouldn't be equal becoming equal,
> then the spec needs to explicitly define all the floating-point operations
> here, which would be a huge PITA and very fragile for web authors, right?

Chrome doesn't throw an exception because of floats losing precision. I'm not sure that's so bad. Anytime we have this kind of comparison we're going to run into the problem. It perhaps would have been better if the src rect was defined as p1, p2 instead of p + size which would have avoided this problem somewhat.

Comment 56

[Boris Zbarsky [:bzbarsky]]

> I'm not sure that's so bad.

It's terrible, because it makes whether an exception is thrown dependent on a browser's internal representations and order of operations.  How would you spec that?

> Anytime we have this kind of comparison we're going to run into the problem.

Not if we don't ever throw.  I believe that's the direction we should be heading; the spec seems to agree.

Comment 57

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #56)
> > I'm not sure that's so bad.
> 
> It's terrible, because it makes whether an exception is thrown dependent on
> a browser's internal representations and order of operations.  How would you
> spec that?

It's not _that_ hard to spec. i.e. it's basically just a test of whether x + width is less than an integer converted to float. There isn't really any different way of evaluating it than that.

I think that's it's better for us to follow the other browsers here than to follow the spec if were going to be the only one following the spec. Certainly if there's an indication that everyone else will switch to follow the spec that would be good, but I'd be a little bit surprised.

Also, with regards to CoreGraphics if we switch to the EXTEND_PAD semantics we're basically trading throwing an exception with a performance cliff because requiring EXTEND_PAD will not let us use CGContextDrawImage anymore.

Comment 58

[Jeff Muizelaar [:jrmuizel]]

Also, does anyone know why the spec changed?

Comment 59

[Boris Zbarsky [:bzbarsky]]

> it's basically just a test of whether x + width is less than an integer
> converted to float.

No, it's a test of whether x+width with the addition done in a very particular way with very particular precision is less than that integer.

> I think that's it's better for us to follow the other browsers here

I think they just haven't updated to follow the spec yet is all...  The performance issue is bad; raise that with the spec?

> Also, does anyone know why the spec changed?

Worth checking, yes.  It does have commit logs.

I think we should back out bug 629875 on beta and aurora effective immediately, while we think about how to deal with this on trunk.  Sound reasonable?

Comment 60

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #59)
> > it's basically just a test of whether x + width is less than an integer
> > converted to float.
> 
> No, it's a test of whether x+width with the addition done in a very
> particular way with very particular precision is less than that integer.

Well, the types of x and width are already specified to be float so it's natural that the addition be done as float.

> > I think that's it's better for us to follow the other browsers here
> 
> I think they just haven't updated to follow the spec yet is all...  The
> performance issue is bad; raise that with the spec?
> 
> > Also, does anyone know why the spec changed?
> 
> Worth checking, yes.  It does have commit logs.
> 
> I think we should back out bug 629875 on beta and aurora effective
> immediately, while we think about how to deal with this on trunk.  Sound
> reasonable?

Very much so.

Comment 61

[Boris Zbarsky [:bzbarsky]]

> Well, the types of x and width are already specified to be float so it's
> natural that the addition be done as float.

Sort of.  For example, these two tests: |x + width > foo|, |x > foo - width| are not equivalent on floats.  So the spec would have to be _very_ explicit about the _exact_ arithmetic operations involved in the check.  And any changes to client code could break something that happened to work before...

I really really don't think we want to go there in the spec.

Yury, can you please file a bug with a backout patch for aurora/beta?  Or do you want me or Jeff to do that?

Comment 62

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #61)
> > Well, the types of x and width are already specified to be float so it's
> > natural that the addition be done as float.
> 
> Sort of.  For example, these two tests: |x + width > foo|, |x > foo - width|
> are not equivalent on floats.  So the spec would have to be _very_ explicit
> about the _exact_ arithmetic operations involved in the check.  And any
> changes to client code could break something that happened to work before...
> 
> I really really don't think we want to go there in the spec.
> 
> Yury, can you please file a bug with a backout patch for aurora/beta?  Or do
> you want me or Jeff to do that?

I've already attached a backout patch to bug 629875. Should it be a separate bug?

Comment 63

[Boris Zbarsky [:bzbarsky]]

No, that's fine as long as we track it landing....

Comment 64

[Karl Tomlinson (:karlt)]

(In reply to comment #57)
> Also, with regards to CoreGraphics if we switch to the EXTEND_PAD semantics
> we're basically trading throwing an exception with a performance cliff
> because requiring EXTEND_PAD will not let us use CGContextDrawImage anymore.

My understanding was that CGContextDrawImage gave us the slowest DrawImage performance on a "modern" system.  I assumed we'd want to replace that with something else anyway, in which case there doesn't seem to be any reason to choose desired behavior based on peculiarities of that function.  (Most graphics hardware seems to know how to do clamp-to-edge.)

Comment 65

[Philip Taylor]

(In reply to comment #58)
> Also, does anyone know why the spec changed?

http://html5.org/r/5373 replaced the exception behaviour with:

  <p>Pixels of the source rectangle that are not entirely within the
  source image must be treated as transparent black.</p> <!-- see
  CORE-32111 http://krijnhoetmer.nl/irc-logs/whatwg/20100818#l-737 -->

because Opera broke on a page due to float rounding. (The general principle is that a small change in input (e.g. from float imprecision) should only cause a small change in output (never something as drastic as an exception) - hopefully the rest of the canvas spec follows that principle too.)

That later changed to clamp-to-edge behaviour as part of http://www.w3.org/Bugs/Public/show_bug.cgi?id=10799 (mainly since it better matched the filtering behaviour of IE9 and Opera, I think).

Comment 66

[Boris Zbarsky [:bzbarsky]]

> because Opera broke on a page due to float rounding.

Precisely the point I was trying to make!

Comment 67

[Jeff Muizelaar [:jrmuizel]]

What if instead of throwing an exception the source rect was clamped to the size of source? This would allow implementations to avoid reading beyond the source and avoid exceptions from float rounding.

Comment 68

[Boris Zbarsky [:bzbarsky]]

I would be fine with that, from my non-graphics perspective.

Comment 69

[Philip Taylor]

What would that gain? EXTEND_PAD-like behaviour would still be required in order to get the specified clamp-to-edge filtering effect near the edges of a (scaled) image, so (as I understand it) it probably wouldn't make the implementation simpler.

Comment 70

[Yury Delendik (:yury)]

Attachment: Removing source boundary check plus tests (clamps to source size)

(In reply to comment #67)
> What if instead of throwing an exception the source rect was clamped to the
> size of source? This would allow implementations to avoid reading beyond the
> source and avoid exceptions from float rounding.

Variant of the attachment 533887 [details] [diff] [review] that clamps the source rectangle to the size of source.

Not sure what's blocking this bug and bug 629875 at the moment. It seams everyone agrees that check/exception shall be removed. The attachment 533887 [details] [diff] [review] + original bug 629875 patch will brings the FF up-to-data with the specification and the documentation on https://developer.mozilla.org/en/DOM/CanvasRenderingContext2D#Notes

Comment 71

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #69)
> What would that gain? EXTEND_PAD-like behaviour would still be required in
> order to get the specified clamp-to-edge filtering effect near the edges of
> a (scaled) image, so (as I understand it) it probably wouldn't make the
> implementation simpler.

It means that implementations don't need to deal with reading pixels outside of the source and would let the actual filtering on the edges be implementation defined because the affected pixels would only be a narrow band along the edge.

Comment 72

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 536088 [details] [diff] [review]
Removing source boundary check plus tests (clamps to source size)

These seems overly complicated. Can't we just intersect the source rect with this rect: (0,0, imgSize.width, imgSize.height)?

Comment 73

[Yury Delendik (:yury)]

(In reply to comment #72)
> Comment on attachment 536088 [details] [diff] [review] [review]
> Removing source boundary check plus tests (clamps to source size)
> 
> These seems overly complicated. Can't we just intersect the source rect with
> this rect: (0,0, imgSize.width, imgSize.height)?

The intersection of the source rect is a simple part, but at the same time the destination rect has to be adjusted proportionally taking the dw/sw and dh/sh ratios into account. I could not find the methods in the gfxRect class to do that. It will not be a simplest solution (so I would agree with comment 69).

Comment 74

[Philip Taylor]

(In reply to comment #71)
> It means that implementations don't need to deal with reading pixels outside
> of the source and would let the actual filtering on the edges be
> implementation defined because the affected pixels would only be a narrow
> band along the edge.

I'd prefer to be moving away from implementation-defined behaviour, not adding more of it on purpose, and http://www.w3.org/Bugs/Public/show_bug.cgi?id=10799 asked for this to be specified. The edge filtering would make a significant difference when someone e.g. draws two images side-by-side (in a tile-based game or whatever) then scales everything up by 2x - it's an interoperability problem if they get unwanted transparent seams between the images in some implementations and not others.

Comment 75

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #74)
> (In reply to comment #71)
> > It means that implementations don't need to deal with reading pixels outside
> > of the source and would let the actual filtering on the edges be
> > implementation defined because the affected pixels would only be a narrow
> > band along the edge.
> 
> I'd prefer to be moving away from implementation-defined behaviour, not
> adding more of it on purpose, and
> http://www.w3.org/Bugs/Public/show_bug.cgi?id=10799 asked for this to be
> specified.

Sure, but having something spec'ed that implementations wont follow isn't much good either.

> The edge filtering would make a significant difference when
> someone e.g. draws two images side-by-side (in a tile-based game or
> whatever) then scales everything up by 2x - it's an interoperability problem
> if they get unwanted transparent seams between the images in some
> implementations and not others.

Can you explain this example in more detail (perhaps attach an example), I'm not sure I understand the issue.

Comment 76

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #73)
> (In reply to comment #72)
> > Comment on attachment 536088 [details] [diff] [review] [review] [review]
> > Removing source boundary check plus tests (clamps to source size)
> > 
> > These seems overly complicated. Can't we just intersect the source rect with
> > this rect: (0,0, imgSize.width, imgSize.height)?
> 
> The intersection of the source rect is a simple part, but at the same time
> the destination rect has to be adjusted proportionally taking the dw/sw and
> dh/sh ratios into account. I could not find the methods in the gfxRect class
> to do that. It will not be a simplest solution (so I would agree with
> comment 69).

Alternatively, we could keep the dest rect the same size. I'm sort of leaning toward this solution. It's certainly easier to spec.

Comment 77

[Philip Taylor]

(In reply to comment #75)
> (In reply to comment #74)
> > The edge filtering would make a significant difference when
> > someone e.g. draws two images side-by-side (in a tile-based game or
> > whatever) then scales everything up by 2x - it's an interoperability problem
> > if they get unwanted transparent seams between the images in some
> > implementations and not others.
> 
> Can you explain this example in more detail (perhaps attach an example), I'm
> not sure I understand the issue.

http://philip.html5.org/demos/canvas/image-tile.html - in Firefox 4.0 on Linux there's a light line between each repetition of the tile; in other browsers there isn't.

When using canvas it's easy to work around that problem once you know about it (e.g. use patterns instead; or if you're going to scale by factor N then add an N pixel border to the original image file and crop out the border in the drawImage call). But leaving the behaviour implementation-defined means people would have to work around every browser's different filtering method, or more likely have their code break (with visible rendering errors) in some browsers. I don't particularly care exactly which behaviour the spec requires, but I'd like browsers to eventually converge on the same behaviour.

Comment 78

[Asa Dotzler [:asa]]

as far as tracking firefox 5, this is fixed by backout at 629875.

Comment 79

[Asa Dotzler [:asa]]

Are we going to need to back out for 6 like we did 5? Time is short for 6 and it's pretty late to be taking any invasive changes.

Comment 80

[Boris Zbarsky [:bzbarsky]]

I _believe_ this was backed out of 6 as well, since the June 2 backout happened on aurora as well as beta.  But someone should double-check this.

On the other hand, for 7 we still need to do the backout.  Imo.  Setting the tracking nomination to track that...

Comment 81

[Justin Lebar (not reading bugmail)]

> But someone should double-check this.

I'm not sure what changeset you're looking for, but is it this the one?

http://hg.mozilla.org/releases/mozilla-beta/rev/5ae373ff7a19

Comment 82

[Boris Zbarsky [:bzbarsky]]

No, that was the backout for 5.

The part I was looking for, I think is http://hg.mozilla.org/releases/mozilla-beta/rev/72b45a62587a (which was merged from aurora 6).

But the question is whether that merge worked as it should and whether as a result the code is correct.  As in, whether this bug is actually reproducible in beta 6.

Comment 83

[Asa Dotzler [:asa]]

Tracking for 7. We need a backout patch for 7 (aurora) and confirmation that it was backed out on 6 (beta). If it has been already covered on beta, please set the status-firefox6 to fixed.

Comment 84

[Christopher Blizzard (:blizzard)]

Pinged Bas & Jeff via email.

Comment 85

[Jeff Muizelaar [:jrmuizel]]

Yes this is fixed on 6:
http://hg.mozilla.org/releases/mozilla-beta/rev/72b45a62587a

Comment 86

[Asa Dotzler [:asa]]

Should we back this out everywhere? We're about to uplift Aurora to Beta again and this is still hanging around. How about back it out from Aurora and m-c try again.

Comment 87

[LegNeato]

This looks like it is backed out on mozilla-beta (7) as well, but jeff please check to make sure.

Comment 88

[Jeff Muizelaar [:jrmuizel]]

(In reply to Christian Legnitto [:LegNeato] from comment #87)
> This looks like it is backed out on mozilla-beta (7) as well, but jeff
> please check to make sure.

This has also been backed out of FF7

Comment 89

[Kang-Hao (Kenny) Lu [:kennyluck]]

I marked this as blocking gecko-games because I have a Cocos2d-html5 game at hand which unfortunately bumps into this in issue. This incompatibility happens because (for anyone interested):

1. Making a sprite frame (in Cocos2d-html5, cc.SpriteFrame) requires specifying an image and a rectangle.
2. The designer changes the images without keeping the sizes of the images same (but not to much that you need to re-specify the rectangle). I've come to realize that designers don't take the size of a picture to be an invariant when they create a new skin.
3. The rectangle now has parts outside the image. Boom!

Comment 90

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 533887 [details] [diff] [review]
Removing source boundary check plus tests (v3)

The spec ended up with clipping the destination bounds based on the source image. (You're second patch)

Comment 91

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 536088 [details] [diff] [review]
Removing source boundary check plus tests (clamps to source size)

Review of attachment 536088 [details] [diff] [review]:
-----------------------------------------------------------------

This is the right idea but will need rebasing as the implementation has changed substantially since you wrote it.

Comment 92

[Lee Salzman [:lsalzman]]

Attachment: part 1 - clip canvas drawImage source/dest rectangles instead of throwing IndexSizeError

Cleaned up variant of the original patches in this thread.

It clips the source rectangle and then clips the destination rectangle in the same proportion as mandated by the canvas spec.

It gets rid of the non-standard behavior of throwing IndexSizeError when the source or dest is out of bounds, since that is now handled by the clipping.

Comment 93

[Lee Salzman [:lsalzman]]

Attachment: part 2 - remove failure meta-data for WPT for 2dcontext/drawing-images-to-the-canvas/drawimage_canvas_8.html

We have a WPT test for this case now, but it was not enabled since we were expecting it to be broken before... but not anymore!

Comment 94

[Lee Salzman [:lsalzman]]

Attachment: part 3 - remove obsolete canvas test test_2d.drawImage.outsidesource since out of bounds rects no longer throw IndexSizeError

This test only checks that our behavior of throwing IndexSizeError when the source rect is outside the canvas is correct. However, we no longer do, so the test has no more reason to exist.

Comment 95

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8659908 [details] [diff] [review]
part 1 - clip canvas drawImage source/dest rectangles instead of throwing IndexSizeError

Review of attachment 8659908 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +4314,5 @@
>  //
>  
> +static void
> +ClipImageRect(double& aSourceCoord, double& aSourceSize, int32_t aImageSize,
> +              double& aDestCoord, double& aDestSize)

ClipImageDimension seems like a better name than Rect which implies we're doing both axes.

Comment 96

[Lee Salzman [:lsalzman]]

Attachment: part 1 - clip canvas drawImage source/dest rectangles instead of throwing IndexSizeError

Cosmetic change only - just renamed ClipImageRect to ClipImageDimension.

Comment 97

[Lee Salzman [:lsalzman]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab765cedc709

Comment 99

[Carsten Book [:Tomcat]]

Hi Lee,

part 3 failed to apply:

adding 655328 to series file
renamed 655328 -> remove-drawimage-outsidesource-test.diff
applying remove-drawimage-outsidesource-test.diff
patching file dom/canvas/test/test_canvas.html
Hunk #1 FAILED at 3545
1 out of 2 hunks FAILED -- saving rejects to file dom/canvas/test/test_canvas.html.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh remove-drawimage-outsidesource-test.diff

could you take a look, thanks!

Comment 100

[Lee Salzman [:lsalzman]]

(In reply to Carsten Book [:Tomcat] from comment #99)
> Hi Lee,
> 
> part 3 failed to apply:
> 
> adding 655328 to series file
> renamed 655328 -> remove-drawimage-outsidesource-test.diff
> applying remove-drawimage-outsidesource-test.diff
> patching file dom/canvas/test/test_canvas.html
> Hunk #1 FAILED at 3545
> 1 out of 2 hunks FAILED -- saving rejects to file
> dom/canvas/test/test_canvas.html.rej
> patch failed, unable to continue (try -v)
> patch failed, rejects left in working directory
> errors during apply, please fix and refresh
> remove-drawimage-outsidesource-test.diff
> 
> could you take a look, thanks!

The patches from bug 1074733 must be applied before the patches from this bug, and then this patch will apply just fine; I just verified. That's why I marked bug 1074733 as a blocker for this bug, as I was under the impression such dependencies were obeyed on checkins.

Comment 101

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c34a4705aa9
https://hg.mozilla.org/integration/mozilla-inbound/rev/83ada9133617
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad99d8bf748d

Comment 102

[Carsten Book [:Tomcat]]

(In reply to Lee Salzman [:eihrul] from comment #100)

yeah as mentioned on irc by the other sheriffs, some information about the checkin (like only part x from y) or like "checkin bug xyz first" help a lot :)

Comment 103

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6c34a4705aa9
https://hg.mozilla.org/mozilla-central/rev/83ada9133617
https://hg.mozilla.org/mozilla-central/rev/ad99d8bf748d