834038 - SDP renegotiation should re-use existing streams when possible

Status: VERIFIED FIXED

(Core :: WebRTC: Signaling, defect, P2)

Description

[Adam Roach [:abr]]

Currently, if a WebRTC session is ongoing and we go through the steps for renegotiation (setRemoteDescription/getAnswer/setLocalDescription), we end up creating new media streams for each m= section (without deallocating the existing ones).

While it is possible that a renegotiation can add and remove streams, we need to compare the new session being negotiated with the current, ongoing session, and only add and remove streams as necessary (rather than adding new streams each time).

Comment 1

[Ethan Hugg [:ehugg]]

Attachment: Patch 1 - Remove existing streams on renegotiation.

Comment 2

[Ethan Hugg [:ehugg]]

Comment on attachment 706101 [details] [diff] [review]
Patch 1 - Remove existing streams on renegotiation.

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

This patch relies on EKR's patch to 816780

This in not the whole fix of course, but it will remove the old stream on renegotiation.  Figuring out which streams/tracks can be re-used is a problem still left to solve.  The renegotiation unittest should now work.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
@@ +205,5 @@
>        return NS_OK;
>      }
>    }
>  
> +  PR_Unlock(mLocalSourceStreamsLock);

Found this not being unlocked on error condition.  I'm guessing this lock is not needed since we changed the threading, but left it in.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h
@@ -320,5 @@
>    PRLock *mLocalSourceStreamsLock;
>    nsTArray<nsRefPtr<LocalSourceStreamInfo> > mLocalSourceStreams;
>  
>    // A list of streams provided by the other side
> -  PRLock *mRemoteSourceStreamsLock;

This was never initialized and was unused.  ClearRemoteStreams does not use a lock currently.

::: media/webrtc/signaling/src/sipcc/core/gsm/h/lsm.h
@@ +105,5 @@
>  void lsm_ui_display_status(const char *pStatusStr, line_t line,
>                             callid_t call_id);
>  string_t lsm_parse_displaystr(string_t displaystr);
>  void lsm_speaker_mode(short mode);
> +void lsm_add_remote_stream (fsmdef_dcb_t *dcb, fsmdef_media_t *media, int *pc_stream_id);

Changed the signature of this a bit since we had been re-finding the dcb pointer by call_id inside it.  Easier I think to just pass in the dcb.

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +1340,5 @@
>  }
>  
>  // XXX [e-mail] -- This test seems questionable; we need to think
>  // through what actually needs to be tested here.
> +TEST_F(SignalingTest, OfferAnswerReNegotiateOfferAnswerDontReceiveVideoNoVideoStream)

This test should now work, no streams or tracks are re-used though of course.

Comment 3

[Ethan Hugg [:ehugg]]

Comment on attachment 706101 [details] [diff] [review]
Patch 1 - Remove existing streams on renegotiation.


Obsoleting this patch.  It looks like we would rather disallow it than restart all streams on renegotiation.

Comment 4

[Adam Roach [:abr]]

Taking this one, as I think I can fix it along with Bug 840344.

Comment 5

[Adam Roach [:abr]]

To be clear, the "fix" for 840344 will wallpaper over this issue (as it does that one). There's still a longer-term fix necessary here for handling stream re-negotiation, which will need to be done as part of Bug 840728.

Comment 6

[Adam Roach [:abr]]

Marking as resolved, since the patch from bug 840344 prevents this condition from arising (as suggested in comment 3 of this bug).

Comment 7

[Jason Smith [:jsmith]]

Verified per testing in the associated bug in the mochitest.