943461 - Camera LED doesn't turn off after stream.stop() is called

Status: VERIFIED FIXED

(Core :: WebRTC: Audio/Video, defect)

Description

[Soledad Penades [:sole] [:spenades]]

In Nightly 20131126, I get the camera stream with getUserMedia. Then I stop it and the video stops and everything, but the LED is still on.

I can turn on the stream again with no issues but no matter how many times this is repeated, the LED will never turn off, unless I reload the page.

This is the demo page I'm using to test "turning it off and on again":
http://people.mozilla.org/~spenades/test_cases/gumhelper/demo/

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Confirmed using gum_test.html.  Regression happened in 27.   26 seems ok.

Select Video.  Share the video device.  Hit Stop.  NotifyFinished() isn't called (it should call
GetUserMediaCallbackMediaStreamListener::NotifyFinished() in MediaManager.cpp).  (Note: depending on the testcase, a GC may later cause it to be destroyed and shut down the camera.)

This appears to be caused by bug 922601's landing, which changed the conditionals and loops surrounding the calls to NotifyFinished() in UpdateCurrentTime().  In particular, StreamTimeToGraphTime() is being used now (and the update of mCurrentTime was moved) and from stepping through the code, the Finished track keeps getting added to streamsReadyToFinish(), and never succeeds at the conditional and so NotifyFinished() isn't called.

Roc: I'm on PTO this week.  Can you look into this ASAP?  We need to get this fixed and also get it uplifted to Aurora.  Thanks!

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Randell Jesup [:jesup] on PTO until Dec 1 from comment #1)
> This appears to be caused by bug 922601's landing, which changed the
> conditionals and loops surrounding the calls to NotifyFinished() in
> UpdateCurrentTime().  In particular, StreamTimeToGraphTime() is being used
> now (and the update of mCurrentTime was moved) and from stepping through the
> code, the Finished track keeps getting added to streamsReadyToFinish(), and
> never succeeds at the conditional and so NotifyFinished() isn't called.

In StreamTimeToGraphTime(stream, stream->GetBufferEnd()), stream->GetBufferEnd() is returning STREAM_TIME_MAX because there are no tracks --- so in principle we know the contents of the buffer to the end of time, but the tracks are empty so there's nothing to play and we may as well finish now.

I think this is an existing bug that bug 922601 revealed.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8340853 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 922601

User impact if declined: Video camera fails to turn off until MediaStream gets GC'd.  This can be concerning/confusing to users since they think they're still "on-camera" or "on-mic".

Testing completed (on m-c, etc.): Not landed yet; preemptive 

Risk to taking this patch (and alternatives if risky): minimal; no real alternatives other than backing out the regressing bug or portion thereof

String or IDL/UUID changes made by this patch: none

Comment 5

[bhavana bajaj [:bajaj]]

Comment on attachment 8340853 [details] [diff] [review]
fix

[approved too early, waiting on m-c landing]

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Test failures on my try push. New patches coming.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1: A stream is fully finished when all its tracks have finished playing out

Revised a bit. In particular the new StreamBuffer API was changed to return the time at which all tracks have finished playing out, because we'll need it later.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: Add assertion to catch cases where we NotifyOuput after NotifyFinished

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 3: Like part 1, blocking calculations should note that a stream is only fully ended whenall of its tracks have finished playing out

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 4: Fix TrackUnionStream track-ended calculation to catch cases where a track ends at the point where the input stream is completely blocked

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

These patches caused more test failures, revealing more underlying bugs, and my patches to fix those have caused more test failures. https://tbpl.mozilla.org/?tree=Try&rev=840608b86575

I think we should back out 922601 everywhere except nightly.

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

Any chance we can get this landed (and up to 27) before uplift, so we don't have to do a beta uplift request?  I'm guessing we can back out 922601...

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Requested backout of bug 922601 from Aurora in that bug.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

In my attempt to make tests green, I have 7 more patches all fixing independent bugs, plus I found bug 947796 :-). I'll attach when I finally get green tests.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Alright, I finally have something green on try. https://tbpl.mozilla.org/?tree=Try&rev=785f7437cde4

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Also https://tbpl.mozilla.org/?tree=Try&rev=04bbcd2f98e9

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 5: Don't allow a stream to finish before it has produced output up to mStateComputedTime

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 6: Fix test to handle automatic stop of MediaRecorder

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 7: DecodedStreamData::mNextVideoTime is not relative to mStartTime

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 8: When a MediaDecoder is decoding to a stream, run PlaybackEnded when the stream finishes

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 9: Handle absence of INCLUDE_TRAILING_BLOCKED_INTERVAL flag correctly

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 10: Fix and simplify setting of mNextMainThreadFinished

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 11: Don't tear down an OfflineAudioContext until its current time has actually reached its e nd, to ensure that all relevant stream state changes have been forwarded to the main thread

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 12: Remove spurious ScriptProcessorNode from test

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 13: Keep producing silence in AudioNodeStreams' mLastChunks even after they've finished

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 14: Cleanup some cruft after part 11

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 15: Make MediaDecoder set its currentTime based on the decoded stream's time directly, if we are decoding to a stream

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 16: Fix test_streams_element_capture_reset to make its expected current times be independent of possibly-buggy v.currentTime

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 17: MediaDecoderStateMachine::StopPlayback should not set mPlayDuration based on real time, but on whatever we're using for GetClock

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 18: Ensure AudioBufferSourceNode's engine always produces an audio block when ProduceAudioBlock is called, and add an assertion to catch this kind of error directly

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 19: nsSpeechTask should explicitly finish its audio track before finishing the stream

Comment 33

[Karl Tomlinson (:karlt)]

Comment on attachment 8346148 [details] [diff] [review]
Part 18: Ensure AudioBufferSourceNode's engine always produces an audio block when ProduceAudioBlock is called, and add an assertion to catch this kind of error directly

Not so keen on uint16_t but it's debug only.

Comment 34

[Karl Tomlinson (:karlt)]

Comment on attachment 8346143 [details] [diff] [review]
Part 14: Cleanup some cruft after part 11

Given this thread is running unconstrained at full CPU, I think it is important to stop this processing when done, instead of continuing to consume available CPU until messages have gone to the main thread and back.

That would also stop audioprocess or onended events from firing after the specified length is processed.

Comment 35

[Karl Tomlinson (:karlt)]

If the changes to OfflineAudioContext processing is just to fix some tests that are currently getting lucky, then perhaps the tests can be adjusted to tolerate and fixed in another bug.

Comment 36

[Paul Adenot (:padenot)]

Comment on attachment 8346139 [details] [diff] [review]
Part 11: Don't tear down an OfflineAudioContext until its current time has actually reached its e nd, to ensure that all relevant stream state changes have been forwarded to the main thread

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

r+, but see comments.

::: content/media/webaudio/AudioDestinationNode.cpp
@@ +63,5 @@
>      if (mInputChannels.IsEmpty()) {
>        return;
>      }
>  
> +    if (mWriteIndex >= mLength) {

Would it be possible to put an == here instead of the >=, and MOZ_ASSERT in case this invariant is not respected?

@@ +100,5 @@
>        PodZero(mInputChannels[i] + mWriteIndex, duration);
>      }
>      mWriteIndex += duration;
>  
> +    if (mWriteIndex >= mLength) {

Same thing, I'm not sure sure this change is necessary, because we have:

> const uint32_t duration = std::min(WEBAUDIO_BLOCK_SIZE, mLength - mWriteIndex);

above.

@@ +115,5 @@
> +    context->Shutdown();
> +    // Shutdown drops self reference, but the context is still referenced by aNode,
> +    // which is strongly referenced by the runnable that called
> +    // AudioDestinationNode::FireOfflineCompletionEvent.
> +    

nit: trailing space.

Comment 37

[Eitan Isaacson [:eeejay]]

Comment on attachment 8346150 [details] [diff] [review]
Part 19: nsSpeechTask should explicitly finish its audio track before finishing the stream

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

This causes SynthStreamListener::NotifyFinished() to not be called, and in turn no 'end' event is emitted. Not sure how to do that correctly.

Comment 38

[Eitan Isaacson [:eeejay]]

Comment on attachment 8346150 [details] [diff] [review]
Part 19: nsSpeechTask should explicitly finish its audio track before finishing the stream

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

I'm going check this again with the entire patch queue.

Comment 39

[Eitan Isaacson [:eeejay]]

Comment on attachment 8346150 [details] [diff] [review]
Part 19: nsSpeechTask should explicitly finish its audio track before finishing the stream

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

Tested, and it indeed works. Sorry for the earlier confusion.

::: content/media/webspeech/synth/nsSpeechTask.cpp
@@ +214,5 @@
>  void
>  nsSpeechTask::SendAudioImpl(int16_t* aData, uint32_t aDataLen)
>  {
>    if (aDataLen == 0) {
> +    mStream->EndTrack(1);

How about mStream->EndAllTrackAndFinish() instead? Looks like it does the same thing.

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 11 v2

Fixes the issues raised by you and Karl.

We shut down the offline MSG as soon as mCurrentTime has advanced far enough *and* all streams that have finished have sent notifications to the main thread. This is needed for AudioDestinationNode to get its finished notification and dispatch its event.

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://tbpl.mozilla.org/?tree=Try&rev=91b6706ab3b9

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Assertion failures on Linux and Mac.

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 20: Don't revive an offline MediaStreamGraph that has finished. Instess any pending events using their RunInShutdown sequence

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://tbpl.mozilla.org/?tree=Try&rev=e5e1c848253d

Comment 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #44)
> https://tbpl.mozilla.org/?tree=Try&rev=e5e1c848253d

This is green.

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://tbpl.mozilla.org/?tree=Try&rev=656ca88e2708

Comment 47

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://tbpl.mozilla.org/?tree=Try&rev=cc58c0061f1c

Comment 48

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e526d37a9a19
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcc3f7ba913e
https://hg.mozilla.org/integration/mozilla-inbound/rev/aecddf4c53d8
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac6bfc6f050e
https://hg.mozilla.org/integration/mozilla-inbound/rev/08e5578eded8
https://hg.mozilla.org/integration/mozilla-inbound/rev/7590e388abaf
https://hg.mozilla.org/integration/mozilla-inbound/rev/249b7a7cec00
https://hg.mozilla.org/integration/mozilla-inbound/rev/9811231a9449
https://hg.mozilla.org/integration/mozilla-inbound/rev/4378e7bf2afc
https://hg.mozilla.org/integration/mozilla-inbound/rev/bed75e8c44b1
https://hg.mozilla.org/integration/mozilla-inbound/rev/939ac6298d2a
https://hg.mozilla.org/integration/mozilla-inbound/rev/76bf4bf34926
https://hg.mozilla.org/integration/mozilla-inbound/rev/adc635735f62
https://hg.mozilla.org/integration/mozilla-inbound/rev/e911ac30c8c1
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a6bae856337
https://hg.mozilla.org/integration/mozilla-inbound/rev/75ca7a7c5df7
https://hg.mozilla.org/integration/mozilla-inbound/rev/0279107b81d3
https://hg.mozilla.org/integration/mozilla-inbound/rev/7677ea877c08
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ebe4da27dd5

Comment 49

[Karl Tomlinson (:karlt)]

Comment on attachment 8351622 [details] [diff] [review]
Part 20: Don't revive an offline MediaStreamGraph that has finished. Instess any pending events using their RunInShutdown sequence

Looks good, thanks.

Comment 50

[Karl Tomlinson (:karlt)]

Attachment: part 21 don't copy messages from mCurrentTaskMessageQueue after it has been emptied

Comment 51

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8356495 [details] [diff] [review]
part 21 don't copy messages from mCurrentTaskMessageQueue after it has been emptied

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

Thanks

Comment 52

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e526d37a9a19
https://hg.mozilla.org/mozilla-central/rev/fcc3f7ba913e
https://hg.mozilla.org/mozilla-central/rev/aecddf4c53d8
https://hg.mozilla.org/mozilla-central/rev/ac6bfc6f050e
https://hg.mozilla.org/mozilla-central/rev/08e5578eded8
https://hg.mozilla.org/mozilla-central/rev/7590e388abaf
https://hg.mozilla.org/mozilla-central/rev/249b7a7cec00
https://hg.mozilla.org/mozilla-central/rev/9811231a9449
https://hg.mozilla.org/mozilla-central/rev/4378e7bf2afc
https://hg.mozilla.org/mozilla-central/rev/bed75e8c44b1
https://hg.mozilla.org/mozilla-central/rev/939ac6298d2a
https://hg.mozilla.org/mozilla-central/rev/76bf4bf34926
https://hg.mozilla.org/mozilla-central/rev/adc635735f62
https://hg.mozilla.org/mozilla-central/rev/e911ac30c8c1
https://hg.mozilla.org/mozilla-central/rev/6a6bae856337
https://hg.mozilla.org/mozilla-central/rev/75ca7a7c5df7
https://hg.mozilla.org/mozilla-central/rev/0279107b81d3
https://hg.mozilla.org/mozilla-central/rev/7677ea877c08
https://hg.mozilla.org/mozilla-central/rev/1ebe4da27dd5

Comment 53

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/02eca42adbbb

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/02eca42adbbb

Comment 56

[Bogdan Maris, Desktop QA]

Reproduced the initial issue on a old Nightly build and verified that the issue is fixed on Firefox 29 beta 1, Firefox 28.0 and 27.0.1 using Windows 7 64bit, Ubuntu 13.10 64bit and Mac OS X 10.8.5.