From a80c8be333f973c88b650527b5de3062352b52f3 Mon Sep 17 00:00:00 2001 From: Lorenzo Miniero Date: Fri, 30 Oct 2020 10:50:47 +0100 Subject: [PATCH] Make NACK buffer cleanup on outgoing keyframe configurable (see #2401) (#2402) --- conf/janus.jcfg.sample.in | 11 +++++++++++ ice.c | 17 +++++++++++++---- ice.h | 12 +++++++++++- janus.c | 27 +++++++++++++++++++++++++++ 4 files changed, 62 insertions(+), 5 deletions(-) diff --git a/conf/janus.jcfg.sample.in b/conf/janus.jcfg.sample.in index 08592116678..a881cfb5251 100644 --- a/conf/janus.jcfg.sample.in +++ b/conf/janus.jcfg.sample.in @@ -206,6 +206,17 @@ media: { #twcc_period = 100 #dtls_timeout = 500 + # Janus can do some optimizations on the NACK queue, specifically when + # keyframes are involved. Namely, you can configure Janus so that any + # time a keyframe is sent to a user, the NACK buffer for that connection + # is emptied. This allows Janus to ignore NACK requests for packets + # sent shortly before the keyframe was sent, since it can be assumed + # that the keyframe will restore a complete working image for the user + # anyway (which is the main reason why video retransmissions are typically + # required). While this optimization is known to work fine in most cases, + # it can backfire in some edge cases, and so is disabled by default. + #nack_optimizations = true + # If you need DSCP packet marking and prioritization, you can configure # the 'dscp' property to a specific values, and Janus will try to # set it on all outgoing packets using libnice. Normally, the specs diff --git a/ice.c b/ice.c index 8a91a20d02c..7be1e4a3d53 100644 --- a/ice.c +++ b/ice.c @@ -95,7 +95,7 @@ gboolean janus_ice_is_mdns_enabled(void) { return janus_mdns_enabled; } -/* IPv6 support (still mostly WIP) */ +/* IPv6 support */ static gboolean janus_ipv6_enabled; gboolean janus_ice_is_ipv6_enabled(void) { return janus_ipv6_enabled; @@ -499,6 +499,14 @@ static void janus_ice_free_queued_packet(janus_ice_queued_packet *pkt) { /* Maximum ignore count after retransmission (200ms) */ #define MAX_NACK_IGNORE 200000 +static gboolean nack_optimizations = FALSE; +void janus_set_nack_optimizations_enabled(gboolean optimize) { + nack_optimizations = optimize; +} +gboolean janus_is_nack_optimizations_enabled(void) { + return nack_optimizations; +} + static uint16_t min_nack_queue = DEFAULT_MIN_NACK_QUEUE; void janus_set_min_nack_queue(uint16_t mnq) { min_nack_queue = mnq < DEFAULT_MAX_NACK_QUEUE ? mnq : DEFAULT_MAX_NACK_QUEUE; @@ -3449,7 +3457,7 @@ int janus_ice_setup_local(janus_ice_handle *handle, int offer, int audio, int vi family = ifa->ifa_addr->sa_family; if(family != AF_INET && family != AF_INET6) continue; - /* We only add IPv6 addresses if support for them has been explicitly enabled (still WIP, mostly) */ + /* We only add IPv6 addresses if support for them has been explicitly enabled */ if(family == AF_INET6 && !janus_ipv6_enabled) continue; /* Check the interface name first, we can ignore that as well: enforce list would be checked later */ @@ -4358,8 +4366,9 @@ static gboolean janus_ice_outgoing_traffic_handle(janus_ice_handle *handle, janu if(g_atomic_int_get(&handle->dump_packets)) janus_text2pcap_dump(handle->text2pcap, JANUS_TEXT2PCAP_RTP, FALSE, pkt->data, pkt->length, "[session=%"SCNu64"][handle=%"SCNu64"]", session->session_id, handle->handle_id); - /* If this is video, check if this is a keyframe: if so, we empty our retransmit buffer for incoming NACKs */ - if(video && stream->video_is_keyframe) { + /* If this is video and NACK optimizations are enabled, check if this is + * a keyframe: if so, we empty our retransmit buffer for incoming NACKs */ + if(video && nack_optimizations && stream->video_is_keyframe) { int plen = 0; char *payload = janus_rtp_payload(pkt->data, pkt->length, &plen); if(stream->video_is_keyframe(payload, plen)) { diff --git a/ice.h b/ice.h index b8c10a8127a..65ce7e5db71 100644 --- a/ice.h +++ b/ice.h @@ -125,7 +125,7 @@ gboolean janus_ice_is_full_trickle_enabled(void); /*! \brief Method to check whether mDNS resolution is enabled or not * @returns true if mDNS resolution is enabled, false otherwise */ gboolean janus_ice_is_mdns_enabled(void); -/*! \brief Method to check whether IPv6 candidates are enabled/supported or not (still WIP) +/*! \brief Method to check whether IPv6 candidates are enabled/supported or not * @returns true if IPv6 candidates are enabled/supported, false otherwise */ gboolean janus_ice_is_ipv6_enabled(void); /*! \brief Method to modify the min NACK value (i.e., the minimum time window of packets per handle to store for retransmissions) @@ -134,6 +134,16 @@ void janus_set_min_nack_queue(uint16_t mnq); /*! \brief Method to get the current min NACK value (i.e., the minimum time window of packets per handle to store for retransmissions) * @returns The current min NACK value */ uint16_t janus_get_min_nack_queue(void); +/*! \brief Method to enable/disable the NACK optimizations on outgoing keyframes: when + * enabled, the NACK buffer for a PeerConnection is cleaned any time Janus sends a + * keyframe, as any missing packet won't be needed since the keyframe will allow the + * media recipient to still restore a complete image anyway. Since this optimization + * seems to cause some issues in some edge cases, it's disabled by default. + * @param[in] optimize Whether the opzimization should be enabled or disabled */ +void janus_set_nack_optimizations_enabled(gboolean optimize); +/*! \brief Method to check whether NACK optimizations on outgoing keyframes are enabled or not + * @returns optimize if optimizations are enabled, false otherwise */ +gboolean janus_is_nack_optimizations_enabled(void); /*! \brief Method to modify the no-media event timer (i.e., the number of seconds where no media arrives before Janus notifies this) * @param[in] timer The new timer value, in seconds */ void janus_set_no_media_timer(uint timer); diff --git a/janus.c b/janus.c index 79c98ab6ad2..e9986d527fe 100644 --- a/janus.c +++ b/janus.c @@ -136,6 +136,9 @@ static struct janus_json_parameter colors_parameters[] = { static struct janus_json_parameter mnq_parameters[] = { {"min_nack_queue", JSON_INTEGER, JANUS_JSON_PARAM_REQUIRED | JANUS_JSON_PARAM_POSITIVE} }; +static struct janus_json_parameter nopt_parameters[] = { + {"nack_optimizations", JANUS_JSON_BOOL, JANUS_JSON_PARAM_REQUIRED} +}; static struct janus_json_parameter nmt_parameters[] = { {"no_media_timer", JSON_INTEGER, JANUS_JSON_PARAM_REQUIRED | JANUS_JSON_PARAM_POSITIVE} }; @@ -332,6 +335,7 @@ static json_t *janus_info(const char *transaction) { json_object_set_new(info, "full-trickle", janus_ice_is_full_trickle_enabled() ? json_true() : json_false()); json_object_set_new(info, "mdns-enabled", janus_ice_is_mdns_enabled() ? json_true() : json_false()); json_object_set_new(info, "min-nack-queue", json_integer(janus_get_min_nack_queue())); + json_object_set_new(info, "nack-optimizations", janus_is_nack_optimizations_enabled() ? json_true() : json_false()); json_object_set_new(info, "twcc-period", json_integer(janus_get_twcc_period())); if(janus_get_dscp() > 0) json_object_set_new(info, "dscp", json_integer(janus_get_dscp())); @@ -1964,6 +1968,7 @@ int janus_process_incoming_admin_request(janus_request *request) { json_object_set_new(status, "refcount_debug", refcount_debug ? json_true() : json_false()); json_object_set_new(status, "libnice_debug", janus_ice_is_ice_debugging_enabled() ? json_true() : json_false()); json_object_set_new(status, "min_nack_queue", json_integer(janus_get_min_nack_queue())); + json_object_set_new(status, "nack-optimizations", janus_is_nack_optimizations_enabled() ? json_true() : json_false()); json_object_set_new(status, "no_media_timer", json_integer(janus_get_no_media_timer())); json_object_set_new(status, "slowlink_threshold", json_integer(janus_get_slowlink_threshold())); json_object_set_new(reply, "status", status); @@ -2127,6 +2132,23 @@ int janus_process_incoming_admin_request(janus_request *request) { /* Send the success reply */ ret = janus_process_success(request, reply); goto jsondone; + } else if(!strcasecmp(message_text, "set_nack_optimizations")) { + /* Enable/disable NACK optimizations */ + JANUS_VALIDATE_JSON_OBJECT(root, nopt_parameters, + error_code, error_cause, FALSE, + JANUS_ERROR_MISSING_MANDATORY_ELEMENT, JANUS_ERROR_INVALID_ELEMENT_TYPE); + if(error_code != 0) { + ret = janus_process_error_string(request, session_id, transaction_text, error_code, error_cause); + goto jsondone; + } + gboolean optimize = json_is_true(json_object_get(root, "nack_optimizations")); + janus_set_nack_optimizations_enabled(optimize); + /* Prepare JSON reply */ + json_t *reply = janus_create_message("success", 0, transaction_text); + json_object_set_new(reply, "nack-optimizations", janus_is_nack_optimizations_enabled() ? json_true() : json_false()); + /* Send the success reply */ + ret = janus_process_success(request, reply); + goto jsondone; } else if(!strcasecmp(message_text, "set_no_media_timer")) { /* Change the current value for the no-media timer */ JANUS_VALIDATE_JSON_OBJECT(root, nmt_parameters, @@ -4822,6 +4844,11 @@ gint main(int argc, char *argv[]) janus_set_min_nack_queue(mnq); } } + item = janus_config_get(config, config_media, janus_config_type_item, "nack_optimizations"); + if(item && item->value) { + gboolean optimize = janus_is_true(item->value); + janus_set_nack_optimizations_enabled(optimize); + } /* no-media timer */ item = janus_config_get(config, config_media, janus_config_type_item, "no_media_timer"); if(item && item->value) {