770377 - PeerConnection needs source media info through AddStream

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Ethan Hugg [:ehugg]]

PeerConnection has AddStream() which needs to be fed enough information to handle the connection setup for audio/video.  In particular we need to know whether we have audio and/or video inputs and the characteristics of each.

Comment 1

[Ethan Hugg [:ehugg]]

Attachment: AddStream to use nsDOMMediaStream, create fake MediaStream classes for unittests.

Comment 2

[Ethan Hugg [:ehugg]]

This patch is really to stimulate discussion.  In particular:

1. Should we change AddStream to take a nsDOMMediaStream.
2. Should we take ROC's suggestion at least temporarily of getting hint flags of whether to expect Audio or Video in the nsDOMMediaStream structure.
3. Should we run with this idea of creating fake mediastream objects?

This patch has these things implemented and applies on top of the transport_integration branch of Alder.

Comment 3

[Ethan Hugg [:ehugg]]

Attachment: AddStream to use nsDOMMediaStream, create fake MediaStream classes for unittests.

Comment 4

[Ethan Hugg [:ehugg]]

A few notes on this patch

This patch applies on top of Enda's from bug 770384 which in turn applies on top of the transport_integration branch of Alder.

Per ROC's suggestion I added the expectAudio/Video to nsDOMMediaStream.  This will probably be temporary until we have some way to query media tracks directly for their media type.  I changed the code where source streams are added to include this hint.  I noticed that the current code seems unable to create a MediaStream with both audio and video, but I assume that will be fixed in a future GUM release.

The information about each MediaStream's tracks and their type is held in the PC object.  It's currently distilled into a couple ints and passed down to the Call object, but not used.  SIPCC needs to be tweaked to handle the idea of video without audio and use this information instead of the current enum sent into Call::CreateOffer (e.g. CC_SDP_DIRECTION_SENDRECV) which only affects video.  Enda has some ideas for how to do this, so I've left it for a future patch.

I've left in the part where LocalSourceStreamInfo registers as a MediaStreamListener as I assume we will need to be notified when tracks are added/removed in the future.  I don't know yet if escalating an audio connection to video will create a new track or new stream for example.

The fake media streams don't do much yet.  I thought we could add the create/start/stop/addtrack as needed and have the fake methods print to log and probably set flags that could be checked with ASSERT_TRUE in the GTest.

Comment 5

[Eric Rescorla (:ekr)]

(In reply to Ethan Hugg [:ehugg] from comment #4)
> A few notes on this patch
> 
> This patch applies on top of Enda's from bug 770384 which in turn applies on
> top of the transport_integration branch of Alder.
> 
> Per ROC's suggestion I added the expectAudio/Video to nsDOMMediaStream. 
> This will probably be temporary until we have some way to query media tracks
> directly for their media type.  I changed the code where source streams are
> added to include this hint.  I noticed that the current code seems unable to
> create a MediaStream with both audio and video, but I assume that will be
> fixed in a future GUM release.
> 
> The information about each MediaStream's tracks and their type is held in
> the PC object.  It's currently distilled into a couple ints and passed down
> to the Call object, but not used.  SIPCC needs to be tweaked to handle the
> idea of video without audio and use this information instead of the current
> enum sent into Call::CreateOffer (e.g. CC_SDP_DIRECTION_SENDRECV) which only
> affects video.  Enda has some ideas for how to do this, so I've left it for
> a future patch.
> 
> I've left in the part where LocalSourceStreamInfo registers as a
> MediaStreamListener as I assume we will need to be notified when tracks are
> added/removed in the future.  I don't know yet if escalating an audio
> connection to video will create a new track or new stream for example.
> 
> The fake media streams don't do much yet.  I thought we could add the
> create/start/stop/addtrack as needed and have the fake methods print to log
> and probably set flags that could be checked with ASSERT_TRUE in the GTest.

Maybe we could have htem simulate source and sink for data?

Comment 6

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

Comment on attachment 640347 [details] [diff] [review]
AddStream to use nsDOMMediaStream, create fake MediaStream classes for unittests.

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

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +1481,5 @@
>  already_AddRefed<nsDOMMediaStream>
>  nsHTMLMediaElement::CaptureStreamInternal(bool aFinishWhenEnded)
>  {
>    OutputMediaStream* out = mOutputStreams.AppendElement();
> +  out->mStream = nsDOMMediaStream::CreateInputStream(nsDOMMediaStream::MEDIASTREAM_EXPECT_AUDIO);

Obviously this can be video in some cases. If we've already decoded metadata, you can check whether there's video by checking mMediaSize. If we haven't decoded metadata yet we just don't know, which is an instance of the general problem that we can't always know what types of tracks a stream is going to have.

::: content/media/nsDOMMediaStream.h
@@ +53,5 @@
> +  enum {
> +    MEDIASTREAM_EXPECT_AUDIO = 0x00000001U,
> +    MEDIASTREAM_EXPECT_VIDEO = 0x00000002U
> +  };
> +  PRUint32 mMediaStreamExpectations;

This field should be protected.

"expectations" usually refers to the receiver and might be a bit confusing in this context. Also MEDIASTREAM_ is redundant here. I suggest "HINT_CONTENTS_AUDIO" and "HINT_CONTENTS_VIDEO", and mHintContents.

Comment 7

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

(In reply to Ethan Hugg [:ehugg] from comment #4)
> Per ROC's suggestion I added the expectAudio/Video to nsDOMMediaStream. 
> This will probably be temporary until we have some way to query media tracks
> directly for their media type.

MediaStreamListeners get that reported to them already when the track becomes available. The problem is that for media resources and in other situations we sometimes just won't know what track types a stream is going to have.

> I changed the code where source streams are
> added to include this hint.  I noticed that the current code seems unable to
> create a MediaStream with both audio and video

Hmm, I think that should work.

Comment 8

[Ethan Hugg [:ehugg]]

> EKR wrote:
> Maybe we could have htem simulate source and sink for data?

Yes, I would like to do that.  Signaling won't know the difference, but it would test transport.  

I don't know yet the best way to do it though and I don't want to hold up fake MediaStreams for it.  I think hooking up MediaStreams into signaling is dependent on this fake version unless we want to re-tackle the unittest link.

Comment 9

[Ethan Hugg [:ehugg]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> (In reply to Ethan Hugg [:ehugg] from comment #4)
> > Per ROC's suggestion I added the expectAudio/Video to nsDOMMediaStream. 
> > This will probably be temporary until we have some way to query media tracks
> > directly for their media type.
> 
> MediaStreamListeners get that reported to them already when the track
> becomes available. The problem is that for media resources and in other
> situations we sometimes just won't know what track types a stream is going
> to have.
>
Yes, we are also registering PeerConnectionImpl as a Listener to keep up with any updates that happen before we make the SDP.
> 
> > I changed the code where source streams are
> > added to include this hint.  I noticed that the current code seems unable to
> > create a MediaStream with both audio and video
> 
> Hmm, I think that should work.
>
I didn't mean to imply that MediaStreams couldn't do this, only that the implementation of GetUserMedia in this branch doesn't appear to do this.  For example, GetUserMediaCallbackRunnable in MediaManager.cpp appears to create a stream from a single source and not ever making one stream that has both camera and mic.

Comment 10

[Ethan Hugg [:ehugg]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> Comment on attachment 640347 [details] [diff] [review]
> AddStream to use nsDOMMediaStream, create fake MediaStream classes for
> unittests.
> 
> Review of attachment 640347 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/html/content/src/nsHTMLMediaElement.cpp
> @@ +1481,5 @@
> >  already_AddRefed<nsDOMMediaStream>
> >  nsHTMLMediaElement::CaptureStreamInternal(bool aFinishWhenEnded)
> >  {
> >    OutputMediaStream* out = mOutputStreams.AppendElement();
> > +  out->mStream = nsDOMMediaStream::CreateInputStream(nsDOMMediaStream::MEDIASTREAM_EXPECT_AUDIO);
> 
> Obviously this can be video in some cases. If we've already decoded
> metadata, you can check whether there's video by checking mMediaSize. If we
> haven't decoded metadata yet we just don't know, which is an instance of the
> general problem that we can't always know what types of tracks a stream is
> going to have.
>
I guess I was confused when I saw that we are setting mAudioCaptured=true and setting up the audio decoder in this method but there's no mention of video.  I'll take a look at using mMediaSize 
>
> ::: content/media/nsDOMMediaStream.h
> @@ +53,5 @@
> > +  enum {
> > +    MEDIASTREAM_EXPECT_AUDIO = 0x00000001U,
> > +    MEDIASTREAM_EXPECT_VIDEO = 0x00000002U
> > +  };
> > +  PRUint32 mMediaStreamExpectations;
> 
> This field should be protected.
> 
> "expectations" usually refers to the receiver and might be a bit confusing
> in this context. Also MEDIASTREAM_ is redundant here. I suggest
> "HINT_CONTENTS_AUDIO" and "HINT_CONTENTS_VIDEO", and mHintContents.

OK good to know. I will rename these and add getter/setter to access mHintContents.

Comment 11

[Ethan Hugg [:ehugg]]

Attachment: AddStream to use nsDOMMediaStream, create fake MediaStream classes for unittests.

Comment 12

[Ethan Hugg [:ehugg]]

Comment on attachment 640730 [details] [diff] [review]
AddStream to use nsDOMMediaStream, create fake MediaStream classes for unittests.


Changed names to HINT_CONTENTS and moved mHintContents to protected as suggested by the review above.

Comment 13

[Ethan Hugg [:ehugg]]

Pushed to Alder/default - https://hg.mozilla.org/projects/alder/rev/1166b1f639cd