856486 - mjpeg improperly displayed/refreshed

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Mr_KrzYch00]

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.

Comment 1

[Alice0775 White]

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

Comment 2

[Alex Keybl [:akeybl]]

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

Comment 3

[Seth Fowler [:seth] [:s2h]]

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.

Comment 4

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Avoid asserts triggered by rapid off-main-thread decoding.

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.

Comment 5

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Buffer the last fully-decoded frame for multipart images.

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.

Comment 6

[Seth Fowler [:seth] [:s2h]]

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

Comment 7

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Buffer the last fully-decoded frame for multipart images.

Fix a bug discovered in the try run.

Comment 8

[Seth Fowler [:seth] [:s2h]]

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

Comment 9

[Timothy Nikkel (:tnikkel)]

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?

Comment 10

[Seth Fowler [:seth] [:s2h]]

(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 11

[Timothy Nikkel (:tnikkel)]

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?

Comment 12

[Timothy Nikkel (:tnikkel)]

(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?

Comment 13

[Seth Fowler [:seth] [:s2h]]

(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.

Comment 14

[Seth Fowler [:seth] [:s2h]]

(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.)

Comment 15

[Timothy Nikkel (:tnikkel)]

(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.

Comment 16

[Timothy Nikkel (:tnikkel)]

(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?

Comment 17

[Seth Fowler [:seth] [:s2h]]

(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.)

Comment 18

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Buffer the last fully-decoded frame for multipart images.

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

Comment 19

[Seth Fowler [:seth] [:s2h]]

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

Comment 20

[Seth Fowler [:seth] [:s2h]]

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.

Comment 21

[Timothy Nikkel (:tnikkel)]

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

Comment 22

[Seth Fowler [:seth] [:s2h]]

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.

Comment 23

[Seth Fowler [:seth] [:s2h]]

Attachment: (Followup) - Make sure we use the buffered frame, when appropriate, for all callers of GetDrawableImgFrame.

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

Comment 24

[Timothy Nikkel (:tnikkel)]

(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.

Comment 25

[Seth Fowler [:seth] [:s2h]]

Attachment: (Followup 2) - Make sure we use the buffered frame, when appropriate, for all callers of 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

Comment 26

[Ryan VanderMeulen [:RyanVM]]

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

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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

Comment 29

[Seth Fowler [:seth] [:s2h]]

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 30

[Seth Fowler [:seth] [:s2h]]

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 31

[Seth Fowler [:seth] [:s2h]]

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 32

[Seth Fowler [:seth] [:s2h]]

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 33

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 34

[Seth Fowler [:seth] [:s2h]]

Thanks Lukas!

Comment 35

[Seth Fowler [:seth] [:s2h]]

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

Comment 36

[Bogdan Maris, Desktop QA]

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).