775559 - Configurable media streams using AddStream and RemoveStream up to 2 streams

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[enda mannion (:emannion)]

Attachment: Configurable media streams patch up to 2 streams, audio, video or both

This patch will allow AddStream to be called to add an Audio and Video stream to to a max of two streams. RemoveStream can remove one or both of the two streams.

This patch does not allow configuration of > 2 streams that is to follow.

There are some references to cc_local_media_track_table_t and cc_remorte_media_track_table_t  that is for future stream handling, please ignore for this patch.

For this patch I am using cc_feature_data_t for the two new methods AddStream and RemoveStream,  I intend to refactor the other JSEP functions to use cc_feature_data_t in a future SIPCC cleanup patch.

Comment 1

[Ethan Hugg [:ehugg]]

Comment on attachment 643854 [details] [diff] [review]
Configurable media streams patch up to 2 streams, audio, video or both

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

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +198,5 @@
> +    /*
> +     * Turn off two default streams, this is temporary
> +     * until we can handle multiple streams properly
> +     */
> +    if (sdpmode == TRUE) {

This could be "if (sdpmode)" the same as you changed the others.

@@ +5676,5 @@
>  
>  	// TODO handle for multiple tracks
>  
> +	dcb_p->remote_media_track_tbl->track[0].ref_id = media->refid;
> +	dcb_p->remote_media_track_tbl->track[0].video = FALSE ? media->type == 0 : TRUE;

Is this what you want?  I'm confused by this line.

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +308,5 @@
>  
> +  void CreateOfferVideoOnly(const std::string hints) {
> +    // Create a media stream as if it came from GUM
> +    nsRefPtr<nsDOMMediaStream> domMediaStream = new nsDOMMediaStream();
> +    // Pretend GUM got both audio and video.

Cut/paste causes things like this now-incorrect comment.  I would recommend adding a couple bools to CreateOffer to specify whether you want audio/video rather than creating these duplicate methods.

@@ +340,5 @@
> +
> +  void CreateOfferRemoveStream(const std::string hints) {
> +
> +    // this should indicate to remove audio
> +    domMediaStream_->SetHintContents(nsDOMMediaStream::HINT_CONTENTS_AUDIO);

Same thing could be done for this RemoveStream probably.

Comment 2

[enda mannion (:emannion)]

(In reply to Ethan Hugg [:ehugg] from comment #1)
> Comment on attachment 643854 [details] [diff] [review]
> Configurable media streams patch up to 2 streams, audio, video or both
> 
> Review of attachment 643854 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
> @@ +198,5 @@
> > +    /*
> > +     * Turn off two default streams, this is temporary
> > +     * until we can handle multiple streams properly
> > +     */
> > +    if (sdpmode == TRUE) {

Changed.

> 
> This could be "if (sdpmode)" the same as you changed the others.
> 
> @@ +5676,5 @@
> >  
> >  	// TODO handle for multiple tracks
> >  
> > +	dcb_p->remote_media_track_tbl->track[0].ref_id = media->refid;
> > +	dcb_p->remote_media_track_tbl->track[0].video = FALSE ? media->type == 0 : TRUE;
> 
> Is this what you want?  I'm confused by this line.

Please ignore code referring to remote_media_track_tbl  this is work in progress for handling multiple media streams, I just changed the type name in this patch.

> 
> ::: media/webrtc/signaling/test/signaling_unittests.cpp
> @@ +308,5 @@
> >  
> > +  void CreateOfferVideoOnly(const std::string hints) {
> > +    // Create a media stream as if it came from GUM
> > +    nsRefPtr<nsDOMMediaStream> domMediaStream = new nsDOMMediaStream();
> > +    // Pretend GUM got both audio and video.
> 
> Cut/paste causes things like this now-incorrect comment.  I would recommend
> adding a couple bools to CreateOffer to specify whether you want audio/video
> rather than creating these duplicate methods.

Done.

> 
> @@ +340,5 @@
> > +
> > +  void CreateOfferRemoveStream(const std::string hints) {
> > +
> > +    // this should indicate to remove audio
> > +    domMediaStream_->SetHintContents(nsDOMMediaStream::HINT_CONTENTS_AUDIO);
> 
> Same thing could be done for this RemoveStream probably.

Done.  Patch tested and re-attached.

Comment 3

[enda mannion (:emannion)]

Attachment: Configurable media streams patch up to 2 streams, audio, video or both

Comment 4

[enda mannion (:emannion)]

Attachment: Configurable media streams patch up to 2 streams, audio, video or both

Comment 5

[Ethan Hugg [:ehugg]]

>> > +	dcb_p->remote_media_track_tbl->track[0].video = FALSE ? media->type == 0 : TRUE;
>> 
>> Is this what you want?  I'm confused by this line.

>Please ignore code referring to remote_media_track_tbl  this is work in progress for handling multiple media streams, I just >changed the type name in this patch.

Sure, but this code can't be right, 

dcb_p->remote_media_track_tbl->track[0].video = FALSE ? media->type == 0 : TRUE;

did you mean this?

dcb_p->remote_media_track_tbl->track[0].video = (media->type == 0 ? FALSE : TRUE);

Comment 6

[enda mannion (:emannion)]

Its noted for when I get to this work, thanks.

Comment 7

[enda mannion (:emannion)]

Attachment: Configurable media streams patch up to 2 streams, audio, video or both

Sorry for all the emails but my last attachment did not have the changes from the review.

Comment 8

[Eric Rescorla (:ekr)]

Comment on attachment 644253 [details] [diff] [review]
Configurable media streams patch up to 2 streams, audio, video or both

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

See attached comments.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +299,5 @@
>    {
>      localSourceStream->ExpectAudio();
> +    // <emannion> adding streams here is temporary until we have implemented
> +    //            multiple streams
> +    mCall->addStream(0, AUDIO);

Is this an implicit promise that the stream indices will be sequential?

More seriously, streams can have multiple tracks, so a stream might be combined audio and video. So, I'm not sure it's great to bake in the assumption that they are one or the other. At minimum, we should have some sort of assert here to detect when >1 stream of either type has been offered. And/or a stream with both audio and video.

I'm also concerned about my ability to access streams later. You need to somehow propagate the stream index downward.

What I would suggest is:

(a) use the actual stream index mLocalSourceStreams here.
(b) Throw errors if a duplicate of any type is generated.

Note that this will result in a random ordering (0/1) of audio/video, but that shouldn't be a problem.

::: 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;
> +    cc_remote_media_track_table_t *remote_media_track_tbl;

How is this allocated?

::: media/webrtc/signaling/src/sipcc/core/ccapp/cc_call_feature.c
@@ +85,5 @@
>  /**
>   * Invoke a call feature.
>   */
> +cc_return_t cc_invokeFeature(cc_call_handle_t call_handle, group_cc_feature_t featureId, cc_sdp_direction_t video_pref,
> +                             cc_jsep_action_t action, string_t data, cc_media_track_id_t track_id, cc_media_type_t media_type) {

Is it really advisable to keep expanding the interface here? The number of arguments is getting kinda bloated and it means that each new argument ripples through the entire file, as seen below. Maybe create a new function call for this feature?

::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
@@ +2965,5 @@
>          return (fsmdef_release(fcb, cause, FALSE));
>      } 
>  
>      if (dcb == NULL) {
> +    	FSM_DEBUG_SM(DEB_F_PREFIX"dcb is NULL.\n", DEB_F_PREFIX_ARGS(FSM, "fsmdef_ev_createanswer"));

__FUNCTION__ ?

@@ +3365,5 @@
> +     * This is temporary code to allow configuration of the two
> +     * default streams. When multiple streams > 2 are supported this
> +     * will be re-implemented.
> +     */
> +    if (msg->data.track.media_type == VIDEO) {

Where is the track ID used?

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +5675,5 @@
>  	cc_media_track_t track;
>  
>  	// TODO handle for multiple tracks
>  
> +	dcb_p->remote_media_track_tbl->track[0].ref_id = media->refid;

I'm trying to figure out where the guarantee that the track_tbl is allocated is...

::: media/webrtc/signaling/src/sipcc/include/cc_constants.h
@@ +572,5 @@
> +typedef enum {
> +  NO_STREAM = -1,
> +  AUDIO,
> +  VIDEO,
> +  TYPE_COUNT

I know that this exists in MediaStreams, but what's the meaning here?

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +274,5 @@
> +
> +    // store in object to be used by RemoveStream
> +    domMediaStream_ = domMediaStream;
> +
> +    if (!audio && video)

This is some pretty confusing logic. What if audio && video?

Comment 9

[Eric Rescorla (:ekr)]

Attachment: Revised patch

I've fixed a number of issues that I noted in my review. Please look over these and comment and/or fix as necessary. I want to land this tomorrow AM.

Comment 10

[enda mannion (:emannion)]

(In reply to Eric Rescorla from comment #8)
> Comment on attachment 644253 [details] [diff] [review]
> Configurable media streams patch up to 2 streams, audio, video or both
> 
> Review of attachment 644253 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> See attached comments.
> 
> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
> @@ +299,5 @@
> >    {
> >      localSourceStream->ExpectAudio();
> > +    // <emannion> adding streams here is temporary until we have implemented
> > +    //            multiple streams
> > +    mCall->addStream(0, AUDIO);
> 
> Is this an implicit promise that the stream indices will be sequential?
> 
> More seriously, streams can have multiple tracks, so a stream might be
> combined audio and video. So, I'm not sure it's great to bake in the
> assumption that they are one or the other. At minimum, we should have some
> sort of assert here to detect when >1 stream of either type has been
> offered. And/or a stream with both audio and video.
> 
> I'm also concerned about my ability to access streams later. You need to
> somehow propagate the stream index downward.
> 
> What I would suggest is:
> 
> (a) use the actual stream index mLocalSourceStreams here.
> (b) Throw errors if a duplicate of any type is generated.

The StreamID is propogated into the GSM but it is not doing anything useful as this patch is only building either and Audio and or Video stream. For streams > than these two the ID will come into use. 

> 
> Note that this will result in a random ordering (0/1) of audio/video, but
> that shouldn't be a problem.

Glad you fixed this in your revised patch.

> 
> ::: 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;
> > +    cc_remote_media_track_table_t *remote_media_track_tbl;
> 
> How is this allocated?
> 
> ::: media/webrtc/signaling/src/sipcc/core/ccapp/cc_call_feature.c
> @@ +85,5 @@
> >  /**
> >   * Invoke a call feature.
> >   */
> > +cc_return_t cc_invokeFeature(cc_call_handle_t call_handle, group_cc_feature_t featureId, cc_sdp_direction_t video_pref,
> > +                             cc_jsep_action_t action, string_t data, cc_media_track_id_t track_id, cc_media_type_t media_type) {
> 
> Is it really advisable to keep expanding the interface here? The number of
> arguments is getting kinda bloated and it means that each new argument
> ripples through the entire file, as seen below. Maybe create a new function
> call for this feature?

I was kinda thinking that also, I created cc_invokeFeatureSDPMode  was going to call it cc_invokeFeatureJsep but went with SDPmode.

> 
> ::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
> @@ +2965,5 @@
> >          return (fsmdef_release(fcb, cause, FALSE));
> >      } 
> >  
> >      if (dcb == NULL) {
> > +    	FSM_DEBUG_SM(DEB_F_PREFIX"dcb is NULL.\n", DEB_F_PREFIX_ARGS(FSM, "fsmdef_ev_createanswer"));
> 
> __FUNCTION__ ?
> 
> @@ +3365,5 @@
> > +     * This is temporary code to allow configuration of the two
> > +     * default streams. When multiple streams > 2 are supported this
> > +     * will be re-implemented.
> > +     */
> > +    if (msg->data.track.media_type == VIDEO) {
> 
> Where is the track ID used?

This is not used until we do more than an Audio and or Video stream

> 
> ::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
> @@ +5675,5 @@
> >  	cc_media_track_t track;
> >  
> >  	// TODO handle for multiple tracks
> >  
> > +	dcb_p->remote_media_track_tbl->track[0].ref_id = media->refid;
> 
> I'm trying to figure out where the guarantee that the track_tbl is allocated
> is...

This code is irrelavant for this patch, I just renamed the structure, I have to re-visit this when doing > 2 streams,  sorry for the confusion, I think I mentioned it in the notes when I created this bug.


> 
> ::: media/webrtc/signaling/src/sipcc/include/cc_constants.h
> @@ +572,5 @@
> > +typedef enum {
> > +  NO_STREAM = -1,
> > +  AUDIO,
> > +  VIDEO,
> > +  TYPE_COUNT
> 
> I know that this exists in MediaStreams, but what's the meaning here?

What are you referring to here?

> 
> ::: media/webrtc/signaling/test/signaling_unittests.cpp
> @@ +274,5 @@
> > +
> > +    // store in object to be used by RemoveStream
> > +    domMediaStream_ = domMediaStream;
> > +
> > +    if (!audio && video)
> 
> This is some pretty confusing logic. What if audio && video?

Fixed now.

Comment 11

[enda mannion (:emannion)]

Attachment: Reviewed Revised Patch

Comment 12

[enda mannion (:emannion)]

Comment on attachment 644908 [details] [diff] [review]
Reviewed Revised Patch

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

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +312,5 @@
>  
>    if (hints & nsDOMMediaStream::HINT_CONTENTS_VIDEO)
>    {
>      localSourceStream->ExpectVideo();
> +    mCall->addStream(track_id, VIDEO);

track_id here will be 0 for both calls to addStream the first time PC.AddStream is invoked. This does not effect how stream handling works internally in SIPCC for this patch but this will need to be revisited for when many streams get implemented.  We will need track_id to be different for each track.

Comment 13

[Eric Rescorla (:ekr)]

Comment on attachment 644908 [details] [diff] [review]
Reviewed Revised Patch

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

This is not a complete review. I have looked over the major parts and they look OKish. 

Could you please get ehugg to review all the places you messed with invokeFeature, etc. and
make sure that there were not cut-and-paste errors, etc. Then land it in alder/default.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +312,5 @@
>  
>    if (hints & nsDOMMediaStream::HINT_CONTENTS_VIDEO)
>    {
>      localSourceStream->ExpectVideo();
> +    mCall->addStream(track_id, VIDEO);

Well, it depends if you are doing addstreams with combined audio/video or not. But yes, that's possible. I need this index to refer to the stream table from vcm.

@@ +352,5 @@
>        mLocalSourceStreams.RemoveElementAt(u);
>        break;
>      }    
>    }
>    PR_Unlock(mLocalSourceStreamsLock);

This logic can be made much simpler. When you have found u, do:
mCall->removeStream(u, hints);

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +334,5 @@
> +    pObserver->state = TestObserver::stateNoResponse;
> +    ASSERT_EQ(pc->CreateOffer(hints), PC_OK);
> +    ASSERT_TRUE_WAIT(pObserver->state == TestObserver::stateSuccess, kDefaultTimeout);
> +    SDPSanityCheck(pObserver->lastString, video, audio);
> +    offer_ = pObserver->lastString;

This is wrong. You can't selectively disable tracks this way.

Just call RemoveStream() on the existing stream and the underlying code needs to handle it. Don't mess with the hints. TBH I don't care if this test works right ow.

Comment 14

[Eric Rescorla (:ekr)]

And of course make the changes here.

Comment 15

[enda mannion (:emannion)]

Attachment: Configurable media streams patch up to 2 streams, audio, video or both

quick eye ball review request, hopefully this patch is near completion.

Comment 16

[Ethan Hugg [:ehugg]]

Comment on attachment 644939 [details] [diff] [review]
Configurable media streams patch up to 2 streams, audio, video or both

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

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +342,5 @@
> +      if (hints & nsDOMMediaStream::HINT_CONTENTS_AUDIO)
> +      {
> +        // <emannion>  This API will change when we implement multiple streams
> +        //             It will only need the ID
> +        mCall->removeStream(0, u, AUDIO);

I think these are backwards.  Stream id comes first, then track id.

@@ +352,1 @@
>        mLocalSourceStreams.RemoveElementAt(u);

Looks like this line should be removed.  It would mess up the stream IDs and jesup indicates we should never remove elements from this.

Comment 17

[enda mannion (:emannion)]

Attachment: Reviewed Revised updated Patch for up to 2 media streams

Comment 18

[Ethan Hugg [:ehugg]]

Pushed to Alder - http://hg.mozilla.org/projects/alder/rev/f8dc0ea61e02