295302 - [FIX,crash] Canvas drawImage (9-argument) method renders incorrectly

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Six Apart, Ltd.]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.7) Gecko/20050414 Firefox/1.0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.7) Gecko/20050414 Firefox/1.0.3

http://www.whatwg.org/specs/web-apps/current-work/#images

The 9-argument variant of drawImage() specifies 8 coordinates. When used, the
output is not what you'd expect. Some cursory analysis of the Canvas/Cairo code
seems to point to an incorrect matrix being set up.

The dx/dy/dh/dw seem to be affected by the sx/sy/sh/sw arguments.

My test case was a rudimentary Doom-style renderer using vertical scanlines,
rendering one drawImage per horizontal pixel for the walls.

Reproducible: Always

Steps to Reproduce:
1. create a 256x256 JPG image and a test page with canvas element
2. ...
3. ctx.drawImage( img, 64, 64, 128, 128, 16, 16, 32, 32 );



Actual Results:  
Borked render, random color fringing, incorrect pixels written to.

Expected Results:  
Should render the center 128x128 pixel section of the image to the canvas @ 16,
16 and 32x32.

Comment 1

[Six Apart, Ltd.]

Created test case:
http://akiba.shaderlab.com/test/canvas-bug.html

User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2)
Gecko/20050524 Firefox/1.0+

Comment 2

[Hixie (not reading bugmail)]

That test case crashes for me (X11; 20050520).

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: 295302-canvas-drawimage.patch

This fixes the crash (gfx was lying to us about the image format), and also
fixes the functionality.  It's probably entirely too late to get this in to
1.8b2, but if at all possible, it should be a low-risk crash fix patch.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(Forgot to set a?)

This fixes the crash (gfx was lying to us about the image format), and also
fixes the functionality.  It's probably entirely too late to get this in to
1.8b2, but if at all possible, it should be a low-risk crash fix patch.

Comment 5

[Stuart Parmenter]

Comment on attachment 184782 [details] [diff] [review]
295302-canvas-drawimage.patch

I'm not sure how we can have an image that claims to be RGB_A8 with no
GetHasAlphaMask.. 
http://lxr.mozilla.org/seamonkey/source/gfx/src/shared/gfxImageFrame.cpp#110

I'd prefer to figure out where that is going wrong that check in that part of
the patch.  The rest seems right and I agree with the comment that the
scale/transform stuff should be fixed once we move to 0.5.

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #5)
> (From update of attachment 184782 [details] [diff] [review] [edit])
> I'm not sure how we can have an image that claims to be RGB_A8 with no
> GetHasAlphaMask.. 
> http://lxr.mozilla.org/seamonkey/source/gfx/src/shared/gfxImageFrame.cpp#110

From stepping through it, it looks like nsImageGTK::Optimize/UpdateCachedImage
checks if the image has an all-opaque alpha, and if so, it just tosses it out. 
Maybe it should also change its format at the same time?

Comment 7

[Stuart Parmenter]

Comment on attachment 184782 [details] [diff] [review]
295302-canvas-drawimage.patch

Change the comment about the GetHasAlphaMask() to reflect that the GTK image
stuff sucks.

Comment 8

[chris hofmann]

Comment on attachment 184782 [details] [diff] [review]
295302-canvas-drawimage.patch

a=chofmann for quick landing in a1

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checked in, thanks!