816817 - Camera - improper MediaStream clean-up when DOMCameraPreview is not consumed

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Comment 1

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: EndTrack()/Finish() in DOMCameraPreview::SetStateStopped()

In some cases the preview will be stopped, but not as a result of the MediaStream changing from "consuming" to "not consumed"--in this case we still need to call EndTrack() and Finish(), to allow the MediaStream to flush its queue so that we don't waste graphics buffers.  To do this, we must call these functions from SetStateStopped().

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=cb33cbc9a472

Comment 2

[Mike Habicher [:mikeh] (high bugzilla latency)]

Inder, can you try this patch out?

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This works great for me.  Nice!

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Rebased to gecko-18

Comment 5

[Inder]

(In reply to Mike Habicher [:mikeh] from comment #2)
> Inder, can you try this patch out?
Yep, works for me too! Thanks for the simple fix. Ship it!

Comment 6

[Inder]

Mike, Can you please get this patch landed today on beta?

Comment 7

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to Inder from comment #6)
> Mike, Can you please get this patch landed today on beta?

As quickly as I can.  It still needs r+.

Comment 8

[Mike Habicher [:mikeh] (high bugzilla latency)]

What's going on:

Calling EndTrack() on a track and Finish() on a MediaStream are required to get a MediaStream buffer to release all of its MediaSegments (which are the objects that contain the actual VideoImages, in the case of a video stream).  EndTrack() calls SetEnded() on the specified track[1], which sets mEnded and which allows the test in StreamBuffer::ForgetUpTo()[2] to pass, so that the final VideoImage can be removed from the mTracks[3].

1. http://mxr.mozilla.org/mozilla-central/source/content/media/MediaStreamGraph.h#635
2. http://mxr.mozilla.org/mozilla-central/source/content/media/StreamBuffer.cpp#50
3. http://mxr.mozilla.org/mozilla-central/source/content/media/StreamBuffer.cpp#296

If EndTrack() and Finish() are not called, any VideoImages retained by the MediaStream will be destroyed when the MediaStream is CCed, but not before.

Without to the above patch, the calls to EndTrack() and Finish() take place in DOMCameraPreview::StopPreview()[4], which is (only) called in response to a PreviewControl::STOP event[5] triggered by a MediaStreamListener consumption change event[6].  This happens when the <video> element's mozSrcObject property is set to something other than the MediaStream, as happens during the onSuccess callback of the CameraControl object's GetPreviewStream() and GetPreviewStreamVideoMode() methods.  When StopPreview() is called, it calls the CameraControl object's StopPreview() method (which is not DOM-facing), which acknowledges the stop request by calling back to DOMCameraPreview::Stopped()[7].

4. http://mxr.mozilla.org/mozilla-central/source/dom/camera/DOMCameraPreview.cpp#242
5. http://mxr.mozilla.org/mozilla-central/source/dom/camera/DOMCameraPreview.cpp#47
6. http://mxr.mozilla.org/mozilla-central/source/dom/camera/DOMCameraPreview.cpp#116
7. http://mxr.mozilla.org/mozilla-central/source/dom/camera/GonkCameraControl.cpp#640

The flow is as follows:

1) open camera
2) get CameraControl
3) default to picture mode, and call getPreviewStream()
3.1) in gecko, stop any running preview (there isn't one, right now)
3.2) configure preview, and
3.3) create a new MediaStream object, which is returned to the onSuccess callback
4) in onSuccess, set viewfinder.mozSrcObject = stream
4.1) this triggers the 'consuming' MediaStreamListener event, which
4.2) calls StartPreview()
5) when the mode-switch button is pressed, switch to video mode and call getPreviewStreamVideoMode()
5.1) in gecko, stop any running preview (there is one this time!) and call DOMCameraPreview::Stopped()
5.2) configure the video-mode preview, and
5.3) create a new MediaStream object, which is returned to the onSuccess callback
6) in onSuccess, set viewfinder.mozSrcObject = (the new) stream
6.1) this triggered the 'not consuming' event on the old stream, and
6.2) triggers the 'consuming' event on the new stream, which
6.3) calls StartPreview()

Without the above patch, step 6.1 will trigger a call to DOMCameraPreview::StopPreview()[4], which may short-circuit return because mState will have already been set to STOPPED[8] by step 5.1.

8. http://mxr.mozilla.org/mozilla-central/source/dom/camera/DOMCameraPreview.cpp#261

Since, as noted above, DOMCameraPreview::StopPreview() only handles the MediaStreamListener event (and only when mState is set to STARTED), but all preview stoppages must pass through SetStateStopped()[8] eventually, the attached patch moves the calls to EndTrack() and Finish() to the latter.

This ensures that SetEnded() is called and that the StreamBuffer can dispose of all MediaSegment objects on its Tracks in a timely fashion, without requiring a MediaStream CC.  Disposing of the MediaSegments is what releases the pmem_adsp-allocated buffers.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 686903 [details] [diff] [review]
EndTrack()/Finish() in DOMCameraPreview::SetStateStopped()

I feel comfortable enough with mikeh's explanation to pinch-review.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4fa764e8854c

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/releases/mozilla-aurora/rev/25aa6973bf76
https://hg.mozilla.org/releases/mozilla-beta/rev/f868cb477ab4

Comment 12

[Kan-Ru Chen [:kanru] (UTC+9)]

nice!

Comment 13

[[:fabrice] Fabrice Desré]

hm, why has this patch never landed on m-c?

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

It did, but I had the wrong bug focused when adding the commit message

https://hg.mozilla.org/mozilla-central/rev/4fa764e8854c