Closed
Bug 37779
Opened 25 years ago
Closed 24 years ago
nsImageFrame repaints entire image instead of just the dirty rect
Categories
(Core :: Layout, defect, P3)
Tracking
()
VERIFIED
FIXED
mozilla0.9
People
(Reporter: pavlov, Assigned: tor)
References
()
Details
(Keywords: crash, perf)
Attachments
(13 files)
3.39 KB,
patch
|
Details | Diff | Splinter Review | |
11.61 KB,
patch
|
Details | Diff | Splinter Review | |
11.67 KB,
patch
|
Details | Diff | Splinter Review | |
10.78 KB,
patch
|
Details | Diff | Splinter Review | |
11.67 KB,
patch
|
Details | Diff | Splinter Review | |
11.45 KB,
patch
|
Details | Diff | Splinter Review | |
21.74 KB,
patch
|
Details | Diff | Splinter Review | |
11.00 KB,
patch
|
Details | Diff | Splinter Review | |
21.82 KB,
patch
|
Details | Diff | Splinter Review | |
21.79 KB,
patch
|
Details | Diff | Splinter Review | |
26.73 KB,
patch
|
Details | Diff | Splinter Review | |
27.26 KB,
patch
|
Details | Diff | Splinter Review | |
2.83 KB,
patch
|
Details | Diff | Splinter Review |
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
Assignee: troy → dcone
Comment 2•25 years ago
|
||
I don't think we will have time for this.. on this go around..
Status: NEW → RESOLVED
Closed: 25 years ago
Resolution: --- → REMIND
Target Milestone: --- → M21
Reporter | ||
Comment 3•25 years ago
|
||
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.
Status: RESOLVED → REOPENED
Resolution: REMIND → ---
Comment 4•25 years ago
|
||
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.
Assignee: dcone → kmcclusk
Status: REOPENED → NEW
Updated•25 years ago
|
Status: NEW → ASSIGNED
Comment 5•25 years ago
|
||
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.
Target Milestone: M21 → Future
Comment 6•24 years ago
|
||
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?
Comment 7•24 years ago
|
||
I think this will work, based on light experimentation. I'll take the bug for
now.
Assignee: kmcclusk → shaver
Status: ASSIGNED → NEW
Comment 8•24 years ago
|
||
Comment 9•24 years ago
|
||
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.
Comment 10•24 years ago
|
||
Comment 11•24 years ago
|
||
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?
Comment 12•24 years ago
|
||
Adding possible mac + windows victims for platform specific work.
I'll try to get this working in Win32.
Comment 14•24 years ago
|
||
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.
Comment 16•24 years ago
|
||
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.
Comment 17•24 years ago
|
||
Comment 18•24 years ago
|
||
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.
Comment 21•24 years ago
|
||
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?
Comment 22•24 years ago
|
||
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.
Comment 24•24 years ago
|
||
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.
Comment 26•24 years ago
|
||
Awgh. You're right. I'll have to fix that.
Comment 27•24 years ago
|
||
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().
Comment 28•24 years ago
|
||
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.
Comment 29•24 years ago
|
||
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.
Assignee | ||
Comment 30•24 years ago
|
||
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.
Comment 32•24 years ago
|
||
adding smfr, mwahhahahaahah
Comment 33•24 years ago
|
||
Comment 34•24 years ago
|
||
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).
Comment 35•24 years ago
|
||
The latest patch won't even apply to current CVS.
Comment 36•24 years ago
|
||
The patch dated 09/08/00 fixed crash bug 52275. Nominating this bug nsbeta3.
Comment 37•24 years ago
|
||
52275 isn't even nominated as an nsbeta3 bug, so marking nsbeta3-.
Whiteboard: [nsbeta3-]
Assignee | ||
Comment 38•24 years ago
|
||
Comment 39•24 years ago
|
||
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.)
Status: NEW → ASSIGNED
Keywords: mozilla0.9
Comment 40•24 years ago
|
||
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.
Assignee | ||
Comment 41•24 years ago
|
||
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).
Assignee | ||
Comment 42•24 years ago
|
||
Since we have zero knowledge about the status of shaver, I'm stealing this bug.
Targeting mozilla0.8.
Assignee: shaver → tor
Status: ASSIGNED → NEW
Target Milestone: Future → mozilla0.8
Assignee | ||
Comment 43•24 years ago
|
||
Comment 44•24 years ago
|
||
Cc'ing ssari for Mac help.
/be
Comment 45•24 years ago
|
||
Ack, okay, thanks Brendan ;-P
I'll see what I can with it.
Comment 46•24 years ago
|
||
Comment 47•24 years ago
|
||
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);
Comment 48•24 years ago
|
||
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.
Assignee | ||
Comment 49•24 years ago
|
||
Comment 50•24 years ago
|
||
+ 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 ...
Assignee | ||
Comment 51•24 years ago
|
||
Comment 52•24 years ago
|
||
go away, timeless.
Comment 53•24 years ago
|
||
be nice, pinkerton. Better yet, how about you or saari help with whatever Mac
work is needed here?
/be
Reporter | ||
Comment 54•24 years ago
|
||
i've got most of these patches merged in to the imagelib2 branch
Comment 55•24 years ago
|
||
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?
Comment 56•24 years ago
|
||
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.
Assignee | ||
Comment 57•24 years ago
|
||
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.
Comment 58•24 years ago
|
||
Waqar, can you help out with this?
Comment 59•24 years ago
|
||
I will build the mac and linux and will report status.
Assignee | ||
Comment 60•24 years ago
|
||
Pushing off to 0.9.
Thank your local Mac person for the 4+ month delay.
Target Milestone: mozilla0.8 → mozilla0.9
Comment 61•24 years ago
|
||
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.
Assignee | ||
Comment 62•24 years ago
|
||
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?
Comment 63•24 years ago
|
||
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.
Assignee | ||
Comment 64•24 years ago
|
||
The patch of a few months ago didn't have the nsRenderingContextMac.cpp change,
and nsImageMac has been rewritten in the interim.
Comment 65•24 years ago
|
||
Waqar is trying the patch on the Mac.
Comment 66•24 years ago
|
||
Linux just finish building, mac is still going. Linux no problems so far.
Comment 67•24 years ago
|
||
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.
Assignee | ||
Comment 68•24 years ago
|
||
Comment 69•24 years ago
|
||
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.
Assignee | ||
Comment 70•24 years ago
|
||
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.
Comment 71•24 years ago
|
||
r=rods for nsImageFrame
Comment 72•24 years ago
|
||
r=blizzard for gtk bits. look sane as near as I can tell.
Comment 73•24 years ago
|
||
we're looking at taking this for 0.8
-asa on behalf of drivers
Whiteboard: [nsbeta3-] → [nsbeta3-] critical for mozilla 0.8
Assignee | ||
Comment 74•24 years ago
|
||
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.
Comment 76•24 years ago
|
||
I will start to review these changes.. and apply the patch.. see what the
current state is..
Updated•24 years ago
|
Whiteboard: [nsbeta3-] critical for mozilla 0.8 → [nsbeta3-]
Comment 77•24 years ago
|
||
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.
Comment 78•24 years ago
|
||
I have looked at the patch.. now I installed it and am running it .. see how
things work. It will be soon..I promise
Comment 79•24 years ago
|
||
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!)
Comment 80•24 years ago
|
||
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
Comment 81•24 years ago
|
||
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.
Reporter | ||
Comment 82•24 years ago
|
||
r=pavlov
Comment 83•24 years ago
|
||
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.
Assignee | ||
Comment 84•24 years ago
|
||
Assignee | ||
Comment 85•24 years ago
|
||
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=
Reporter | ||
Comment 86•24 years ago
|
||
Reporter | ||
Comment 87•24 years ago
|
||
the printing bug will be fixed as soon as i turn on the new image lib (which is
dependant upon this landing)
Assignee | ||
Comment 88•24 years ago
|
||
pav broke (or at least extremely bent) rules and checked this in. whatever.
Status: NEW → RESOLVED
Closed: 25 years ago → 24 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•