816780 - onaddstream fires twice on a peerconnection when you add a stream with both audio and video

Status: VERIFIED FIXED

(Core :: WebRTC, defect, P2)

Description

[Adam Ullman]

I am calling getUserMedia with both audio and video and adding it to a peerconnection. I'm then going through the usual connection steps to connect this peerconnection to another peerconnection locally.

On the receiving peerconnection I get two onaddstream events. One with type "audio" and one with type "video". Both of which have separate streams. 

I expected to receive one stream containing both audio and video. mozGetUserMedia has been updated to have one stream with both audio and video (773649) but expected one stream on the receiving end as well.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Probably wouldn't block on it, but it would be very nice.  Even if only a hack for FF20 that assumes that an incoming PC with 1 audio and 1 video tracks should be made into one a+v stream (until we can get proper multiple stream support in, which should handle this).

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Actually this will be fixed by bug 818714.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Oh sorry, my fault. It's not true. Bug 818714 is something different. So as talked with Jesup yesterday we can't actually fix this problem unless we can merge MediaStream tracks. This will be implemented in bug 782194.

Comment 4

[Brendan Eich [:brendan]]

Randell: should we get this bug assigned to somebody rather than [e-mail]?

/be

Comment 5

[Suhas]

Randell, I can take it if its fine ... please let me know

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Sure, suhas - if you can a patch for the hack in, that would be great.  We'll need to revise the mochitests when we land it (note).

The "real" work here is partly blocked on the working group (MMUSIC in this case) getting it's act together and specifying the relationships between m= lines in the SDP.

Comment 7

[Eric Rescorla (:ekr)]

I'm not sold that this is the right approach. Currently, it's an open issue in
WebRTC/RTCWEB how to represent incoming SDP as media streams. The
specifications do not define a deterministic set of mappings from input
media stream to SDP and then back to output media stream. Until that's
resolved it seems easier to leave things as-is

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

"get a patch for the hack in".  Silly fingers.

Like I said, MMUSIC needs to get it's act together.  However, with 1 video and 1 audio max, I think a hack is reasonable for now and will be closer to parity with Chrome's current impl.   Few if anyone is feeding truly separate audio+video streams in as the source currently - and fewer still who want to separate them at the output.

Comment 9

[Eric Rescorla (:ekr)]

Suhas, this is going to be pretty complicated to get right. If it's to be done, I would prefer to do it myself. Re-assigning to myself.

Comment 10

[Eric Rescorla (:ekr)]

Attachment: Change MediaPipelineReceiveVideo to pull rather than push

Comment 11

[Eric Rescorla (:ekr)]

Attachment: Merge all incoming media streams

Comment 12

[Eric Rescorla (:ekr)]

Comment on attachment 705540 [details] [diff] [review]
Merge all incoming media streams

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

Here is a first cut at this. It's not done and the unit tests fail, but I wanted comments on strategy

Comment 13

[Ethan Hugg [:ehugg]]

Comment on attachment 705540 [details] [diff] [review]
Merge all incoming media streams

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

I don't see a reason not to put them always in a single stream for now - and it builds so it must be ready to ship.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +888,5 @@
>    return MediaPipelineReceive::Init();
>  }
>  
> +MediaPipelineReceiveVideo::PipelineListener::PipelineListener(SourceMediaStream * source,
> +                                                              TrackID track_id) 

nit: These long lines with indents look confusing in splinter.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +178,5 @@
>  
>            if (!stream) {
>              CSFLogErrorS(logTag, __FUNCTION__ << " GetMediaStream returned NULL");
>            } else {
>              hint = stream->GetHintContents();

hint is now set but not used, I assume you will remove along with GetHintContents and all that stuff.

@@ +179,5 @@
>            if (!stream) {
>              CSFLogErrorS(logTag, __FUNCTION__ << " GetMediaStream returned NULL");
>            } else {
>              hint = stream->GetHintContents();
> +            mObserver->OnAddStream(stream, "unknown_type: TODO([e-mail]");

So, I assume you will remove this second parameter altogether because you're hoping that the stream itself will be able to enumerate audio/video streams.  And you haven't removed it yet because it's over in IPeerConection.idl.  Is this correct, or do you plan to use this second string param for something else?

@@ +268,5 @@
>    PC_AUTO_ENTER_API_CALL_NO_CHECK();
>  
>    nsIDOMMediaStream* stream;
>  
> +  nsresult res = MakeMediaStream(0, &stream);

Similarly this first param gets removed, and someone updates nsDOMMediaStream so that CreateSourceStream no longer takes parameters.

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +6671,5 @@
> + * dcb_p - Pointer to the DCB whose SDP is to be manipulated.
> + * media - the media object to add.
> + *
> + * returns TRUE for success and FALSE for failure
> + */

Comment cut/paste without update

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 705540 [details] [diff] [review]
Merge all incoming media streams

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

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h
@@ +488,2 @@
>  #endif
> +    mozilla::ReentrantMonitor monitor_; // Monitor for processing WebRTC frames.

Indicate what data is protected by it (and probably what the reason for it is, i.e. concurrent access by MSG thread and from DeliverFrame from a GIPS thread)

Comment 15

[Eric Rescorla (:ekr)]

Attachment: Merge all incoming media streams

Comment 16

[Eric Rescorla (:ekr)]

Attachment: Merge all incoming media streams

Comment 17

[Eric Rescorla (:ekr)]

Attachment: Merge all incoming media streams

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 705673 [details] [diff] [review]
Merge all incoming media streams

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

That looks great, Eric. I have some comments on the test code which I have added inline. Further I can't find test_peerConnection_bug827843.html in the tree. You are making changes here. Do we depend on another patch which is not referenced?

::: dom/media/tests/mochitest/test_peerConnection_basicAudio.html
@@ +69,5 @@
>               "A single local stream has been attached to the local peer");
>            is(pcRemote.localStreams.length, 1,
>               "A single local stream has been attached to the remote peer");
>  
> +          // TODO: check that the streams are of the expected types.

I don't think that this is the right way. We have two options here:

1) make it a strong todo() call so it's visible all the time
2) Add the checks to ensure we do it correctly.

Is there a blocker which doesn't let us check for the expected media type? If yes we should get it filed or referenced.

::: dom/media/tests/mochitest/test_peerConnection_basicAudioVideo.html
@@ +56,3 @@
>  
> +      videoPCRemote.mozSrcObject = aObj.stream;
> +      videoPCRemote.play();

Nice! I love that, really.

@@ +100,2 @@
>  
> +	      // TODO: check that the streams are of the expected types.

Same question as before.

::: dom/media/tests/mochitest/test_peerConnection_basicAudioVideoCombined.html
@@ +18,1 @@
>    <audio id="audioLocal" controls></audio>

I don't see a reason why we shouldn't also remove audioLocal. It's not necessary in this test and wasn't even used before.

::: dom/media/tests/mochitest/test_peerConnection_basicVideo.html
@@ +68,5 @@
>            is(pcLocal.localStreams.length, 1,
>               "A single local stream has been attached to the local peer");
>            is(pcRemote.localStreams.length, 1,
>               "A single local stream has been attached to the remote peer");
> +        

nit: white-space change.

Comment 19

[Eric Rescorla (:ekr)]

(In reply to Henrik Skupin (:whimboo) from comment #18)
> Comment on attachment 705673 [details] [diff] [review]
> Merge all incoming media streams
> 
> Review of attachment 705673 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> That looks great, Eric. I have some comments on the test code which I have
> added inline. Further I can't find test_peerConnection_bug827843.html in the
> tree. You are making changes here. Do we depend on another patch which is
> not referenced?

just tree skew:
http://hg.mozilla.org/integration/mozilla-inbound/file/a941576624be/dom/media/tests/mochitest/test_peerConnection_bug827843.html


> ::: dom/media/tests/mochitest/test_peerConnection_basicAudio.html
> @@ +69,5 @@
> >               "A single local stream has been attached to the local peer");
> >            is(pcRemote.localStreams.length, 1,
> >               "A single local stream has been attached to the remote peer");
> >  
> > +          // TODO: check that the streams are of the expected types.
> 
> I don't think that this is the right way. We have two options here:
> 
> 1) make it a strong todo() call so it's visible all the time
> 2) Add the checks to ensure we do it correctly.
> 
> Is there a blocker which doesn't let us check for the expected media type?
> If yes we should get it filed or referenced.

I don't know if there is a bug. There have been a number of exchanges
between me, roc, ehugg, and jesup about it/


> ::: dom/media/tests/mochitest/test_peerConnection_basicAudioVideo.html
> @@ +56,3 @@
> >  
> > +      videoPCRemote.mozSrcObject = aObj.stream;
> > +      videoPCRemote.play();
> 
> Nice! I love that, really.
> 
> @@ +100,2 @@
> >  
> > +	      // TODO: check that the streams are of the expected types.
> 
> Same question as before.
> 
> :::
> dom/media/tests/mochitest/test_peerConnection_basicAudioVideoCombined.html
> @@ +18,1 @@
> >    <audio id="audioLocal" controls></audio>
> 
> I don't see a reason why we shouldn't also remove audioLocal. It's not
> necessary in this test and wasn't even used before.

Sure.


> ::: dom/media/tests/mochitest/test_peerConnection_basicVideo.html
> @@ +68,5 @@
> >            is(pcLocal.localStreams.length, 1,
> >               "A single local stream has been attached to the local peer");
> >            is(pcRemote.localStreams.length, 1,
> >               "A single local stream has been attached to the remote peer");
> > +        
> 
> nit: white-space change.


At a high level, these tests need refactoring to remove all the common cut-and-pasted
code. When I talked to jsmith he proposed just commenting this stuff and then dealing
with it in the refactor.

Comment 20

[Eric Rescorla (:ekr)]

Attachment: Merge all incoming media streams

Comment 21

[Ethan Hugg [:ehugg]]

Comment on attachment 705875 [details] [diff] [review]
Merge all incoming media streams

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

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +4593,5 @@
> +			    TODO([e-mail]): revisit when we have media assigned to streams
> +			    in the SDP */
> +                         if (!created_media_stream){
> +			     lsm_add_remote_stream (dcb_p->line, dcb_p->call_id, media, &pc_stream_id);
> +                             PR_ASSERT(pc_stream_id == 0);

MOZ_ASSERT does work in this file.  According to Jesup we should be using it instead of PR_ASSERT whenever we can.

Comment 22

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 705875 [details] [diff] [review]
Merge all incoming media streams

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

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h
@@ +485,4 @@
>  #endif
> +    mozilla::ReentrantMonitor monitor_; // Monitor for processing WebRTC frames.
> +    			      		// Protects against writing (from the
> +    					// GIPS thread and) reading (from MSG.)

"thread) and reading"

I think.  Trivial nit.

It applies to which data?  All of it?  Please specify.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +271,5 @@
>  
>    nsIDOMMediaStream* stream;
>  
> +  // We need to pass a dummy hint here because FakeMediaStream currently
> +  // needs to actually propagate a hint for local streams. 

trailing space

Comment 23

[Adam Roach [:abr]]

Comment on attachment 705875 [details] [diff] [review]
Merge all incoming media streams

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

This generally looks good. I have quite a few nits, and one potential logic issue with the new code in gsm_sdp.c. I'd like to come to a common understanding about onstreamadd behavior versus "adding new tracks to an existing stream" before this lands.

Note that my understanding of the MediaPipeline stuff is very rudimentary, so I can't vouch for the correctness of the code in there. I'm counting on jesup's review to catch any issues with MediaPipeline.

::: dom/media/tests/mochitest/test_peerConnection_basicAudio.html
@@ +69,5 @@
>               "A single local stream has been attached to the local peer");
>            is(pcRemote.localStreams.length, 1,
>               "A single local stream has been attached to the remote peer");
>  
> +          // TODO: check that the streams are of the expected types.

Please either complete this TODO or open a bug for it and cite the bug number here.

Applies to the other mochi tests as well.

::: dom/media/tests/mochitest/test_peerConnection_basicAudioVideo.html
@@ +102,5 @@
> +
> +	      ok(PeerConnection.findStream(pcLocal.remoteStreams, test_data.pcLocal[0]) !== -1,
> +		 "Remote stream for local peer is accessible");
> +	      ok(PeerConnection.findStream(pcRemote.remoteStreams, test_data.pcRemote[0]) !== -1,
> +		 "Remote stream for remote peer is accessible");

This is a very minor nit, but can we please use spaces rather than tabs here? It's too easy for editors to get tabwidths wrong when you have literal ASCII 0x09's written into the file (cf. line 969 of this lsm.c, which was clearly written by someone with an editor set to tabwidth=4)

I've used "Tabs" as shorthand for this comment in subsequent files.

::: dom/media/tests/mochitest/test_peerConnection_basicAudioVideoCombined.html
@@ +85,5 @@
> +
> +	  ok(PeerConnection.findStream(pcLocal.remoteStreams, test_data.pcLocal[0]) !== -1,
> +	  	 "Remote stream for local peer is accessible");
> +	  ok(PeerConnection.findStream(pcRemote.remoteStreams, test_data.pcRemote[0]) !== -1,
> +	  	 "Remote stream for remote peer is accessible");

Tabs

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +888,5 @@
>    return MediaPipelineReceive::Init();
>  }
>  
> +MediaPipelineReceiveVideo::PipelineListener::PipelineListener(
> +  SourceMediaStream* source, TrackID track_id) 

WS

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h
@@ +66,5 @@
>                  Direction direction,
>                  nsCOMPtr<nsIEventTarget> main_thread,
>                  nsCOMPtr<nsIEventTarget> sts_thread,
>                  MediaStream *stream,
> +                TrackID track_id,

If we're not using the track_id in the base class, it's a bit confusing to have it in the constructors all the way up the inheritance tree. We probably want to either store this in a class variable in the base class (possibly for debugging and/or logging purposes?) or remove it from the constructors of everything but MediaPipelineReceive{Audio,Video}.

@@ +485,4 @@
>  #endif
> +    mozilla::ReentrantMonitor monitor_; // Monitor for processing WebRTC frames.
> +    			      		// Protects against writing (from the
> +    					// GIPS thread and) reading (from MSG.)

Tabs

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +174,1 @@
>              CSFLogErrorS(logTag, __FUNCTION__ << " GetRemoteStream returned NULL");

"uint32_t hint" (three lines above this line) is now unused, due to the changes that follow.

@@ +180,5 @@
>              CSFLogErrorS(logTag, __FUNCTION__ << " GetMediaStream returned NULL");
>            } else {
> +            // We provide a type field because it is in the IDL
> +            // and we want code that looks at it not to crash.
> +            // TODO([e-mail]): remove.

Please add a bug # to this TODO.

Also, this comment implies that including the media type is vestigial. Isn't this necessary for the application to know what to do? It wouldn't make sense, for example, for them to instantiate a <video> element for audio-only calls. It seems reasonable that some apps will dynamically create their media elements based on this callback.

@@ +271,5 @@
>  
>    nsIDOMMediaStream* stream;
>  
> +  // We need to pass a dummy hint here because FakeMediaStream currently
> +  // needs to actually propagate a hint for local streams. 

WS

@@ +272,5 @@
>    nsIDOMMediaStream* stream;
>  
> +  // We need to pass a dummy hint here because FakeMediaStream currently
> +  // needs to actually propagate a hint for local streams. 
> +  // TODO([e-mail]): Clean up when we have explicit track lists.

Please add a bug # to this TODO.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
@@ +125,5 @@
>      PC_AUTO_ENTER_API_CALL_NO_CHECK();
>      return mRole;
>    }
>  
> +  nsresult CreateRemoteSourceStreamInfo(nsRefPtr<RemoteSourceStreamInfo>* aInfo);

Wrap

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +4590,5 @@
> +                        int pc_stream_id = -1;
> +
> +			 /* This is a hack to keep all the media in a single stream.
> +			    TODO([e-mail]): revisit when we have media assigned to streams
> +			    in the SDP */

Tabs

@@ +4592,5 @@
> +			 /* This is a hack to keep all the media in a single stream.
> +			    TODO([e-mail]): revisit when we have media assigned to streams
> +			    in the SDP */
> +                         if (!created_media_stream){
> +			     lsm_add_remote_stream (dcb_p->line, dcb_p->call_id, media, &pc_stream_id);

Tabs

@@ +4599,5 @@
> +                             result = gsmsdp_add_remote_stream(0, pc_stream_id, dcb_p);
> +                             PR_ASSERT(result);  /* TODO([e-mail]) add real error checking, but
> +                                                    this "can't fail" */
> +                             created_media_stream = TRUE;
> +			 }

Tabs

@@ +4612,5 @@
>                            * Inform VCM that a Data Channel has been negotiated
>                            */
> +			 int pc_stream_id;  /* Set but unused. Provided to fulfill the API contract
> +						     TODO([e-mail]): use or remove
> +						   */

Tabs -- this comment actually suffers visual tab damage no matter what I use to render it.

In terms of the parameter itself, I don't think this part of the design has been fleshed out at all. Ultimately, the call into lsm_data_channel_negotiated just emits a log message. What we ultimately need here probably depends on what ends up in the WebRTC spec.

@@ +4618,5 @@
>                           lsm_data_channel_negotiated(dcb_p->line, dcb_p->call_id, media, &pc_stream_id);
>                       }
>                    }
>                }
>              }

The entire foregoing section extends past column 80 on just about every line. Given that this function is approaching 500 lines in length, perhaps it just needs refactoring into smaller functions, which would fix the fact that we're at somewhere between 7 and 14 levels of indentation here, but that's clearly a task for a later time.

It would be really nice if you could reflow this to fit in 80 columns (it's really hard to read in splinter), but leaving it as-is for now is completely understandable.

(I'm of the understanding that sipcc is to be brought into compliance with Moz coding standards as it gets touched, since we're not upstreaming it -- correct me if I'm mistaken).

@@ +4714,3 @@
>                      }
>                  }
>              }

I'm not sure this logic is right. As it's currently coded, adding new tracks to an existing stream would trigger a "OnRemoteStreamAdd" to the PeerConnection, even though we haven't added a stream (just a track).

Arguably, this *is* something the application needs to know about, and I think the lack of an "onaddtrack" and "onremovetrack" is probably a gap in the WebRTC spec. And I know that we don't currently have any logic that would handle adding new tracks to a stream, but I'd rather this code not do the wrong thing when we do.

My suggestion is to change the comparison so it checks against zero rather than num_tracks_notified; and then adding a TODO that we need to inform the application of track changes if and when the spec is updated to support them.

@@ +6670,5 @@
> + * media - the media object to add.
> + *
> + * returns TRUE for success and FALSE for failure
> + */
> +static boolean gsmsdp_add_remote_track(uint16_t idx, uint16_t track, fsmdef_dcb_t *dcb_p, fsmdef_media_t *media) {

Wrap

@@ +6687,5 @@
> +  if (stream->num_tracks > (CC_MAX_TRACKS - 1))
> +    return FALSE;
> +
> +  stream->track[stream->num_tracks].media_stream_track_id = track;
> +  stream->track[stream->num_tracks].video = (media->type == SDP_MEDIA_VIDEO) ? TRUE : FALSE;

Wrap

::: media/webrtc/signaling/src/sipcc/core/gsm/lsm.c
@@ +980,5 @@
>                  /* TODO([e-mail]): Needs changing for when we
>                     have > 2 streams. ([e-mail]): For now,
> +		   we use all the same stream so pc_stream_id == 0
> +		   and the tracks are assigned in order and are
> +		   equal to the level in the media objects */

Tabs

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +1687,5 @@
>    std::string answer = a2_.answer();
>    ASSERT_NE(answer.find("111 opus/"), std::string::npos);
>  }
>  
> +TEST_F(SignalingTest, DISABLED_OfferAllDynamicTypes)

If we're disabling this, please add a comment referencing Bug 818640.

Comment 24

[Adam Roach [:abr]]

Argh! Sorry, my review appears to have reset jesup's review flag to '?'. If someone (not me) could go in and set it back to "+", I'd appreciate it. (I'm not quite ready to r+ mine, and I think that setting the flag for jesup's review would clear mine -- not sure, though).

Comment 25

[Adam Roach [:abr]]

Comment on attachment 705875 [details] [diff] [review]
Merge all incoming media streams

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

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +4612,5 @@
>                            * Inform VCM that a Data Channel has been negotiated
>                            */
> +			 int pc_stream_id;  /* Set but unused. Provided to fulfill the API contract
> +						     TODO([e-mail]): use or remove
> +						   */

One more minor nit -- you probably want to change my email address to either "[e-mail]" or "[e-mail]". :)

Comment 26

[Eric Rescorla (:ekr)]

Attachment: Merge all incoming media streams

Comment 27

[Adam Roach [:abr]]

Comment on attachment 706574 [details] [diff] [review]
Merge all incoming media streams

Looks good to me.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5c6403393246

Comment 29

[Jason Smith [:jsmith]]

Verified through running mochitests and through implementing cleanup work for mochitests in this area and running them.