Closed Bug 834038 Opened 8 years ago Closed 8 years ago

SDP renegotiation should re-use existing streams when possible

Categories

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

21 Branch
defect

Tracking

()

VERIFIED FIXED

People

(Reporter: abr, Assigned: abr)

Details

(Whiteboard: [WebRTC] [blocking-webrtc+])

Attachments

(1 obsolete file)

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 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 adam@nostrum.com -- 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.
Attachment #706101 - Flags: review?(ekr)
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.
Attachment #706101 - Attachment is obsolete: true
Attachment #706101 - Flags: review?(ekr)
Taking this one, as I think I can fix it along with Bug 840344.
Assignee: nobody → adam
Whiteboard: [WebRTC] [blocking-webrtc-] → [WebRTC] [blocking-webrtc+]
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.
Marking as resolved, since the patch from bug 840344 prevents this condition from arising (as suggested in comment 3 of this bug).
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Verified per testing in the associated bug in the mochitest.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.