Last Comment Bug 625012 - Flickering Motion JPEG (mjpg)
: Flickering Motion JPEG (mjpg)
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: All All
: -- normal with 7 votes (vote)
: mozilla34
Assigned To: Artem Sobolev [:asobolev]
:
Mentors:
http://91.143.176.52/mjpg/video.mjpg
: 726271 (view as bug list)
Depends on:
Blocks: 667206
  Show dependency treegraph
 
Reported: 2011-01-12 03:40 PST by Hugh
Modified: 2014-08-15 13:17 PDT (History)
17 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed fix (7.38 KB, patch)
2014-07-18 14:39 PDT, Artem Sobolev [:asobolev]
no flags Details | Diff | Review
Proposed fix v2 (7.31 KB, patch)
2014-07-18 16:45 PDT, Artem Sobolev [:asobolev]
no flags Details | Diff | Review
Bugfix v3 (9.79 KB, patch)
2014-07-23 17:27 PDT, Artem Sobolev [:asobolev]
bzbarsky: review+
Details | Diff | Review
Bugfix v4 (9.15 KB, patch)
2014-07-29 15:34 PDT, Artem Sobolev [:asobolev]
bzbarsky: review+
Details | Diff | Review

Description Hugh 2011-01-12 03:40:07 PST
User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b10pre) Gecko/20110111 Firefox/4.0b10pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b10pre) Gecko/20110111 Firefox/4.0b10pre

http://202.213.248.205:12080/mjpeg

The above link (when it works, it can be a bit flakey) points to a Sony network camera using motion JPEG. The video continuously flickers in Firefox 3.6.x and Firefox 4.0 Nightly. Works fine is Chrome

Reproducible: Always

Steps to Reproduce:
1.Visit http://202.213.248.205:12080/mjpeg in Firefox
2. Notice that video flickers

(note link works spasmodically but does work)
Actual Results:  
Video stream flickers horribly

Expected Results:  
Video stream should play without flickering

Works fine in Google Chrome
Comment 1 Alice0775 White 2011-01-12 04:40:11 PST
Confirmed with clean profile
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b10pre) Gecko/20110111 Firefox/4.0b10pre ID:20110111030357
Comment 2 Hugh 2011-01-12 04:48:01 PST
http://202.213.248.205:12080/en/index.html
The above url uses a JAVA applet / active x depending on your browser, same stream though.
Comment 3 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-01-12 19:29:30 PST
Is the issue that we blow away the previous jpeg as soon as the new one starts coming in, and the site sends them continuously but with pauses in the middle of the individual jpegs?

This doesn't seem to be a regression from 3.5 either, which would make sense if the above hypothesis is correct.
Comment 4 Hugh 2011-01-14 02:16:12 PST
Sorry I do not have the technical ability to figure out the exact course however I can tell you one thing, this seriously limits Firefox's usefulness for view IP camera streams.

I hope this is fixed before the final release of Firefox 4
Comment 5 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-01-14 05:24:03 PST
Hugh, there's no way this is being fixed before 4 ships.  It's months past freeze, this isn't a new problem introduced since 3.6, the impact is low (yes, I know it sucks for this particular situation, but "high" impact at this point would be something like "gmail is broken").

It's a bug; we need to fix it.  But it's not a super-high-priority bug, and it's far too late in the Firefox 4 cycle for it to be fixed for Firefox 4 without dropping some things that are higher priority in the process.
Comment 6 Hugh 2011-01-14 14:59:50 PST
Ok, thanks for your candid reply. I understand you have bigger problems to worry about, hopefully it does get fixed in the not too distant future and doesn't just drop off the radar.
Comment 7 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-01-14 17:20:27 PST
Bobby, want to take this once you're done with school stuff?  ;)
Comment 8 David Humphrey (:humph) 2011-06-25 08:35:37 PDT
I'm hitting this bug today working on a web app for a security camera.  Many web cams (and especially high-end ones) use mjpg for streaming the feeds.  With the ones I'm working against, Chrome and Safari are smooth, and Firefox is really uneven.  Finding examples of mjpgs online is hard, but here's another one that is bad for us:

http://91.143.176.52/mjpg/video.mjpg

We should fix this.
Comment 9 David Humphrey (:humph) 2011-06-25 09:19:25 PDT
Possibly related to bug 667203.
Comment 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-06-26 08:13:56 PDT
This is fundamentally the same issue as bug 667206.  See comment 3...  Basically, we don't keep the previous frame's data once a new frame starts coming in.  It would make a lot of sense to me to buffer it up internally and then swap when the new frame is completely done.  That would incidentally fix bug 667206 too.
Comment 11 David Schleef 2011-08-10 16:08:43 PDT
I've noticed the same thing if the server does not send the '\r\n' to close out the multipart/x-mixed-replace chunk.  The bottom row of JPEG macroblocks does not get drawn, so it is always one frame behind, which looks rather odd.

Upon request, I can easily create streams to reproduce this problem.
Comment 12 Hugh 2012-01-30 17:36:03 PST
The following stream

http://www.themercury.com.au/images/uploads/webcams/webcam2%28MTVIEW%29.html

appears to be motion JPEG and works fine in Firefox.
Comment 13 Justin Dolske [:Dolske] 2012-02-11 00:28:34 PST
*** Bug 726271 has been marked as a duplicate of this bug. ***
Comment 14 Alex Haan 2012-06-13 23:35:24 PDT
(In reply to Hugh from comment #12)
> The following stream
> 
> http://www.themercury.com.au/images/uploads/webcams/webcam2%28MTVIEW%29.html
> 
> appears to be motion JPEG and works fine in Firefox.

On that page the mjpeg is embedded in html. If you open the stream directly, it will show the issue: http://120.29.240.93/axis-cgi/mjpg/video.cgi?resolution=640x360
Comment 15 Jeena Paradies 2013-12-10 14:04:12 PST
When I try it on Firefox on my FirefoxOS phone then this doesn't happen. But it could be because it takes it quite some time to show the next frame, 2-5 seconds.
Comment 16 Seth Fowler [:seth] [:s2h] 2014-07-01 16:21:59 PDT
This looks like a good first bug. I'm assigning this to asobolev. I'll act as mentor.
Comment 17 Seth Fowler [:seth] [:s2h] 2014-07-01 16:29:56 PDT
Artem, this is still quite visible at when you view an MJPEG webcam directly (i.e., not embedded in a web page).

Here's an example that demonstrates the problem for me:

http://204.248.124.203/mjpg/video.mjpg?camera=1

Here's the same webcam embedded in a web page. I don't see the problem here:

http://www.mjpeg.net/webcams/14921

It's odd that this happens in one case but not the other. It'd be good to verify that we aren't somehow refusing to use mMultipartDecodedFrame when drawing this image. RasterImage::mMultipartDecodedFrame exists specifically to store the previous completely decoded image of a multipart/x-mixed-replace content stream, so that we can draw it instead of the currently-decoding one and avoid this kind of tearing.

Incidentally, SpeedLimit is very useful for investigating this kind of problem on OS X:

http://mschrag.github.io/
Comment 18 Seth Fowler [:seth] [:s2h] 2014-07-01 17:10:13 PDT
It may be that the fix for this problem will also fix the rapidly-changing tab title that we see with MJPEG images. If so, that'd be great.

Artem, there are some files I didn't show you that are relevant to this bug. |content/html/document/src/ImageDocument.h| creates a "synthetic document" that wraps a raw image when you navigate to it directly. |layout/generic/nsImageFrame.h| represents <img> elements in the layout system. ImageDocument creates a synthetic <img> element to display its image. You'll probably encounter both of those files when debugging this problem.
Comment 19 Artem Sobolev [:asobolev] 2014-07-07 18:44:59 PDT
So the problem seem to be in following:

0. Some low-level stuff happens
1. Networking code handles OnDataAvailable event and calls nsMultiMixedConv to prase multipart data
2. nsMultiMixedConv calls some other methods (nsDocumentOpenInfo::OnStartRequest -> nsDocumentOpenInfo::DispatchContent -> nsDocumentOpenInfo::TryContentListener -> nsDSURIContentListener::DoContent)
3. nsDSURIContentListener::DoContent needs to create new content viewer via nsDocShell::CreateContentViewer

The problem is that
a. The document itself keeps listening for events.tt
b. The document creates new viewers though it already created one.
Comment 20 Boris Zbarsky [:bz] (Out June 25-July 6) 2014-07-07 18:56:43 PDT
Ah, interesting.  This is the full-page view, right.  Normally, if this were loaded via an <img> tag, all the multipart handling would be local to imagelib.

mconley, I bet you'll be interested in the discussion in this bug too.  ;)

So the fundamental problem is that nothing guarantees that the parts in a multipart/x-mixed-replace will be the same type.  We could have a JPEG part, then an HTML part...  We could try to do something special for the particular case when we have a JPEG part coming in, and it's part of a multipart response, and we are already showing an image document for another part of the same multipart respose.  That "something special" would presumably involve having nsDSURIContentListener::DoContent just punch through to the existing image document instead of creating a new viewer.
Comment 21 Reto Diethelm 2014-07-08 08:06:01 PDT
It seems bug 987135 is a duplicate of this bug. I don't know if it contains some useful hints and test examples.

But I think you are on the right way. JPEG parts with same width and height is the usual use case for multipart/x-mixed-replace. A direct, "low level" image replace for this special case would be a nice solution.
Comment 22 Boris Zbarsky [:bz] (Out June 25-July 6) 2014-07-08 08:21:37 PDT
> It seems bug 987135 is a duplicate of this bug.

No.  That bug is about multipart JPEG loaded via <img> while this bug is about them loaded standalone.
Comment 23 Seth Fowler [:seth] [:s2h] 2014-07-10 17:04:00 PDT
(In reply to Reto Diethelm from comment #21)
> But I think you are on the right way. JPEG parts with same width and height
> is the usual use case for multipart/x-mixed-replace. A direct, "low level"
> image replace for this special case would be a nice solution.

Same width/height should not matter, in theory. We have notifications to handle that. Whether the content code correctly handles those notifications is another matter. =)
Comment 24 Artem Sobolev [:asobolev] 2014-07-18 12:07:49 PDT
Ok, so the bug is in ImageDocument. Its OnStartRequest can't handle several calls: on each call it adds an observer (which increases memory consumption) and starts image loading. The image is already being loaded, so we get a cache hit and request "aborts".

Solution is simple: check if we already loading the image in ImageDocument::OnStartRequest and if we do, don't start new load.
Comment 25 Artem Sobolev [:asobolev] 2014-07-18 14:39:00 PDT
Created attachment 8458961 [details] [diff] [review]
Proposed fix

Changes description:
 - A check to prevent multiple requests in ImageListener::OnStartRequest
Check
 - Content viewer reuse in nsDSURIContentListener::DoContent
Comment 26 Seth Fowler [:seth] [:s2h] 2014-07-18 15:49:08 PDT
Pushed a try job:

https://tbpl.mozilla.org/?tree=Try&rev=852ffd77310f
Comment 27 Seth Fowler [:seth] [:s2h] 2014-07-18 16:08:34 PDT
Comment on attachment 8458961 [details] [diff] [review]
Proposed fix

Review of attachment 8458961 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for getting this figured out, Artem!

I've gone ahead and taken a look over the patch so we can start getting it ready for review. My comments are below.

::: content/html/document/src/ImageDocument.cpp
@@ +457,5 @@
>  NS_IMETHODIMP
>  ImageDocument::Notify(imgIRequest* aRequest, int32_t aType, const nsIntRect* aData)
>  {
> +  if (!mImageContent) {
> +    return NS_OK;

This is the one part of the patch I don't understand. Were you hitting a problem that this change fixed?

::: docshell/base/nsDSURIContentListener.cpp
@@ +28,5 @@
>  //*****************************************************************************
>  
>  nsDSURIContentListener::nsDSURIContentListener(nsDocShell* aDocShell)
> +    : mDocShell(aDocShell)
> +    , mExistingJPEGContentViewer(nullptr)

Note that nsRefPtr's default constructor initializes to null by default, so you don't need this. (And nsCOMPtr does the same.)

@@ +122,5 @@
>  
> +    // In case of multipart jpeg request (mjpeg) we don't really want to
> +    // create new viewer since that one we already have is capable of
> +    // rendering multipart jpeg correctly (see bug 625012)
> +    if (mExistingJPEGContentViewer && strcmp(aContentType, "image/jpeg") == 0) {

It's better to avoid strcmp, which is a notoriously error-prone function. The appropriate API is not exactly obvious, I know, but you can achieve the same thing with:

> nsDependentCString(aContentType).EqualsLiteral("image/jpeg")

::: docshell/base/nsDSURIContentListener.h
@@ +54,5 @@
>                              XFOHeader aHeader);
>  protected:
>      nsDocShell*                      mDocShell;
> +    // Hack to handle multipart images without creating a new viewer
> +    nsRefPtr<nsIStreamListener>      mExistingJPEGContentViewer;

Since you're holding an XPCOM interface here, this should be an nsCOMPtr rather than an nsRefPtr.
Comment 28 Artem Sobolev [:asobolev] 2014-07-18 16:42:21 PDT
> 
> This is the one part of the patch I don't understand. Were you hitting a
> problem that this change fixed?
> 
Originally, that fix was to fix crashes on shutdown (or tab close) caused by multiple loaders (looks like several end notifications were send). Now it seems to be unnecessary.
Comment 29 Artem Sobolev [:asobolev] 2014-07-18 16:45:13 PDT
Created attachment 8459025 [details] [diff] [review]
Proposed fix v2

2nd version of bugfix: unnecessary code is deleted, strcmp is replaced with EqualsLiteral
Comment 30 Seth Fowler [:seth] [:s2h] 2014-07-18 18:13:08 PDT
Comment on attachment 8459025 [details] [diff] [review]
Proposed fix v2

Review of attachment 8459025 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

We should get a reviewer from the content team if the try results look good.

(By the way, when you upload a new version of a patch, remember to mark the previous version obsolete. Using 'hg bzexport' will take care of all this for you.)

::: content/html/document/src/ImageDocument.cpp
@@ +456,5 @@
>  
>  NS_IMETHODIMP
>  ImageDocument::Notify(imgIRequest* aRequest, int32_t aType, const nsIntRect* aData)
>  {
> +

You'll want to get rid of the empty line here, too.
Comment 31 Boris Zbarsky [:bz] (Out June 25-July 6) 2014-07-18 18:23:05 PDT
Mike, more docshell bits you might be interested in.
Comment 32 Seth Fowler [:seth] [:s2h] 2014-07-19 14:46:43 PDT
It looks like there are some orange tests in the try results. Artem, can you take a look and see if you can reproduce those issues locally?
Comment 33 Artem Sobolev [:asobolev] 2014-07-23 17:27:23 PDT
Created attachment 8461235 [details] [diff] [review]
Bugfix v3
Comment 34 Seth Fowler [:seth] [:s2h] 2014-07-24 17:12:08 PDT
Pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=fb3150790102
Comment 35 Seth Fowler [:seth] [:s2h] 2014-07-24 17:31:34 PDT
Ack, just realized that repo was updated to an older revision. Here's a new try push against tip:

https://tbpl.mozilla.org/?tree=Try&rev=9c8a75b3888a
Comment 36 Artem Sobolev [:asobolev] 2014-07-25 17:06:57 PDT
Comment on attachment 8461235 [details] [diff] [review]
Bugfix v3

Tests are green, review is required
Comment 37 Boris Zbarsky [:bz] (Out June 25-July 6) 2014-07-28 21:18:20 PDT
Comment on attachment 8461235 [details] [diff] [review]
Bugfix v3

> ImageDocument::Notify(imgIRequest* aRequest, int32_t aType, const nsIntRect* aData)
> {
>+

Stray blank line?  Please remove.

>+  // Don't release mDocument here since it's not the end
>+  // in case of multipart response (see bug 625012)

How about:

  Don't release mDocument here if we're in the middle of a multipart response.

>@@ -113,18 +115,40 @@ nsDSURIContentListener::DoContent(const 
>+    // create new viewer since that one we already have is capable of

s/that/the/

>+    nsCOMPtr<nsIChannel> baseChannel = 0;

Please drop the "= 0" bit: nsCOMPtr pre-initializes to null.

>+    bool reuseCV = nsDependentCString(aContentType).EqualsLiteral("image/jpeg") && (baseChannel && baseChannel == mExistingJPEGRequest);

Please watch the 80-column boundary.  Wrap as needed.

>+++ b/docshell/base/nsDSURIContentListener.h
>+    nsCOMPtr<nsIStreamListener>      mExistingJPEGContentViewer;

Maybe call this mExistingJPEGStreamListener?

>+    nsIChannel*                      mExistingJPEGRequest;

This needs to be a strong reference.

>+++ b/image/src/imgRequest.cpp
>+    statusTracker->SetIsMultipart();

Why move that line?

>+    NS_ABORT_IF_FALSE(!mIsMultiPartChannel,
>+                    "Something went wrong");

Please fix the indent (tabs?).

This looks very nice!  r=me
Comment 38 Artem Sobolev [:asobolev] 2014-07-29 14:32:19 PDT
> This needs to be a strong reference.
What do you mean by strong reference? It shouldn't be an owning (smart-)pointer since that pointer serves as a request's id only.
Comment 39 Boris Zbarsky [:bz] (Out June 25-July 6) 2014-07-29 14:34:07 PDT
> It shouldn't be an owning (smart-)pointer 

That's what I was saying it should be.

> since that pointer serves as a request's id only.

A new object can get allocated at the same location if you don't keep the old one alive.
Comment 40 Artem Sobolev [:asobolev] 2014-07-29 15:34:57 PDT
Created attachment 8464311 [details] [diff] [review]
Bugfix v4

Edited version of the patch
Comment 41 Boris Zbarsky [:bz] (Out June 25-July 6) 2014-07-29 18:44:04 PDT
Comment on attachment 8464311 [details] [diff] [review]
Bugfix v4

r=me
Comment 42 Ryan VanderMeulen [:RyanVM] 2014-08-15 08:26:04 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/9422c84b143b
Comment 43 Ryan VanderMeulen [:RyanVM] 2014-08-15 13:17:25 PDT
https://hg.mozilla.org/mozilla-central/rev/9422c84b143b

Note You need to log in before you can comment on or make changes to this bug.