From 0f8fc27892dd4cdac898bceb9264614261c3c207 Mon Sep 17 00:00:00 2001 From: Jan Schmidt Date: Thu, 1 Aug 2024 13:42:52 +1000 Subject: [PATCH] webrtcbin: Fix renegotiation checks When checking for renegotiation against a local offer, reverse the remote direction in the corresponding answer to fix falsely not triggering on-negotiation needed when switching (for example) from local sendrecv -> recvonly against a peer that answered 'recvonly'. In the other direction, when the local was the answerer, renegotiation might trigger when it didn't need to - whenever the local transceiver direction differs from the intersected direction we chose. Instead what we want is to check if the intersected direction we would now choose differs from what was previously chosen. This makes the behaviour in both cases match the behaviour described in https://www.w3.org/TR/webrtc/#dfn-check-if-negotiation-is-needed Part-of: --- .../gst-plugins-bad/ext/webrtc/gstwebrtcbin.c | 70 ++++++----- .../tests/check/elements/webrtcbin.c | 113 ++++++++++++++++++ 2 files changed, 151 insertions(+), 32 deletions(-) diff --git a/subprojects/gst-plugins-bad/ext/webrtc/gstwebrtcbin.c b/subprojects/gst-plugins-bad/ext/webrtc/gstwebrtcbin.c index 4a5ee6300b..d72b067d3e 100644 --- a/subprojects/gst-plugins-bad/ext/webrtc/gstwebrtcbin.c +++ b/subprojects/gst-plugins-bad/ext/webrtc/gstwebrtcbin.c @@ -1773,6 +1773,22 @@ done: return res; } +static GstWebRTCRTPTransceiverDirection +_reverse_direction (GstWebRTCRTPTransceiverDirection direction) +{ + switch (direction) { + case GST_WEBRTC_RTP_TRANSCEIVER_DIRECTION_NONE: + case GST_WEBRTC_RTP_TRANSCEIVER_DIRECTION_INACTIVE: + case GST_WEBRTC_RTP_TRANSCEIVER_DIRECTION_SENDRECV: + return direction; + case GST_WEBRTC_RTP_TRANSCEIVER_DIRECTION_SENDONLY: + return GST_WEBRTC_RTP_TRANSCEIVER_DIRECTION_RECVONLY; + case GST_WEBRTC_RTP_TRANSCEIVER_DIRECTION_RECVONLY: + return GST_WEBRTC_RTP_TRANSCEIVER_DIRECTION_SENDONLY; + } + return GST_WEBRTC_RTP_TRANSCEIVER_DIRECTION_NONE; +} + /* http://w3c.github.io/webrtc-pc/#dfn-check-if-negotiation-is-needed */ static gboolean _check_if_negotiation_is_needed (GstWebRTCBin * webrtc) @@ -1862,33 +1878,39 @@ _check_if_negotiation_is_needed (GstWebRTCBin * webrtc) /* If connection's currentLocalDescription if of type "offer", and * the direction of the associated m= section in neither the offer * nor answer matches t's direction, return "true". */ - - if (local_dir != trans->direction && remote_dir != trans->direction) { - GST_LOG_OBJECT (webrtc, "transceiver direction (%s) doesn't match " - "description (local %s remote %s)", + if (local_dir != trans->direction + && _reverse_direction (remote_dir) != trans->direction) { + GST_LOG_OBJECT (webrtc, + "transceiver direction (%s) doesn't match " + "description (local %s remote %s (reversed %s))", gst_webrtc_rtp_transceiver_direction_to_string (trans->direction), gst_webrtc_rtp_transceiver_direction_to_string (local_dir), - gst_webrtc_rtp_transceiver_direction_to_string (remote_dir)); + gst_webrtc_rtp_transceiver_direction_to_string (remote_dir), + gst_webrtc_rtp_transceiver_direction_to_string (_reverse_direction + (remote_dir)) + ); return TRUE; } } else if (webrtc->current_local_description->type == GST_WEBRTC_SDP_TYPE_ANSWER) { - GstWebRTCRTPTransceiverDirection intersect_dir; - /* If connection's currentLocalDescription if of type "answer", and - * the direction of the associated m= section in the answer does not - * match t's direction intersected with the offered direction (as - * described in [JSEP] (section 5.3.1.)), return "true". */ + * the direction of the associated m= section in the answer we sent + * (local_dir) does not match t's direction intersected with the + * offer direction (as described in [JSEP] (section 5.3.1.)), + * return "true" because we want to propose a different + * direction now. */ /* remote is the offer, local is the answer */ - intersect_dir = _intersect_answer_directions (remote_dir, local_dir); - - if (intersect_dir != trans->direction) { - GST_LOG_OBJECT (webrtc, "transceiver direction (%s) doesn't match " - "description intersected direction %s (local %s remote %s)", + GstWebRTCRTPTransceiverDirection now_intersect_dir = + _intersect_answer_directions (remote_dir, trans->direction); + if (now_intersect_dir != local_dir) { + GST_LOG_OBJECT (webrtc, + "transceiver direction (%s) doesn't match for the " + "new description intersected direction %s (prev local %s remote %s)", gst_webrtc_rtp_transceiver_direction_to_string (trans->direction), gst_webrtc_rtp_transceiver_direction_to_string (local_dir), - gst_webrtc_rtp_transceiver_direction_to_string (intersect_dir), + gst_webrtc_rtp_transceiver_direction_to_string + (now_intersect_dir), gst_webrtc_rtp_transceiver_direction_to_string (remote_dir)); return TRUE; } @@ -6283,22 +6305,6 @@ get_last_generated_description (GstWebRTCBin * webrtc, SDPSource source, return NULL; } -static GstWebRTCRTPTransceiverDirection -_reverse_direction (GstWebRTCRTPTransceiverDirection direction) -{ - switch (direction) { - case GST_WEBRTC_RTP_TRANSCEIVER_DIRECTION_NONE: - case GST_WEBRTC_RTP_TRANSCEIVER_DIRECTION_INACTIVE: - case GST_WEBRTC_RTP_TRANSCEIVER_DIRECTION_SENDRECV: - return direction; - case GST_WEBRTC_RTP_TRANSCEIVER_DIRECTION_SENDONLY: - return GST_WEBRTC_RTP_TRANSCEIVER_DIRECTION_RECVONLY; - case GST_WEBRTC_RTP_TRANSCEIVER_DIRECTION_RECVONLY: - return GST_WEBRTC_RTP_TRANSCEIVER_DIRECTION_SENDONLY; - } - return GST_WEBRTC_RTP_TRANSCEIVER_DIRECTION_NONE; -} - /* https://w3c.github.io/webrtc-pc/#set-description (steps in 4.6.10.) */ static gboolean _create_and_associate_transceivers_from_sdp (GstWebRTCBin * webrtc, diff --git a/subprojects/gst-plugins-bad/tests/check/elements/webrtcbin.c b/subprojects/gst-plugins-bad/tests/check/elements/webrtcbin.c index 957f6420df..19363ac794 100644 --- a/subprojects/gst-plugins-bad/tests/check/elements/webrtcbin.c +++ b/subprojects/gst-plugins-bad/tests/check/elements/webrtcbin.c @@ -719,6 +719,14 @@ test_webrtc_reset_negotiation (struct test_webrtc *t) test_webrtc_signal_state (t, STATE_NEGOTIATION_NEEDED); } +static void +test_webrtc_clear_states (struct test_webrtc *t) +{ + GST_DEBUG ("clearing states"); + g_array_free (t->states, TRUE); + t->states = g_array_new (FALSE, TRUE, sizeof (TestState)); +} + static void test_webrtc_free (struct test_webrtc *t) { @@ -3789,6 +3797,110 @@ GST_START_TEST (test_renego_transceiver_set_direction) GST_END_TEST; +GST_START_TEST (test_renego_triggering) +{ + + struct test_webrtc *t = create_audio_test (); + + VAL_SDP_INIT (no_duplicate_payloads, on_sdp_media_no_duplicate_payloads, + NULL, NULL); + + guint media_format_count[] = { 1 }; + VAL_SDP_INIT (media_formats, on_sdp_media_count_formats, + media_format_count, &no_duplicate_payloads); + VAL_SDP_INIT (count, _count_num_sdp_media, GUINT_TO_POINTER (1), + &media_formats); + + const gchar *expected_offer_setup[] = { "actpass" }; + VAL_SDP_INIT (offer_setup, on_sdp_media_setup, expected_offer_setup, &count); + const gchar *expected_answer_setup[] = { "active" }; + VAL_SDP_INIT (answer_setup, on_sdp_media_setup, expected_answer_setup, + &count); + const gchar *expected_offer_direction[] = { "sendrecv" }; + VAL_SDP_INIT (offer, on_sdp_media_direction, expected_offer_direction, + &offer_setup); + const gchar *expected_answer_direction[] = { "recvonly" }; + VAL_SDP_INIT (answer, on_sdp_media_direction, expected_answer_direction, + &answer_setup); + GstCaps *caps; + GArray *transceivers; + + /* Ensure sendrecv stream on webrtc1 */ + g_signal_emit_by_name (t->webrtc1, "get-transceivers", &transceivers); + fail_unless (transceivers != NULL); + fail_unless_equals_int (transceivers->len, 1); + + GstWebRTCRTPTransceiver *trans_local = + g_array_index (transceivers, GstWebRTCRTPTransceiver *, 0); + g_object_set (trans_local, "direction", + GST_WEBRTC_RTP_TRANSCEIVER_DIRECTION_SENDRECV, NULL); + + /* setup recvonly peer */ + caps = gst_caps_from_string (OPUS_RTP_CAPS (96)); + gst_caps_set_simple (caps, "ssrc", G_TYPE_UINT, 0xDEADBEEF, NULL); + + GstWebRTCRTPTransceiver *trans_remote = NULL; + GstWebRTCRTPTransceiverDirection direction = + GST_WEBRTC_RTP_TRANSCEIVER_DIRECTION_RECVONLY; + g_signal_emit_by_name (t->webrtc2, "add-transceiver", direction, caps, + &trans_remote); + gst_caps_unref (caps); + fail_unless (trans_remote != NULL); + gst_object_unref (trans_remote); + + test_validate_sdp (t, &offer, &answer); + + GST_LOG + ("Finished validating sendrecv <-> recvonly nego. Triggering renego with recvonly <-> recvonly peers"); + + /* Now change the sender to recvonly and expect to renegotiate to inactive */ + test_webrtc_reset_negotiation (t); + test_webrtc_clear_states (t); + + GST_LOG ("Setting local transceiver to RECVONLY"); + g_object_set (trans_local, "direction", + GST_WEBRTC_RTP_TRANSCEIVER_DIRECTION_RECVONLY, NULL); + + GST_LOG ("Waiting for on-negotiation-needed"); + test_webrtc_wait_for_state_mask (t, 1 << STATE_NEGOTIATION_NEEDED); + + const gchar *new_expected_offer_direction[] = { "recvonly" }; + VAL_SDP_INIT (new_offer, on_sdp_media_direction, new_expected_offer_direction, + NULL); + const gchar *new_expected_answer_direction[] = { "inactive" }; + VAL_SDP_INIT (new_answer, on_sdp_media_direction, + new_expected_answer_direction, NULL); + + test_validate_sdp (t, &new_offer, &new_answer); + + g_array_unref (transceivers); + + /* At this point webrtc2 is the answerer. Check that it also triggers nego + * if we change the direction to sendonly */ + test_webrtc_reset_negotiation (t); + test_webrtc_clear_states (t); + + GST_LOG ("Setting remote transceiver to SENDONLY"); + g_signal_emit_by_name (t->webrtc2, "get-transceivers", &transceivers); + fail_unless (transceivers != NULL); + fail_unless_equals_int (transceivers->len, 1); + + trans_remote = g_array_index (transceivers, GstWebRTCRTPTransceiver *, 0); + + g_object_set (trans_remote, "direction", + GST_WEBRTC_RTP_TRANSCEIVER_DIRECTION_SENDONLY, NULL); + + g_array_unref (transceivers); + + GST_LOG ("Waiting for on-negotiation-needed"); + test_webrtc_wait_for_state_mask (t, 1 << STATE_NEGOTIATION_NEEDED); + + test_webrtc_free (t); +} + +GST_END_TEST; + + static void offer_remove_last_media (struct test_webrtc *t, GstElement * element, GstPromise * promise, gpointer user_data) @@ -6140,6 +6252,7 @@ webrtcbin_suite (void) tcase_add_test (tc, test_bundle_renego_add_stream); tcase_add_test (tc, test_bundle_max_compat_max_bundle_renego_add_stream); tcase_add_test (tc, test_renego_transceiver_set_direction); + tcase_add_test (tc, test_renego_triggering); tcase_add_test (tc, test_renego_lose_media_fails); tcase_add_test (tc, test_bundle_codec_preferences_rtx_no_duplicate_payloads);