Closed Bug 625012 Opened 13 years ago Closed 10 years ago

Flickering Motion JPEG (mjpg)

Categories

(Core :: Graphics: ImageLib, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: Hubird.au, Assigned: asobolev)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file, 3 obsolete files)

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
Confirmed with clean profile
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b10pre) Gecko/20110111 Firefox/4.0b10pre ID:20110111030357
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.
Component: General → ImageLib
Product: Firefox → Core
QA Contact: general → imagelib
Version: unspecified → Trunk
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → All
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
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.
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.
Bobby, want to take this once you're done with school stuff?  ;)
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.
Summary: Flickering Motion JPEG → Flickering Motion JPEG (mjpg)
Possibly related to bug 667203.
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.
Blocks: 667206
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.
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.
(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
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.
This looks like a good first bug. I'm assigning this to asobolev. I'll act as mentor.
Assignee: nobody → asobolev
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/
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.
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.
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.
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.
> 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.
(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. =)
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.
Attached patch Proposed fix (obsolete) — Splinter Review
Changes description:
 - A check to prevent multiple requests in ImageListener::OnStartRequest
Check
 - Content viewer reuse in nsDSURIContentListener::DoContent
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.
> 
> 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.
Attached patch Proposed fix v2 (obsolete) — Splinter Review
2nd version of bugfix: unnecessary code is deleted, strcmp is replaced with EqualsLiteral
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.
Attachment #8458961 - Attachment is obsolete: true
Mike, more docshell bits you might be interested in.
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?
Attached patch Bugfix v3 (obsolete) — Splinter Review
Attachment #8459025 - Attachment is obsolete: true
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 on attachment 8461235 [details] [diff] [review]
Bugfix v3

Tests are green, review is required
Attachment #8461235 - Flags: review?(bzbarsky)
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
Attachment #8461235 - Flags: review?(bzbarsky) → review+
> 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.
> 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.
Attached patch Bugfix v4Splinter Review
Edited version of the patch
Attachment #8461235 - Attachment is obsolete: true
Attachment #8464311 - Flags: review?(bzbarsky)
Comment on attachment 8464311 [details] [diff] [review]
Bugfix v4

r=me
Attachment #8464311 - Flags: review?(bzbarsky) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9422c84b143b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.