Configurable media streams using AddStream and RemoveStream up to 2 streams

RESOLVED FIXED

Status

()

Core
WebRTC: Signaling
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: emannion, Assigned: emannion)

Tracking

Trunk
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 643854 [details] [diff] [review]
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.
Attachment #643854 - Flags: feedback?(ethanhugg)
Attachment #643854 - Flags: feedback?(ekr)

Comment 1

6 years ago
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.
Attachment #643854 - Flags: feedback?(ethanhugg) → feedback+
(Assignee)

Comment 2

6 years ago
(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.
(Assignee)

Comment 3

6 years ago
Created attachment 643970 [details] [diff] [review]
Configurable media streams patch up to 2 streams, audio, video or both
Attachment #643970 - Flags: feedback?(ethanhugg)
(Assignee)

Comment 4

6 years ago
Created attachment 643971 [details] [diff] [review]
Configurable media streams patch up to 2 streams, audio, video or both
Attachment #643854 - Attachment is obsolete: true
Attachment #643970 - Attachment is obsolete: true
Attachment #643854 - Flags: feedback?(ekr)
Attachment #643970 - Flags: feedback?(ethanhugg)

Comment 5

6 years ago
>> > +	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);
(Assignee)

Comment 6

6 years ago
Its noted for when I get to this work, thanks.
(Assignee)

Comment 7

6 years ago
Created attachment 644253 [details] [diff] [review]
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.
Attachment #643971 - Attachment is obsolete: true

Comment 8

6 years ago
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

6 years ago
Created attachment 644828 [details] [diff] [review]
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.
Attachment #644828 - Flags: feedback?(emannion)

Updated

6 years ago
Attachment #644253 - Attachment is obsolete: true
(Assignee)

Comment 10

6 years ago
(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.
(Assignee)

Comment 11

6 years ago
Created attachment 644908 [details] [diff] [review]
Reviewed Revised Patch
Attachment #644828 - Attachment is obsolete: true
Attachment #644828 - Flags: feedback?(emannion)
Attachment #644908 - Flags: feedback?(ekr)
(Assignee)

Comment 12

6 years ago
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 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.
And of course make the changes here.

Updated

6 years ago
Attachment #644908 - Flags: review?(ethanhugg)
Attachment #644908 - Flags: feedback?(ekr)
Attachment #644908 - Flags: feedback+
(Assignee)

Comment 15

6 years ago
Created attachment 644939 [details] [diff] [review]
Configurable media streams patch up to 2 streams, audio, video or both

quick eye ball review request, hopefully this patch is near completion.
Attachment #644908 - Attachment is obsolete: true
Attachment #644908 - Flags: review?(ethanhugg)
Attachment #644939 - Flags: feedback?(ethanhugg)

Comment 16

6 years ago
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.
Attachment #644939 - Flags: feedback?(ethanhugg) → feedback+
(Assignee)

Comment 17

6 years ago
Created attachment 644986 [details] [diff] [review]
Reviewed Revised updated Patch for up to 2 media streams
Attachment #644939 - Attachment is obsolete: true

Comment 18

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

Updated

5 years ago
QA Contact: jsmith

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.