Merge pull request #1680 from private-octopus/media_test_check_reset

Notification of loss of feedback

Author: huitema

CMakeLists.txt

@@ -8,7 +8,7 @@ else()
 endif()
 
 project(picoquic
-        VERSION 1.1.19.10
+        VERSION 1.1.19.11
         DESCRIPTION "picoquic library"
         LANGUAGES C CXX)
 

UnitTest1/unittest1.cpp

@@ -2273,6 +2273,12 @@ namespace UnitTest1
             Assert::AreEqual(ret, 0);
         }
 
+        TEST_METHOD(mediatest_suspension) {
+            int ret = mediatest_suspension_test();
+
+            Assert::AreEqual(ret, 0);
+        }
+
         TEST_METHOD(warptest_video) {
             int ret = warptest_video_test();
 

picoquic/bbr.c

@@ -230,6 +230,10 @@ typedef struct st_picoquic_bbr_state_t {
     uint64_t recovery_packet_number;
     uint64_t recovery_delivered;
 
+    /* Management of lost feedback */
+    unsigned int is_handling_lost_feedback;
+    uint64_t cwin_before_lost_feedback;
+
     /* Management of ECN marks */
     uint64_t ecn_ect1_last_round;
     uint64_t ecn_ce_last_round;
@@ -607,6 +611,31 @@ static void BBROnEnterFastRecovery(picoquic_bbr_state_t* bbr_state, picoquic_pat
     bbr_state->recovery_delivered = path_x->delivered;
 }
 
+/* Handling of the "lost feedback" state.
+ */
+static void BBREnterLostFeedback(picoquic_bbr_state_t* bbr_state, picoquic_path_t* path_x)
+{
+    if ((IsInAProbeBWState(bbr_state) || bbr_state->state == picoquic_bbr_alg_drain) &&
+        !bbr_state->is_handling_lost_feedback &&
+        path_x->cnx->cnx_state == picoquic_state_ready) {
+        /* Remembering the old cwin, so the state can be restored when the
+        * condition is lifted. */
+        bbr_state->cwin_before_lost_feedback = path_x->cwin;
+        /* setting the congestion window to exactly the bytes in transit, thus
+         * preventing any further transmission until the condition is lifted */
+        path_x->cwin = path_x->bytes_in_transit;
+        bbr_state->is_handling_lost_feedback = 1;
+    }
+}
+
+static void BBRExitLostFeedback(picoquic_bbr_state_t* bbr_state, picoquic_path_t* path_x)
+{
+    if (bbr_state->is_handling_lost_feedback) {
+        path_x->cwin = bbr_state->cwin_before_lost_feedback;
+        bbr_state->is_handling_lost_feedback = 0;
+    }
+}
+
 /* In picoquic, the arrival of an RTO maps to a "timer based" packet loss.
 * Question: do we want this on a loss signal, or simply when observing
 * loss data in ack notification?
@@ -2177,18 +2206,24 @@ static void picoquic_bbr_notify(
             BBRUpdateRecoveryOnLoss(bbr_state, path_x, ack_state->nb_bytes_newly_lost);
             break;
         case picoquic_congestion_notification_timeout:
+            BBRExitLostFeedback(bbr_state, path_x);
             /* if loss is PTO, we should start the OnPto processing */
             BBROnEnterRTO(bbr_state, path_x, ack_state->lost_packet_number);
             break;
         case picoquic_congestion_notification_spurious_repeat:
             /* handling of suspension */
             BBROnSpuriousLoss(bbr_state, path_x, ack_state->lost_packet_number, current_time);
             break;
+        case picoquic_congestion_notification_lost_feedback:
+            /* Feedback has been lost. It will be restored at the next notification. */
+            BBREnterLostFeedback(bbr_state, path_x);
+            break;
         case picoquic_congestion_notification_rtt_measurement:
             /* TODO: this call is subsumed by the acknowledgement notification.
              * Consider removing it from the API once other CC algorithms are updated.  */
             break;
-        case picoquic_congestion_notification_acknowledgement: 
+        case picoquic_congestion_notification_acknowledgement:
+            BBRExitLostFeedback(bbr_state, path_x);
             picoquic_bbr_notify_ack(bbr_state, path_x, ack_state, current_time);
             if (bbr_state->state == picoquic_bbr_alg_startup_long_rtt) {
                 picoquic_update_pacing_data(cnx, path_x, 1);

picoquic/frames.c

@@ -2885,6 +2885,7 @@ void process_decoded_packet_data(picoquic_cnx_t* cnx, picoquic_path_t * path_x,
             ack_state.inflight_prior = packet_data->path_ack[i].inflight_prior;
             ack_state.is_app_limited = packet_data->path_ack[i].rs_is_path_limited;
             ack_state.is_cwnd_limited = packet_data->path_ack[i].rs_is_cwnd_limited;
+            packet_data->path_ack[i].acked_path->is_lost_feedback_notified = 0;
             cnx->congestion_alg->alg_notify(cnx, packet_data->path_ack[i].acked_path,
                 picoquic_congestion_notification_acknowledgement,
                 &ack_state, current_time);

picoquic/picoquic.h

@@ -40,7 +40,7 @@
 extern "C" {
 #endif
 
-#define PICOQUIC_VERSION "1.1.19.10"
+#define PICOQUIC_VERSION "1.1.19.11"
 #define PICOQUIC_ERROR_CLASS 0x400
 #define PICOQUIC_ERROR_DUPLICATE (PICOQUIC_ERROR_CLASS + 1)
 #define PICOQUIC_ERROR_AEAD_CHECK (PICOQUIC_ERROR_CLASS + 3)
@@ -1406,7 +1406,8 @@ typedef enum {
     picoquic_congestion_notification_ecn_ec,
     picoquic_congestion_notification_cwin_blocked,
     picoquic_congestion_notification_seed_cwin,
-    picoquic_congestion_notification_reset
+    picoquic_congestion_notification_reset,
+    picoquic_congestion_notification_lost_feedback /* notification of lost feedback */
 } picoquic_congestion_notification_t;
 
 typedef struct st_picoquic_per_ack_state_t {
@@ -1485,6 +1486,32 @@ void picoquic_set_default_wifi_shadow_rtt(picoquic_quic_t* quic, uint64_t wifi_s
 */
 void picoquic_set_default_bbr_quantum_ratio(picoquic_quic_t* quic, double quantum_ratio);
 
+/* The experimental API `picoquic_set_feedback_loss_notification` allow applications
+* to turn on the "feedback lost" event notification. These events are
+* passed to the congestion control algorithm, allowing it to react
+* quickly to a temporary loss of connectivity, instead of waiting
+* for retransmission timers. Delay sensitive applications use this
+* feature to stop queuing more data when connectivity is lost,
+* and thus avoid the queues of less urgent data to delay
+* arrival of urgent real time frames when connectivity is restored.
+* On the other hand, this feature may lower the performance of
+* applications sending lots of data, and thus should only be
+* used when applications require it.
+* 
+* The lost control events fires if there is more that 2 "ack delay max" between
+* the last ACK received and the next one. In practice, that means 1 RTT + 2 ack delays
+* after the first non acked packet was sent. In contrast, the RTO fires
+* 1 RTT + 4 STDEV + 1 ack delay after the last packet was sent. Given congestion
+* control and CWIN, this "last packet" is typically sent 1 RTT after the "first
+* packet not acknowledged". Thus, the "lost control" event will typically
+* happen 1 RTT before the RTO event.
+* 
+* The `should_notify` should be set 1 to enable the feature, or to 0
+* to stop notifications. It is set by default to zero when a connection
+* is created.
+*/
+void picoquic_set_feedback_loss_notification(picoquic_cnx_t* cnx, unsigned int should_notify);
+
 /* Bandwidth update and congestion control parameters value.
  * Congestion control in picoquic is characterized by three values:
  * - pacing rate, expressed in bytes per second (for example, 10Mbps would be noted as 1250000)

picoquic/picoquic_internal.h

@@ -1079,7 +1079,8 @@ typedef struct st_picoquic_path_t {
     unsigned int is_datagram_ready : 1;
     unsigned int is_pto_required : 1; /* Should send PTO probe */
     unsigned int is_probing_nat : 1; /* When path transmission is scheduled only for NAT probing */
-
+    unsigned int is_lost_feedback_notified : 1; /* Lost feedback has been notified */
+    
     /* Management of retransmissions in a path.
      * The "path_packet" variables are used for the RACK algorithm, per path, to avoid
      * declaring packets lost just because another path is delivering them faster.
@@ -1292,7 +1293,8 @@ typedef struct st_picoquic_cnx_t {
     unsigned int is_datagram_ready : 1; /* Active polling for datagrams */
     unsigned int is_immediate_ack_required : 1; /* Should send an ACK asap */
     unsigned int is_multipath_enabled : 1; /* Unique path ID extension has been negotiated */
-
+    unsigned int is_lost_feedback_notification_required : 1; /* CC algorithm requests lost feedback notification */
+    
     /* PMTUD policy */
     picoquic_pmtud_policy_enum pmtud_policy;
     /* Spin bit policy */

picoquic/quicctx.c

@@ -4782,6 +4782,11 @@ void picoquic_set_default_bbr_quantum_ratio(picoquic_quic_t* quic, double quantu
     quic->bbr_quantum_ratio = quantum_ratio;
 }
 
+void picoquic_set_feedback_loss_notification(picoquic_cnx_t* cnx, unsigned int should_notify)
+{
+    cnx->is_lost_feedback_notification_required = should_notify;
+}
+
 void picoquic_subscribe_pacing_rate_updates(picoquic_cnx_t* cnx, uint64_t decrease_threshold, uint64_t increase_threshold)
 {
     cnx->pacing_decrease_threshold = decrease_threshold;

picoquic/sender.c

@@ -4433,6 +4433,39 @@ static int picoquic_select_next_path(picoquic_cnx_t * cnx, uint64_t current_time
     return path_id;
 }
 
+/* manage the CC timer, if any */
+static int picoquic_check_cc_feedback_timer(picoquic_cnx_t* cnx, uint64_t* next_wake_time, uint64_t current_time)
+{
+    int ret = 0;
+
+    if (cnx->is_lost_feedback_notification_required && cnx->congestion_alg != NULL) {
+        for (int i = 0; i < cnx->nb_paths; i++) {
+            picoquic_path_t* path_x = cnx->path[i];
+            if (!path_x->is_lost_feedback_notified){
+                picoquic_packet_context_t* pkt_ctx = (cnx->is_multipath_enabled)?
+                    &path_x->pkt_ctx:&cnx->pkt_ctx[picoquic_packet_context_application];
+                if (pkt_ctx->pending_first != NULL) {
+                    uint64_t delta_sent = (pkt_ctx->pending_first->send_time <= path_x->last_time_acked_data_frame_sent) ? 0 :
+                        (pkt_ctx->pending_first->send_time - path_x->last_time_acked_data_frame_sent);
+                    uint64_t lost_feedback_time = pkt_ctx->highest_acknowledged_time + delta_sent + 2 * cnx->ack_frequency_delay_local;
+                            
+                    if (lost_feedback_time <= current_time) {
+                        path_x->is_lost_feedback_notified = 1;
+                        cnx->congestion_alg->alg_notify(cnx, path_x,
+                            picoquic_congestion_notification_lost_feedback,
+                            NULL, current_time);
+                    }
+                    else if (lost_feedback_time < *next_wake_time) {
+                        *next_wake_time = lost_feedback_time;
+                        SET_LAST_WAKE(cnx->quic, PICOQUIC_SENDER);
+                    }
+                }
+            }
+        }
+    }
+    return ret;
+}
+
 /* Prepare next packet to send, or nothing.. */
 int picoquic_prepare_packet_ex(picoquic_cnx_t* cnx,
     uint64_t current_time, uint8_t* send_buffer, size_t send_buffer_max, size_t* send_length,
@@ -4458,6 +4491,10 @@ int picoquic_prepare_packet_ex(picoquic_cnx_t* cnx,
 
     ret = picoquic_check_idle_timer(cnx, &next_wake_time, current_time);
 
+    if (ret == 0) {
+        ret = picoquic_check_cc_feedback_timer(cnx, &next_wake_time, current_time);
+    }
+
     if (send_buffer_max < PICOQUIC_ENFORCED_INITIAL_MTU) {
         DBG_PRINTF("Invalid buffer size: %zu", send_buffer_max);
         ret = -1;

picoquic_t/picoquic_t.c

@@ -397,6 +397,7 @@ static const picoquic_test_def_t test_table[] = {
     { "mediatest_video2_back", mediatest_video2_back_test },
     { "mediatest_wifi", mediatest_wifi_test },
     { "mediatest_worst", mediatest_worst_test },
+    { "mediatest_suspension", mediatest_suspension_test },
     { "warptest_video", warptest_video_test },
     { "warptest_video_audio", warptest_video_audio_test },
     { "warptest_video_data_audio", warptest_video_data_audio_test },

picoquictest/mediatest.c

@@ -68,7 +68,8 @@ typedef enum {
     mediatest_worst = 4,
     mediatest_video2_down = 5,
     mediatest_wifi = 6,
-    mediatest_video2_back = 7
+    mediatest_video2_back = 7,
+    mediatest_suspension = 8
 } mediatest_id_enum;
 
 typedef enum {
@@ -1121,6 +1122,7 @@ mediatest_ctx_t * mediatest_configure(int media_test_id,  mediatest_spec_t * spe
             picoquic_tp_t client_parameters;
             mediatest_init_transport_parameters(&client_parameters, 1);
             picoquic_set_transport_parameters(cnx, &client_parameters);
+            picoquic_set_feedback_loss_notification(cnx, 1);
 
             if (picoquic_start_client_cnx(cnx) != 0) {
                 picoquic_delete_cnx(cnx);
@@ -1197,6 +1199,7 @@ int mediatest_one(mediatest_id_enum media_test_id, mediatest_spec_t * spec)
     if (mt_ctx == NULL) {
         ret = -1;
     }
+
     /* Three special cases in which we manipulate the configuration
     * to simulate various downgrade or suspension patterns.
      */
@@ -1210,15 +1213,14 @@ int mediatest_one(mediatest_id_enum media_test_id, mediatest_spec_t * spec)
             ret = mediatest_loop(mt_ctx, 2000000, 1, &is_finished);
         }
     }
-
-    if (media_test_id == mediatest_video2_down ||
+    else if (media_test_id == mediatest_video2_down ||
         media_test_id == mediatest_video2_back) {
         uint64_t picosec_per_byte_ref[2];
         uint64_t latency_ref[2];
         uint64_t down_time = (media_test_id == mediatest_video2_down) ? 4000000 : 2000000;
         uint64_t back_time = (media_test_id == mediatest_video2_down) ? 24000000 : 4000000;
 
-        /* Run the simulation for 2 second. */
+        /* Run the simulation for the first period. */
         ret = mediatest_loop(mt_ctx, down_time, 0, &is_finished);
         /* Drop the bandwidth and increase latency for specified down time */
         for (int i = 0; i < 2; i++) {
@@ -1235,8 +1237,19 @@ int mediatest_one(mediatest_id_enum media_test_id, mediatest_spec_t * spec)
             mt_ctx->link[i]->microsec_latency = latency_ref[i];
         }
     }
-
-    if (media_test_id == mediatest_wifi) {
+    else if (media_test_id == mediatest_suspension) {
+        /* Simulate one single wifi suspension, for 150 ms at
+         * t = 4000000 */
+        uint64_t sim_time = 4000000;
+        /* Run the simulation for the first period. */
+        ret = mediatest_loop(mt_ctx, sim_time, 0, &is_finished);
+        /* Simulate the suspension for 150 ms */
+        sim_time += 150000;
+        for (int i = 0; i < 2; i++) {
+            picoquic_test_simlink_suspend(mt_ctx->link[i], sim_time, 0);
+        }
+    }
+    else if (media_test_id == mediatest_wifi) {
         /* For 10 seconds, run a test loop in which the simulated Wi-Fi
         * link alternates between 100Mbps, low delay and just blocked,
         * 0 bandwidth. The duration of the block unblock phase varies.
@@ -1412,6 +1425,24 @@ int mediatest_worst_test()
 }
 
 int mediatest_wifi_test()
+{
+    int ret;
+    mediatest_spec_t spec = { 0 };
+    spec.ccalgo = picoquic_bbr_algorithm;
+    spec.bandwidth = 0.1;
+    spec.do_video = 1;
+    spec.do_video2 = 1;
+    spec.do_audio = 1;
+    spec.data_size = 0;
+    spec.latency_average = 60000;
+    spec.latency_max = 350000;
+    spec.do_not_check_video2 = 1;
+    ret = mediatest_one(mediatest_wifi, &spec);
+
+    return ret;
+}
+
+int mediatest_suspension_test()
 {
     int ret;
     mediatest_spec_t spec = { 0 };
@@ -1424,7 +1455,7 @@ int mediatest_wifi_test()
     spec.latency_average = 50000;
     spec.latency_max = 300000;
     spec.do_not_check_video2 = 1;
-    ret = mediatest_one(mediatest_wifi, &spec);
+    ret = mediatest_one(mediatest_suspension, &spec);
 
     return ret;
 }