Open Bug 62026 Opened 24 years ago Updated 2 years ago

Inline images in news/imap messages not rendered progressively

Categories

(MailNews Core :: MIME, enhancement, P3)

enhancement

Tracking

(Not tracked)

Future

People

(Reporter: jg, Unassigned)

References

Details

(Keywords: perf, Whiteboard: [patchlove][has draft patch][needs owner])

Attachments

(2 obsolete files)

In Nav4.x, a really nice feature (and mentioned as such in mag reviews) is that
if you're on a modem or slow link and you click on a news post that contains an
image attachment, rather than wait for the entire post to be downloaded before
rendering the image, the image slowly renders as it comes down.

This is the same behaviour as with progressive jpeg/gifs in webpages, and can be
extreemly useful if you don't want to download potentially 400k+ of image(s)
when you could hit the stop button after seeing the top section of the image and
save time.

Granted, not the most serious of bugs, but it will be missed from
Mozilla/Netscape 6 until it's done.

Assigning to MailNews/Front End in the absence of any other clue about the
owner. Also ccing pnunn as I'm told it could be an imagelib issue. Did look for
existing bug on this but was unsuccessful.

Note this only happens in MailNews, not browser, affects all builds to current
(2000120508) Linux.
Nominating for mozilla0.9. Adding 4xp keyword.
Keywords: 4xp, mozilla0.9
reassigning to sspitzer
Assignee: putterman → sspitzer
changing qa assign
QA Contact: esther → pmock
A usability problem for those of us that have a slow internet connection and
download large images from newsgroups.
This would increase usability enormously as far as
I'm concerned.  Here, have another vote.
I would suggest 0.9.3 nomination, except we have no 'mozilla0.9.3' keyword. 1.0
it is then. Can someone suggest what needs to be done here? CC pavlov who might
have a clue.
Keywords: mozilla1.0
Isn't it expensive to reflow each time you get yet another chunk of data to 
display, if doing it progressively?
No, in the image header you will find the properties for width and height; you
then create an approriately sized frame (one reflow) and fill it with colour.
Continue this for each image attachment.
Then I think this is something that should be in libpr0n code.

probably saari's or pavlov's.
the image should be getting flowed at its size pretty early.  as for it drawing 
progressivly, it should, but i wouldn't be too suprised if it didn't.  there 
could be a problem with the uriloader code that does the conversion between the 
news: url to pull the image data out... it could be doing this all syncronously 
and not passing data to imagelib in chunks.  cc'ing mscott
It looks like the whole kaboodle is only being
rendered in just one shot, since if there is a
textual pre-amble and then an image you don't
even get to see the preamble until both the text
AND the image are downloaded.  i.e. it's not
(solely) an image-display problem.
This feature is also missing in the Windows version
This feature is also missing in the Windows Version
Blocks: advocacybugs
If so, it should be All/All. (I have too fast access to my News serverto be
sure). Marking.
OS: Linux → All
Hardware: PC → All
This may be a related issue, please comment before I open another bug:

http://www.fuckedcompany.com/images/911/5th_avenue_facing_south.jpg

Clear your cache before loading above image.
This is a good sized jpg which should progressivly load as data is received, but
it is only displayed after it's completely received.
This doesn't look like a news bug, it happens in browser as well.

Reassign to imglib, redired if I'm wrong.
Assignee: sspitzer → pavlov
Component: Mail Window Front End → ImageLib
Product: MailNews → Browser
QA Contact: pmock → tpreston
this is a dup of the "jpegs dont' render progressivly" bug..
lophat's example is indeed a dup, pavlov, but isn't what THIS bug
is about.
Can we agree, since the non-image preamble to posts
doesn't render even while we're waiting for the image,
that this is not strictly an imagelib problem?

... and in that case, reassign it (ie. to the MailNews
module owner)?

Ahh, what the cripes.  Cor lummy.

product -> mailnews

component -> networking (would like input from the net folk about where this
data is probably logjamming)
Component: ImageLib → Networking - News
Product: Browser → MailNews
Target Milestone: --- → mozilla1.0
Target Milestone: mozilla1.0 → mozilla1.1
Eh, actually Mozilla already have this feature. When loading image, press stop
button -> it takes few seconds and mozilla starts loading image again, but this
time image is loaded progressively. So, *why* it's so hard to do that instantly? 
This should give some insight into the bug (or spawn off another bug :-) :
Subscribe to a newsgroup that typically deals with UUencoded binaries (either
movies or warez, or anything else that is sent as large files *that are broken
into parts*).
At first you have not clicked on a message yet, so your message panel should be
empty.
Now find a binary that's broken into parts (ie 1/6, 2/6, ...) and click on any
part *except for the first one*.
What you'll see:
First you will get the header info show up with absolutely no text underneath.
Then a few seconds later you'll see *some* uuencoded text (it won't be decoded
since it doesn't have the info to do that).  Then, every few seconds, you'll get
more uuencoded text until it's complete.

My belief is that this is due to
1. The newsreader buffering too much of the message before updating the display
2. The newsreader flushing the buffer after getting the header information only
(Which, on slow connections, means seeing the header and then waiting a few
seconds before getting the text of long messages.)

IANAC (I am not a coder) but it appears that this can be fixed by decreasing the
buffer size and flushing it more often.  And if the buffer size is small enough,
images will display progressively (Getting back to the actual problem. :-)
From following your steps I'll agree that *something*
is buffering too much, but that is only part of the problem
because I can view postings with images which are hundreds
of K in size and still not see a thing until the end, and
the thought that something in mozilla could be buffering hundreds
of K when my line speed is equivilent to a wet piece of
string -- well, it sends shivers down my spine.
It just occurred to me how generous we're
being with this bug's severity.  If inline
images in web pages all stopped rendering
progressively I'd be surprised if the tree
didn't get closed; why then allow the same
problem to languish in mailnews?

That mozilla1.0 keyword has also been there
for quite a while; who should be CC'd
to + or - it?
According to the Mozilla 1.0 manifesto, this is definitely not a Mozilla 1.0
blocker.  However, I'm sure a patch would get into the trunk pretty quickly if
you wrote it. ;-)
*fx: wind blows, tumbleweed rolls by*
You forgot: 

*fx: coyote howls in distance, add cricket and toad vocalizations to taste*
Adorable one-line fix.	Too cute to be on the trunk, for a few
reasons (including that depending on buffer sizes in various
inpenetrable parts of mozilla, this might cause sucky CPU usage
on fast connections, though will likely improve percieved
speed -- flushes should probably be on a timer like the
browser's doc reflows).

Also fixes the same incremental rendering problem over IMAP and
the incremental-text problem described in bug 135925
I verify that this causes CPU/responsiveness suckage from
news/mail cache, so I expect the same problem from high-speed
connections.  Pretty predictable from the nature of the
patch, but it does indicate where the problem roughly lies.
I tell a lie, the CPU suckage when loading from cache
was rather due to my layout prefs.  OnWrite() calls
and hence the subsequent OnFull() flushes are actually
getting coelesced fairly well when loading from a fast input.

I don't know why, without the patch, OnFull() is not getting
called until the input stream finishes.  That's the crux of the
bug.  I can't imagine that there really is hundreds of KB of
buffer at this level (mBufferMgr's allocated hunk size doesn't
ever get above about 150 bytes here).

Interestingly, if I don't do the OnFull() flush from OnWrite()
then OnWrite() just stops being called quite soon into the
load, never to be called again until the input ends and gets
its final completion-flush.

Something broken in the buffering logic?  Or is nsMimeBaseEmitter
/supposed/ to be pretty synchronous and hence this patch is
more or less the correct thing to do, and its omission the bug?

Who knows nsMimeBaseEmitter?  sspitzer?


Anyone...?
Blocks: 123569
Keywords: mozilla1.0patch, review
Amending the summary since the symptoms (and apparent fix)
are exactly the same when reading remote email over IMAP.
Summary: Inline images in news posts not rendered progressively → Inline images in news/imap messages not rendered progressively
If it turns out that the submitted patch is conceptually
pretty much right then I'd be willing to un-cute it (factor
out the flushing to a private function that gets called
from OnWrite() and OnFull() [and possibly Complete()], etc)
since it's mime, ducarroz would be the person to ask for review.

removing TM, since 1.1 alpha has passed.
Assignee: pavlov → ducarroz
Target Milestone: mozilla1.1alpha → ---
My concern is that mime decodes the stream line per line and therefore
nsMimeBaseEmitter::OnWrite can be called very often (with only couple bytse of
data to write), several hundred time per message depending on the content.
Therefore this patch might cause a performance degradation issue. We nee to call
onFlush using a timer...
Thanks for looking at it!

Yes, OnWrite occurs every fewscore decoded bytes, which is fairly
likely to hurt on a faster connection.

I'd rather see flushing happen on a timer too.  Who should own the timer
though?  It doesn't feel right to put that stuff in the low-level
emitters.  Higher-level users of nsMimeBaseEmitter could individually
poll the stream for new data to 'pull' on a timer, but... I still wonder
why OnFull doesn't fire until completion, anyway.  Any idea?


You are right, mime should own the timer. It's not the job of the emitter to do
that! That should be trivial to add the timer in mime...
n.b. if anyone is waiting for me to handle this, I'm unlikely
to have time for about a hundred years.  At least hopefully we
have a better idea now of where the problem lays and how to
fix it (though I still fail to understand whether this is a real
bug in that buffer-management is broken because flushes apparently
never implicity happen, or just an oversight or bad assumption by
[all?] the consumers of the emitters).
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.3alpha
I am taking care of this issue...
I cnahe my mind, emitter should own the timer. Mime has nothing to do with the
internal processing of the emitter, it just feed him with data. I'll post a
patch right now...
Attached patch Proposed fix, v2 (obsolete) — Splinter Review
this fix is based on previous patch, this time it use a timer. I havn't tested
this patch on slow connection (I don't have one :-( ) but on broadband even
with large image (400K), I don't see a difference! same with previous patch.

Mike, do you have couple free cycles to test this patch? would be nice. Thanks
Attachment #101985 - Attachment is obsolete: true
Good news and bad news.

The good news is that the patch technically works in the sense
that OnWrites are coalesced into intermittant OnFulls on the
timer.

The bad news is that this doesn't actually fix the problem.  A
few days ago Something on the trunk changed (I don't recall fiddling
with any prefs in that period) that makes nsMimeBaseEmitter only very
rarely see any OnWrites, even though data is coming down the wire
and the minutes are ticking away.  A small flurry of OnWrites happen
when the stream starts loading, a giant armful of OnWrites happen when
the stream has finished loading, but nothing happens in-between for
the timer to be able to act upon.

I first noticed this behaviour on the morning of 2002-11-12, or
perhaps the day before, when I was wondering why my older simple
patch wasn't doing the trick any more.  It may have been happening a
little longer though.
I had a version which instead of setting mFlushPending = PR_TRUE in OnWrite, I
was doing it in the Write function. That did not help either!
move to futur until we get a new solution...
Severity: normal → enhancement
Target Milestone: mozilla1.3alpha → Future
*** Bug 182977 has been marked as a duplicate of this bug. ***
Related to comment #35:
nsMimeBaseEmitter::OnWrite 

Function may receive a "granularity" parameter that would override the default - 
call OnWrite event after every single line is decoded, instead :

- if num. lines is known, call the above event every X lines, where X is an 
integer fraction of the total number
- if num lines is not known, call the event every X lines, where X is a 
predefined constant to give a reasonable granularity, but not create too much 
overhead.

Considering the above, if a 200k jpeg will display progressively  (while 
loading) in 25 steps... it ok, much better then after entire dl.
I hope nobody confuses 
progressive render of an image (general case, while loading) with progressive 
render of an image that has been encoded for progressive display.

The very first time when I read the summary of this bug, I though it referred to 
 a problem displaying progressive JPEGS in the news reader window, as opposed to 
 normally encoded JPEGS... (png, etc)
*** Bug 185062 has been marked as a duplicate of this bug. ***
No longer blocks: 123569
The main reason why I still use Netscape 3 for newsreading is the fact that
images download progressively. Until Mozilla adopts this feature I will stick to
Netscape.
Awww this bug has died... :-(
*** Bug 182983 has been marked as a duplicate of this bug. ***
Product: MailNews → Core
Ducarroz, are you still working on these? If not, please reassign to default assignee.
According to wsmwk, ducarroz is no longer taking bugs any more.
Assignee: ducarroz → nobody
Status: ASSIGNED → NEW
QA Contact: tpreston → networking.news
Product: Core → MailNews Core
Blocks: 135925
Whiteboard: [patchlove][has draft patch][needs owner]
Comment on attachment 106061 [details] [diff] [review]
Proposed fix, v2

Patch has bitrotted:

$ patch -p1 --dry-run < ~/Desktop/patch.txt 
(Stripping trailing CRs from patch.)
patching file nsMimeBaseEmitter.cpp
Hunk #1 FAILED at 136.
Hunk #2 FAILED at 360.
Hunk #3 FAILED at 378.
Hunk #4 FAILED at 990.
Hunk #5 succeeded at 995 (offset -44 lines).
4 out of 5 hunks FAILED -- saving rejects to file nsMimeBaseEmitter.cpp.rej
(Stripping trailing CRs from patch.)
patching file nsMimeBaseEmitter.h
Hunk #1 succeeded at 54 (offset -4 lines).
Hunk #2 FAILED at 84.
Hunk #3 FAILED at 98.
Hunk #4 succeeded at 172 (offset -6 lines).
2 out of 4 hunks FAILED -- saving rejects to file nsMimeBaseEmitter.h.rej
Keywords: perf
No longer blocks: 135925
Component: Networking: NNTP → MIME
QA Contact: networking.nntp → mime
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: