76776 - Progressive JPEGs should display progressively

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect, P3)

Description

[Daniel Mario Vega (Restore MNG Support --> #18574)]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:0.8.1+) Gecko/20010417
BuildID:    2001041704

Progressive Jpegs should display a low quality aproximation of the image, with
gradual improvement of quality as the image downloads. But now it does not work
this way, the image is displayed when the download has finished.

Reproducible: Always
Steps to Reproduce:
Go to http://fog.ccsf.cc.ca.us/~pthiry/multimedia/monalisabig.jpg

Or to http://home.hkstar.com/~hyytep/jpeg.html

Or to http://www.netadvies.nl/advies/pjpgdemo.html

Actual Results:  The image displays when download has finished

Expected Results:  The image should display progressive with increasing quality

For more information:

http://www.faqs.org/faqs/jpeg-faq/part1/section-11.html

Comment 1

[Daniel Mario Vega (Restore MNG Support --> #18574)]

It also happens in Linux. Changing OS to all

It works fine in Navigator 4.x

Comment 2

[Keyser Sose]

Marking NEW.

Comment 3

[Terri Preston]

changing status to [imglib]

Comment 4

[Skewer]

What is this RFE business? Progressive JPEG support is very important,
especially for 56K users. Supporting this is a fundamental part of supporting
JPEG images. Upgrading to normal severity.

Comment 5

[Stephen Walker]

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

Comment 6

[Skewer]

This really needs to be fixed for nsbeta1 as well. Adding appropriate keyword.

Comment 7

[siuyin1]

I am seeing the bug - progressive/interlaced jpegs not loading progressively in
build: 2001062815 as well.

Comment 8

[Keyser Sose]

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

Comment 9

[lohphat]

It depends how you define "progressive".

It's broken from the fundimental viewpoint that it won't render in a "raster"
mode as with gif -- at full res while being received from top to bottom.

Usually "progressive jpeg" means low-res overlayed by incresingly higer-res detail.

I'd change the summary to "jpegs don't render as they are received".  This is
not related to progressive or not.

Comment 10

[Warner Young]

lohphat, I just tested this quickly, and confirmed that non-progressive JPEGs do
display as they are loaded.  It's only progressive JPEGs that don't, so this has
everything to do with whether a jpeg is progressive or not.  No need to change
the title.

Comment 11

[lohphat]

You're correct. After testing the following URL on NSCP 4.78 I could catch that
it is a progressive jpeg.
http://www.fuckedcompany.com/images/911/5th_avenue_facing_south.jpg

Comment 12

[Adam D. Moss]

Attachment: W.I.P. patch for modules/libpr0n/decoders/jpeg/nsJPEGDecoder.cpp

Comment 13

[Adam D. Moss]

The attached patch has only been minimally tested
and contains a whole load of debugging printf's and
cruft at the moment, but does the trick.

Feedback welcomed.

Comment 14

[Adam D. Moss]

Hmm.  Still has issues with aborting decoding.

Comment 15

[Adam D. Moss]

Attachment: W.I.P. patch version #2

Comment 16

[Adam D. Moss]

Okay, this one acknowledges that jpeg_finish_output()
may suspend, and hence seems stable.  The same caveats apply
though, including a pretty slow line-by-line way of notifying the
image observer, plus any JPEG decoding errors having been
temporarily promoted to fatal.

Comment 17

[Adam D. Moss]

Attachment: W.I.P. patch version #3 (.cpp patch)

Comment 18

[Adam D. Moss]

Attachment: W.I.P. patch version #3 (.h file)

Comment 19

[Adam D. Moss]

Okay, http://www.battletrolls.com/html/homepage/noflash.php seems to be a good
(nasty) testcase.

This latest (#3) pair of patches against nsJPEGDecoder.h and nsJPEGDecoder.cpp
(supposedly) improve the suspendability of jpeg_finish_output() calls
and adds suspendability for jpeg_start_output().  It's the stablest
version yet (that is, I can't make it emit a libjpeg error and
racily abort decoding).

The fly in the ointment is at line 499 of the patched nsJPEGDecoder.cpp,
where I have inserted a |break| to keep us in the 'more data
please'/JPEG_WAIT_STARTOUT state forever; this is because if I let it
pass the point where jpeg_start_output() has suspended once, it starts
corrupting memory whether I retry, consume, whatever.  Possibly a
libjpeg bug, more likely I really need someone with superior libjpeg juju
to take a look and test my sanity (it's late).

Comment 20

[Keyser Sose]

Adding keywords...this needs a review/super review

Comment 21

[Adam D. Moss]

Removing 'patch' and 'review' keywords -- this
is strictly a work-in-progress with known problems,
and right now I'm looking for comments, not r/sr.

Comment 22

[Skewer]

adam: What is the status of your patch? If you plan to finish it, you might take
the assignment to get it off pavlov's list.

Comment 23

[Adam D. Moss]

I hope to finish it some time, but realistically that's probably at least weeks
away (I'm very busy at the moment).  Meanwhile I'd rather leave it as pav's if
he wants it.  :)

Comment 24

[Jaime Rodriguez, Jr.]

Removing nsbeta1 nomination because this bug has been plussed.

Comment 25

[Greg K.]

Setting Severity=Minor since final image display isn't actually interdicted.

Comment 26

[Stuart Parmenter]

giving to nivedita.  you can work with [e-mail] on this as he has already
spent some time working on it.

Comment 27

[Adam D. Moss]

I'm out of time, so my last patch I think is as
far as I go.  IIRC when I left it I was wondering if
jpeglib was wanting the ability to 'rewind' a few bytes
to pick up rendering an interrupted stream, and we
hadn't implemented such an ability in the IO methods
we were providing.  It does look to me like something
buffering-related, either way.

Two progressive quick-win options if we don't get things
working properly in our short timescales:

1) Turn off smoothing of the incomplete levels.  We might
be paying the smoothing cost even though we're not
displaying progressively and don't see the visual benefit.

2) At least display the final pass progressively.  The
final pass accounts for probably the majority of the file
size (but not the majority of visually-useful data, hence
the benefits of progressive rendering) so at least we'll
have started to show *something* good before we most of
the stream down.  I *think* that the last version of the
WIP patch will reliably do this with a small change or
two for a quick implementation, but is better being
implemented from scratch for less code-impact if you're
not interested in picking up the other baggage which
was aimed towards full progressive rendering support.

Comment 28

[Nivedita (gone)]

I applied the patches but I could observe progressive jpeg behaviour. The image 
got displayed as in sequential jpg, rather than the one which improves in 
quality over the subsequent scans. Am I missing some thing here.

Comment 29

[Adam D. Moss]

Well, that's odd.  The patch looks likes what's been in
my tree for months, and I do get progressive rendering
(modulo the known intermittant early-bailout bug).

I'm on unix/X11.  I'd be surprised if that made a
difference, but...

Perhaps your connection is too fast?  The progressive
rendering will coalesce earlier passes if there is
enough buffered data coming in to skip ahead to a later,
better pass.

Comment 30

[Nivedita (gone)]

My observations were specific to windowsNT platform, let me check it out on 
Linux as well. 

Comment 31

[Nivedita (gone)]

I could observe some difference in the jpeg diplay on the Linux build. It 
appeared fazy to clear. But it was too fast. I think I with agree you about   
the connection being fast .

Comment 32

[Nivedita (gone)]

moving to  1.0.1 

Comment 33

[Nivedita (gone)]

.

Comment 34

[Jaime Rodriguez, Jr.]

pav/gagan - what are the chnaces that we'd be able to get a fix for this before
06.14.2002? pls add eta, or nsbeta1- (indicating that it is not needed for
MachV). thanks!

Comment 35

[tor]

Attachment: patch for a simpler approach for displaying progressive jpegs

Comment 36

[tor]

Attachment: pick up a missing i/o suspension case

Comment 37

[Adam D. Moss]

Comment on attachment 86982 [details] [diff] [review]
pick up a missing i/o suspension case

Looks good.  r=[e-mail]

Comment 38

[tor]

Attachment: handle a couple more (last?) i/o suspension cases

Comment 39

[tor]

Taking bug.

Comment 40

[Miloslaw Smyk]

I've been playing with a patched trunk built today and it's worked flawlessly
for me. A big image to test progressive loading with is here:
http://wfmh.org.pl/~thorgal/wtc-photo-small.jpg

Comment 41

[Stuart Parmenter]

Comment on attachment 86995 [details] [diff] [review]
handle a couple more (last?) i/o suspension cases

I don't like the:
+    /* There's been an error - return NS_OK so that
+	libpr0n doesn't throw away a partial image load */

If you don't return an error, you will end up with the partially loaded image
in the cache.  That doesn't seem like a good thing.

The flag image status flag STATUS_LOAD_PARTIAL is meant to be used for this
kind of thing.

Perhaps an additional error should be added and returned.
http://lxr.mozilla.org/seamonkey/source/modules/libpr0n/public/ImageErrors.h

Comment 42

[tor]

That bit of code will only get triggered if the image stream is corrupt, so
what's the harm in caching the image?  It's a lot better than tantalizing the
user by incrementally loading an image and then whipping it away if libjpeg
sees something wrong.

Comment 43

[Stuart Parmenter]

Attachment: check the error codes

we should add the non-fatal errors to this list so that they can return a
successful error code instead of always returning success.

Comment 44

[tor]

Attachment: as above, with pav's suggest and a cynical comment

Comment 45

[Stuart Parmenter]

Comment on attachment 87257 [details] [diff] [review]
as above, with pav's suggest and a cynical comment

r=pavlov

Comment 46

[jag (Peter Annema)]

Comment on attachment 87257 [details] [diff] [review]
as above, with pav's suggest and a cynical comment

sr=jag

Comment 47

[tor]

Checked into trunk.

Comment 48

[tor]

Attachment: as above plus fix for bug 153433 (background pjpeg broken)

Comment 49

[tor]

Backed out due to Tp regression (bug 153280).

Comment 50

[tor]

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

Comment 51

[Warner Young]

Crap!  I hope this patch makes it back in soon.  It seems to me that to render
pjepgs progressively, you'd almost *have* to take a little performance hit, so I
hope people aren't thinking of taking this feature out completely!  I would
think that even if you change it so it doesn't render to screen quite as often,
there'll still be a slight perf hit.

Comment 52

[tor]

The trick is quantifying what is an acceptable perf hit, since it depend on
the network speed.  See http://bugzilla.mozilla.org/show_bug.cgi?id=153280#c2

Comment 53

[Skewer]

Compare the way this works in Mozilla to the way this works in NS4. In NS4 it
only draws one pass using the data that's available, then does a final pass at
full quality. Maybe if it was rendered like this in Mozilla the performance hit
wouldn't be as big of a deal.

Comment 54

[Stuart Parmenter]

Comment on attachment 88723 [details] [diff] [review]
as above plus fix for bug 153433 (background pjpeg broken)

r=pavlov

Comment 55

[jag (Peter Annema)]

Comment on attachment 88723 [details] [diff] [review]
as above plus fix for bug 153433 (background pjpeg broken)

sr=jag

Comment 56

[tor]

Attachment: collapse progressive scans if we have the data - should help Tp

Comment 57

[tor]

Attachment: make sure we show a complete scan during incremental loading

Comment 58

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 90154 [details] [diff] [review]
make sure we show a complete scan during incremental loading

r=biesi if you move the do..while above the for(;;) as discussed on irc

Comment 59

[tor]

Attachment: loop moved per biesi's comments

Comment 60

[jag (Peter Annema)]

Comment on attachment 90242 [details] [diff] [review]
loop moved per biesi's comments

sr=jag

Comment 61

[tor]

Checked in on trunk.

Comment 62

[Daniel Mario Vega (Restore MNG Support --> #18574)]

Fixed

Comment 63

[tor]

Leaving open for possible branch pickup.

Comment 64

[Greg K.]

Updating URL; original is 404.

Comment 65

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 90242 [details] [diff] [review]
loop moved per biesi's comments

branch approval granted; please change mozilla1.0.1+ to fixed1.0.1 when this is
checked into branch.

Comment 66

[tor]

Checked into branch.

Comment 67

[Terri Preston]

Verified fixed win XP branch build 2002073108, Mac branch build 2002073112 and
linux branch build 2002073006, marking verified1.0.1

Comment 68

[sairuh (rarely reading bugmail)]

looks good with recent builds, so verifying on the trunk.