Closed Bug 834835 Opened 8 years ago Closed 8 years ago

Implement getAudioTracks and getVideoTracks

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla23
Tracking Status
firefox22 + fixed
firefox23 + fixed

People

(Reporter: ekr, Assigned: roc)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [getUserMedia][blocking-gum-][WebRTC][blocking-webrtc+])

Attachments

(4 files, 1 obsolete file)

Blocks: 834837
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.
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [automation-blocked]
This doesn't block automation. This is extending the feature set of gum.
Whiteboard: [automation-blocked]
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.
Whiteboard: [getUserMedia]
-> roc for real implementation

I believe ekr has a bandaid implementation in the tree that covers the usage of "is there video?"
Assignee: nobody → roc
Priority: -- → P2
Whiteboard: [getUserMedia] → [getUserMedia][blocking-gum+]
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.
Whiteboard: [getUserMedia][blocking-gum+] → [getUserMedia][blocking-gum?]
Sorry, I was thinking of PeerConnection and get*Tracks.  Agree it doesn't block release of gUM.
Whiteboard: [getUserMedia][blocking-gum?] → [getUserMedia][blocking-gum-]
Should we block on this for the full stack implementation however?
Whiteboard: [getUserMedia][blocking-gum-] → [getUserMedia][blocking-gum-][WebRTC][blocking-webrtc?]
Making this blocking because we *really* want this in Fx22.
Whiteboard: [getUserMedia][blocking-gum-][WebRTC][blocking-webrtc?] → [getUserMedia][blocking-gum-][WebRTC][blocking-webrtc+]
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.
Whiteboard: [getUserMedia][blocking-gum-][WebRTC][blocking-webrtc+] → [getUserMedia][blocking-gum-][WebRTC][blocking-webrtc+][webrtc-uplift]
Duplicate of this bug: 860734
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.
Summary: Implement get{Audio,Video}Tracks → Implement getAudioTracks and getVideoTracks
(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* :-)
Attachment #737460 - Flags: review?(rjesup) → review-
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.
Attachment #737461 - Flags: review?(rjesup) → review+
Attachment #737462 - Flags: review?(rjesup) → review+
Attachment #737463 - Flags: review?(rjesup) → review+
Attachment #737823 - Flags: review?(rjesup) → review+
(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.
Keywords: verifyme
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
Attachment #737823 - Flags: approval-mozilla-aurora?
Attachment #737460 - Attachment is obsolete: true
Attachment #737461 - Flags: approval-mozilla-aurora?
Attachment #737462 - Flags: approval-mozilla-aurora?
Attachment #737463 - Flags: approval-mozilla-aurora?
Backed for making bug 786539 occur frequently on OSX.
https://hg.mozilla.org/integration/mozilla-inbound/rev/76fb1757892d
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla23 → ---
Keywords: verifyme
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.
Attachment #737461 - Flags: approval-mozilla-aurora?
Attachment #737462 - Flags: approval-mozilla-aurora?
Attachment #737463 - Flags: approval-mozilla-aurora?
Attachment #737823 - Flags: approval-mozilla-aurora?
(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?
Flags: needinfo?(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.
Flags: needinfo?(mreavy)
Depends on: 863224
Keywords: verifyme
Comment on attachment 737461 [details] [diff] [review]
Part 2: Make DOMMediaStream maintain a list of MediaStreamTrack objects

[Approval Request Comment]
See comment 28
Attachment #737461 - Flags: approval-mozilla-aurora?
Comment on attachment 737462 [details] [diff] [review]
Part 3: Implement DOMMediaStream::GetAudio/VideoTracks

[Approval Request Comment]
See comment 28
Attachment #737462 - Flags: approval-mozilla-aurora?
Comment on attachment 737463 [details] [diff] [review]
Part 4: Test basic MediaStreamTrack functionality

[Approval Request Comment]
See comment 28
Attachment #737463 - Flags: approval-mozilla-aurora?
Comment on attachment 737823 [details] [diff] [review]
part 1 v2

[Approval Request Comment]
See comment 28
Attachment #737823 - Flags: approval-mozilla-aurora?
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.
Attachment #737461 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #737462 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #737463 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #737823 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 866513
Depends on: 866514
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
Status: RESOLVED → VERIFIED
Keywords: verifyme
No longer depends on: 873049
Blocks: 1050930
You need to log in before you can comment on or make changes to this bug.