245407 - Convert nsImageMac to use Quartz instead of Quickdraw

Status: RESOLVED FIXED

(Core Graveyard :: GFX: Mac, defect)

Description

[jhp (no longer active)]

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.

Comment 1

[jhp (no longer active)]

Attachment: work in progress

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

[Simon Fraser [no longer active]]

Doesn't this mean that we'd have to rewrite the rest of Mac GFX to use quartz?
What about clipping etc?

Comment 3

[jhp (no longer active)]

(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.

Comment 4

[jhp (no longer active)]

Attachment: work in progress v2

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?).

Comment 5

[Simon Fraser [no longer active]]

I thought that the icon functions were used by the code that shows file icons
for downloads etc.

Comment 6

[jhp (no longer active)]

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

[Mike Pinkerton (not reading bugmail)]

fwiw, camino on the trunk is 10.2+ only. we've dropped 10.1 support, maybe
mozilla might want to think about it too?

Comment 8

[jhp (no longer active)]

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

[Mike Pinkerton (not reading bugmail)]

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.

Comment 10

[jhp (no longer active)]

Attachment: work in progress v2.1

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!

Comment 11

[jhp (no longer active)]

Attachment: patch

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.

Comment 12

[jhp (no longer active)]

Just compiled this patch in Camino, and it looks good.

Comment 13

[Simon Fraser [no longer active]]

I'm pretty wary of this patch; can you produce test builds of Mozilla/Firefox
and Camino with it?

Comment 14

[jhp (no longer active)]

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.

Comment 15

[jhp (no longer active)]

*** Bug 254495 has been marked as a duplicate of this bug. ***

Comment 16

[Daniel (Leaf) Nunes]

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

[Stefan [:stefanh]]

The problem reported in bug 254495 seems to be gone in the test build provided
by Leaf.

Comment 18

[Ludovic Hirlimann [:Usul]]

ne need to mark this one blocking aviary Mac

Comment 19

[Paul Bergsagel]

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

[waverider]

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

[waverider]

I notice that my quartz-camino use incorrect font at viewing html source.
quartz-mozilla in comment#16 is no problem.

Comment 22

[jhp (no longer active)]

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

[Hidehiro Kozawa]

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

[Josh Aas]

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

[Ludovic Hirlimann [:Usul]]

(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

[waverider]

My wrong preferences caused bug of comment#21.
Viewing html don't have any problem after correcting my wrong preferences.

Comment 27

[jhp (no longer active)]

Attachment: patch v1.1

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().

Comment 28

[Olivier Kaloudoff]

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

[Stefan [:stefanh]]

(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

[Hiro]

Attachment: The screen shot as which the gray quadrangle was displayed

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

[Hiro]

oops...
The URL is as follows.
http://northiland.upper.jp/news/software/new_firefox_and_thunderbird.htm?seemore=y

Comment 32

[Simon Fraser [no longer active]]

Comment on attachment 156442 [details] [diff] [review]
patch v1.1

Sorry, I don't have time to sr this before I leave on vacation.  :-(

Comment 33

[jhp (no longer active)]

(In reply to comment #30)
I don't have a scroll mouse to test with.  Can anyone copied on this bug confirm
this issue?

Comment 34

[jhp (no longer active)]

Attachment: patch v1.2

Take the kTilingCopyThreshold implementation from the old nsImageMac.

Comment 35

[jhp (no longer active)]

sfraser, now that you are back, can you take a look at this patch?

Comment 36

[jhp (no longer active)]

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

[Mike Pinkerton (not reading bugmail)]

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

Comment 38

[jhp (no longer active)]

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?

Comment 39

[jhp (no longer active)]

pinkerton, is there a way to get a Quartz context (CGContextRef) directly in
Camino, rather than calling ::QDBeginCGContext()?

Comment 40

[Mike Pinkerton (not reading bugmail)]

where does camino do that?

Comment 41

[jhp (no longer active)]

On irc, pinkerton mentioned that we can get the cgcontext by calling
[[NSGraphicsContext currentContext] graphicsPort].  Now, how do we plug that
into StartQuartzDrawing()?

Comment 42

[jhp (no longer active)]

Attachment: patch - try to fix camino

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

[Stefan [:stefanh]]

*** Bug 279140 has been marked as a duplicate of this bug. ***

Comment 44

[jhp (no longer active)]

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

[Simon Fraser [no longer active]]

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.

Comment 46

[jhp (no longer active)]

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?

Comment 47

[jhp (no longer active)]

Attachment: camino only patch

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?

Comment 48

[David Smith]

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

[Josh Aas]

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.

Comment 50

[jhp (no longer active)]

Attachment: patch v1.3 (moz & camino)

Comment 51

[weijia]

I have used the 1.3 patch for a couple of days now with no problems.

Comment 52

[Mike Pinkerton (not reading bugmail)]

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?

Comment 53

[jhp (no longer active)]

(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

[Mike Pinkerton (not reading bugmail)]

"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

[Mike Pinkerton (not reading bugmail)]

"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.

Comment 56

[jhp (no longer active)]

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.

Comment 57

[jhp (no longer active)]

Attachment: patch v1.4

Patch with Pinkerton's suggestions.

Pinkerton, wasn't sure what you meant about |nsIDrawingSurfaceMac|.  Can you
explain?

Comment 58

[jhp (no longer active)]

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?

Comment 59

[Mike Pinkerton (not reading bugmail)]

+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.

Comment 60

[jhp (no longer active)]

Attachment: patch v1.5

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.

Comment 61

[Simon Fraser [no longer active]]

I'm working on a review of the patch.

Comment 62

[Brian Ryner (not reading)]

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

Comment 63

[Simon Fraser [no longer active]]

Attachment: Comments on the patch.

Comment 64

[jhp (no longer active)]

> 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.

Comment 65

[jhp (no longer active)]

Attachment: patch v1.6

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.

Comment 66

[Simon Fraser [no longer active]]

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.

Comment 67

[jhp (no longer active)]

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.

Comment 68

[tor]

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.

Comment 69

[jhp (no longer active)]

Checked in.  Let's see what happens.

Comment 70

[ArronM (:paper)]

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.

Comment 71

[jhp (no longer active)]

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

[Simon Fraser [no longer active]]

Maybe caused printing crash in bug 283852?

Comment 73

[Martijn Wargers (dead)]

Could this have caused bug 287058?

Comment 74

[jhp (no longer active)]

*** Bug 258023 has been marked as a duplicate of this bug. ***