Closed Bug 803976 Opened 7 years ago Closed 7 years ago

Need support for LocalMediaStreams (and .stop())

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla19
Tracking Status
firefox18 --- affected

People

(Reporter: jesup, Assigned: jesup)

References

Details

(Whiteboard: [getUserMedia], [blocking-gum+])

Attachments

(2 files, 2 obsolete files)

As part of the getUserMedia spec, it returns a LocalMediaStream which supports .stop() to revoke access (and stop capture).  See the draft spec at
http://dev.w3.org/2011/webrtc/editor/getusermedia.html

The becomes critical if we move to enabling the camera as soon as the stream is acquired and ending it only when the stream is stopped or all refs are gone.  (We shouldn't wait for GC; it should stop the camera/mic on a refcount of 0 - though note if there's a cycle including it, we'll end up waiting for the CC, so having apps call stop() is important.)

Alternatively, we could start immediately but shut down camera access when it's unassigned from a consumer.  This does leave a hole for an application to re-enable the camera an arbitrary period later with no prompt.
Adds .stop() to DOMMediaStream instead of adding DOMLocalMediaStream; code for that is here but commented out because of a compile issue with nsISupports ambiguity
Comment on attachment 673691 [details] [diff] [review]
Implementation of .stop() and partial implementation of LocalMediaStreams

Once the two? compile issues with nsISupports (which must be a simple thing to fix) are dealt with, this probably is ready for review.
Attachment #673691 - Flags: feedback?(roc)
Attachment #673691 - Flags: feedback?(anant)
Note that this *does* cause assertions due to pushing to ended tracks, since they're getting ended from an external request.  We'll need to suppress that at least for this case.
No longer blocks: 798037
Duplicate of this bug: 798037
Whiteboard: [getUserMedia], [blocking-gum+]
Comment on attachment 673691 [details] [diff] [review]
Implementation of .stop() and partial implementation of LocalMediaStreams

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

General approach is fine.

::: content/media/MediaStreamGraph.h
@@ +327,5 @@
>      return mMainThreadDestroyed;
>    }
>  
> +  // To support LocalMediaStream objects, force a SourceMediaStream to Finish
> +  void Stop();

This doesn't belong here. Remove it.

@@ +561,5 @@
> +  /**
> +   * End all tracks and Finish() this stream.  Used to voluntarily revoke access
> +   * to a LocalMediaStream.
> +   */
> +  void Stop();

Call this EndAllTracksAndFinish().
Attachment #673691 - Flags: feedback?(roc) → feedback+
WIP patch; works except for the GetMozSrcObject returns a MediaStream, not a LocalMediaStream, and so you can't call .stop() on it from JS
Attachment #673691 - Attachment is obsolete: true
Attachment #673691 - Flags: feedback?(anant)
Now fully working and ready for review
Attachment #674619 - Attachment is obsolete: true
Comment on attachment 674674 [details] [diff] [review]
Implementation of LocalMediaStreams for .stop()

Wow is subclassing a pain with this... but finally got it all working right.
Attachment #674674 - Flags: review?(roc)
Attachment #674674 - Flags: review?(anant)
Priority: -- → P1
Comment on attachment 674674 [details] [diff] [review]
Implementation of LocalMediaStreams for .stop()

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

r+ with that

::: content/media/MediaStreamGraph.cpp
@@ +2006,5 @@
>  void
>  SourceMediaStream::AppendToTrack(TrackID aID, MediaSegment* aSegment)
>  {
>    MutexAutoLock lock(mMutex);
> +  // ::EndAllTracksAndFinished() can end these before the sources notice

EndAllTrackAndFinish

@@ +2054,5 @@
>  void
>  SourceMediaStream::EndTrack(TrackID aID)
>  {
>    MutexAutoLock lock(mMutex);
> +  // ::EndAllTracksAndFinished() can end these before the sources call this

EndAllTracksAndFinish

@@ +2098,5 @@
> +      SourceMediaStream::TrackData* data = &mUpdateTracks[i];
> +      data->mCommands |= TRACK_END;
> +    }
> +  }
> +  Finish();

There's a race condition here where mMutex is unlocked and then re-locked by Finish. I think an AppendToTrack call could sneak in there and append to a track that's already in TRACK_END state. Add a FinishWithLockHeld method with the guts of Finish(), make it assert that the lock is already held, and call it here with the lock held.

::: content/media/nsDOMMediaStream.cpp
@@ +68,5 @@
> +NS_IMETHODIMP
> +nsDOMLocalMediaStream::Stop()
> +{
> +  if (mStream && mStream->AsSourceStream())
> +    mStream->AsSourceStream()->EndAllTracksAndFinish();

{}
Attachment #674674 - Flags: review?(roc) → review+
Comment on attachment 674674 [details] [diff] [review]
Implementation of LocalMediaStreams for .stop()

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

I only looked at the changes to MediaManager, they look sane.
Attachment #674674 - Flags: review?(anant) → review+
Attachment #674924 - Flags: review?(ehsan)
Attachment #674924 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/0b3cc07a299a
https://hg.mozilla.org/mozilla-central/rev/593c26e36849
Assignee: nobody → rjesup
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
I wouldn't call it in-testsuite+ when we only check the interface:
https://hg.mozilla.org/mozilla-central/diff/593c26e36849/dom/tests/mochitest/general/test_interfaces.html

We certainly would need some tests here. Randell, I assume those could even be unit tests?
Flags: in-testsuite+ → in-testsuite?
Basic test: .stop() exists and doesn't cause an error when run on a stream.
Better test: .stop() media to stop flowing.  Is there a way wqe can query how many frames a <video> or <audio> element have received from JS, or time it has played?  In that case, it should be easy.  Because these are interactive streams, however, those might not be available.
(In reply to Randell Jesup [:jesup] from comment #16)
> Basic test: .stop() exists and doesn't cause an error when run on a stream.
> Better test: .stop() media to stop flowing.  Is there a way wqe can query
> how many frames a <video> or <audio> element have received from JS, or time
> it has played?  In that case, it should be easy.  Because these are
> interactive streams, however, those might not be available.

Should be possible to get the time it's played by calling currentTime on the MediaStream. So the simple sanity check here that we could do is just start a video/audio tag with a MediaStream, wait until the media is confirmed playing, wait a couple of seconds, call stop() on the stream, get the currentTime off of the stream, wait a few seconds, and verify that the currentTime you got a few seconds back is equilvalent to the time now.

After I get the basic automated coverage landed, I can probably look into this.
Keywords: verifyme
Notes - LocalMediaStream test cases

Smoke

- Test that success callback of video, audio, video & audio gets you a LocalMediaStream

BFT

- Test that if I call stop() while a HTMLMediaElement with a video is running, the camera stream stops running
- Test that if I call stop() while a HTMLMediaElement with an audio is running, the mic no longer takes in input
- Test that if I call stop() while a HTMLMediaElement with a video+audio is running, the camera stream and mic stops running
- Test that if I call stop() before a video stream is attached to a media element, then attach it onto the a media element, and play the media element, the stream should not start
- Test that if I call stop() before an audio stream is attached to a media element, then attach it onto the a media element, and play the media element, the stream should not start
- Test that if I call stop() before a video+audio stream is attached to a media element, then attach it onto the a media element, and play the media element, the stream should not start
- Test that if I call stop() on a video stream used on a media element being played, and a gUM call is made again and placed on a media element and played, the camera stream shall be played without error
- Test that if I call stop() on a video+audio stream used on a media element being played, and a gUM call is made again and placed on a media element and played, the camera and mic stream shall be played without error
- Test that if I call stop() on an audio stream used on a media element being played, and a gUM call is made again and placed on a media element and played, the mic stream shall be played without error
- Test that if I call stop() on a video stream already playing in one tab, then another tab requests access to the camera and places the stream into a media element and plays it, the media element should play it without error
- Test that if I call stop() on an audio stream already playing in one tab, then another tab requests access to the camera and places the stream into a media element and plays it, the media element should play it without error
- Test that if I call stop() on a video+audio stream already playing in one tab, then another tab requests access to the camera and places the stream into a media element and plays it, the media element should play it without error
- Test that if I call stop() on a video media stream not actively in use, but was previously in use, then the camera access shall be released
- Test that if I call stop() on an audio media stream not actively in use, but was previously in use, then the mic access shall be released
- Test that if I call stop() on a video+audio media stream not actively in use, but was previously in use, then the camera access shall be released
- Test that if I have a USB camera and integrated camera hooked up, but the USB camera is in use by a media stream in a media element, and I call stop() on it, then request access to the camera, I should see both cameras listed. If I select the USB camera, then I should be able to lock access to it.
- Test that if I have a USB mic and integrated mic hooked up, but the USB mic is in use by a media stream in a media element, and I call stop() on it, then request access to the mic, I should see both cameras listed. If I select the USB camera, then I should be able to lock access to it.
- Test that if I have a USB camera/mic and integrated camera/mic hooked up, but the USB camera/mic is in use by a media stream in a media element, and I call stop() on it, then request access to the camera/mic, I should see both cameras/mics listed. If I select the USB camera/mic, then I should be able to lock access to it.
- Test that if I request access to the USB camera and integrated camera in tab #1 and #2 respectively, play them in media elements, and call stop() on one of them, then the other camera should remain active in use, while the other one should stop being used
- Test that if I request access to the USB mic and integrated mic in tab #1 and #2 respectively, play them in media elements, and call stop() on one of them, then the other mic should remain active in use, while the other one should stop being used
- Test that if I request access to the USB camera/mic and integrated camera/mic in tab #1 and #2 respectively, play them in media elements, and call stop() on one of them, then the other camera/mic should remain active in use, while the other one should stop being used
Depends on: 817976
Sorry for taking so long on finish this. Verified through test run and followup bugs have been filed.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Added automation coverage for this in bug 822109's patch.
Flags: in-testsuite? → in-testsuite+
Blocks: 845741
You need to log in before you can comment on or make changes to this bug.