Make NACK buffer cleanup on outgoing keyframe configurable (see #2401) (

#2402)

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

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)) {

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);

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) {