6074 - Multipart x-mixed-replace , aka server JPEG Push

Status: VERIFIED FIXED

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

Description

[Brett Wilson]

This site (I believe) uses an infinite length progressive JPEG to display a live
video feed, and only the initial image is displayed (the video does not update).

An additional (and more serious problem) is that soon after the page is loaded,
all the text on the page disappears, leaving the image in the upper left (just
like I clicked "view image" in Netscape 4).

There is also a similar video here:
http://www.berkeley.edu/sproul_cam.html

and here (this one will not be up past the end of May):
http://asterisk.reshall.berkeley.edu/room/

Comment 1

[pnunn]

hmmm.
The links now longer work.
This may still be a valid bug, but we need a test case.
-pn

Comment 2

[Brett Wilson]

Here is a new testcase:
http://www.berkeley.edu/webcams/dwinelle.html
http://www.berkeley.edu/webcams/evans.html

Comment 3

[Eli Goldberg]

Verifying as to be fixed in a later date. (Brett, if you know of reasons why it's
absolutely imperative that this bug should be fixed in Gecko 1.0, please do add
your $.02 so that it can prioritized accordingly.)

Thanks!

Comment 4

[Brett Wilson]

I think it would be OK if this did not work completely correctly in Gecko 1.0.

However, I think that it should not freak out and clear the page like it does,
which makes the pages not appear at all. They should at least appear correctly
with the initial frame of the image (this is what IE5 does). It may not be 100%
correct but at least its not broken and you can see what the page is supposed to
contain.

Comment 5

[WD]

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

Comment 6

[WD]

Just wanted to drop my $.02, that I feel this is an important issue.

The majority of the webcam community relies on JPEG push technology!
Webcam32 does work with IE, but if it detects Netscape (Mozilla), it defaults to
using JPEG push.   (which doesn't currently work with Mozilla)

Comment 7

[WD]

I have found some more information concerning JPEG push.
First, directly from Netscape, concerning server push:
http://www.netscape.com/assist/net_sites/pushpull.html
(there is a link here with a sample "push" image, which actually uses GIF,
rather than JPEG.   The concept is similar, I believe)

Another site further describing the JPEG push mechanism:
http://www-vs.informatik.uni-ulm.de/Papers/IDMS/IDMS.html#RTFToC15

And finally, the best site I've found describing JPEG push:
http://www.crs4.it/~france/webvideo/www6/


(An excerpt describing the  (psdudo)code behind JPEG push:)

print "Content-type: multipart/x-mixed-replace;boundary=---ThisRandomString---"
print "---ThisRandomString---"

while true { print "Content-type: image/jpg"
grabbing and compressing <image>
print <image>
print "---ThisRandomString---"
}



P.S.    -- I think the Summary of this bug should include "JPEG PUSH", as that
would better describe the bug.   It really doesn't have anything to do with
"progressive JPEG", as far as I can tell. . . .

Comment 8

[WD]

I am Re-opening this bug because I believe it is more important to users than
was originally believed.

One of the main applications of JPEG push technology is in the "webcam" world.
Live cameras jacked into the internet have become increasingly popular in the
past year or two.    What made this possible was the "JPEG Push" technology used
by Netscape.    In time, Java applets were designed to allow IE users to also
view webcam sites, but the core of webcam sites often relies on JPEG push.

I think it would be a shame if Mozilla / Netscape 6 would not include this
feature which the previous versions of Netscape supported fine.  The only thing
keeping me from running Mozilla as my main browser at this time is the fact that
I cannot view *my own* webcam site when using it.

Is this a feature that can be added to NS Beta2?   (or Mozilla in general?)  As
you well know by now, I'm a big supporter of getting this feature added, but I
know that there are many others who rely on this functionality on a daily basis.
 A quick google search for "webcam32" (one of the various webcam packages
relying on PUSH) comes up with 2,719 matches.  "Webcam" alone comes up with
82,599       The webcam community needs this!       (ok, I'll step off my soap
box now! ;)

In some of my previous comments in this thread, I have some information and
helpful links about server "push".    Not really being a programmer, I wish I
could do more. . . .

-WD

Comment 9

[pnunn]

I just checked in fixes last night for bug#37909 which
has fixed this bug as well.
Closing as fixed.
-P

Comment 10

[WD]

Wow...   that was quicker than I had expected!    Great job!
-WD

Comment 11

[WD]

Verified.

Comment 12

[WD]

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

Comment 13

[pnunn]

<humble grin>
-P

Comment 14

[Jonathan Walther]

<a href="http://216.100.231.10/">http://216.100.231.10</a> should be up
for a few months at least; when you access it it crashes Linux version 90% of
the time; on remaining platforms it either works fine, or works fine for a while
then displays only the alt tag.  If you get that far, pressing reload or
backward then forward also either crashes the browser or makes it display the
alt text in place of the image and hang.  This has been tested on Win2K, WinNT,
and Linux (Debian potato with kernel 2.2.14)
Hopefully this can be made to work soon?

Comment 15

[Kathleen :Brade]

Reopening per request of others on IRC

Comment 16

[Peter "jag" Annema]

Linux, build ID 2000060908

To check what was going on, I loaded this URL:
http://216.100.231.10:2230/xxx.jpg

This works for a while, appending ##<number> to the URL, with number increasing.

It doesn't crash, but stops loading after a while and just displays the URL:
http://216.100.231.10:2230/xxx.jpg##11

(or ##6, or ##3)

Comment 17

[pnunn]

Lets open a new bug for the problem rather than
mutating an old, closed bug.
-P

Comment 18

[pnunn]

The current bug is now covered by #42224.
Closing this bug.
-P

Comment 19

[John Unruh]

Verified per pnunn's comments

Comment 20

[WD]

This no longer works...
2001032304

Comment 21

[WD]

-> Pav

Comment 22

[Stuart Parmenter]

accepting.  i need to run the url through a streamconverter for
multipart/x-mixedreplace

Comment 23

[Stuart Parmenter]

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

Comment 24

[Stuart Parmenter]

Attachment: fix for this

Comment 25

[Stuart Parmenter]

Attachment: newer fix

Comment 26

[Stuart Parmenter]

added some better comments in a few places and updated to darin's new 
nsIRequestObserver changes

Comment 27

[Stuart Parmenter]

Attachment: patch that works

Comment 28

[Brendan Eich [:brendan]]

Too much he-man ADDREF/RELEASE usage.  It leads to leaks on errors, as here:

+  nsresult rv = proxyRequest->Init(aRequest, aLoadGroup, aObserver, cx);
+  if (NS_FAILED(rv)) {
+    NS_RELEASE(proxyRequest);
+    return rv;
+  }

Doesn't request have a ref that was added above, either on ImageCache::Get
hitting the cache, or by the code in this method that does NS_NEWXPCOM?

The following should initialize decodeResult (to NS_OK? not clear):

+  nsresult decodeResult;
+  if (mStatus & imgIRequest::STATUS_ERROR)
+    decodeResult = NS_IMAGELIB_ERROR_FAILURE;
+  else if (mStatus & imgIRequest::STATUS_LOAD_COMPLETE)
+    decodeResult = NS_IMAGELIB_SUCCESS_LOAD_FINISHED;
 
   // OnStopDecode
   if (mState & onStopDecode)
-    observer->OnStopDecode(nsnull, nsnull, status, nsnull);
+    observer->OnStopDecode(nsnull, nsnull, decodeResult, nsnull);
 
   if (mImage && (mObservers.Count() == 1)) {

Otherwise it may be used without being set.  Isn't it also used a bit later, in
the obs->OnStopRequest(null, null, status) call?  I don't see a diff, but status
is no more.  ???

BTW, you still haven't fixed that PR_ASSERT(observer); to be NS_ASSERTION(obs,
...) here, or changed away from PR_ASSERT elsewhere.

From imgRequest::RemoveObserver, predicated by mObservers.Count() == 0 just
after removing the observer:

-    if (mChannel && mLoading) {
+    /* If |aStatus| is a failure code, then cancel the load if it is still in
progress.
+       Otherwise, let the load continue, keeping 'this' in the cache with no
observers.
+       This way, if a proxy is destroyed without calling cancel on it, it won't
leak
+       and won't leave a bad pointer in mObservers.
+     */
+    if (mChannel && mLoading && NS_FAILED(aStatus)) {

So before, you'd always cancel, even on NS_OK status?  Why do you still always
stop animations on the last observer being removed (not shown by the diff or my
excerpt of it here, just above this comment)?

Also, after this code, you call observer->OnStopDecode (the comment before this
wrongly talks about OnStopRequest) if !(mState & onStopDecode), and then QIs to
imgRequestObserver and (bogus PR_ASSERT, but at least it's of obs this time)
calls obs->OnStopRequest if !(mState & onStopRequest).

Apart from the cut/paste comment error and the PR_ASSERT, why is removing an
observer the cause of all these callbacks?  Maybe this routine is misnamed: is
it really always caused by some kind of channel event such as a load error or
success?  A comment talking about the whole routine would help.

@@ -333,8 +343,6 @@
   PR_LOG(gImgLog, PR_LOG_DEBUG,
          ("[this=%p] imgRequest::SetImage\n", this));
 
-  if (mImage) return NS_ERROR_FAILURE;
-  

NS_ASSERTION(!mImage, "double SetImage?"); first?  Isn't this a bad thing?  I'm
concerned that the NS_ERROR_FAILURE won't be noticed, given the tendency not to
check all r.v.'s here and elsewhere (which can be ok -- sometimes it's not worth
all the code bloat if you can carry on, or ultimately detect the error later).

Aside: what's wrong with a for (i = 0; i < count; i++) loop instead of setting i
= -1 before the loop and using while (++i < count)?  At least move those i and
count initialized declarations down below this code:

   nsresult status;
-  if (mStatus & imgIRequest::STATUS_LOAD_COMPLETE)
-    status = NS_IMAGELIB_SUCCESS_LOAD_FINISHED;
-  else if (mStatus & imgIRequest::STATUS_ERROR)
+  if (mStatus & imgIRequest::STATUS_ERROR)
     status = NS_IMAGELIB_ERROR_FAILURE;
+  else if (mStatus & imgIRequest::STATUS_LOAD_COMPLETE)
+    status = NS_IMAGELIB_SUCCESS_LOAD_FINISHED;

and rename status to decodeResult, as you did earlier.  And initialize this var,
it could be used without being set, AFAICT.

The code in imgRequest.cpp always calls RemoveFromCache and Cancel in tandem,
except for the no-content-type in OnDataAvailable, where you call
RemoveFromCache only -- should Cancel be called there too?  Given the coupling,
should Cancel call RemoveFromCache to consolidate logic and reduce code bloat?

+        return NS_BINDING_ABORTED;
+      } else {
+        PR_LOG(gImgLog, PR_LOG_DEBUG,
+               ("[this=%p] imgRequest::OnDataAvailable -- Got content type from
the channel\n",
+                this));
       }

       mContentType = contentType;

else after return makes no sense -- especially given how the else governs only
the PR_LOG call, not the rest of the method starting with mContentType = .... 
Get rid of that else!

Ok, now I see why RemoveObserver is doing stuff when called with a non-failure
status (nsresult):

-  // XXX pav
-  // it isn't the job of the request proxy to cancel itself.
-  // if your object goes away and you want to cancel the load, then do it yourself.
-
-  // cancel here for now until i make this work right like the above comment
-  Cancel(NS_ERROR_FAILURE);
+  /* Call cancel with a successful status.  This will be passed to imgRequest's
RemoveObserver call
+     and will not cancel the load of the channel if 'this' is the last observer
+     of |mOwner|.
+   */
+  Cancel(NS_OK);
 }

But isn't this bogus?  The proxy's destruction shouldn't cancel anything, and
who knows when it might run in relation to other events.  Why do you use this
hack to stop animations?  What is the logic you're trying to implement?

/be

Comment 29

[Stuart Parmenter]

Attachment: better patch

Comment 30

[Stuart Parmenter]

> Too much he-man ADDREF/RELEASE usage.  It leads to leaks on errors, as here:
> 
> +  nsresult rv = proxyRequest->Init(aRequest, aLoadGroup, aObserver, cx);
> +  if (NS_FAILED(rv)) {
> +    NS_RELEASE(proxyRequest);
> +    return rv;
> +  }
> 
> Doesn't request have a ref that was added above, either on ImageCache::Get
> hitting the cache, or by the code in this method that does NS_NEWXPCOM?
> 
> The following should initialize decodeResult (to NS_OK? not clear):
> 

Request gets released outside of the function that this code is in.


> +  nsresult decodeResult;
> +  if (mStatus & imgIRequest::STATUS_ERROR)
> +    decodeResult = NS_IMAGELIB_ERROR_FAILURE;
> +  else if (mStatus & imgIRequest::STATUS_LOAD_COMPLETE)
> +    decodeResult = NS_IMAGELIB_SUCCESS_LOAD_FINISHED;
>  
>    // OnStopDecode
>    if (mState & onStopDecode)
> -    observer->OnStopDecode(nsnull, nsnull, status, nsnull);
> +    observer->OnStopDecode(nsnull, nsnull, decodeResult, nsnull);
>  
>    if (mImage && (mObservers.Count() == 1)) {
> 
> Otherwise it may be used without being set.  Isn't it also used a bit later, 
in
> the obs->OnStopRequest(null, null, status) call?  I don't see a diff, but 
status
> is no more.  ???

Yes,  I moved the status -> result code into a function and call the function 
in the places that need to do this.  I init the result to NS_OK before I do 
anything.


> 
> BTW, you still haven't fixed that PR_ASSERT(observer); to be NS_ASSERTION(obs,
> ...) here, or changed away from PR_ASSERT elsewhere.

fixed

> From imgRequest::RemoveObserver, predicated by mObservers.Count() == 0 just
> after removing the observer:
> 
> -    if (mChannel && mLoading) {
> +    /* If |aStatus| is a failure code, then cancel the load if it is still 
in progress.
> +       Otherwise, let the load continue, keeping 'this' in the cache with no 
observers.
> +       This way, if a proxy is destroyed without calling cancel on it, it 
won't leak
> +       and won't leave a bad pointer in mObservers.
> +     */
> +    if (mChannel && mLoading && NS_FAILED(aStatus)) {
> 
> So before, you'd always cancel, even on NS_OK status?  Why do you still always
> stop animations on the last observer being removed (not shown by the diff or 
my
> excerpt of it here, just above this comment)?
> 

Stoping the animation causes the timer on the animation to stop.  There is no 
point in the timer firing if no one is paying attention to the image.

The check here will shutdown an image load in progress if:
1) it has a channel and we havn't had an OnStopRequest (mLoading = PR_FALSE in 
OnStopRequest)
AND
2) the proxyrequest that has been Canceled (and is calling RemoveObserver) was 
called with an error code.  This is the case in almost every situation, except 
when the proxy gets destroyed without someone calling Cancel on it.  This is 
the case with image preloading, in which they no longer want to hold on to the 
request, but they want the image load to continue to happen.


> Also, after this code, you call observer->OnStopDecode (the comment before 
this
> wrongly talks about OnStopRequest) if !(mState & onStopDecode), and then QIs 
to
> imgRequestObserver and (bogus PR_ASSERT, but at least it's of obs this time)
> calls obs->OnStopRequest if !(mState & onStopRequest).
> 
> Apart from the cut/paste comment error and the PR_ASSERT, why is removing an
> observer the cause of all these callbacks?  Maybe this routine is misnamed: is
> it really always caused by some kind of channel event such as a load error or
> success?  A comment talking about the whole routine would help.
> 

Fixed the comment and PR_ASSERT.

It is important that the Stop callbacks get called if they have not been called 
before. This is usually hit when an image is canceled before it has finished 
loading. These should have always been fired if they needed to be and not 
conditionally as they previously were.


> @@ -333,8 +343,6 @@
>    PR_LOG(gImgLog, PR_LOG_DEBUG,
>           ("[this=%p] imgRequest::SetImage\n", this));
>  
> -  if (mImage) return NS_ERROR_FAILURE;
> -  
> 
> NS_ASSERTION(!mImage, "double SetImage?"); first?  Isn't this a bad thing?  
I'm
> concerned that the NS_ERROR_FAILURE won't be noticed, given the tendency not 
to
> check all r.v.'s here and elsewhere (which can be ok -- sometimes it's not 
worth
> all the code bloat if you can carry on, or ultimately detect the error later).
> 

It is OK for mImage to be set multiple times.  The only person with access to 
the imgRequest that can call SetImage is the image decoder.  People calling 
SetImage on the imgRequestProxy will get back an error.


> Aside: what's wrong with a for (i = 0; i < count; i++) loop instead of 
setting i
> = -1 before the loop and using while (++i < count)?  At least move those i and
> count initialized declarations down below this code:

fixed.

> 
>    nsresult status;
> -  if (mStatus & imgIRequest::STATUS_LOAD_COMPLETE)
> -    status = NS_IMAGELIB_SUCCESS_LOAD_FINISHED;
> -  else if (mStatus & imgIRequest::STATUS_ERROR)
> +  if (mStatus & imgIRequest::STATUS_ERROR)
>      status = NS_IMAGELIB_ERROR_FAILURE;
> +  else if (mStatus & imgIRequest::STATUS_LOAD_COMPLETE)
> +    status = NS_IMAGELIB_SUCCESS_LOAD_FINISHED;
> 
> and rename status to decodeResult, as you did earlier.  And initialize this 
var,
> it could be used without being set, AFAICT.

fixed the same way as earlier stated.


> 
> The code in imgRequest.cpp always calls RemoveFromCache and Cancel in tandem,
> except for the no-content-type in OnDataAvailable, where you call
> RemoveFromCache only -- should Cancel be called there too?  Given the 
coupling,
> should Cancel call RemoveFromCache to consolidate logic and reduce code bloat?
> 

It should have been calling Cancel in that place too.  I believe that was in my 
previous patch. Cancel now calls RemoveFromCache and sets mStatus with 
STATUS_ERROR.


> +        return NS_BINDING_ABORTED;
> +      } else {
> +        PR_LOG(gImgLog, PR_LOG_DEBUG,
> +               ("[this=%p] imgRequest::OnDataAvailable -- Got content type 
from the channel\n",
> +                this));
>        }
> 
>        mContentType = contentType;
> 
> else after return makes no sense -- especially given how the else governs only
> the PR_LOG call, not the rest of the method starting with mContentType = .... 
> Get rid of that else!

done

> 
> Ok, now I see why RemoveObserver is doing stuff when called with a non-failure
> status (nsresult):
> 
> -  // XXX pav
> -  // it isn't the job of the request proxy to cancel itself.
> -  // if your object goes away and you want to cancel the load, then do it 
yourself.
> -
> -  // cancel here for now until i make this work right like the above comment
> -  Cancel(NS_ERROR_FAILURE);
> +  /* Call cancel with a successful status.  This will be passed to 
imgRequest's RemoveObserver call
> +     and will not cancel the load of the channel if 'this' is the last 
observer
> +     of |mOwner|.
> +   */
> +  Cancel(NS_OK);
>  }
> 
> But isn't this bogus?  The proxy's destruction shouldn't cancel anything, and
> who knows when it might run in relation to other events.  Why do you use this
> hack to stop animations?  What is the logic you're trying to implement?

The proxyRequest owns the request.  The request has a weak pointer back to the 
proxies. The proxy therefore *must* remove itself from the request.
The first thing RemoveObserver does is:
  mObservers.RemoveElement(NS_STATIC_CAST(void*, observer));

so any future calls that might be generated from mChannel->Cancel will not ever 
reach the proxy that is calling.  This is also why we must send the 
OnStopDecode and OnStopRequest messages in RemoveObserver if needed since the 
observer will not get any more calls.

I have made is so that imgRequestProxy's destructor calls RemoveObserver 
directly after setting mObserver to null, so that messages that RemoveObserver 
may generate do not get forwarded to the proxy's observer.

Comment 31

[Stuart Parmenter]

Attachment: previous patch + code handle AsyncOpen failure

Comment 32

[Stuart Parmenter]

new patch.  same as the previous patch except this patch adds code to handle 
AsyncOpen failures (bug 74165).

Comment 33

[Brendan Eich [:brendan]]

> Request gets released outside of the function that this code is in.

No it doesn't.  We're talking about the request local variable in
imgLoader.cpp's imgLoader::LoadImageWithChannel, right?

> I have made is so that imgRequestProxy's destructor calls RemoveObserver
> directly after setting mObserver to null, so that messages that RemoveObserver
> may generate do not get forwarded to the proxy's observer.

I see mObserver = nsnull; in imgRequestProxy::Cancel, but not in the dtor.  Do
you want it in both places?  Maybe the dtor should just call Cancel(NS_OK); now
that everything looks about the same between the two?

Is the lack of OnStopRequest after Cancel a bug elsewhere?  waterson et al. have
pointed to a problem in the URI loader (IIRC) where OnStartRequest may be called
but OnStopRequest may not be called.  Just confirming you're not being bitten by
the same bug.

Thanks for the answers and comments -- I'll defer wordsmithing nits till later.
See if the above comments don't lead to a new patch that I can sr=.

/be

Comment 34

[Stuart Parmenter]

> > Request gets released outside of the function that this code is in.
> 
> No it doesn't.  We're talking about the request local variable in
> imgLoader.cpp's imgLoader::LoadImageWithChannel, right?
> 
|request| gets released at the end of imgLoader::LoadImageWithChannel().  I 
*really* don't see a leak there.  Maybe i'm blind.  Can you point it out to me?


> > I have made is so that imgRequestProxy's destructor calls RemoveObserver
> > directly after setting mObserver to null, so that messages that 
RemoveObserver
> > may generate do not get forwarded to the proxy's observer.
> 
> I see mObserver = nsnull; in imgRequestProxy::Cancel, but not in the dtor.  Do
> you want it in both places?  Maybe the dtor should just call Cancel(NS_OK); 
now
> that everything looks about the same between the two?
Ooops... yeah, my comment in the destructor was right, but i was missing the 
mObserver = nsnull;
The dtor shouldn't call Cancel() since the order of nulling things out should 
be different.
Cancel(), which can be called from outside should leave mObserver alone until 
after RemoveObserver has been called on mOwner, whereas in the destructor you 
want to null it out before.

> 
> Is the lack of OnStopRequest after Cancel a bug elsewhere?  waterson et al. 
have
> pointed to a problem in the URI loader (IIRC) where OnStartRequest may be 
called
> but OnStopRequest may not be called.  Just confirming you're not being bitten 
by
> the same bug.

The problem is lack of OnStopRequest *before* a Cancel.  The OnStop needs to 
fire to an observer even if it hasn't actually happened on the real channel yet 
so that the proxy can clean up after itself (remove itself from the loadgroup, 
etc.)

---

I'll post a new patch once I figure out what leak you see.

Comment 35

[Chris Waterson]

 // XXX are we calling this too early?
-    newChannel->AsyncOpen(NS_STATIC_CAST(nsIStreamListener *, request), nsnull);

Do you want to leave the XXX comment in, even though it's not attached to the
AsyncOpen() call any more?


+      nsresult rv = CreateNewProxyForRequest(request, aLoadGroup, aObserver,
cx, _retval);
+      if (NS_FAILED(rv)) {
+        NS_RELEASE(request);
+        return rv;
+      }
+
+      request->OnStartRequest(newChannel, nsnull);
+      request->OnStopRequest(newChannel, nsnull, NS_BINDING_ABORTED);
+
+


How about

  if (NS_SUCCEEDED(rv)) {
    request->OnStartRequest(newChannel, nsnull);
    request->OnStopRequest(newChannel, nsnull, NS_BINDING_ABORTED);
  }

  NS_RELEASE(request);
  ...

There are still lots of early returns that could cause you to drop the request
on the floor. Why not do something like this (it's a bit of a hack, but...)

  nsCOMPtr<nsICacheEntryDescriptor> entry;
  imgRequest* tmp;
  ImageCache::Get(aURI, &tmp, getter_AddRefs(entry)); // addrefs request

  nsCOMPtr<imgIRequest> kungFuDeathGrip = do_QueryInterface(tmp);
  request = tmp;
  NS_IF_RELEASE(tmp);

Might simplify some of the other logic in the rest of the routine.

+      if (nsLiteralCString("multipart/x-mixed-replace").Equals(contentType)) {

Use NS_LITERAL_CSTRING, to insulate you from random scc string damage.

Also, add a comment here that describes the trickery you're playing. ``If
multipart/x-mixed-replace content, we'll insert a MIME decoder in the pipeline
to handle the content and pass it along to our original listener.''

+      nsCOMPtr<nsIChannel> chan;
+      if (mChannel)
+        chan = mChannel;
+      else
+        chan = do_QueryInterface(aRequest, &rv);

In imgRequest::OnDataAvailable(), stuff like this boggles my mind. Why do we
sometimes use the mChannel, and other times use the request that's been passed
into us?

+
+      if (chan) {
+        rv = mChannel->GetContentType(getter_Copies(contentType));
+      }

But wait, what if mChannel == 0?

I have to confess, I don't understand your state machine. I'd try harder, but it
looks like brendan has this one covered.

Comment 36

[Stuart Parmenter]

Attachment: patch after brendan's review

Comment 37

[Stuart Parmenter]

Attachment: latest patch

Comment 38

[Stuart Parmenter]

ok, here is the (hopefully) final patch.  this latest patch includes code to 
put chrome images into their own cache session and provides a new service to 
allow people to clear the chrome cache or the everythingelse cache.  a few 
comment cleanups, etc.

Comment 39

[Brendan Eich [:brendan]]

r/sr=[e-mail], get waterson to do likewise (pav's gonna add checks
for do_CreateInstance failures in the JPEG decoder, and maybe ADD A FEW FRIGGIN'
COMMENTS explaining all the funny business about getting a pre-existing image
from the request just to handle the multipart/x-mixed-replace weirdo case).

/be

Comment 40

[Chris Waterson]

Looks pretty good. r=waterson

Comment 41

[Stuart Parmenter]

fix checked in.

Comment 42

[WD]

JPEG Push does seem to be working, at least initially, but it fails after a
certain amount of time.   

In the URL here, usually within a minute or so, the image will stop "pushing". 
If I move another window over top of the JPEG Push image and move it away, what
was behind does not re-draw.  

this also happens at http://dormcam.mine.nu:8080/index1.shtml

re-opening.

Comment 43

[Stuart Parmenter]

As far as i can tell, i see the same behavior in netscape 4.x and IE.  I 
believe that the server is shutting down the connection.

Comment 44

[WD]

Actually, try http://dormcam.mine.nu:8080/index1.shtml

This is my computer, and even viewing it from localhost gives the same results.
  The main frame refreshes once every 120 seconds.    With mozilla, the JPEG
push stops after 60 or 90 seconds.    (leaving the image frozen until the 120
seconds is up and the frame refreshes)    Sometimes the image comes up black
even after re-loading.

NS4 works fine for the above URL, and also the URL listed with this bug. 
(continuous streaming...  no stops)

Comment 45

[WD]

OK, with http://www.berkeley.edu:80/webcams/evans.html there may be a limit to
the time that the image can stream.    With the above URL, I know there is no
such limit. 

But with Mozilla, it will stop streaming at any time.  (seemingly random).   
Sometimes it will stop streaming within seconds of starting.    Moving another
window over top of the image and back again is a good test, because once Mozilla
stops receiving the JPEG push, the image doesn't repaint.

Comment 46

[Stuart Parmenter]

everytime i try and load http://dormcam.mine.nu:8080/index1.shtml, i get a 
really dark image that looks like the camera is off or something.

Comment 47

[WD]

Try again during daylight hours.   :)

Comment 48

[WD]

What I'm seeing with build 2001041604 is just like what's described in bug 42224

When the JPEG Push fails, I see the ALT text "Please Wait".   
Should this bug be marked as fixed and bug 42224 re-opened?    Or is it OK to 
keep it as this bug?

Comment 49

[Stuart Parmenter]

yeah, lets mark this one fixed.  i'll look at bug 42224

Comment 50

[Roland Mainz]

Pavlov: Why did you remove the dependicy to bug 72087 (issue: images for
PostScript&&Xprint print modules are rendered after print job is "done"(=after
EndDocument())) ?

Comment 51

[Stuart Parmenter]

cause i checked in the fix, and the bug wasn't *really* dependant on this bug 
itself, only the patch in it.

Comment 52

[Roland Mainz]

"dependant" - I assumed that means that another bug "depends on" the referenced
one and cannot be "closed" until this issue has been resolved, right ?

Sad to say... for some reason the patch
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=30390 did not solve the
issue. Do you really think we're both talking about the same issue ? Should I
try a newer patch ?

Comment 53

[Stuart Parmenter]

the patch has been checked in.  update your tree.

Comment 54

[Terri Preston]

Reopening, camera shows briefly but then goes blank, saw this behavior on linux
build 2001053008 and w98 build 2001053004

Comment 55

[WD]

I think that this bug should probably be left as fixed, as it was filed under
the old ImgLib.

The symptoms you are seeing Terri are really bug 42224

Comment 56

[selmer (gone)]

m.9 is past, resetting milestone to ---

Comment 57

[Stuart Parmenter]

i agree.  re-marking fixed.  moving target milestone back to 0.9.  Look at bug 
42224.

Comment 58

[Terri Preston]

Okay, will mark verified then