776752 - Handling adding Remote Media Streams to PeerConnection

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[enda mannion (:emannion)]

Attachment: Handle adding remote media streams

Some of the code for this patch was added in a previous patch and this may make reading a little difficult. 

lsm_add_remote_stream  has the stubbed out call to vcmCreateRemoteStream

So far I am not dynamically allocating the dcb array cc_remote_media_track_table_t  I may do that if required but for now my testing is good.

function ui_on_remote_stream_added bubbles the stream id up to the UI.

Comment 1

[Eric Rescorla (:ekr)]

Comment on attachment 645112 [details] [diff] [review]
Handle adding remote media streams

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

Enda: I'm concerned we aren't clear on our concepts.

Here are the entities I consider relevant:

1. m-line index (level)
2. peer-connection stream (corresponding to a MediaStream)
3. peer-connection track (inside a MediaStream)
4. Capabilities (in the capabilities table).

IIRC there is a 1-1 mapping between 1 and 4 and between the pair (2,3) and both 1 and 4 for any
given direction.

I'm not sure what stream_id is but it appears to be none of them. 

On the transmit side, I am storing (2) and (3) as indexed by (4). Then when you call vcmTxStartICE (new API), I get all of these together. I use (1) to get the ICE stream, (2) and (3) to isolate the MediaStream/track to encode and (4) to determine whether I am audio or video. 

On the receive side I think we want the dual of this.

::: media/webrtc/signaling/include/CC_CallInfo.h
@@ +364,5 @@
>             get media tracks
>             @param [in] handle - call info handle
>             @return code
>           */
> +        virtual unsigned int getMediaStreamID() = 0;

Why is this only a single value. There are presumably a number of streams, no?

::: media/webrtc/signaling/include/PeerConnection.h
@@ +96,5 @@
>    // Notification of one of several types of state changed
>    virtual void OnStateChange(StateType state_changed) = 0;
>  
>    // Changes to MediaStreams
> +  virtual void OnAddStream(unsigned int streamID) = 0;

I think this should probably take a nsRefPtr<nsDOMMediaStream>. that's what the caller wants. We should translate the integer to a media stream.

::: media/webrtc/signaling/src/sipcc/core/ccapp/CCProvider.h
@@ +130,5 @@
>      cc_boolean    audio_mute;
>      cc_boolean    video_mute;
>      cc_call_conference_Info_t call_conference;
>      cc_string_t   sdp;
> +    unsigned int  streamID;

I'm not sure what is being passed here.

Basically, we need to have floating around a bidirectional map between m-line (i.e, level) and media-stream/track (I.e., PC concepts). I'm not sure how this captures that.

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +110,2 @@
>  cc_remote_media_track_table_t g_remote_media_track_table = {
> +      0,

Why did this change to 0 from 1?

Comment 2

[enda mannion (:emannion)]

(In reply to Eric Rescorla from comment #1)
> Comment on attachment 645112 [details] [diff] [review]
> Handle adding remote media streams
> 
> Review of attachment 645112 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Enda: I'm concerned we aren't clear on our concepts.
> 
> Here are the entities I consider relevant:
> 
> 1. m-line index (level)
> 2. peer-connection stream (corresponding to a MediaStream)
> 3. peer-connection track (inside a MediaStream)
> 4. Capabilities (in the capabilities table).
> 
> IIRC there is a 1-1 mapping between 1 and 4 and between the pair (2,3) and
> both 1 and 4 for any
> given direction.
> 
> I'm not sure what stream_id is but it appears to be none of them. 

stream_id is peer-connection stream, in SIPCC this referres to the group of m= lines and internally is known as group_id.

I call vcmCreateRemoteStream  with the group_id and the media->type and it is invoked for each track_id.  Then from our IRC chat yesterday I understood it was OK to pass the streamID (group_ID) up to OnAddStream. This would identify the stream added by calls to vcmCreateRemoteStream.

Originally I was passing a media stream structure upwards, I can re-implement that again. 


> 
> On the transmit side, I am storing (2) and (3) as indexed by (4). Then when
> you call vcmTxStartICE (new API), I get all of these together. I use (1) to
> get the ICE stream, (2) and (3) to isolate the MediaStream/track to encode
> and (4) to determine whether I am audio or video. 
> 
> On the receive side I think we want the dual of this.
> 
> ::: media/webrtc/signaling/include/CC_CallInfo.h
> @@ +364,5 @@
> >             get media tracks
> >             @param [in] handle - call info handle
> >             @return code
> >           */
> > +        virtual unsigned int getMediaStreamID() = 0;
> 
> Why is this only a single value. There are presumably a number of streams,
> no?
> 
> ::: media/webrtc/signaling/include/PeerConnection.h
> @@ +96,5 @@
> >    // Notification of one of several types of state changed
> >    virtual void OnStateChange(StateType state_changed) = 0;
> >  
> >    // Changes to MediaStreams
> > +  virtual void OnAddStream(unsigned int streamID) = 0;
> 
> I think this should probably take a nsRefPtr<nsDOMMediaStream>. that's what
> the caller wants. We should translate the integer to a media stream.

I agree.

> 
> ::: media/webrtc/signaling/src/sipcc/core/ccapp/CCProvider.h
> @@ +130,5 @@
> >      cc_boolean    audio_mute;
> >      cc_boolean    video_mute;
> >      cc_call_conference_Info_t call_conference;
> >      cc_string_t   sdp;
> > +    unsigned int  streamID;
> 
> I'm not sure what is being passed here.
> 
> Basically, we need to have floating around a bidirectional map between
> m-line (i.e, level) and media-stream/track (I.e., PC concepts). I'm not sure
> how this captures that.
> 
> ::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
> @@ +110,2 @@
> >  cc_remote_media_track_table_t g_remote_media_track_table = {
> > +      0,
> 
> Why did this change to 0 from 1?

SIPCC assigns the first stream ID (groupID) = 0

Comment 3

[enda mannion (:emannion)]

Attachment: Revised onAddStream Patch

Comment 4

[Eric Rescorla (:ekr)]

Comment on attachment 645622 [details] [diff] [review]
Revised onAddStream Patch

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

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +5717,5 @@
> +
> +  dcb_p->remote_media_stream_tbl->num_streams++;
> +  dcb_p->remote_media_stream_tbl->streams[idx].num_tracks = 1;
> +  dcb_p->remote_media_stream_tbl->streams[idx].stream_id = pc_stream_id;
> +  dcb_p->remote_media_stream_tbl->streams[idx].track[0].ref_id = media->refid;

So, this ref_id isn't right. What we want is the index of the track we added. For now, just set this to 0.

Comment 5

[enda mannion (:emannion)]

Attachment: Revised onAddStream Patch

Ready to push.

Comment 6

[Ethan Hugg [:ehugg]]

Pushed to Alder - https://hg.mozilla.org/projects/alder/rev/3f94ccd57513