User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130330 Firefox/22.0 Build ID: 20130330030828 Steps to reproduce: Opened a site: http://webcams.4my.eu/402 to display motionJPEG camera stream (similar to axis mJPG camera stream when accessing camera through it's panel without using applets etc. - that it is said to work in Firefox but not IE for example) Actual results: mJPEG camera stream started to blink and disappeared after 3 seconds or so. Expected results: It should show motion like normal GIF banner, showing next frame of mJPEG image after it was downloaded by the browser and not disappear after some time. In Google Chrome it works correctly as well as older version of Firefox.

Regression window(m-c) Good: http://hg.mozilla.org/mozilla-central/rev/1d6fe70c79c5 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130320 Firefox/22.0 ID:20130320044721 Bad: http://hg.mozilla.org/mozilla-central/rev/a73a2b5c423b Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130321 Firefox/22.0 ID:20130321090706 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1d6fe70c79c5&tochange=a73a2b5c423b Regression window(m-i) Good: http://hg.mozilla.org/integration/mozilla-inbound/rev/c9c29cc97af0 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130320 Firefox/22.0 ID:20130320062640 Bad: http://hg.mozilla.org/integration/mozilla-inbound/rev/1407288d820b Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130320 Firefox/22.0 ID:20130320074043 Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c9c29cc97af0&tochange=1407288d820b Suspected: Bug 716140 Probably, this is duplication of Bug 854762

Seth - can you confirm comment 1 and dupe as you see fit?

Not totally sure if this is a dupe. I actually get a crash for this one. Will investigate a little more and see what I can learn.

This issue exposes some minor bugs that cause assertions to eventually be fired when doing rapid decoding again and again for the same thread, as seen in multipart images. One issue is a wrong return value that causes an error to be reported when it should not, and one issue results in FinishedSomeDecoding trying to decode more even if it is called with eShutdownIntent_Error, as might happen when you hit the first bug.

If part 1 is applied, the mJPEG stream stops disappearing, but it still blinks and tears extremely severely. To fix this, I've added a double buffering scheme that keeps around the last completely decoded frame and draws that whenever it's available, instead of drawing the current frame, which is almost always only partially decoded.

Try job here: https://tbpl.mozilla.org/?tree=Try&rev=6a78b2e8ecb9

Fix a bug discovered in the try run.

New try job here: https://tbpl.mozilla.org/?tree=Try&rev=9aa6bef16179

Comment on attachment 733001 [details] [diff] [review] (Part 2) - Buffer the last fully-decoded frame for multipart images. >@@ -1566,16 +1569,27 @@ RasterImage::DecodingComplete() >+ // Double-buffer our frame in the multipart case, since we'll start decoding >+ // into mFrames again immediately and this produces severe tearing. >+ if (mMultipart && mFrames.Length() == 1) { >+ imgFrame* swapFrame = mMultipartDecodedFrame; >+ mMultipartDecodedFrame = GetImgFrameNoDecode(GetCurrentFrameIndex()); >+ mFrames.Clear(); >+ if (swapFrame) { >+ mFrames.AppendElement(swapFrame); >+ } >+ } >+ How is this going to interact with the multipart code in AddSourceData that messes with mFrames as well?

(In reply to Timothy Nikkel (:tn) from comment #9) > How is this going to interact with the multipart code in AddSourceData that > messes with mFrames as well? Should be OK. That code ensures that if the previous part was animated (more than one frame), when we get the next part we delete the now-unneeded animation frames. If there is only one frame, it does nothing. This is fine, since we only do the swap when there's only one frame (meaning no interaction was possible) and even if AddSourceData for some reason did decide to delete that frame, it won't affect correctness; EnsureFrame will create a new one later.

Comment on attachment 732605 [details] [diff] [review] (Part 1) - Avoid asserts triggered by rapid off-main-thread decoding. SyncDecode has the same "return NS_ERROR_NOT_AVAILABLE". Is that the correct thing to do there because we specifically want a sync decode there?

(In reply to Seth Fowler [:seth] from comment #10) > (In reply to Timothy Nikkel (:tn) from comment #9) > > How is this going to interact with the multipart code in AddSourceData that > > messes with mFrames as well? > > Should be OK. That code ensures that if the previous part was animated (more > than one frame), when we get the next part we delete the now-unneeded > animation frames. If there is only one frame, it does nothing. This is fine, > since we only do the swap when there's only one frame (meaning no > interaction was possible) and even if AddSourceData for some reason did > decide to delete that frame, it won't affect correctness; EnsureFrame will > create a new one later. Hmm, but if it is an animated image we will have mFrames.Length() == 1 until we add the second frame of the animation, and then later mMultipartDecodedFrame pointer could be dangling?

(In reply to Timothy Nikkel (:tn) from comment #12) > Hmm, but if it is an animated image we will have mFrames.Length() == 1 until > we add the second frame of the animation, and then later > mMultipartDecodedFrame pointer could be dangling? Nah, because mMultipartDecodedFrame doesn't point to a frame that's in mFrames. Deleting every frame in mFrames would have no effect on the frame pointed to by mMultipartDecodedFrame.

(In reply to Timothy Nikkel (:tn) from comment #11) > Comment on attachment 732605 [details] [diff] [review] > (Part 1) - Avoid asserts triggered by rapid off-main-thread decoding. > > SyncDecode has the same "return NS_ERROR_NOT_AVAILABLE". Is that the correct > thing to do there because we specifically want a sync decode there? I think so, and I think that's probably how the problematic behavior found its way into RequestDecodeCore. The distinction is that callers expect decoding to be finished after SyncDecode unless it reports an error, while they only expect decoding to be _requested_ after RequestDecodeCore, which is still true even if we don't have enough data to actually start the job yet when they initially call it. (We store their intention in mWantFullDecode, and will start it later.)

(In reply to Seth Fowler [:seth] from comment #14) > (In reply to Timothy Nikkel (:tn) from comment #11) > > Comment on attachment 732605 [details] [diff] [review] > > (Part 1) - Avoid asserts triggered by rapid off-main-thread decoding. > > > > SyncDecode has the same "return NS_ERROR_NOT_AVAILABLE". Is that the correct > > thing to do there because we specifically want a sync decode there? > > I think so, and I think that's probably how the problematic behavior found > its way into RequestDecodeCore. The distinction is that callers expect > decoding to be finished after SyncDecode unless it reports an error, while > they only expect decoding to be _requested_ after RequestDecodeCore, which > is still true even if we don't have enough data to actually start the job > yet when they initially call it. (We store their intention in > mWantFullDecode, and will start it later.) Yeah, I was thinking the same, just wanted to confirm.

(In reply to Seth Fowler [:seth] from comment #13) > (In reply to Timothy Nikkel (:tn) from comment #12) > > Hmm, but if it is an animated image we will have mFrames.Length() == 1 until > > we add the second frame of the animation, and then later > > mMultipartDecodedFrame pointer could be dangling? > > Nah, because mMultipartDecodedFrame doesn't point to a frame that's in > mFrames. Deleting every frame in mFrames would have no effect on the frame > pointed to by mMultipartDecodedFrame. Ok, but if we become animated after mMultipartDecodedFrame has been set to something non-null shouldn't we clean up mMultipartDecodedFrame?

(In reply to Timothy Nikkel (:tn) from comment #16) > Ok, but if we become animated after mMultipartDecodedFrame has been set to > something non-null shouldn't we clean up mMultipartDecodedFrame? Hmm yeah, we should! I'll update the code in DecodingComplete to check for that case. If we have |mMultipart && mFrames.Length() > 1| then we should delete any existing mMultipartDecodedFrame, because we won't use it anymore. (I actually used to update mMultipartDecodedFrame in the animated case too, but there are some subtleties there that make it substantially more complex, and the animated case shouldn't really need this hack anyway.)

Addressed cleanup of mMultipartDecodedFrame in the case where we become animated after we already buffered a frame.

Thanks for the review! Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/499692d0ad63 https://hg.mozilla.org/integration/mozilla-inbound/rev/f5059c01c197

Ah drat, and I just realized I forgot to update the commit messages when pushing. As you can see above this was reviewed by :tn, not :jrmuizel. If (heaven forbid!) it's backed out for some reason, I'll fix that.

Do any of the other consumers (ExtractFrame, CopyFrame) need to use the buffered frame?

Thanks for your thoroughness here, Timothy! They should use the buffered frame, indeed. I think the right approach is to essentially move the change in RasterImage::Draw to RasterImage::GetDrawableImgFrame. That will automatically take care of all the cases. That's a very simple patch, so I'll push it in as a followup shortly.

Followup patch that moves the code for using the buffered frame from RasterImage::Draw to RasterImage::GetDrawableImgFrame. Pushed in as: https://hg.mozilla.org/integration/mozilla-inbound/rev/91de875536e8

(In reply to Seth Fowler [:seth] from comment #23) > https://hg.mozilla.org/integration/mozilla-inbound/rev/91de875536e8 I noticed in there that we should probably still use GetImgFrame(framenum) if mMultipartDecodedFrame is null in GetDrawableImgFrame.

Sigh. I should've gotten you to review it, in retrospect. I pushed a fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/24d4bd4b5fd7

https://hg.mozilla.org/mozilla-central/rev/499692d0ad63 https://hg.mozilla.org/mozilla-central/rev/f5059c01c197

https://hg.mozilla.org/mozilla-central/rev/91de875536e8 https://hg.mozilla.org/mozilla-central/rev/24d4bd4b5fd7

Comment on attachment 732605 [details] [diff] [review] (Part 1) - Avoid asserts triggered by rapid off-main-thread decoding. [Approval Request Comment] Bug caused by (feature/regressing bug #): 716140 User impact if declined: mJPEGs (which are frequently used for webcams) will become unwatchable - the user will observe severe tearing, with most of the displayed image often being garbage. Testing completed (on m-c, etc.): On m-c since 2013-04-04 with no problems. Risk to taking this patch (and alternatives if risky): This patch makes us store an extra frame of the mJPEG image so that we always have one completely decoded frame to display. This will increase memory usage for pages with mJPEG images. I do not see an alternative that will work in all situations, unfortunately.

Comment on attachment 733147 [details] [diff] [review] (Part 2) - Buffer the last fully-decoded frame for multipart images. (Also requesting uplift for the remaining parts.)

Comment on attachment 733567 [details] [diff] [review] (Followup) - Make sure we use the buffered frame, when appropriate, for all callers of GetDrawableImgFrame. (Also requesting uplift for the remaining parts.)

Comment on attachment 733598 [details] [diff] [review] (Followup 2) - Make sure we use the buffered frame, when appropriate, for all callers of GetDrawableImgFrame. (Also requesting uplift for the remaining parts.) BTW, I noticed that the "String or IDL changes made by this patch" line got accidentally deleted from my original request. There are no string or IDL changes made by this patch.

Comment on attachment 733598 [details] [diff] [review] (Followup 2) - Make sure we use the buffered frame, when appropriate, for all callers of GetDrawableImgFrame. approving - we should get more eyes on this in aurora to help shake out any further regressions/memory hogging before getting to beta.

Thanks Lukas!

https://hg.mozilla.org/releases/mozilla-aurora/rev/35461c20c883 https://hg.mozilla.org/releases/mozilla-aurora/rev/4293821af365 https://hg.mozilla.org/releases/mozilla-aurora/rev/195d5b99252c https://hg.mozilla.org/releases/mozilla-aurora/rev/761c42b1494c https://hg.mozilla.org/releases/mozilla-aurora/rev/f800ea754b99

Mozilla/5.0 (X11; Linux i686; rv:23.0) Gecko/20100101 Firefox/23.0 Mozilla/5.0 (Windows NT 6.1; rv:23.0) Gecko/20100101 Firefox/23.0 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:23.0) Gecko/20100101 Firefox/23.0 Verified as fixed on Firefox 23 beta 9 (buildID: 20130725195523) and latest Nightly (buildID: 20130725171558).