Closed
Bug 245407
Opened 20 years ago
Closed 20 years ago
Convert nsImageMac to use Quartz instead of Quickdraw
Categories
(Core Graveyard :: GFX: Mac, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: jhpedemonte, Assigned: jhpedemonte)
References
Details
Attachments
(3 files, 11 obsolete files)
Some of the Quickdraw functions (particularly CopyDeepMask) have some bugs (see bug 195022, bug 239701). While we have some workarounds for those issues, it would be nice if we could use the OS functions. And it appears that the Quartz functions do not have those bugs.
Assignee | ||
Comment 1•20 years ago
|
||
This is what I've got so far. Only made changes to the nsImageMac::Draw() function, but it looks pretty good. And it doesn't exhibit any of the Quickdraw bugs.
Comment 2•20 years ago
|
||
Doesn't this mean that we'd have to rewrite the rest of Mac GFX to use quartz? What about clipping etc?
Assignee | ||
Comment 3•20 years ago
|
||
(In reply to comment #2) > Doesn't this mean that we'd have to rewrite the rest of Mac GFX to use quartz? I don't think so. QDBeginCGContext/QDEndCGContext allow us to use Quartz in the middle of Quickdraw calls. The attached patch seems, for the most part, to be working quite well. > What about clipping etc? Not sure. I saw some odd clipping issues at one point when I was writing the patch, but I don't know if that was a problem with my some of my earlier code, or an issue that is still around. I'd like to get nsImageMac coded up properly with Quartz, and then test it like crazy to see if there are going to be any issues from intermingling Quarz and Quickdraw.
Assignee | ||
Updated•20 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•20 years ago
|
Assignee: sfraser → jhpedemonte
Status: ASSIGNED → NEW
Assignee | ||
Comment 4•20 years ago
|
||
Draw() and DrawToImage() seem to be working well. However, DrawTile() (for background images) has issues. I started using CGPattern, since having the OS do the tiling for us should be faster than manually doing it. But there are some odd clipping issues with background images that I have not been able to figure out. Plus, CGPattern is only available in 10.2 and up, so I'm going to have to do the manual tiling anyway, at least for 10.1. This patch also gets rid of GWorlds and associated functions. Plus, I removed the icon functions, since they are no longer used (remnants from OS 9?).
Assignee | ||
Updated•20 years ago
|
Attachment #149880 -
Attachment is obsolete: true
Comment 5•20 years ago
|
||
I thought that the icon functions were used by the code that shows file icons for downloads etc.
Assignee | ||
Comment 6•20 years ago
|
||
Nope, they don't seem to be called anywhere. And when I brought up the Download Manager in my build, there were icons displayed. So it must be coming from some other place.
Comment 7•20 years ago
|
||
fwiw, camino on the trunk is 10.2+ only. we've dropped 10.1 support, maybe mozilla might want to think about it too?
Assignee | ||
Comment 8•20 years ago
|
||
What was the reason for dropping 10.1 support for Camino? I don't think the tiling code is reason enough for Mozilla; it shouldn't be too hard to create a Quartz version of the DrawTileQuickly function in order to make this work on 10.1. Or at the very least, have those users fall back on SlowTile. Plus, I'm still having some problems with CGPattern. It's working for the most part, but it's still giving me some issues.
Comment 9•20 years ago
|
||
a lot of stuff in the cocoa frontend was the main reason for dropping 10.1. just wanted to throw that out there. there's also quartz apis (AddPathToPath, for example) which don't exist in 10.1 and we may want to use those in the more general quartz gfx port.
Assignee | ||
Comment 10•20 years ago
|
||
Fixed the clipping issues with CGPattern tiling and SlowTile. Also, the CGPattern code won't get run on anything older than 10.2. Still having some problems with the CGPattern code. Has to do with the transform that I am using to offset the image when scrolling. Currently, I am using aTileRect.height for the vertical offset, but aTileRect.height changes whether there is a horizontal scrollbar or not, causing the background to shift when a horizontal scrollbar appears (something similar happens in the X direction). So I tried using |portRect.bottom - portRect.top|, but scrolling is still somewhat broken with that, and the background image does not start in the correct location. It's all quite confusing!
Assignee | ||
Updated•20 years ago
|
Attachment #152611 -
Attachment is obsolete: true
Assignee | ||
Comment 11•20 years ago
|
||
I could never get CGPattern to work correctly, so I rolled a new DrawTileQuickly. Since Quartz doesn't have the capacity to blit form one context to another, and since I didn't want to go through the loop continuously creating and destroying CGImageRefs, the code just manipulates the memory directly using memcpy. I have left the CGPattern code there, though, in case someone ever finds out what I was doing wrong. I still have to use GWorlds and CopyBits for ConvertToPICT, since you can't create a PICT in Quartz. The recommended way is to convert the image to a PDF, and put that on the clipboard, but that would take a rewrite of the clipboard code. And I finally figured out some of the remaining clipping problems. It seems that ClipCGContextToRegion is buggy; so I used QDRegionToRects to create a CG path and clip the context to that path. Now, everything works great. As for performance, this new code is slightly faster (~2%), but probably not noticeable. Have a look. Compile it and test it out, particulary if you have Mac OS X 10.1 or 10.2, since I don't have those systems to test on. Plus, I also have not tested this with Camino. I may get to that today or tomorrow.
Assignee | ||
Updated•20 years ago
|
Attachment #153052 -
Attachment is obsolete: true
Assignee | ||
Comment 12•20 years ago
|
||
Just compiled this patch in Camino, and it looks good.
Assignee | ||
Updated•20 years ago
|
Attachment #154389 -
Flags: review?(sfraser)
Comment 13•20 years ago
|
||
I'm pretty wary of this patch; can you produce test builds of Mozilla/Firefox and Camino with it?
Assignee | ||
Comment 14•20 years ago
|
||
Simon: I personally cannot create test builds for release, due to legal reasons here. So someone else would have to do it. My feeling is that we can get this checked in on the trunk, and use the nightly builds as our test builds. I'll even post something to the n.p.m.macosx newsgroup, asking for testers. There are about 6 days left until 1.8a3; if the code doesn't need to be backed out by then, then we'll get even more testing in 1.8a3. I just really want the code to get some testing before 1.8beta goes out, and 1.8a3 looks like the best bet.
Assignee | ||
Comment 15•20 years ago
|
||
*** Bug 254495 has been marked as a duplicate of this bug. ***
Comment 16•20 years ago
|
||
test mozilla build: http://ftp.mozilla.org/pub/mozilla.org/mozilla/nightly/2004-08-06-13-test-245407/mozilla-mac-MachO.dmg.gz created from the same tree which created the trunk build this morning (2004-08-06-08-trunk); after a distclean and applying the patch. Test build should have talkback, so any crashes should be trackable.
Comment 17•20 years ago
|
||
The problem reported in bug 254495 seems to be gone in the test build provided by Leaf.
Updated•20 years ago
|
Flags: blocking-aviary1.0mac?
Comment 18•20 years ago
|
||
ne need to mark this one blocking aviary Mac
Flags: blocking-aviary1.0mac? → blocking-aviary1.0mac-
Comment 19•20 years ago
|
||
Is the patch for this bug the cause of this bug? "Scrollbars misdrawn in 8/11/04 Mozilla Mac build with Classic" theme bug 255400
Comment 20•20 years ago
|
||
I put test camino0.8 build in this page: http://caminofreak.hp.infoseek.co.jp/subset/caminol10nJP.html bug 239701 seem to be fixed.
Comment 21•20 years ago
|
||
I notice that my quartz-camino use incorrect font at viewing html source. quartz-mozilla in comment#16 is no problem.
Assignee | ||
Comment 22•20 years ago
|
||
Paul: This patch has not been checked in yet, so it could not cause a regression. waverider: Thanks for the camino build. My view source text looks just fine with this build. Plus, this patch doesn't affects fonts. sfraser: We've got test builds up for both Mozilla and Camino, and I haven't heard of any issues. When you get the chance, could you review the patch?
Comment 23•20 years ago
|
||
Waverider want to say his japanese resources get in official camino build instead of current japanese resources in his japanese modest way(says his japanese blog). He seems to believe superstition that he says so directly cause tangle. (I think his superstition causes tangle now and many japanese users believe that his direct saying save it) http://forums.mozillazine.org/viewtopic.php?p=722708 http://perso.hirlimann.net/~ludo/blog/archives/000272.html I don't know whether caminofreak is waverider or not. but they use same web site.
Comment 24•20 years ago
|
||
I'm not having any problems with this patch, and things are even performing at least a few percent faster in my simple benchmarking.
Comment 25•20 years ago
|
||
(In reply to comment #24) > I'm not having any problems with this patch, and things are even performing at > least a few percent faster in my simple benchmarking. dito
Comment 26•20 years ago
|
||
My wrong preferences caused bug of comment#21. Viewing html don't have any problem after correcting my wrong preferences.
Assignee | ||
Comment 27•20 years ago
|
||
Built Firefox with this patch, and noticed some odd drawing issues with the throbber. The issue was that I was not clearing the bitmap context before drawing into it in DrawToImage(). I also clear it in LockImagePixels().
Attachment #154389 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #156442 -
Flags: review?(sfraser)
Assignee | ||
Updated•20 years ago
|
Attachment #154389 -
Flags: review?(sfraser)
Comment 28•20 years ago
|
||
Hi, I'm experiencing a bug with the following site and tab browsing: http://www.carillionbtp.fr/indexmat.htm I open this site in one site, and other sites in other tabs, play with the sites, scrolling bar, back on the .fr site, and one time more on any other site, and the top of .fr site appears on other tabs. Here is my current build version; Mozilla 1.8a3 Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a3) Gecko/20040806 (don't see no message about Quartz patch, but I'm sure this is the patched build) Don't know if it appears with other build of mozilla, but I may test if you tell me the exact one to test. Regards, Kalou
Comment 29•20 years ago
|
||
(In reply to comment #28) > Hi, > > > I'm experiencing a bug with the following site and tab browsing: > http://www.carillionbtp.fr/indexmat.htm > > I open this site in one site, and other sites in other tabs, play > with the sites, scrolling bar, back on the .fr site, and one time more > on any other site, > > and the top of .fr site appears on other tabs. > > Here is my current build version; > > Mozilla 1.8a3 > Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a3) Gecko/20040806 > (don't see no message about Quartz patch, but I'm sure this is the patched build) > > Don't know if it appears with other build of mozilla, but I may test > if you tell me the exact one to test. > > > Regards, > > Kalou > > Actually, you've just experienced bug 162134 ;)
Comment 30•20 years ago
|
||
When patch is applied, a gray quadrangle will be displayed if wheel scrolling is performed on the following pages. There is no problem in official Camino nightly build. Mac OS X 10.3.5 Camino, 2004082704 (v0.8+)
Comment 31•20 years ago
|
||
oops... The URL is as follows. http://northiland.upper.jp/news/software/new_firefox_and_thunderbird.htm?seemore=y
Comment 32•20 years ago
|
||
Comment on attachment 156442 [details] [diff] [review] patch v1.1 Sorry, I don't have time to sr this before I leave on vacation. :-(
Attachment #156442 -
Flags: review?(sfraser) → review?
Assignee | ||
Comment 33•20 years ago
|
||
(In reply to comment #30) I don't have a scroll mouse to test with. Can anyone copied on this bug confirm this issue?
Assignee | ||
Updated•20 years ago
|
Attachment #156442 -
Flags: review?
Assignee | ||
Comment 34•20 years ago
|
||
Take the kTilingCopyThreshold implementation from the old nsImageMac.
Attachment #156442 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #159099 -
Flags: review?(pinkerton)
Assignee | ||
Comment 35•20 years ago
|
||
sfraser, now that you are back, can you take a look at this patch?
Assignee | ||
Updated•20 years ago
|
Attachment #159099 -
Flags: review?(pinkerton) → review?(peterv)
Assignee | ||
Comment 36•20 years ago
|
||
OK, so John Aas pointed out that this bug causes some issues in Camino. Specifically, some images seem to move around on the screen. Go to http://www.surfstation.lu/ and resize the Camino window, and you'll see some images move around on the screen. So I added the following printf in the StartQuartsDrawing() function: // Translate to QuickDraw coordinate system Rect portRect; ::GetPortBounds(mPort, &portRect); ::CGContextTranslateCTM(context, 0, (float)(portRect.bottom - portRect.top)); ::CGContextScaleCTM(context, 1, -1); + printf("==> portRect.bottom = %d, portRect.top = %d, portRect height = %d\n", + portRect.bottom, portRect.top, portRect.bottom - portRect.top); I then created a reduced testcase (I'll post it soon), and tested Firefox and Camino. In Firefox, when I resize the window, I see portRect.bottom change. In Quartz, the origin is in the lower left of a window, so the portRect height is used to move the origin to the top left of the window. In Camino, portRect.bottom stays the same, and the height is always 40 (in the test case) when resizing. My guess is that the top frame in the test case has a height of 40, and in Camino, each frame has it's own QD port. Not sure how to fix that, though.
Comment 37•20 years ago
|
||
yes, in cocoa widget, each html frame is a nsChildView which is its own NSQuickDrawView. carbon "widgets" all share a common quickdraw port, that of the window. maybe we should focus on doing a quartz gfx and remove the dependence on NSQuickdrawView entirely! that would make this problem go away :D
Assignee | ||
Comment 38•20 years ago
|
||
Yeah, but I don't have the time to move all of gfx to Quartz, and I wanted to get this in so we wouldn't get any more bugs about the broken QD image code. Is there anyway to make this patch work in Camino?
Assignee | ||
Comment 39•20 years ago
|
||
pinkerton, is there a way to get a Quartz context (CGContextRef) directly in Camino, rather than calling ::QDBeginCGContext()?
Comment 40•20 years ago
|
||
where does camino do that?
Assignee | ||
Comment 41•20 years ago
|
||
On irc, pinkerton mentioned that we can get the cgcontext by calling [[NSGraphicsContext currentContext] graphicsPort]. Now, how do we plug that into StartQuartzDrawing()?
Assignee | ||
Comment 42•20 years ago
|
||
I created a function that would call the Cocoa methods mentioned above. This does seem to fix the moving images. Unfortunately, now only about half the images actually display. Not sure why that is. Josh, Pinkerton, any ideas?
Comment 43•20 years ago
|
||
*** Bug 279140 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 44•20 years ago
|
||
Josh, Pinkerton, or Simon, can one of you guys help me get this working on Camino? Right now this seems like the best way to fix all the image bugs.
Comment 45•20 years ago
|
||
It's probably only legal to call [[NSGraphicsContext currentContext] graphicsPort] when lockFocus is called on a view: do we know that images are only ever drawn inside of lockFocus? Also, we're using NSQuickdrawViews. I'm not sure if you can mix and match QuickDraw drawing with drawing to an NSView's CGContextRef. You might be better off sticking to the quickdraw port -> CGContextRef code path.
Assignee | ||
Comment 46•20 years ago
|
||
Simon: Then I'm stuck here. Don't know why Camino does what it does, although I assume that it's because each frame in Camino has its own QD port, whereas on Firefox it's all on port. Any ideas?
Assignee | ||
Comment 47•20 years ago
|
||
According to the online samples for NSQuickDrawView, it should be possible to
just use the qdPort directly from the view (nsDrawingSurfaceMac::mPort is the
view's qdPort); but that doesn't seem to work when resizing the window.
So I took Simon's suggestion of locking the focus on the view before getting
its context. This seems to work well.
> Also, we're using NSQuickdrawViews. I'm not sure if you can mix and match
> QuickDraw drawing with drawing to an NSView's CGContextRef.
According to the online samples, there shouldn't be any problem with this, as
long as I don't do any QD calls between QDBegin/QDEndCGContext calls.
Can anyone else try this patch and the previous one and confirm that Camino
works fine?
Assignee | ||
Updated•20 years ago
|
Attachment #165638 -
Attachment is obsolete: true
Comment 48•20 years ago
|
||
Given the discussion and the video linked here: http://episteme.arstechnica.com/eve/ubb.x/a/tpc/f/ 8300945231/m/886008328631/r/886008328631#886008328631 which basically say that a) Quartz is getting a lot faster in 10.4, and b) Quickdraw is being deprecated, would it make sense to file a bug about moving all of Mozilla's graphics stuff to Quartz? Basically a metabug about it depending on ones like this and the ATSUI text bug.
Comment 49•20 years ago
|
||
pedemonte - I have been using the "camino only patch" on top of "patch v1.2" heavily for about 2-3 days now and everything works great. I think its time to try to get this in.
Assignee | ||
Comment 50•20 years ago
|
||
Attachment #159099 -
Attachment is obsolete: true
Attachment #172698 -
Attachment is obsolete: true
Attachment #172979 -
Flags: superreview?(sfraser_bugs)
Attachment #172979 -
Flags: review?(pinkerton)
Assignee | ||
Updated•20 years ago
|
Attachment #159099 -
Flags: review?(peterv)
Comment 51•20 years ago
|
||
I have used the 1.3 patch for a couple of days now with no problems.
Comment 52•20 years ago
|
||
since gerv just went around fixing license headers, does nsCocoaImageUtils have the correct one? +#ifdef MOZ_WIDGET_COCOA +extern CGContextRef Cocoa_LockFocus(void* view); +extern void Cocoa_UnlockFocus(void* view); +#endif how about some comments on why these are necessary and what they do, both here and in the implementation file. mLockFlags = 0; mIsOffscreen = PR_FALSE; mIsLocked = PR_FALSE; - +#ifdef MOZ_WIDGET_COCOA + mWidgetView = NULL; +#endif ewwwwwww someone put tabs into nsDrawingSurfaceMac.cpp. should we bother cleaning them up? + CGRect rect = ::CGRectMake(aRect->left, aRect->top, + aRect->right - aRect->left, + aRect->bottom - aRect->top); what if |aRect| is null? + // In Camino, we get the context directly from the NSQuickDrawView shouldn't make statements based on product name, what happens when Firefox goes over to cocoa widget as well? + // Simple wrapper functions for drawing with Quartz into this drawing + // surface. You MUST call EndQuartzDrawing() when you are done. + CGContextRef StartQuartzDrawing(); + void EndQuartzDrawing(CGContextRef aContext); would it be worth making a stack-based class (maybe |StQuartzDrawing|) that handles calling start/end automatically? +#ifdef MOZ_WIDGET_COCOA + void* mWidgetView; // ref to NSView +#endif i assume this is a weak reference (not retained)? you should be explicit. + if (mAlphaDepth == 8) + data = (PRUint8*) PR_Malloc(mWidth * mHeight * 4); nit, 2space indent +#define NS_GET_BIT(rowptr, x) (rowptr[(x)>>3] & (1<<(7-(x)&0x7))) can we make this an inline function to get some type checking? + if (mPendingUpdate) + UpdateCachedImage(); why would we need to do this? what about the cache isn't ready at first? can you explain this in some comments? nsDrawingSurfaceMac* surface = static_cast<nsDrawingSurfaceMac*>(aSurface); I know this is code that was already there, but I never liked it. Can't we make a nsIDrawingSurfaceMac like we've done elsewhere? or is it overkill? + // set new image in destination + ::CGImageRelease(dest->mImage); + PR_Free(dest->mImageBits); + dest->mImage = newImageRef; + dest->mImageBits = bitmap; make this a new method, we might want to replace the bits elsewhere as well and keeping the memory mgmt in one place is better. also, are we guaranteed that |dest| or |dest->mImage| is non-null? +nsresult nsImageMac::Optimize(nsIDeviceContext* aContext) how about a function comment that describes what's being optimized and how? the routine name by itself doesn't give any clue to what's going on (it's such an overloaded term these days) + bitmapContext = ::CGBitmapContextCreate(mImageBits, mWidth, mHeight, 8, + mRowBytes, cs, + kCGImageAlphaNoneSkipFirst); what's the |8| in this context? NS_IMETHODIMP nsImageMac::ConvertToPICT(PicHandle* outPicture) has this functionality been tested? i could easily see people forgetting about testing it until later on down the road. + CGColorSpaceRef cs = ::CGColorSpaceCreateDeviceRGB(); as we move more and more to CG, would stack-based "smart pointers" for color spaces and device contexts be good? We do a lot of "create color space ... release color space" and it's very leak-prone if someone just sticks a simple |if (!foo) return NS_ERROR| anywhere in between. + CGColorSpaceRef cs = ::CGColorSpaceCreateDeviceRGB(); + + PRUint32 tiledCols = (aTileRect.width + aSXOffset + mWidth - 1) / mWidth; + PRUint32 tiledRows = (aTileRect.height + aSYOffset + mHeight - 1) / mHeight; + + // The manual blitting that we do below can be expensive for large background + // images, for which we have few tiles. Plus, the SlowTile call eventually + // calls the Quartz drawing functions, which can take advantage of hardware + // acceleration. So if we have few tiles, we just call SlowTile. + const PRUint32 kTilingCopyThreshold = 64; + if (tiledCols * tiledRows < kTilingCopyThreshold) { + return SlowTile(aContext, aSurface, aSXOffset, aSYOffset, 0, 0, aTileRect); + } for example, this will leak |cs|. there prolly are a few more in this vein. + PRPackedBool mPendingUpdate; + PRBool mOptimized; can you add comments on when each of these are true vs. false?
Assignee | ||
Comment 53•20 years ago
|
||
(In reply to comment #52) > since gerv just went around fixing license headers, does nsCocoaImageUtils have > the correct one? License is up to date. > what if |aRect| is null? I'm not sure that will ever happen. I've been using a build with this patch for a long time, and never had problems. Of course, since Apple provides no documentation on this function, I don't know what to expect. I'll just add a check. > + // Simple wrapper functions for drawing with Quartz into this drawing > + // surface. You MUST call EndQuartzDrawing() when you are done. > + CGContextRef StartQuartzDrawing(); > + void EndQuartzDrawing(CGContextRef aContext); > > would it be worth making a stack-based class (maybe |StQuartzDrawing|) that > handles calling start/end automatically? I'm not sure how to code that, especially since I need to get a CGContextRef in return, so I can use the Quartz functions. How were you thinking of doing it? > NS_IMETHODIMP > nsImageMac::ConvertToPICT(PicHandle* outPicture) > > has this functionality been tested? i could easily see people forgetting about > testing it until later on down the road. Yes, I tested this by dragging images in and out of Mozilla. Took me a while, but I think I got it right. I'll attach a new patch shortly.
Comment 54•20 years ago
|
||
"I'm not sure how to code that, especially since I need to get a CGContextRef in return, so I can use the Quartz functions. How were you thinking of doing it?" a utility class that takes a CGContextRef as its ctor's out parameter and calls StartQuartzDrawing in the ctor and EndQuartzDrawing in the dtor. You use it whenever you want to do drawing. CGContextRef ref = NULL; StQuartzDrawing q (ref); ...draw with |ref| EndQD is automatically called when the scope ends.
Comment 55•20 years ago
|
||
"I'm not sure that will ever happen. I've been using a build with this patch for a long time, and never had problems. Of course, since Apple provides no documentation on this function, I don't know what to expect. I'll just add a check." it's got nothing to do with the Apple routine, you're passing |aRect->...| to the fucntion which will crash before it even gets to apple code.
Assignee | ||
Comment 56•20 years ago
|
||
What I meant was that the function |CreatePathFromRectsProc| is only ever called from the system function |QDRegionToRects|, and I'm not sure if |QDRegionToRects| will ever call |CreatePathFromRectsProc| with an empty rect.
Assignee | ||
Comment 57•20 years ago
|
||
Patch with Pinkerton's suggestions. Pinkerton, wasn't sure what you meant about |nsIDrawingSurfaceMac|. Can you explain?
Attachment #172979 -
Attachment is obsolete: true
Attachment #173391 -
Flags: review?(pinkerton)
Assignee | ||
Updated•20 years ago
|
Attachment #172979 -
Flags: superreview?(sfraser_bugs)
Attachment #172979 -
Flags: review?(pinkerton)
Assignee | ||
Comment 58•20 years ago
|
||
Comment on attachment 173391 [details] [diff] [review] patch v1.4 1.8 beta is tomorrow night. Any chance we can get this reviewed and checked in by then?
Attachment #173391 -
Flags: superreview?(sfraser_bugs)
Comment 59•20 years ago
|
||
+public: + StQuartzDrawing(nsDrawingSurfaceMac* aSurface, CGContextRef* aContext); it's invalid to pass null for |aSurface| or |aContext|, you should document that explicitly, especially for |aContext| where someone might think they can pass null if they don't want the context (for some reason). You should also document that callers must not release |aContext| as the class owns the context. + CGContextRef* mContext; you shouldn't store a pointer to the ref. This should be just a |CGContextRef|. Storing a ptr means that you need an externally defined CGContextRef in order to have the storage defined for this to work at all. As a result, the class cannot stand on its own. Also it's an owned reference, you should mention that. +private: + CGColorSpaceRef* cs; same comment as above, don't store a ptr to the ref, and the caller must not release it. + // Sets the given image and bitmap into the nsImageMac object. + void SetImageIntoDest(CGImageRef aNewImage, PRUint8* aNewBitmap, + nsImageMac* aDest); SetImageIntoDest() takes ownership of |aNewImage|. This either needs to be explicitly stated so callers don't release it or it should retain it internally and make the caller release it.
Assignee | ||
Comment 60•20 years ago
|
||
SVG support for Mac needs to make use of the Start/EndQuartzDrawing, so I made them functions again. Shouldn't be a problem, though; if someone forgets to call EndQuartzDrawing, then most things won't draw, and they'll get many error messages on the console. Patch also includes Pinkerton's suggestions. I made |SetImageIntoDest()| retain the new image.
Attachment #173391 -
Attachment is obsolete: true
Attachment #173765 -
Flags: review?(pinkerton)
Comment 61•20 years ago
|
||
I'm working on a review of the patch.
Comment 62•20 years ago
|
||
I ran the latest patch through the pageload test on a dual g5/1.8, 1.25 GB, 10.3.8. Here are the results: before: 298 ms after: 286 ms
Updated•20 years ago
|
Flags: blocking-aviary1.0mac-
Comment 63•20 years ago
|
||
Assignee | ||
Comment 64•20 years ago
|
||
> This loop cries out for Altivec. I wonder if vecLib has anything suitable? We can handle this in another bug once this gets checked in. > This is probably the most common case. Maybe we can set the alpha component > to 255 in libpr0n, and avoid an extra pass over the image? Also handle this in another bug. Actually, if we're looking at changing libpr0n, we may as well keep it from splitting the alpha data from the image data. > CGReleaseDataProcPtr dataReleaser = nsnull; Where did you get this? I can't find it anywhere in the developer headers.
Assignee | ||
Comment 65•20 years ago
|
||
Patch with Simon's suggestions.
> It would be nice to figure out this crash.
> Speaking of printing, do images still print OK with these
> changes? What about GIFs with transparency?
I couldn't get it to crash printing anymore. I wonder if I changed something
that 'fixed' this. Images print fine, as do GIFs with transparency.
Attachment #173765 -
Attachment is obsolete: true
Attachment #174646 -
Flags: review?(sfraser_bugs)
Assignee | ||
Updated•20 years ago
|
Attachment #173391 -
Flags: superreview?(sfraser_bugs)
Attachment #173391 -
Flags: review?(pinkerton)
Assignee | ||
Updated•20 years ago
|
Attachment #173765 -
Flags: review?(pinkerton)
Comment 66•20 years ago
|
||
Comment on attachment 174646 [details] [diff] [review] patch v1.6 Looks good! Thanks for all the hard work. BTW, CGReleaseDataProcPtr only seems to be in CodeWarrior headers, not those in the framework.
Attachment #174646 -
Flags: review?(sfraser_bugs) → review+
Assignee | ||
Comment 67•20 years ago
|
||
Comment on attachment 174646 [details] [diff] [review] patch v1.6 tor, can you please look over the nsIImage.h, gfxImageFrame.cpp, and imgContainerGIF.cpp changes.
Attachment #174646 -
Flags: superreview?(tor)
Comment 68•20 years ago
|
||
Comment on attachment 174646 [details] [diff] [review] patch v1.6 The changes to those three files look file, though a global define like that should probably be prefixed with MOZ_. sr=tor for that portion.
Assignee | ||
Comment 69•20 years ago
|
||
Checked in. Let's see what happens.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Comment 70•20 years ago
|
||
I noticed that the comments for DrawTileQuickly have been removed. Does this mean that DrawTileQuickly is (in theory) able to tile images that are partially decoded? Question is related to Bug 274244, which I need to update the patch to trunk, and need to know whether I should skip DrawTileQuickly if image is not complete.
Assignee | ||
Comment 71•20 years ago
|
||
ArronM: DrawTileQuickly now does the tiling manually, so I don't think there should be any problems with partially decoded images. Of course, it would be great if you could point me to a worthy testcase for this.
Comment 72•20 years ago
|
||
Maybe caused printing crash in bug 283852?
Attachment #174646 -
Flags: superreview?(tor)
Comment 73•20 years ago
|
||
Could this have caused bug 287058?
Assignee | ||
Comment 74•20 years ago
|
||
*** Bug 258023 has been marked as a duplicate of this bug. ***
Updated•16 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•