See: http://dev.w3.org/2011/webrtc/editor/getusermedia.html#widl-MediaStream-getAudioTracks-sequence-MediaStreamTrack

Just to add this blocks our automation to correctly verify the streams of the local and remote peer. See the bug Eric has referenced earlier.

This doesn't block automation. This is extending the feature set of gum.

Really should only use an automation blocker flag for things that will block the ability to get automation running cleanly. It shouldn't be used on features.

-> roc for real implementation I believe ekr has a bandaid implementation in the tree that covers the usage of "is there video?"

I don't know if I'd block on this for v1. We're already halfway through the Aurora cycle right now, and this would be the equivalent of a feature. There's a low chance of an uplift here. I am not sure this is critical for a v1 ship of gUM.

Sorry, I was thinking of PeerConnection and get*Tracks. Agree it doesn't block release of gUM.

Should we block on this for the full stack implementation however?

Making this blocking because we *really* want this in Fx22.

What is the relative priority of the features of MediaStreamTrack?

Probably (MediaStream): 1) GetAudio/VideoTracks() (which means creating MediaStreamTrack as an object type) 2) id 3) ended/onended 4) getTrackById() 5) addTrack/removeTrack 6) onaddtrack/onremovetrack (comments?) Not in the spec anymore, but I want to see it return (and we have it): stop() (stop all tracks) MediaStreamTrack: 1) kind, id 2) stop 3) label 4) sourceType 5) sourceId photo/onphoto? readyState? onstopped/etc? enable/mute/etc? Honestly, I'm not sure about priority (mute and enable might be higher). there are a bunch of methods there, and some we'll need to control the devices as well (attributes, but that's a bigger project). (comments?)

apprtc currently uses: - one track for each track in the media stream - the tracks to show enabled I also need currentTime to work. So I think these minimally need to be supported. If those work, then apprtc will work and I think that's the high order bit.

(In reply to Eric Rescorla (:ekr) from comment #11) > apprtc currently uses: > > - one track for each track in the media stream > - the tracks to show enabled apprtc uses the .enabled property of tracks to mute/unmute the audio/video. The mute feature is important to have when doing real calls (as opposed to just demo'ing WebRTC), so you may want to prioritize it.

It is really hard to find this issue, please consider to rename it to prevent many duplications.

(In reply to Peter Nagy from comment #14) > It is really hard to find this issue, please consider to rename it to > prevent many duplications. Sorry you had trouble finding it. Renamed to help prevent future dups.

(In reply to Eric Rescorla (:ekr) from comment #11) > I also need currentTime to work. currentTime is not specced on MediaStream or MediaStreamTrack. It should already work on media elements with MediaStream sources.

Obviously this is just the beginning. It sounds like the next thing to implement is MediaStreamTrack.enabled? We'll have to have some discussions about how to implement that.

Comment on attachment 737460 [details] [diff] [review] Part 1: Add initial AudioStreamTrack/VideoStreamTrack/MediaStreamTrack interfaces and implementations Review of attachment 737460 [details] [diff] [review]: ----------------------------------------------------------------- r- -- VideoStreamTrack.cpp is missing (though in the Makefile.in). Otherwise, r+ ::: content/media/MediaStreamTrack.cpp @@ +32,5 @@ > +} > + > +NS_IMPL_CYCLE_COLLECTION_INHERITED_1(MediaStreamTrack, nsDOMEventTargetHelper, > + mStream) > + trailing space - had to find *something* :-)

Comment on attachment 737461 [details] [diff] [review] Part 2: Make DOMMediaStream maintain a list of MediaStreamTrack objects Review of attachment 737461 [details] [diff] [review]: ----------------------------------------------------------------- r+, none of these nits need to be addressed unless you feel like doing so ::: content/media/DOMMediaStream.cpp @@ +33,5 @@ > + StreamListener(DOMMediaStream* aStream) > + : mStream(aStream) > + {} > + > + // Main thread only Are there any particularly good points for a MainThread assertion, to catch people trying to use it off main-thread? @@ +61,5 @@ > + track = stream->GetDOMTrackFor(mID); > + } > + if (mEvents & MediaStreamListener::TRACK_EVENT_ENDED) { > + track->NotifyEnded(); > + } Just wondering, because this would handle it - can you have mEvents with both CREATED and ENDED? If not, you could move this into the else above (and comment/assert it), though the performance difference is miniscule. @@ +205,5 @@ > + > +MediaStreamTrack* > +DOMMediaStream::GetDOMTrackFor(TrackID aTrackID) > +{ > + for (uint32_t i = 0; i < mTracks.Length(); ++i) { As a general coding pattern, does it make sense when processing an array to use: uint32_t len = foo.Length(); for (uint32_t i = 0; i < len; ++i) { ... } and if you're paranoid, stick a MOZ_ASSERT(len == foo.Length()); at the start of the loop to catch anyone breaking the invariant. Not really important *here*, more of a general question triggered by seeing this.

(In reply to Randell Jesup [:jesup] from comment #23) > ::: content/media/DOMMediaStream.cpp > @@ +33,5 @@ > > + StreamListener(DOMMediaStream* aStream) > > + : mStream(aStream) > > + {} > > + > > + // Main thread only > > Are there any particularly good points for a MainThread assertion, to catch > people trying to use it off main-thread? Probably worth putting one in the Run() method. > @@ +61,5 @@ > > + track = stream->GetDOMTrackFor(mID); > > + } > > + if (mEvents & MediaStreamListener::TRACK_EVENT_ENDED) { > > + track->NotifyEnded(); > > + } > > Just wondering, because this would handle it - can you have mEvents with > both CREATED and ENDED? You can. > As a general coding pattern, does it make sense when processing an array to > use: > uint32_t len = foo.Length(); > for (uint32_t i = 0; i < len; ++i) { > ... > } > and if you're paranoid, stick a > MOZ_ASSERT(len == foo.Length()); > at the start of the loop to catch anyone breaking the invariant. I don't bother hoisting the length fetch as a rule. This is not hot code. If I thought the code was super-hot I might.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6efeef98993c https://hg.mozilla.org/integration/mozilla-inbound/rev/7f2943d34a13 https://hg.mozilla.org/integration/mozilla-inbound/rev/025b03b9e8d9 https://hg.mozilla.org/integration/mozilla-inbound/rev/f9236671098e

https://hg.mozilla.org/mozilla-central/rev/6efeef98993c https://hg.mozilla.org/mozilla-central/rev/7f2943d34a13 https://hg.mozilla.org/mozilla-central/rev/025b03b9e8d9 https://hg.mozilla.org/mozilla-central/rev/f9236671098e

Comment on attachment 737823 [details] [diff] [review] part 1 v2 Note: this request also includes the other 3 patches on this bug [Approval Request Comment] Bug caused by (feature/regressing bug #): N/A User impact if declined: Major feature of MediaStreams that app developers need to determine if a call has video or not will be missing, forcing them to use workarounds. Important to developers writing code that will work on Chrome or FF (and to avoid them writing Chrome-only code). Testing completed (on m-c, etc.): on m-c, with mochitests Risk to taking this patch (and alternatives if risky): low risk; mostly just exposing some simple attributes (kind, id, label) on a new object. String or IDL/UUID changes made by this patch: none

Backed for making bug 786539 occur frequently on OSX. https://hg.mozilla.org/integration/mozilla-inbound/rev/76fb1757892d

Chris P is going to try to fix 786539 after which we'll reland this.

[Relman comment]Approval on aurora is waiting for a clean landing on m-c and resolution of 786539.

Comment on attachment 737461 [details] [diff] [review] Part 2: Make DOMMediaStream maintain a list of MediaStreamTrack objects Pulling back the approval request for now.

(In reply to Randell Jesup [:jesup] from comment #28) > User impact if declined: Major feature of MediaStreams that app developers > need to determine if a call has video or not will be missing, forcing them > to use workarounds. Important to developers writing code that will work on > Chrome or FF (and to avoid them writing Chrome-only code). Bugs like this should be nominated for tracking-firefox22, especially if they have unfinished feature work. Maire, can you make sure that's the case?

(In reply to Alex Keybl [:akeybl] from comment #33) > > Bugs like this should be nominated for tracking-firefox22, especially if > they have unfinished feature work. Maire, can you make sure that's the case? Yes, I can. My apologies for any confusion. All "must have" bugs for WebRTC are now nominated for tracking-firefox22. I'll make sure to keep on top of this for the rest of the 22 release cycle and in the future.

https://hg.mozilla.org/integration/mozilla-inbound/rev/925d5b6c9171 https://hg.mozilla.org/integration/mozilla-inbound/rev/86a8ca51e847 https://hg.mozilla.org/integration/mozilla-inbound/rev/fed72d279071 https://hg.mozilla.org/integration/mozilla-inbound/rev/1065d2c7fd37 With the fix for bug 786539, tryserver run https://tbpl.mozilla.org/?tree=Try&rev=8d015f7a4dec showed no test_timeupdate_small_files timeouts on Mac in 30 runs of mochitest-1.

https://hg.mozilla.org/mozilla-central/rev/925d5b6c9171 https://hg.mozilla.org/mozilla-central/rev/86a8ca51e847 https://hg.mozilla.org/mozilla-central/rev/fed72d279071 https://hg.mozilla.org/mozilla-central/rev/1065d2c7fd37

Comment on attachment 737461 [details] [diff] [review] Part 2: Make DOMMediaStream maintain a list of MediaStreamTrack objects [Approval Request Comment] See comment 28

Comment on attachment 737462 [details] [diff] [review] Part 3: Implement DOMMediaStream::GetAudio/VideoTracks [Approval Request Comment] See comment 28

Comment on attachment 737463 [details] [diff] [review] Part 4: Test basic MediaStreamTrack functionality [Approval Request Comment] See comment 28

Comment on attachment 737823 [details] [diff] [review] part 1 v2 [Approval Request Comment] See comment 28

Comment on attachment 737461 [details] [diff] [review] Part 2: Make DOMMediaStream maintain a list of MediaStreamTrack objects Approving as this is low risk, 786539 is fixed as well & this is blocking webRTC.

Verified with two followups: - label property always empty in bug 866513 - early track access on streams fails to find the correct amount of tracks in bug 866514

https://hg.mozilla.org/releases/mozilla-aurora/rev/d890d87490d0 https://hg.mozilla.org/releases/mozilla-aurora/rev/4387c29602cc https://hg.mozilla.org/releases/mozilla-aurora/rev/5e457e748898 https://hg.mozilla.org/releases/mozilla-aurora/rev/1627c89d6a4c