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)

PowerPC
macOS
defect
Not set
normal

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.
Attached patch work in progress (obsolete) — Splinter Review
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.
Doesn't this mean that we'd have to rewrite the rest of Mac GFX to use quartz?
What about clipping etc?
(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.

Status: NEW → ASSIGNED
Assignee: sfraser → jhpedemonte
Status: ASSIGNED → NEW
Attached patch work in progress v2 (obsolete) — Splinter Review
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?).
Attachment #149880 - Attachment is obsolete: true
I thought that the icon functions were used by the code that shows file icons
for downloads etc.
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.
fwiw, camino on the trunk is 10.2+ only. we've dropped 10.1 support, maybe
mozilla might want to think about it too?
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.
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.
Attached patch work in progress v2.1 (obsolete) — Splinter Review
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!
Attachment #152611 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #153052 - Attachment is obsolete: true
Just compiled this patch in Camino, and it looks good.
Attachment #154389 - Flags: review?(sfraser)
I'm pretty wary of this patch; can you produce test builds of Mozilla/Firefox
and Camino with it?
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.
*** Bug 254495 has been marked as a duplicate of this bug. ***
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.
The problem reported in bug 254495 seems to be gone in the test build provided
by Leaf.
Flags: blocking-aviary1.0mac?
ne need to mark this one blocking aviary Mac
Flags: blocking-aviary1.0mac? → blocking-aviary1.0mac-
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
I put test camino0.8 build in this page:
http://caminofreak.hp.infoseek.co.jp/subset/caminol10nJP.html

bug 239701 seem to be fixed.
I notice that my quartz-camino use incorrect font at viewing html source.
quartz-mozilla in comment#16 is no problem.
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?
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.
I'm not having any problems with this patch, and things are even performing at
least a few percent faster in my simple benchmarking.
(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
My wrong preferences caused bug of comment#21.
Viewing html don't have any problem after correcting my wrong preferences.
Attached patch patch v1.1 (obsolete) — Splinter Review
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
Attachment #156442 - Flags: review?(sfraser)
Attachment #154389 - Flags: review?(sfraser)
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

   
(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 ;)
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 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?
(In reply to comment #30)
I don't have a scroll mouse to test with.  Can anyone copied on this bug confirm
this issue?
Attachment #156442 - Flags: review?
Attached patch patch v1.2 (obsolete) — Splinter Review
Take the kTilingCopyThreshold implementation from the old nsImageMac.
Attachment #156442 - Attachment is obsolete: true
Attachment #159099 - Flags: review?(pinkerton)
sfraser, now that you are back, can you take a look at this patch?
Attachment #159099 - Flags: review?(pinkerton) → review?(peterv)
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.
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
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?
pinkerton, is there a way to get a Quartz context (CGContextRef) directly in
Camino, rather than calling ::QDBeginCGContext()?
where does camino do that?
On irc, pinkerton mentioned that we can get the cgcontext by calling
[[NSGraphicsContext currentContext] graphicsPort].  Now, how do we plug that
into StartQuartzDrawing()?
Attached patch patch - try to fix camino (obsolete) — Splinter Review
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?
Blocks: 239701
*** Bug 279140 has been marked as a duplicate of this bug. ***
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.
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.
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?
Attached patch camino only patch (obsolete) — Splinter Review
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?
Attachment #165638 - Attachment is obsolete: true
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.
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.
Attached patch patch v1.3 (moz & camino) (obsolete) — Splinter Review
Attachment #159099 - Attachment is obsolete: true
Attachment #172698 - Attachment is obsolete: true
Attachment #172979 - Flags: superreview?(sfraser_bugs)
Attachment #172979 - Flags: review?(pinkerton)
Attachment #159099 - Flags: review?(peterv)
I have used the 1.3 patch for a couple of days now with no problems.
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?


(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.
"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. 
"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.
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.
Attached patch patch v1.4 (obsolete) — Splinter Review
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)
Attachment #172979 - Flags: superreview?(sfraser_bugs)
Attachment #172979 - Flags: review?(pinkerton)
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)
+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.

Attached patch patch v1.5 (obsolete) — Splinter Review
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)
I'm working on a review of the patch.
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
Flags: blocking-aviary1.0mac-
Blocks: 275808
> 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.
Attached patch patch v1.6Splinter Review
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)
Attachment #173391 - Flags: superreview?(sfraser_bugs)
Attachment #173391 - Flags: review?(pinkerton)
Attachment #173765 - Flags: review?(pinkerton)
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+
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 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.
Checked in.  Let's see what happens.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
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.
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.
Maybe caused printing crash in bug 283852?
Attachment #174646 - Flags: superreview?(tor)
Could this have caused bug 287058?
*** Bug 258023 has been marked as a duplicate of this bug. ***
Blocks: deermac
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: