Closed Bug 159796 Opened 22 years ago Closed 22 years ago

Images are sometimes displayed with horizontal garbage line(s)

Categories

(Core Graveyard :: GFX: Mac, defect)

PowerPC
macOS
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.3final

People

(Reporter: stf, Assigned: saari)

References

()

Details

(Whiteboard: fixed on chimera branch)

Attachments

(6 files, 2 obsolete files)

Sometimes some images are displayed for the FIRST time with some horizontal
lines missing, they are black.
If you scroll to force a redraw of that page, the problem does not show. It
seems to occur only the first time it's displayed.
It does not seems to be linked to the image size or position, however it seems
to happen more often with big images.

Here is a site with lot of images, wallpaper popup menu.
http://www.digitalblasphemy.com/dbhome.shtml

I'm on iMac 400 Mhz, QT 6, 1024x768, Millions colors.
Build 0726 16:11
Stephane, can you attach a screenshot?
Summary: Image with horizontal garbage line(s) → Images are sometimes displayed with horizontal garbage line(s)
Attached image capture
capture
Stephane, I don't get the black horizontal lines like the ones shown in your
screen capture.  I tried going to the two URL's that you noted.  Could you
provide more example URL's of when this happens?

copying petersen to see if he's seen this elsewhere, and figure out what's
possibly causing these black lines.
I've seen this before, but I'm not sure what triggers it. Slow connection maybe?
Not sure, we need to figure it out.
http://homepage.mac.com/blackintosh_graphics/Zeichnungen-Galerie/Grafik.htm
I got (at least I've seen it only) on the lady bug (bottom left) and the saxo
man (bottom right), the others seem to be ok.
Not on the global/catalog view, but on the big images!
I've started from the first top left image then use the right finger to go to
the next images.

It seems this bug happens only when displaying the image for the first time:
- does never occurs again if it's in the cache
- or if you have scrolled to see it
- if you delete the cash and restart, it's the same.
What I mean in my last line (cash -> cache) is that it does not matter if the
cache is empty... I made several tests by cleaning it each time before opening a
new image because I thought it might be linked to an interruption to empty part
of the cache as it mainly occurs on big images...
I'm on ADSL with a 104K download speed.

What's strange is that a scroll update the missing lines...
Suppose you have an image inside all the page and the missing lines are at the
top. You press the Down key to scroll it a little, the missing lines are still
'visible' in page contents but they are redraw correctly... shouldn't this be
clipped ?
Perhaps that's normal if all the image is entierly redrawn.
My part 2 of Comments 6, redraw correctly if key down is pressed to scroll,
without making the garbage lines area disappear out of the window, is not always
true.
I did not find any correlation, why sometimes it does redraw correctly somtimes
the garbage lines remain, with:
- image type, size
- window size (odd, even width or height)
- web site

What's always true is that if the garbage lines area goes out of the window
area, it's always redrawn correctly when you show it back, eg. Page Up then Page
Down.
FYI, this one is very slow to download the image url (not the page) is from
Belgium, like me, and even on ADSL it was very slow to download.
However it was displyed correctly and just before I had the test URL gleenwood
that was showing again few garbages lines.

http://www.digitalblasphemy.com/userg/images/1025339283.shtml
Stephane, does this one happen using Mozilla?
Sorry, I don't have a public server I can put this up at. It's just a simple
perl cgi that outputs an image in 1k chunks, once a second. Some images viewed
through this will repeatably display a line of garbage after the first chunk
loads. I'll attach two such images. You'll have to save the script and the
images into your own cgi-bin directory to test this out
(/Library/WebServer/CGI-Executables/ on OS X).

This bug is only visible if the image is viewed from an <img> tag. Images
viewed by themselves refresh after they finish loading, which clears any
garbage. Resizing the window, or otherwise refreshing the display, clears the
garbage too.

Mozilla displays these images without any visible garbage.
Attached image Test image for cgi
Component: General → Page Layout
I'm getting this fairly reliably on Asa's blog
(http://www.mozillazine.org/weblogs/asa/) on the shuttle picture.  Shift+reload
a few times and you should be able to see it.  It looks like it's simply missing
drawing a row as the image is decoded.  Seems likely that slower connections
make this worse.  We should really try to fix this for Chimera 1.0.
I think I know what's happening here.  In order for this problem to appear (at
least the variation that I debugged), the image cannot have any width or height
specified in HTML or CSS.  Then, the following series of events happens:

- nsImageFrame::Reflow calls GetDesiredSize(), which must not find an image for
the request (I think this is why it doesn't show up when the image is in the cache).

- GetDesiredSize sets the intrinsic size of the image and the desired size to
the size of the broken image icon.

- nsImageFrame::OnStartContainer is called, and the real image dimensions are
now available.  We see that it doesn't match the image's "intrinsic size", and
set up the transform to scale between the real size and the computed size.  This
also schedules a reflow if the image is not size-constrained.

Now, for any calls to nsImageFrame::OnDataAvailable() between this point and the
next time Reflow is called, the transform will be doing something completely
bogus -- scaling the rects to invalidate as if the image was forced to be the
size of the broken image icon.  This causes us to miss invalidating parts of the
image as it's decoded.  We'll miss bigger chunks the larger the image is.

The next time Reflow is called, GetDesiredSize will be called again and will
update mComputedSize to be the real image size.  Any rects invalidated after
that will be handled correctly.

There are a couple of options I can see here:

a) Refactor GetDesiredSize so that we can update mComputedSize and fix the
transform as soon as the image dimensions are available.

b) Force the scheduled reflow to be processed immediately, by calling
FlushPendingNotifications() on the PresShell as soon as we call ReflowDirtyChild.

In either case, the goal is to make sure everything is correct before the first
call to OnDataAvailable.  I think option (a) would require caching the min and
max width and min and max height from the previous reflow to check that the
intrinsic size is within the legal bounds.  That would bloat image frames by 16
bytes.  Option (b) could have a performance impact though.

cc'ing bz, who is in the process of ripping up all of this code anyway.
> This causes us to miss invalidating parts of the image as it's decoded.

Mmm... but if we have a reflow scheduled anyway, don't we get repainted after
the reflow?
Depends on: 83774
>> This causes us to miss invalidating parts of the image as it's decoded.

>Mmm... but if we have a reflow scheduled anyway, don't we get repainted after
>the reflow?

Where should that happen at?
That's a good question....

Perhaps the image needs to invalidate itself if its size changed during a
reflow? (this is why I cced roc...)
I'm not completely sure what invariants we currently assume about when you
should call Invalidate() on a frame. But I can tell you what I think it should
be: whenever you change something that affects the appearance of a frame, you
need to Invalidate() it --- inside Reflow(), as well as anywhere else.
Attached patch patch to flush reflows (obsolete) — Splinter Review
this ensures that the imageframe is reflowed at the correct size before
starting to draw the image
This could slow down page load. I think it would be better if we just did
Invalidate() when the image size changes during reflow.
Yaeh, having looked about it looks like other frames Invalidate() themselves
during reflow as needed.... so just adding a Invalidate() in GetDesiredSize
would probably work dandy.  I could roll that into the patch to bug 83774
(already have, in fact).
My concern with doing this is that we'll be wasting time invalidating prior to
getting this first reflow, if we're going to invalidate the whole frame later
anyway.  Maybe we should track whether there's a reflow pending and not
invalidate during OnDataAvailable in that case?
We already have a mGotInitialReflow boolean in the image frame....  Could just
use that.
It's not the initial reflow.  We need to specifically track whether we've been
reflowed in the case where we queue a reflow in OnStartContainer().
Though then we need to check to make sure we have a specified size.  Which may
be comparable, timewise, to an invalidate...
I meant do _not_ have a specified size.

If we need an extra bool here or there, that should be doable -- the
nsImageFrame in my tree currently has two PRPackedBool members....
Er, nevermind.  mSizeConstrained is just a bool too... ;)

So we could do something like that, yes....
Oddly, this approach doesn't fix the problem for me, even though it looks like
it should. (it looks like it will need to be a reflow callback since the frame's
rect hasn't changed to the new size during Reflow).

I'm still looking at this; it looks like there might be a height/last-row
(off-by-one) error in nsImageMac.
Attached patch patch to nsImageMac (obsolete) — Splinter Review
yep, nsImageMac::Draw can introduce a rounding error that causes the rect to be
1 row too short if an area larger than the decoded rect is invalidated.  To fix
it, I copied the more thorough code from nsImageGTK.
Attachment #113988 - Attachment is obsolete: true
I don't think we can do this safely from inside Reflow, because the frame's
rect isn't yet set to the larger size.	So, I used nsIReflowCallback.
Attachment #113994 - Attachment is obsolete: true
Comment on attachment 114004 [details] [diff] [review]
patch to nsImageMac, take 2

I think this needs to go in regardless of any other patches here.
Attachment #114004 - Flags: superreview?(sfraser)
Attachment #114004 - Flags: review?(pinkerton)
I don't think we need to worry about the cost of invalidation. It ought to be
very fast, and I believe it generally is. The real cost is the corresponding
painting, but during pageload we're suppressing paints like crazy so that
shouldn't be a problem. OTOH the invalidation suppression code you haven't isn't
too complex (yet), so I don't really object.
Comment on attachment 114004 [details] [diff] [review]
patch to nsImageMac, take 2

sr=sfraser

But tell me; what are the manifestations of the XP part of this issue? Do they
deserve a separate bug?
Attachment #114004 - Flags: superreview?(sfraser) → superreview+
i'd like to see a comment or two on the nsImageMac addition. I, for one, can't
follow what is going on there at all or why this code is needed.
simon: The XP part of what issue, exactly?  This problem with rows missing from
images or the reason that nsImageMac::Draw needs to have this code to check
invalidates against the decoded rect?

pinkerton: I'm adding the following comment:

// Ensure that the source rect is no larger than our decoded rect.
// If it is, adjust the source rect and scale the difference to adjust
// the dest rect.

Look ok?
actually...

If I'm reading the line-in-box reflow code correctly (nsBoxFrame:ReflowLine), it
invalidates itself in the combined old and new rects of the frame that changed
size.  Which fits with a behavior I'm seeing where the image frame is told to
paint its entire rect, after the reflow.  So, I now believe that the nsImageMac
patch is _all_ that's necessary to fix this prolem on OS X.  I certainly can't
reproduce the bug with just that patch applied.
You can have r/sr=sfraser on that patch if you like.
Comment on attachment 114004 [details] [diff] [review]
patch to nsImageMac, take 2

verbal r=pinkerton with the comment change.
Attachment #114004 - Flags: review?(pinkerton) → review+
sorry, I meant nsBlockFrame::ReflowLine in comment 39
Checked in the patch.  I'm going to go ahead and close this as fixed, please
reopen if you see the missing-row problem on nightly builds after 2002-02-11.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
don't mind me... shuffling things around to request trunk approval for this
patch as well.
Status: RESOLVED → REOPENED
Component: Page Layout → GFX: Mac
Product: Chimera → Browser
Resolution: FIXED → ---
Target Milestone: --- → mozilla1.3final
Version: unspecified → Trunk
Comment on attachment 114004 [details] [diff] [review]
patch to nsImageMac, take 2

This affects trunk Mac builds too.  I'd like to get it in 1.3.
Attachment #114004 - Flags: approval1.3?
Status: REOPENED → ASSIGNED
Whiteboard: fixed on chimera branch
Using the 2003-02-11-07 Chimera build, I can no longer reproduce the problem
with the image urls provided in this report. 
Comment on attachment 114004 [details] [diff] [review]
patch to nsImageMac, take 2

a=asa (on behalf of drivers) for checkin to 1.3 final.
Attachment #114004 - Flags: approval1.3? → approval1.3+
checked in on the trunk.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago22 years ago
Resolution: --- → FIXED
I wonder if this fixes bug 190881 as well.
*** Bug 190881 has been marked as a duplicate of this bug. ***
Sweet ! Verified on the 2003-02-12-03 Mach-O trunk build. 
Status: RESOLVED → VERIFIED
I wondered if this might fix bug 187419 as well, but it didn't.
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: