Bug 13578

Summary: When QD plugins draw to an offscreen bitmap the origin is not correct
Product: WebKit Reporter: Bruce Q. Hammond <bruceq>
Component: Plug-insAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal Keywords: InRadar
Priority: P2    
Version: 523.x (Safari 3)   
Hardware: Mac   
OS: OS X 10.4   
Attachments:
Description Flags
QD plugins now respect the origin of CGBitmapContext
darin: review-
respect the origin implied by the bitmap contexts affine tranformation matrix
darin: review+
Fixes the sign of the Y-Origin adjustment darin: review+

Description Bruce Q. Hammond 2007-05-03 16:55:35 PDT
The CGContextRef's origin  is ignored when the offscreen GWorld is constructed. 
The origin is implied by the CGContextRef's CGAffineTransform.
Comment 1 Bruce Q. Hammond 2007-05-03 17:05:33 PDT
Created attachment 14323 [details]
QD plugins now respect the origin of CGBitmapContext
Comment 2 Darin Adler 2007-05-04 08:01:13 PDT
Comment on attachment 14323 [details]
QD plugins now respect the origin of CGBitmapContext 

Looks fine. But why not use CGPointApplyAffineTransform instead of doing explicit multiplication?
Comment 3 David Harrison 2007-05-04 10:28:33 PDT
rdar://5182103
Comment 4 Timothy Hatcher 2007-05-04 14:12:59 PDT
Comment on attachment 14323 [details]
QD plugins now respect the origin of CGBitmapContext 

(int32)-boundsInWindow.origin.y - (offscreenBounds.bottom + offscreenMatrix.ty * offscreenMatrix.d)

Please explain why you need to multiply offscreenMatrix.ty by offscreenMatrix.d but not do something similar for offscreenMatrix.tx. Please explain this code better in the comments.

Can you also use CGPointApplyAffineTransform like Darin asked?
Comment 5 Darin Adler 2007-05-07 14:50:53 PDT
Comment on attachment 14323 [details]
QD plugins now respect the origin of CGBitmapContext 

Someone told me that Bruce is working on a version of this that uses CGPointApplyAffineTransform, so review- until we have that one.
Comment 6 Bruce Q. Hammond 2007-05-07 16:29:38 PDT
Created attachment 14404 [details]
respect the origin implied by the bitmap contexts affine tranformation matrix

Implements the changes requested by reviewers to go though CGPointApplyAffineTransform to create the origin data.  Personally I don't like this patch as much as the previous one because it requires yet another call to CGPointApplyAffineTransform in order to determine if the axes are flipped.  However it is simply a mater of taste and is functionally identical;  it may be more clear to some maintainers what is happening.
I also now treat Y-Axis and and X-Axis identically even though inverting the X-Axis would be unusual.
Comment 7 Darin Adler 2007-05-07 16:33:57 PDT
Comment on attachment 14404 [details]
respect the origin implied by the bitmap contexts affine tranformation matrix

r=me, assuming that you (Bruce) have a test case that shows this works well, since Safari doesn't use this code path.
Comment 8 Darin Adler 2007-05-08 16:34:44 PDT
(In reply to comment #7)
> r=me, assuming that you (Bruce) have a test case that shows this works well,
> since Safari doesn't use this code path.

Bruce confirmed that he does have a test, so confirmed: r=me.
Comment 9 Mark Rowe (bdash) 2007-05-08 21:31:23 PDT
Landed in r21322.
Comment 10 Mark Rowe (bdash) 2007-05-08 22:39:46 PDT
A build fix in r21323 was needed to keep things building with the ancient version of Xcode the buildslaves are currently using.
Comment 11 Bruce Q. Hammond 2007-05-15 14:29:07 PDT
Created attachment 14573 [details]
Fixes the sign of the Y-Origin adjustment

The Y-origin adjustment was backwards on the previous patch.  This fixes it.
Comment 12 Mark Rowe (bdash) 2007-05-15 16:40:17 PDT
Comment on attachment 14573 [details]
Fixes the sign of the Y-Origin adjustment

It'd be *really* handy if all of these changes had some form of regression testing.  There have been several issues with past iterations of related patches, and without any way to test these (Safari doesn't touch this code path) it's impossible to know if and when this codepath breaks in the future.