This isn't a big deal, but we should fix it before we ship. If we get a repaint (from say an exposure event) we'll ask to paint the entire area of an image. The image win code (on Windows anyway) will ask GDI to render all of the image bits including the bits that haven't been set yet. This results in us displaying some random (gray under NT) color in the unitialized bits What we need to do is either change the image lib for which part of the image is valid (currently it doesn't provide that information), or if that isn't possible track it ourselves.

I've implemented some code to keep track of and clip out the grey portion of partially loaded images. It fixes the bug. The patches are available at: http://www.geocities.com/SiliconValley/Haven/8120/patch_diff.txt

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

i'm assigning this to you because i think what we need to do is have imagelib tell *us* what the current "valid rect" of the image is rather than us doing some sort of hack like assume that images appear in top to bottom order (so we could consider the top N lines valid, or something similar). let me know what you think.

My 2¢. I agree with Michael that the image lib should deal with this rather than forcing the gfx code to try and track what part is valid

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

Setting all current Open/Normal to M4.

per leger, assigning QA contacts to all open bugs without QA contacts according to list at http://bugzilla.mozilla.org/describecomponents.cgi?product=Browser

Reassigning qa contact to elig@netscape.com.

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

pnunn's not here for the m4 endgame. moving to m5

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

[Changed bug title to one that more clearly identifies problem.]

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

How can this be marked as WORKSKFORME. Did the problem actually get fixed?

I assume the problem has been fixed; it hasn't shown up in months. Neeti? I'm going to rubber-stamp this as WORKSFORME; please feel free re-open and re- resolve as FIXED if you prefer.

I'm re-opening the bug. Pam is the only one who should actually close this bug

To clarify, I did specifically confirm that --- using this morning's builds --- the unfilled portion of image rectangles are filled with black, rather than random bits. *** Note that on Mac OS, the unfilled portion of image rectangles are filled with white. Don't see how this would matter, though. ***

Sure. (I believe neeti is taking a bunch of pnunn's bugs.)

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

The background image used by the testcase is 426 by 126 px; 58K - typical sizes for a background created by a bandwidth-unaware, LAN-connected, non-web-professional person. { The following should be its own bug report, if tiling of background images is the responsibility of another component (in that case the new bug would depend on this bug and bug 19540 would be a special case of the new bug (bg image taller than viewport)). Appending to this bug for now because bug 19540 was marked a DUP of this bug. } * Overview: Bug 1248 actually *is* a big deal for users with a slow modem link who need to view pages with slow-loading background images (image size and file size don't matter except in how they contribute to slow loading). In parts of the page below the initial view, the display of black (as happend with WinNT and Linux) in unloaded regions of the background image makes normal black text unreadable. (Random colour data, as seen with Win98, may not be much better). * Steps to Reproduce: 0. Find a machine with a modem link to an ISP, or set up a null-modem connection at no faster than 38400 kbps to a router or routing-capable host to emulate a modem link. This is a UE problem for slow background image loading; testing it with a fast link will mask the problem. 1. Start an ftp download to mostly saturate the link to emulate download of mail while viewing webpage, or loading of several images for the webpage during background image load, or any other real-world slowdown; this also will give more time to observe the bug. 2. View the attached testcase. 3. As soon as the background starts to appear, scroll down by one page. 4. Do nothing more, just observe. 5. Exit and restart mozilla (necessary to defeat memory cache - neither Cache preferences nor [Shift]-[Reload] is working yet) 6. View the attached testcase. 7. As soon as the background starts to appear, scroll down to the bottom. 8. Move the scrollbar slider up and down while the page is loading. * Actual Results: A. In step 3, this bug appears fixed (the unloaded part of the background image is undisplayed or transparent) until the page is scrolled away from the initial view. B. In step 4, a louvered effect is seen, with most of the text unreadable. Slivers of the portion of the image that have loaded so far appear under the text that was below the initial view before paging down, but behind the rest of the text is black, until the background image is completely loaded. C. In Step 8. the same effect is seen, except that the blacked-out regions get progressively narrower as the background image loads. * Expected results: The portions of the background that have not yet been loaded do not get painted, either in the initial view or anywhere else in the document; neither zero nor random colour data appears under any of the webpage during and as a result of background image loading. All text is readable throughout, assuming that the background image gives contrast to the text. * Tested with: 1999-12-08-08-M12 nightly binary on Windows NT 4.0sp3. M11 Win32 binary on Windows NT 4.0sp3 (same exact results)

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

[note to self: be sure to check all bugs marked as dupes of this one when performing verification.]

awaiting pre code-review feedback. -pn

This isn't a difficult fix, but changes are needed in every platform gfx FE....which could be messy checkin. I'm electing to check in after the m13 panic deluge. Moving target to m14.

I'd like to get this in, but its not critical..... especially since the gfx frontends won't be getting any more info than they have now. This change really just adds the potential for different decoding schemes. Currently, the imagelib always starts at (0,0) and the decoded block will always be (imagewidth, last_decoded_row_number). This may change in the future, but not in the near future. As I said, the changes are not difficult, its just there are alot of little ones scattered through all the platforms. I still need to test a mac version to be checkin ready.

Am I right in thinking that pnunn's fix will not fix the bug, but is more of a side-issue? As far as I can make out from the notes herein, imagelib already provides information about how much of the image is valid so far, but the higher-level code isn't making good use of it. pnunn's fix futureproofs this interface a little, but doesn't address the issue that the higher-level code may still trying to paint data that hasn't been validated yet. Michael Lowe's patch is perhaps a fair way to achieve the higher-level fix when modified to account for the new interface (although I haven't verified the fix or its cleanliness). Is that a fair summation?

Right. I'll pull in the submitted patch with my changes and make the necessary code adjustments. And retest. -p

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

I've just checked in the changes needed in the imglib and gfx. And I've migrated michael.lowe@bigfoot.com's changes in nsImageFrame.cpp over. And it looks nice. Thanks Michael. Would you mind looking over my adaption of your patch? I couldn't get the changes in nsCSSRendering.cpp to work. -pn ps. Bugzilla is wonky today and won't let me create an attachment. I'll include the diff here and later attach the diff. Index: nsImageFrame.cpp =================================================================== RCS file: /cvsroot/mozilla/layout/html/base/src/nsImageFrame.cpp,v retrieving revision 1.109 diff -r1.109 nsImageFrame.cpp 493c493 < if (NS_FRAME_PAINT_LAYER_FOREGROUND == aWhichLayer) { --- > if (NS_FRAME_PAINT_LAYER_FOREGROUND == aWhichLayer) { 502a503,528 > aRenderingContext.DrawImage(image, inner); > } else { > > if (image->GetDecodedY2() == image->GetHeight()) { > aRenderingContext.DrawImage(image, inner); > } > else { > > aRenderingContext.PushState(); > > nsRect clipRect = inner; > clipRect.height = inner.height* (image->GetDecodedY2())/image->GetHeight(); > > PRBool clipState; > aRenderingContext.SetClipRect(clipRect, nsClipCombine_kReplace, clipState); > aRenderingContext.DrawImage(image, inner); > > clipRect.y = clipRect.height; > clipRect.height = inner.height; > aRenderingContext.SetClipRect(clipRect, nsClipCombine_kReplace, clipState); > DisplayAltFeedback(aPresContext, aRenderingContext, > mImageLoader.GetLoadImageFailed() ? NS_ICON_BROKEN_IMAGE : > NS_ICON_LOADING_IMAGE); > > aRenderingContext.PopState(clipState); > } 504d529 < aRenderingContext.DrawImage(image, inner); I'll attach my changes to this bug for review and reassign the bug over to troy. I don't feel competent to safely make changes in layout code. I'm not sure that every aRenderingContext::DrawImage() needs code added to determine the "real" decoded data.... but it might. Its your call. -pn

Don, re-assigning to you because it's image related

Since the patch no longer applies against current CVS I've hand-applied it (non-context diffs suck for this, however, so let me know if I made a mistake) and created a revised patch against the current CVS, attached it to this bug. Apply against layout/html/base/src/nsImageFrame.cpp Pam, the patch seems to work splendidly here. It does not address trying to draw partially-loaded background images and in this case the original problem persists, but for foreground images it is a wonderful cosmetic improvement. Cheers, --Adam

I've made an amendment to layout/html/style/src/nsCSSRendering.cpp (patch attached) to avoid the background-image blackout. Something similar could once again be applied to the DrawImage() earlier up this function, but since I never witnessed the problem whilst that code was tickled I fixed the most obviously problematic case instead. --Adam

Okay, now as far as I'm concerned the problem is identified and patches are in-hand. When can we have movement on this? --Adam

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

This is also a performance problem see Bug 18992. We are drawing too much when we have large incrementally loaded images. When this bug is fixed we also need to remove the calls on Linux and Mac which clear out the nsImageMac and nsImageGTK when they are created. There is no need to clear them out since we will no longer be painted the non-loaded portion of the nsImage.

I suspect that we can also remove one of the image-clearing cases which were part of the Bug 13048 fix when this goes in. I'll verify that when this fix is in. --Adam

Aaaaand additionally, I'd like to note that my patch for background painting here is only 75% of a fix because ideally the not-painted portion of the background should be the page's fallback background colour, not neutral grey. I will also address this once someone confirms that the fix as it stands is heading in the right direction. --Adam

I have the fix in my tree, nsImagexxx, will use the amount read in as the maximum amount a blit can do. On the Mac this accounted for about a 50% speed increase of page loading.

Don: I made these changes some time back for faster image rendering, which also ensure that only dirty parts of the imaage are rendered: Index: nsImageFrame.cpp =================================================================== RCS file: /cvsroot/mozilla/layout/html/base/src/nsImageFrame.cpp,v retrieving revision 1.110 diff -w -c -1 -r1.110 nsImageFrame.cpp *** nsImageFrame.cpp 2000/02/11 01:24:52 1.110 --- nsImageFrame.cpp 2000/03/01 23:50:23 *************** *** 503,505 **** } ! aRenderingContext.DrawImage(image, inner); } --- 503,509 ---- } ! else { ! // only redraw the part of the image that needs refreshing ! inner.IntersectRect(inner, aDirtyRect); ! } ! aRenderingContext.DrawImage(image, inner, inner); } This change in Mac GFX is required to fix a bug with coordinate transforms for the DrawImage(image, nsRect, nsRect) version of the call: Index: nsRenderingContextMac.cpp =================================================================== RCS file: /cvsroot/mozilla/gfx/src/mac/nsRenderingContextMac.cpp,v retrieving revision 1.111 diff -w -c -1 -r1.111 nsRenderingContextMac.cpp *** nsRenderingContextMac.cpp 2000/02/05 23:02:32 1.111 --- nsRenderingContextMac.cpp 2000/03/01 23:53:26 *************** *** 1375,1379 **** ! nsRect sr = aSRect; nsRect dr = aDRect; - mGS->mTMatrix.TransformCoord(&sr.x,&sr.y,&sr.width,&sr.height); mGS->mTMatrix.TransformCoord(&dr.x,&dr.y,&dr.width,&dr.height); --- 1375,1385 ---- ! nsRect sr; ! //mGS->mTMatrix.TransformCoord(&sr.x,&sr.y,&sr.width,&sr.height); ! sr.x = NSToCoordRound((float)aSRect.x / mP2T); ! sr.y = NSToCoordRound((float)aSRect.y / mP2T); ! sr.width = NSToCoordRound((float)aSRect.width / mP2T); ! sr.height = NSToCoordRound((float)aSRect.height / mP2T); ! //sr.ScaleRoundIn(1.0 / mP2T); ! nsRect dr = aDRect; mGS->mTMatrix.TransformCoord(&dr.x,&dr.y,&dr.width,&dr.height); The change in nsImageFrame makes more sense to me than changing each nsImage< Platform>

Another change you might consider is to alter the pixel mode for CopyBits from ditherCpy to srcCpy: Index: nsImageMac.cpp =================================================================== RCS file: /cvsroot/mozilla/gfx/src/mac/nsImageMac.cpp,v retrieving revision 1.22 diff -w -c -1 -r1.22 nsImageMac.cpp *** nsImageMac.cpp 2000/02/01 22:24:08 1.22 --- nsImageMac.cpp 2000/03/01 23:54:17 *************** *** 330,332 **** { ! ::CopyBits((BitMap*)*imagePixMap, (BitMap*)*destPixels, &srcRect, & dstRect, ditherCopy, 0L); } --- 330,332 ---- { ! ::CopyBits((BitMap*)*imagePixMap, (BitMap*)*destPixels, &srcRect, & dstRect, srcCopy, 0L); } This speeds up blitting large images when the src and dest pixmaps are of different depths (e.g. the 32-bit GWorlds used for images, and a 16-bit screen).

If we only copy the part of the image read in there is no need to clear out the buffer since the part of the buffer not filled in with the image is not used. The ditherCpy to srcCpy is a great idea, to use that just in the cases were it is needed, like maybee only 8 bit? The change I put in only took 4 lines of code and made a huge difference.. the CNN page loaded in half the usual time.

dcone: can you attach your patch please?

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

This has been fixed on Mac and Windows.. I need this to be implemented on Unix.. can you do this Waqar. Basically all you need to do is limit the amount of the image drawn is nsImageGTK to the amount of the image that has been read in. The variable you need to use is mDecodedY2 which is a member nsImageGTK, look at nsImageMac.cpp in the draw method for an example. Should only be a few lines of code... and only affects the draw method. Call anytime if you have questions.

Attached patch seems to work okay for GTK. Additionally will probably work for non-topdown-decoded images if we ever support those. After consideration I agree that despite not being a trivially XP solution this is a better approach than just setting a cliprect, since we can half-avoid going through the slow gdk_draw_rgb_image() conversion process for the whole image and likewise with pushing superfluous image data through the X socket.

The patch seems to work fine for me as well.

Fixed checked in.

I haven't noticed problems on a black box level from this bug in the past few months. Troy, Sean, Simon, Don, et al --- shall I rubber-stamp this as verified or do any problems still remain? Thanks!

[doh, and yes, I did check duplicate bug #28600, which is no longer taking place.]

Since nobody who has seen a problem more recently than I have seems to feel otherwise, I'm rubber-stamping this as Verified without Inspection.

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