nsImageFrame repaints the entire image even when the dirty rect is smaller than the image. An example is on www.mozilla.org, when scrolling up from the bottom, the top image begins to come in to view... Instead of simply drawing a piece of the image, we draw the entire thing with negative y coords... While this will get clipped, it wastes time. Looking at nsIRenderingContext/nsIImage, there is no Draw(Image) method that takes: source rect of the image including x,y location to draw to dest width, height x,y offset from the source. so if I'm scrolling up on mozilla.org I may have a source rect of (0, -28, 600, 58) and a direct rect with a width/height of 600, 2 we really don't want to redraw the entire thing. It would be nice to fix this... we should add an api to nsIImage and nsIRenderingContext that can do this so that nsImageFrame can call this instead of repainting the entire image.

Reassigning to Don. Don, in the 4.x products we had the sort of API that Pav is talking about

I don't think we will have time for this.. on this go around..

This is pretty important, espically with images with 8bit alpha masks which take MUCH longer to render since they have to blend with the background.

This is a major issue.. all the API's will need to be changed.. in nsRenderingContent, nsImage, and the routines that call them. I don't think the payoff is that great considering the clipping should mostly take care of this. I agree we need to fix this.. but I don't think this is the time, we really need to finish this thing up.

This bug has been marked "future" because the original netscape engineer working on this is over-burdened. If you feel this is an error, that you or another known resource will be working on this bug,or if it blocks your work in some way -- please attach your concern to the bug for reconsideration.

So I'm not sure why we can't just call nsIRenderingContext::DrawImage(nsRect(source-x, source-y, source-w, source-h), nsRect(dest-x, dest-y, source-w, source-h)); Will that not copy only a portion of the image? The comment for that flavour of DrawImage says that the image can be ``scaled/clipped'' if the sizes don't match, which seems insufficiently specific, but assuming that we clip, it seems suitable. Pav, do you agree?

I think this will work, based on light experimentation. I'll take the bug for now.

I'm not quite sure why the attached patch doesn't work. I fear some bad interaction with scrolling on Linux, but it's even more likely that I'm just missing something really fundamental about how we manage our coordinates. The problem manifests itself as follows: if I scroll http://www.mozilla.org/ to the right, such that the left portion of the image is obscured by the sidebar, the new portions of the image are drawn as thought the image were at (0,0) of the _visible_ area, rather than (0,0) of the frame. To put it another way, if I obscure the scrolled window and then expose it, the image is redrawn such that the leftmost portions of the image are visible: it's as though it wasn't scrolled.

Ok. I've added a patch that fixes this for Linux. The patch contains changes to both nsImageFrame and gfx. There was some major borkenness in nsRenderingContextGTK::DrawImage(nsIImage *aImage, const nsRect& aSRect, const nsRect& aDRect) where it translated the source position acording to the place on the screen to draw to. This has been fixed for Linux, but will probably need to be fixed on other platforms. Small change. The other changes is to correctly implement image drawing with separate source and destination rectangles on Linux. This stuff is pretty much needed for speedy scrolling with 8bit alpha png:s using remote X display. I've tested it quite a bit. Comments? Anyone wanna try it on win32?

Adding possible mac + windows victims for platform specific work.

I'll try to get this working in Win32.

i tried applying just the nsImageFrame part of this patch on mac and, wow, does it break stuff. images no longer scroll correctly, and on pages like CNN, the images all draw as garbage. please don't land this yet ;)

The patch exercises code paths that have seriously rotted. I am cleaning up the Windows portion now. It's not easy though.

Pinkerton: You have to at least apply something like this: Index: nsRenderingContextMac.cpp =================================================================== RCS file: /cvsroot/mozilla/gfx/src/mac/nsRenderingContextMac.cpp,v retrieving revision 1.119 diff -u -r1.119 nsRenderingContextMac.cpp --- nsRenderingContextMac.cpp 2000/06/23 23:47:08 1.119 +++ nsRenderingContextMac.cpp 2000/09/08 08:36:30 @@ -1388,6 +1388,8 @@ nsRect sr = aSRect; nsRect dr = aDRect; mGS->mTMatrix.TransformCoord(&sr.x,&sr.y,&sr.width,&sr.height); + sr.x -= mTranMatrix->GetXTranslationCoord(); + sr.y -= mTranMatrix->GetYTranslationCoord(); mGS->mTMatrix.TransformCoord(&dr.x,&dr.y,&dr.width,&dr.height); return aImage->Draw(*this, mCurrentSurface, sr.x, sr.y, sr.width, sr.height, Or it will never ever work.

There was a small bug concerning images with 1-bit masks that tomi found. I've attached a new version of the patch.

Voila! I tested it on all different kinds of images (including 8-bit alpha etc). Seems to work well. Now I scroll smoothly through pages containing large images. Alex, I think that in general when you reduce the source rectangle to the decoded rect, you need to scale down the dest rectangle as I do rather than just using subtraction.

roc: I agree. There might be rounding errors when done my way. New patch coming right up. Any mac weenies wanna take a look at this? pinkerton?

That's not what I meant. You need to consider what happens when there's scaling involved. The decoded rectangle and the source coordinates can't simply be added or subtracted with respect to the destination coordinates.

I don't understand what you mean. Especially not the "destination coordinates" part, as the code only modifies the source coords. I just studied this a bit closer, and it seems they should produce equal results, only my way is more efficient: ROCs way: ========= TransformCoord: x = *aX * m00 + NSToCoordRound(m20); *aX = NSToCoordRound(x); ex = x - float(NSToCoordRound(x)); *aWidth = NSToCoordRound(*aWidth * m00 + ex); TransformNoXLateCoord: x = (float)*ptX; *ptX = NSToCoordRound(x * m00 + y * m10); Since m10==0 for scaling and round(x + int) == round(x) + int we have the result: x == NSToCoordRound(x * m00) == NSToCoordRound(x * m00) + NSToCoordRound(m20) - NSToCoordRound(m20) == NSToCoordRound(x * m00 + NSToCoordRound(m20)) - NSToCoordRound(m20) Alex way: ========= TransformCoord: x = *aX * m00 + NSToCoordRound(m20); *aX = NSToCoordRound(x); ex = x - float(NSToCoordRound(x)); *aWidth = NSToCoordRound(*aWidth * m00 + ex); GetXTranslationCoord: *aX = *aX - NSToCoordRound(m20) Result: x = NSToCoordRound(x * m00 + NSToCoordRound(m20)) - NSToCoordRound(m20) And, while on the rounding subject, the ex error calculation is still correct since we only displace the x coord an integer amount.

That's not the code I meant ... that code is fine :-). I'm looking at nsImageGTK::Draw, there are lines like this: + aDY += mDecodedY1 - aSY; This seems to assume that there is no scaling going on so aDY and aSY have the same units.

Awgh. You're right. I'll have to fix that.

Funny. When i test my code on scaled images it seems to work perfectly well. I think all images are stored pre-scaled, and no stretching is ever done by nsImage::Draw().

I wonder if its worth fixing that. The Gtk+ code doesn't currently know how to scale the images, and its not used by the current layout code. Also nsImageGTK::Draw(x,y,w,h) has this code and comment: // XXX kipp: this is temporary code until we eliminate the // width/height arguments from the draw method. if ((aWidth != mWidth) || (aHeight != mHeight)) { aWidth = mWidth; aHeight = mHeight; } So it seems scaling of images is being removed. The patch with id 14255 works well enough with the current codebase.

The scaling is, yes, all done before it gets to the rendering context. According to tor, that's going to stop In The Future, and platforms will each get to scale their own images, but for now I think the assumption is safe. I was frightened by that prospect as well, back when I was making a mess of things, but tor comforted me.

Actually there are still scaling paths through the gfx code, which is why the modern2 nav toolbar turned white on unix last week (they tried using scaling without testing to see that unix doesn't support it currently). Adding bryner to the CC list, since he knows more about this.

I've noticed quite a few places where it's really hard to understand the Mozilla code because there are N possible code paths, but only a few are actually used and the rest don't work.

adding smfr, mwahhahahaahah

I filed several bugs related to this in the past: bug 18992, which got dupped to bug 1248, and bug 41134 (which is still open). I think this is really a dup of bug 41134. However, a pach I proposed for that breaks the fix for bug 36811. What a tangled web...

So simon, does that mean you're willing to do the Mac lifting on this one? It is possible that just applying the nsImageFrame part of the patches and the nsRenderingContextMac.cpp patch i gave in a comment will be enough for it to work. Otherwise you'll have to fix nsImageMac::Draw(nsIRenderingContext &aContext, nsDrawingSurface aSurface, PRInt32 aSX, PRInt32 aSY, PRInt32 aSWidth, PRInt32 aSHeight, PRInt32 aDX, PRInt32 aDY, PRInt32 aDWidth, PRInt32 aDHeight).

The latest patch won't even apply to current CVS.

The patch dated 09/08/00 fixed crash bug 52275. Nominating this bug nsbeta3.

52275 isn't even nominated as an nsbeta3 bug, so marking nsbeta3-.

Nominating for Mozilla 0.9, on the basis that the API is being misimplemented on all platforms, and it's costing us performance. I'll also accept this bug, from the perspective of hounding platform experts to catch up. (On that note: welcome, Mr. Kaply.)

52275 (mentioned above) is fixed (it got rtm++)... it's a shame nobody thought to fix it by using *this* patch (which solves a perf issue too) rather than coming up with another one.

52275 was a frequent crasher, so a minimal fix was created to get the fix through PDT. The changes involved in this bug are more invasive and would have never gotten through PDT (not to mention we still don't have the needed Mac changes).

Since we have zero knowledge about the status of shaver, I'm stealing this bug. Targeting mozilla0.8.

Cc'ing ssari for Mac help. /be

Ack, okay, thanks Brendan ;-P I'll see what I can with it.

I've updated the nsImageGTK patch to apply to current CVS. It should now work with the changes introduced from bug 65315. I'm not very familiar with the gfx code, but I've tested it and it seems to work correctly. What I did: I got rid of the below code in nsImageGTK::Draw, since it's now handled in nsImageGTK::ImageUpdated() + CreateOffscreenPixmap(mWidth, mHeight); + DrawImageOffscreen(mDecodedX1, mDecodedY1, mDecodedX2-mDecodedX1, mDecodedY2-mDecodedY1);

What does it take to get some Mac help here? It's low-hanging performance fruit, which seems to be the concept of the day.

+ aSHeight -= mDecodedY1 - aSY; + aDY += mDecodedY1 - aSY;; + aSY += mDecodedY1 - aSY;; extra ;'s, i presume that caching these evaluations was ruled out... extra ()s here [i think, new bug against me if you don't want to do it]: if ((aWidth==0) || (aHeight==0)) @@ -715,23 +765,23 @@ mGS->mTMatrix.TransformCoord(&sr.x,&sr.y,&sr.width,&sr.height); + sr.x -= mTranMatrix->GetXTranslationCoord(); someone used a tab? i'll let brendan chide me for something. jag hates if( 1==mAlphaDepth) [he prefers if (var == const) i don't like if (image != nsnull) [i prefer if (var)] .. if you don't mind, please change if( string) to if (string) [none of use like the former] tabs are bad; if you don't want to do the cleanup, say so and I'll file a bug against myself to do it later. Overall, it looks cool, thanks for doing it ...

go away, timeless.

be nice, pinkerton. Better yet, how about you or saari help with whatever Mac work is needed here? /be

i've got most of these patches merged in to the imagelib2 branch

So there are Mac people in rendering/layout who should be on this. Where are dcone, waqar, pierre? Why is it that a few Mac people have to do all of the heavy lifting the whole time?

like pav, said this optimization is covered in libimg2 and that is where my effort is going right now. Bad GIF decoder, no biscut! But for this patch, do we want to do this for XUL frames too? It will help in the case of updating an exposed window, which is quite slow on Mac I know. I'd argue for having it in XUL just to keep the code paths in synch. Or better yet, merge base frame implementations.

I'm not as confident as others regarding when libimg2 will land, so would like the current libimg running as fast as possible. As you can see from the history of this patch, it stalled out waiting for Mac help long before libimg2 started moving.

Waqar, can you help out with this?

I will build the mac and linux and will report status.

Pushing off to 0.9. Thank your local Mac person for the 4+ month delay.

given that saari and pav both have said that this is already fixed by the imagelib rewrite, i don't see much need to pull people off more important work for the couple of weeks until it lands.

I'm just asking for someone to try compiling/running this on the Mac. Looking at the Mac gfx code it looks like everything is ready to go, it just needs someone to verify by testing. If it doesn't work it'll be pretty obvious. Surely a big performance win is worth one patch/compile cycle?

about 4-5 months ago, i did apply the patches, and images didn't draw right at all. I didn't have time to look into why. Do you think this new patch is worth another try? I have no problem applying a patch and giving it a quick run, but my plate is already overflowing and I can't give it the time to debug.

The patch of a few months ago didn't have the nsRenderingContextMac.cpp change, and nsImageMac has been rewritten in the interim.

Waqar is trying the patch on the Mac.

Linux just finish building, mac is still going. Linux no problems so far.

Mac just finished building with the patch and no obvious problems. Limited testing. Compile went fine. Went to about 20 sites and nothing out of the ordinary.

Works well for me too, although I still want to see the xul frame patched too. I don't like having the "XUL is slow" conversation over and over. Just for the record, it wasn't clear to me that testing was all that was needed, I though it still needed a mac implementation. Sorry.

I don't think it's worthwhile doing for nsImageBoxFrame - these changes help the most with large images that are partially visible. XUL images are rarely either.

r=rods for nsImageFrame

r=blizzard for gtk bits. look sane as near as I can tell.

we're looking at taking this for 0.8 -asa on behalf of drivers

The two concerns I have right now are the win32 code and printing. While I trust roc to do the right thing for win32, the new code probably hasn't been hammered on a lot (unless roc has been running with it in his tree?). Printing needs a little QA love to make sure it didn't break. There's also a chance this might break BeOS. OS/2 appears fine from a look through their gfx code.

I ran with the Win32 code for a few months (until I switched over to Linux for everyday Web browsing). It seemed OK, except that partially decoded images which were clipped at the top seemed to draw oddly. It happened so rarely that I never bothered to track it down. I haven't tried it in a recent build.

I will start to review these changes.. and apply the patch.. see what the current state is..

done, it's been 5 days since you posted. Have you had the chance to look at them yet? This bug has been around forever.

I have looked at the patch.. now I installed it and am running it .. see how things work. It will be soon..I promise

What did your looking at the patch reveal? Are there changes needed? Do you have questions? (I'm getting either desperate or frustrated, or perhaps a tantalizing blend of both!)

the code looked ok.. I was worried about the comments of some strange artifacts.. but nothing seemed apparent. The one change that worried me was in nsImageWin. When I ran the patch on windows everything seemed fine.. until I printed.. the images were then scaled so output was tiny images.. so for the time being.. the patch failed the printing tests. I suspect something in nsImageWin.. and I will look a little deeper.. and see what might be the problem. Sorry for the bad news

dcone: so have you looked yet? It'd be a shame to see this nice patch, which will speed up all platforms, be blocked by a Windows print bug.

r=pavlov

Don, didn't you say the problem with this patch is that it changes the interpretation of the destination rect. Currently the destination rect indicates the size of the image on the output device (stretching or shrinking the image to fit), and this patch changes the destination rect to indicate what portion of the image to render (essentially a cliprect)? If this is true then printing will be broken on all platforms unless a second rect is passed that has the current interpretation.

What's needed for this to land: * review of the nsRenderContext* changes * new review of nsImageFrame changes (needed to remerge over IMG2) * fix for the win32 printing resolution problem * review of the win32 changes * sr=

the printing bug will be fixed as soon as i turn on the new image lib (which is dependant upon this landing)

pav broke (or at least extremely bent) rules and checked this in. whatever.

Marking verified per last comments.