834835 - Implement getAudioTracks and getVideoTracks

Status: VERIFIED FIXED

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

Description

[Eric Rescorla (:ekr)]

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

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 2

[Jason Smith [:jsmith]]

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

Comment 3

[Jason Smith [:jsmith]]

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.

Comment 4

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

-> roc for real implementation

I believe ekr has a bandaid implementation in the tree that covers the usage of "is there video?"

Comment 5

[Jason Smith [:jsmith]]

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.

Comment 6

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

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

Comment 7

[Jason Smith [:jsmith]]

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

Comment 8

[Maire Reavy [:mreavy]]

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

Comment 9

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

What is the relative priority of the features of MediaStreamTrack?

Comment 10

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

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?)

Comment 11

[Eric Rescorla (:ekr)]

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.

Comment 12

[Florian Quèze [:florian]]

(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.

Comment 14

[Peter Nagy]

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

Comment 15

[Maire Reavy [:mreavy]]

(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.

Comment 16

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

(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.

Comment 17

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

Attachment: Part 1: Add initial AudioStreamTrack/VideoStreamTrack/MediaStreamTrack interfaces and implementations

Comment 18

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

Attachment: Part 2: Make DOMMediaStream maintain a list of MediaStreamTrack objects

Comment 19

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

Attachment: Part 3: Implement DOMMediaStream::GetAudio/VideoTracks

Comment 20

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

Attachment: Part 4: Test basic MediaStreamTrack functionality

Comment 21

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

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 22

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

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 23

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

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.

Comment 24

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

Attachment: part 1 v2

Comment 25

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

(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.

Comment 26

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

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

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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 28

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

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

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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

Comment 30

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

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

Comment 31

[bhavana bajaj [:bajaj]]

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

Comment 32

[Alex Keybl [:akeybl]]

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

Pulling back the approval request for now.

Comment 33

[Alex Keybl [:akeybl]]

(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?

Comment 34

[Maire Reavy [:mreavy]]

(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.

Comment 35

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

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.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

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 37

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

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

[Approval Request Comment]
See comment 28

Comment 38

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

Comment on attachment 737462 [details] [diff] [review]
Part 3: Implement DOMMediaStream::GetAudio/VideoTracks

[Approval Request Comment]
See comment 28

Comment 39

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

Comment on attachment 737463 [details] [diff] [review]
Part 4: Test basic MediaStreamTrack functionality

[Approval Request Comment]
See comment 28

Comment 40

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

Comment on attachment 737823 [details] [diff] [review]
part 1 v2

[Approval Request Comment]
See comment 28

Comment 41

[bhavana bajaj [:bajaj]]

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.

Comment 42

[Jason Smith [:jsmith]]

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

Comment 43

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

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