Closed
Bug 1057955
Opened 10 years ago
Closed 10 years ago
Support MediaStreamTrack.stop()
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: jesup, Assigned: jesup)
References
Details
(Keywords: dev-doc-complete)
Attachments
(4 files, 1 obsolete file)
3.79 KB,
patch
|
roc
:
review+
smaug
:
review+
|
Details | Diff | Splinter Review |
2.79 KB,
patch
|
jesup
:
review+
|
Details | Diff | Splinter Review |
8.47 KB,
patch
|
jib
:
review+
|
Details | Diff | Splinter Review |
1.79 KB,
patch
|
padenot
:
review+
|
Details | Diff | Splinter Review |
To properly support uses of replaceTrack() (and per the spec), we need to be able to stop() a track in order to do things like avoid having two camera open at once: fake_track.enabled = false; // use black for transition old_track = sender.track; sender.replaceTrack(fake_track, function() { old_track.stop(); navigator.getUserMedia(constraints, function(stream) { sender.replaceTrack(stream.getXxxxxTracks[0], function () {...}, fail); }, fail); }, fail); Note that B2G camera doesn't seem to be able to open both at once; Not sure about android. Even without the 2-step replacement, we'd like to be able to stop a track once it's replaced.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8478104 -
Flags: review?(paul)
Attachment #8478104 -
Flags: review?(bugs)
Assignee | ||
Comment 2•10 years ago
|
||
Comment on attachment 8478104 [details] [diff] [review] Support MediaStreamTrack.stop() Need some more thought about the interaction with getUserMedia/TrackUnion (ugh)
Attachment #8478104 -
Flags: review?(paul)
Attachment #8478104 -
Flags: review?(bugs)
Updated•10 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Comment 4•10 years ago
|
||
Comment on attachment 8478104 [details] [diff] [review] Support MediaStreamTrack.stop() trivial DOM change (uncomment out a method we hadn't implemented before); simple change to DOMMediaStream
Attachment #8478104 -
Flags: review?(roc)
Attachment #8478104 -
Flags: review?(bugs)
Assignee | ||
Updated•10 years ago
|
Attachment #8479238 -
Flags: review?(jib)
Assignee | ||
Comment 5•10 years ago
|
||
roc's patch via email; I r+'d it
Assignee | ||
Updated•10 years ago
|
Attachment #8479259 -
Flags: review+
Comment 6•10 years ago
|
||
(In reply to Randell Jesup [:jesup] from comment #0) > Note that B2G camera doesn't seem to be able to open both at once; Not sure > about android. Even without the 2-step replacement, we'd like to be able to > stop a track once it's replaced. The one-camera-at-a-time on B2G is a limitation of the AOSP layer, not Gecko, so I would expect Android to have the same behaviour.
Comment 7•10 years ago
|
||
Comment on attachment 8479238 [details] [diff] [review] Stop the backend capture when the MSG track is stopped Review of attachment 8479238 [details] [diff] [review]: ----------------------------------------------------------------- r- for potential null-dereference. ::: dom/media/MediaManager.cpp @@ +546,5 @@ > + mSourceStream->EndTrack(aTrackID); > + // We could override NotifyMediaStreamTrackEnded(), and maybe should, but it's > + // risky to do late in a release since that will affect all track ends, and not > + // just StopTrack()s. > + mListener->StopTrack(aTrackID, !!GetDOMTrackFor(aTrackID)->AsAudioStreamTrack()); GetDOMTrackFor may return nullptr which would cause a null dereference here. http://mxr.mozilla.org/mozilla-central/source/content/media/DOMMediaStream.cpp#332 also has this comment: // We may add streams to our track list that are actually owned by // a different DOMMediaStream. Ignore those. from which I infer that replaceTrack on b2g wont work more than once, i.e. it wont work to replace a track that's been replaced once already. Am I correct, and is that OK? @@ +561,5 @@ > + MOZ_ASSERT(aTrack->AsVideoStreamTrack() || aTrack->AsAudioStreamTrack()); > + mListener->StopTrack(trackID, !!aTrack->AsAudioStreamTrack()); > + > + // forward to superclass > + DOMMediaStream::NotifyMediaStreamTrackEnded(aTrack); Superclass is DOMLocalMediaStream @@ +2328,5 @@ > + { > + // XXX to support multiple tracks of a type in a stream, this should key off > + // the TrackID and not just the type > + nsRefPtr<MediaOperationRunnable> runnable( > + new MediaOperationRunnable(MEDIA_STOP_TRACK, Is DOMMediaStream going to be sad without its last track? I'm asking because the only other use of MEDIA_STOP_TRACK (which I know you added recently) seems to only send MEDIA_STOP_TRACK in the case where it knows there's another track left in the stream, otherwise it does MEDIA_STOP.
Attachment #8479238 -
Flags: review?(jib) → review-
Attachment #8478104 -
Flags: review?(roc) → review+
Updated•10 years ago
|
Attachment #8478104 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 8•10 years ago
|
||
> ::: dom/media/MediaManager.cpp > @@ +546,5 @@ > > + mSourceStream->EndTrack(aTrackID); > > + // We could override NotifyMediaStreamTrackEnded(), and maybe should, but it's > > + // risky to do late in a release since that will affect all track ends, and not > > + // just StopTrack()s. > > + mListener->StopTrack(aTrackID, !!GetDOMTrackFor(aTrackID)->AsAudioStreamTrack()); > > GetDOMTrackFor may return nullptr which would cause a null dereference here. Thanks. Better safe than sorry. > http://mxr.mozilla.org/mozilla-central/source/content/media/DOMMediaStream. > cpp#332 also has this comment: > > // We may add streams to our track list that are actually owned by > // a different DOMMediaStream. Ignore those. > > from which I infer that replaceTrack on b2g wont work more than once, i.e. > it wont work to replace a track that's been replaced once already. Am I > correct, and is that OK? I probably can just remove that restriction... > > @@ +561,5 @@ > > + MOZ_ASSERT(aTrack->AsVideoStreamTrack() || aTrack->AsAudioStreamTrack()); > > + mListener->StopTrack(trackID, !!aTrack->AsAudioStreamTrack()); > > + > > + // forward to superclass > > + DOMMediaStream::NotifyMediaStreamTrackEnded(aTrack); > > Superclass is DOMLocalMediaStream Right > > @@ +2328,5 @@ > > + { > > + // XXX to support multiple tracks of a type in a stream, this should key off > > + // the TrackID and not just the type > > + nsRefPtr<MediaOperationRunnable> runnable( > > + new MediaOperationRunnable(MEDIA_STOP_TRACK, > > Is DOMMediaStream going to be sad without its last track? > > I'm asking because the only other use of MEDIA_STOP_TRACK (which I know you > added recently) seems to only send MEDIA_STOP_TRACK in the case where it > knows there's another track left in the stream, otherwise it does MEDIA_STOP. In this case, it's possible someone would stop() a track attached to a peerconnection, then replaceTrack() it with a different one. I was worried that Finishing the stream there would make that impossible, and I don't think it hurts.
Assignee | ||
Comment 9•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8479238 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8479584 -
Flags: review?(jib)
Comment 10•10 years ago
|
||
Comment on attachment 8479584 [details] [diff] [review] Stop the backend capture when the MSG track is stopped Review of attachment 8479584 [details] [diff] [review]: ----------------------------------------------------------------- r=me with more parens. ::: dom/media/MediaManager.cpp @@ +548,5 @@ > + // risky to do late in a release since that will affect all track ends, and not > + // just StopTrack()s. > + if (GetDOMTrackFor(aTrackID)) { > + mListener->StopTrack(aTrackID, !!GetDOMTrackFor(aTrackID)->AsAudioStreamTrack()); > + } So StopTrack on an invalid track completes silently? @@ +2325,5 @@ > +void > +GetUserMediaCallbackMediaStreamListener::StopTrack(TrackID aID, bool aIsAudio) > +{ > + if (((aIsAudio && mAudioSource) || > + (!aIsAudio && mVideoSource) && !mStopped) Unless mStopped is only for video, you need more parens here.
Attachment #8479584 -
Flags: review?(jib) → review+
Assignee | ||
Comment 11•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7e4ef74441d https://hg.mozilla.org/integration/mozilla-inbound/rev/ef1c321a4a32 https://hg.mozilla.org/integration/mozilla-inbound/rev/0edf5afdc13c
Target Milestone: --- → mozilla34
Comment 12•10 years ago
|
||
sorry had to backout this changes for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=46831559&tree=Mozilla-Inbound burning inbound to hell :)
Assignee | ||
Comment 13•10 years ago
|
||
Assignee | ||
Comment 14•10 years ago
|
||
Comment on attachment 8479881 [details] [diff] [review] interdiffs to fix TrackID problems with crashtests Slight tweak from pastebin version - searches for empty slot in StreamBuffer instead of assuming local max+1 is free
Attachment #8479881 -
Flags: review?(paul)
Updated•10 years ago
|
Attachment #8479881 -
Flags: review?(paul) → review+
Assignee | ||
Comment 15•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e5111c421e1 https://hg.mozilla.org/integration/mozilla-inbound/rev/c7683d561e01 https://hg.mozilla.org/integration/mozilla-inbound/rev/3a0146f0b776 Tries: https://tbpl.mozilla.org/?tree=Try&rev=8bb8fe636201 (M3/7/10/Crash) https://tbpl.mozilla.org/?tree=Try&rev=49c84780ea2a (for M1)
https://hg.mozilla.org/mozilla-central/rev/3e5111c421e1 https://hg.mozilla.org/mozilla-central/rev/c7683d561e01 https://hg.mozilla.org/mozilla-central/rev/3a0146f0b776
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 17•7 years ago
|
||
Documented this a few months ago as part of updating other material.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•