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

VERIFIED FIXED in mozilla29

Status

()

Core
WebRTC: Audio/Video
--
critical
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: sole, Assigned: roc)

Tracking

({regression})

27 Branch
mozilla29
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox26 unaffected, firefox27 affected, firefox28 affected)

Details

(URL)

Attachments

(20 attachments, 3 obsolete attachments)

3.09 KB, patch
padenot
: review+
Details | Diff | Splinter Review
1.38 KB, patch
padenot
: review+
Details | Diff | Splinter Review
1.36 KB, patch
padenot
: review+
Details | Diff | Splinter Review
2.80 KB, patch
padenot
: review+
Details | Diff | Splinter Review
11.65 KB, patch
padenot
: review+
Details | Diff | Splinter Review
2.63 KB, patch
rlin
: review+
Details | Diff | Splinter Review
3.87 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
12.26 KB, patch
padenot
: review+
Details | Diff | Splinter Review
1.10 KB, patch
padenot
: review+
Details | Diff | Splinter Review
1.29 KB, patch
padenot
: review+
Details | Diff | Splinter Review
1.16 KB, patch
karlt
: review+
Details | Diff | Splinter Review
5.23 KB, patch
padenot
: review+
Details | Diff | Splinter Review
4.63 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
1.64 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
2.61 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
2.71 KB, patch
karlt
: review+
Details | Diff | Splinter Review
876 bytes, patch
eeejay
: review+
Details | Diff | Splinter Review
18.30 KB, patch
padenot
: review+
Details | Diff | Splinter Review
5.09 KB, patch
padenot
: review+
karlt
: review+
Details | Diff | Splinter Review
1.31 KB, patch
roc
: review+
Details | Diff | Splinter Review
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/
Component: General → WebRTC
Product: Firefox → Core

Updated

4 years ago
Component: WebRTC → WebRTC: Audio/Video
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!
Assignee: nobody → roc
Severity: normal → critical
status-firefox26: --- → unaffected
status-firefox27: --- → affected
status-firefox28: --- → affected
Depends on: 922601
Flags: needinfo?(roc)
Keywords: regression
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla27
Blocks: 922601
No longer depends on: 922601
Flags: needinfo?(roc)
(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.

Updated

4 years ago
Attachment #8340853 - Flags: review?(paul) → review+
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
Attachment #8340853 - Flags: approval-mozilla-aurora?

Updated

4 years ago
Attachment #8340853 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

4 years ago
Keywords: verifyme
Comment on attachment 8340853 [details] [diff] [review]
fix

[approved too early, waiting on m-c landing]
Attachment #8340853 - Flags: approval-mozilla-aurora+ → approval-mozilla-aurora?
Test failures on my try push. New patches coming.
Created attachment 8342189 [details] [diff] [review]
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.
Attachment #8340853 - Attachment is obsolete: true
Attachment #8340853 - Flags: approval-mozilla-aurora?
Attachment #8342189 - Flags: review?(paul)
Created attachment 8342190 [details] [diff] [review]
Part 2: Add assertion to catch cases where we NotifyOuput after NotifyFinished
Attachment #8342190 - Flags: review?(paul)
Created attachment 8342191 [details] [diff] [review]
Part 3: Like part 1, blocking calculations should note that a stream is only fully ended whenall of its tracks have finished playing out
Attachment #8342191 - Flags: review?(paul)
Created attachment 8342192 [details] [diff] [review]
Part 4: Fix TrackUnionStream track-ended calculation to catch cases where a track ends at the point where the input stream is completely blocked
Attachment #8342192 - Flags: review?(paul)

Updated

4 years ago
Attachment #8342190 - Flags: review?(paul) → review+

Updated

4 years ago
Attachment #8342191 - Flags: review?(paul) → review+

Updated

4 years ago
Attachment #8342189 - Flags: review?(paul) → review+

Updated

4 years ago
Attachment #8342192 - Flags: review?(paul) → review+
Duplicate of this bug: 942069
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.
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...
Flags: needinfo?(roc)
Requested backout of bug 922601 from Aurora in that bug.
Flags: needinfo?(roc)
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.
Created attachment 8346127 [details] [diff] [review]
Part 5: Don't allow a stream to finish before it has produced output up to mStateComputedTime
Attachment #8346127 - Flags: review?(paul)
Created attachment 8346128 [details] [diff] [review]
Part 6: Fix test to handle automatic stop of MediaRecorder
Attachment #8346128 - Flags: review?(rlin)
Created attachment 8346130 [details] [diff] [review]
Part 7: DecodedStreamData::mNextVideoTime is not relative to mStartTime
Attachment #8346130 - Flags: review?(cpearce)
Created attachment 8346136 [details] [diff] [review]
Part 8: When a MediaDecoder is decoding to a stream, run PlaybackEnded when the stream finishes
Attachment #8346136 - Flags: review?(paul)
Created attachment 8346137 [details] [diff] [review]
Part 9: Handle absence of INCLUDE_TRAILING_BLOCKED_INTERVAL flag correctly
Attachment #8346137 - Flags: review?(paul)
Created attachment 8346138 [details] [diff] [review]
Part 10: Fix and simplify setting of mNextMainThreadFinished
Attachment #8346138 - Flags: review?(paul)
Created 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
Attachment #8346139 - Flags: review?(paul)
Created attachment 8346141 [details] [diff] [review]
Part 12: Remove spurious ScriptProcessorNode from test
Attachment #8346141 - Flags: review?(karlt)
Created attachment 8346142 [details] [diff] [review]
Part 13: Keep producing silence in AudioNodeStreams' mLastChunks even after they've finished
Attachment #8346142 - Flags: review?(paul)
Created attachment 8346143 [details] [diff] [review]
Part 14: Cleanup some cruft after part 11
Attachment #8346143 - Flags: review?(karlt)
Created attachment 8346145 [details] [diff] [review]
Part 15: Make MediaDecoder set its currentTime based on the decoded stream's time directly, if we  are decoding to a stream
Attachment #8346145 - Flags: review?(cpearce)
Created attachment 8346146 [details] [diff] [review]
Part 16: Fix test_streams_element_capture_reset to make its expected current times be independent  of possibly-buggy v.currentTime
Attachment #8346146 - Flags: review?(cpearce)
Created attachment 8346147 [details] [diff] [review]
Part 17: MediaDecoderStateMachine::StopPlayback should not set mPlayDuration based on real time, but on whatever we're using for GetClock
Attachment #8346147 - Flags: review?(cpearce)
Created 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
Attachment #8346148 - Flags: review?(karlt)
Created attachment 8346150 [details] [diff] [review]
Part 19: nsSpeechTask should explicitly finish its audio track before finishing the stream
Attachment #8346150 - Flags: review?(eitan)
Attachment #8346128 - Flags: review?(rlin) → review+
Attachment #8346141 - Flags: review?(karlt) → review+
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.
Attachment #8346148 - Flags: review?(karlt) → review+
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.
Attachment #8346143 - Flags: review?(karlt) → review-
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.
Attachment #8346130 - Flags: review?(cpearce) → review+
Attachment #8346145 - Flags: review?(cpearce) → review+
Attachment #8346146 - Flags: review?(cpearce) → review+
Attachment #8346147 - Flags: review?(cpearce) → review+

Updated

4 years ago
Attachment #8346136 - Flags: review?(paul) → review+

Updated

4 years ago
Attachment #8346137 - Flags: review?(paul) → review+

Updated

4 years ago
Attachment #8346138 - Flags: review?(paul) → review+
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.
Attachment #8346139 - Flags: review?(paul) → review+

Updated

4 years ago
Attachment #8346127 - Flags: review?(paul) → review+

Updated

4 years ago
Attachment #8346142 - Flags: review?(paul) → review+
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.
Attachment #8346150 - Flags: review?(eitan) → review-
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.
Attachment #8346150 - Flags: review- → review?(eitan)
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.
Attachment #8346150 - Flags: review?(eitan) → review+
Attachment #8346143 - Attachment is obsolete: true
Created attachment 8349337 [details] [diff] [review]
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.
Attachment #8346139 - Attachment is obsolete: true
Attachment #8349337 - Flags: review?(paul)

Updated

4 years ago
Attachment #8349337 - Flags: review?(paul) → review+
Assertion failures on Linux and Mac.
Created attachment 8351622 [details] [diff] [review]
Part 20: Don't revive an offline MediaStreamGraph that has finished. Instess any pending events using their RunInShutdown sequence
Attachment #8351622 - Flags: review?(paul)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #44)
> https://tbpl.mozilla.org/?tree=Try&rev=e5e1c848253d

This is green.

Updated

4 years ago
Attachment #8351622 - Flags: review?(paul) → review+
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.
Attachment #8351622 - Flags: review?(karlt) → review+
Created attachment 8356495 [details] [diff] [review]
part 21 don't copy messages from mCurrentTaskMessageQueue after it has been emptied
Attachment #8356495 - Flags: review?(roc)
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
Attachment #8356495 - Flags: review?(roc) → review+
Depends on: 957359

Updated

4 years ago
Depends on: 957832
Depends on: 961996

Updated

4 years ago
Depends on: 964376
Target Milestone: mozilla27 → mozilla29
Version: Trunk → 27 Branch
Duplicate of this bug: 956489
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.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Depends on: 1010000
You need to log in before you can comment on or make changes to this bug.