From 6962e73e20a699ae3b09577b6a786700a94a557f Mon Sep 17 00:00:00 2001 From: Christian Huitema Date: Fri, 12 Apr 2024 21:17:37 -0700 Subject: [PATCH 1/3] Fix BBR probe UP and Probe RTT --- UnitTest1/unittest1.cpp | 6 ++++++ picoquic/bbr.c | 8 ++++++-- picoquic_t/picoquic_t.c | 1 + picoquictest/mediatest.c | 38 +++++++++++++++++++++++++++++-------- picoquictest/picoquictest.h | 1 + 5 files changed, 44 insertions(+), 10 deletions(-) diff --git a/UnitTest1/unittest1.cpp b/UnitTest1/unittest1.cpp index 26e8f6779..a09d2f5cb 100644 --- a/UnitTest1/unittest1.cpp +++ b/UnitTest1/unittest1.cpp @@ -2255,6 +2255,12 @@ namespace UnitTest1 Assert::AreEqual(ret, 0); } + TEST_METHOD(mediatest_video2_back) { + int ret = mediatest_video2_back_test(); + + Assert::AreEqual(ret, 0); + } + TEST_METHOD(mediatest_wifi) { int ret = mediatest_wifi_test(); diff --git a/picoquic/bbr.c b/picoquic/bbr.c index 656e68523..d76ab22f5 100644 --- a/picoquic/bbr.c +++ b/picoquic/bbr.c @@ -1251,6 +1251,7 @@ static void BBRUpdateMinRTT(picoquic_bbr_state_t* bbr_state, picoquic_path_t* pa static void BBRExitProbeRTT(picoquic_bbr_state_t* bbr_state, picoquic_path_t * path_x, uint64_t current_time) { BBRResetLowerBounds(bbr_state); + path_x->rtt_min = bbr_state->min_rtt; if (bbr_state->filled_pipe) { BBRStartProbeBW_DOWN(bbr_state, path_x, current_time); BBRStartProbeBW_CRUISE(bbr_state); @@ -1303,7 +1304,6 @@ static void BBRHandleProbeRTT(picoquic_bbr_state_t* bbr_state, picoquic_path_t * } } - static void BBREnterProbeRTT(picoquic_bbr_state_t* bbr_state) { bbr_state->state = picoquic_bbr_alg_probe_rtt; @@ -1317,6 +1317,7 @@ static void BBRCheckProbeRTT(picoquic_bbr_state_t* bbr_state, picoquic_path_t * bbr_state->probe_rtt_expired && !bbr_state->idle_restart) { BBREnterProbeRTT(bbr_state); + bbr_state->min_rtt = rs->rtt_sample; bbr_state->prior_cwnd = BBRSaveCwnd(bbr_state, path_x); bbr_state->probe_rtt_done_stamp = 0; bbr_state->ack_phase = picoquic_bbr_acks_probe_stopping; @@ -1579,6 +1580,7 @@ static void BBRStartProbeBW_DOWN(picoquic_bbr_state_t* bbr_state, picoquic_path_ bbr_state->ack_phase = picoquic_bbr_acks_probe_stopping; BBRStartRound(bbr_state, path_x); bbr_state->state = picoquic_bbr_alg_probe_bw_down; + bbr_state->nb_rtt_excess = 0; } static void BBRStartProbeBW_CRUISE(picoquic_bbr_state_t* bbr_state) @@ -1603,6 +1605,7 @@ static void BBRStartProbeBW_REFILL(picoquic_bbr_state_t* bbr_state, picoquic_pat static void BBRStartProbeBW_UP(picoquic_bbr_state_t* bbr_state, picoquic_path_t * path_x, uint64_t current_time) { + bbr_state->nb_rtt_excess = 0; bbr_state->pacing_gain = BBRProbeBwUpPacingGain; /* pace at rate */ bbr_state->cwnd_gain = BBRProbeBwUpCwndGain; /* maintain cwnd */ bbr_state->ack_phase = picoquic_bbr_acks_probe_starting; @@ -1669,7 +1672,8 @@ static void BBRUpdateProbeBWCyclePhase(picoquic_bbr_state_t* bbr_state, picoquic case picoquic_bbr_alg_probe_bw_up: if (BBRHasElapsedInPhase(bbr_state, bbr_state->min_rtt, current_time) && - path_x->bytes_in_transit > BBRInflightWithBw(bbr_state, path_x, 1.25, bbr_state->max_bw)) { + (bbr_state->nb_rtt_excess > 0 || + path_x->bytes_in_transit > BBRInflightWithBw(bbr_state, path_x, 1.25, bbr_state->max_bw))) { BBRStartProbeBW_DOWN(bbr_state, path_x, current_time); } break; diff --git a/picoquic_t/picoquic_t.c b/picoquic_t/picoquic_t.c index 5cdecfe16..7dbc16301 100644 --- a/picoquic_t/picoquic_t.c +++ b/picoquic_t/picoquic_t.c @@ -394,6 +394,7 @@ static const picoquic_test_def_t test_table[] = { { "mediatest_video_audio", mediatest_video_audio_test }, { "mediatest_video_data_audio", mediatest_video_data_audio_test }, { "mediatest_video2_down", mediatest_video2_down_test }, + { "mediatest_video2_back", mediatest_video2_back_test }, { "mediatest_wifi", mediatest_wifi_test }, { "mediatest_worst", mediatest_worst_test }, { "warptest_video", warptest_video_test }, diff --git a/picoquictest/mediatest.c b/picoquictest/mediatest.c index ea31b9ca6..991e41439 100644 --- a/picoquictest/mediatest.c +++ b/picoquictest/mediatest.c @@ -67,7 +67,8 @@ typedef enum { mediatest_video_data_audio = 3, mediatest_worst = 4, mediatest_video2_down = 5, - mediatest_wifi = 6 + mediatest_wifi = 6, + mediatest_video2_back = 7 } mediatest_id_enum; typedef enum { @@ -1154,7 +1155,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 manipualte the configuration + /* Three special cases in which we manipulate the configuration * to simulate various downgrade or suspension patterns. */ if (media_test_id == mediatest_worst) { @@ -1168,20 +1169,23 @@ int mediatest_one(mediatest_id_enum media_test_id, mediatest_spec_t * spec) } } - if (media_test_id == mediatest_video2_down) { + 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. */ - ret = mediatest_loop(mt_ctx, 2000000, 0, &is_finished); - /* Drop the bandwidth and increase latency for 4 seconds */ + 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++) { picosec_per_byte_ref[i] = mt_ctx->link[i]->picosec_per_byte; mt_ctx->link[i]->picosec_per_byte = 8000000; /* 8 us per byte, i.e., 1Mbps*/ latency_ref[i] = mt_ctx->link[i]->microsec_latency; } if (ret == 0) { - ret = mediatest_loop(mt_ctx, 6000000, 0, &is_finished); + ret = mediatest_loop(mt_ctx, back_time, 0, &is_finished); } /* restore the bandwidth */ for (int i = 0; i < 2; i++) { @@ -1316,14 +1320,32 @@ int mediatest_video2_down_test() spec.do_video2 = 1; spec.do_audio = 1; spec.data_size = 0; - spec.latency_average = 110000; - spec.latency_max = 600000; + spec.latency_average = 100000; + spec.latency_max = 450000; spec.do_not_check_video2 = 1; ret = mediatest_one(mediatest_video2_down, &spec); return ret; } +int mediatest_video2_back_test() +{ + int ret; + mediatest_spec_t spec = { 0 }; + spec.ccalgo = picoquic_bbr_algorithm; + spec.bandwidth = 0.01; + spec.do_video = 1; + spec.do_video2 = 1; + spec.do_audio = 1; + spec.data_size = 0; + spec.latency_average = 90000; + spec.latency_max = 500000; + spec.do_not_check_video2 = 1; + ret = mediatest_one(mediatest_video2_back, &spec); + + return ret; +} + int mediatest_worst_test() { int ret; diff --git a/picoquictest/picoquictest.h b/picoquictest/picoquictest.h index 2e1f3b18f..1e0d57aa4 100644 --- a/picoquictest/picoquictest.h +++ b/picoquictest/picoquictest.h @@ -388,6 +388,7 @@ int mediatest_video_test(); int mediatest_video_audio_test(); int mediatest_video_data_audio_test(); int mediatest_video2_down_test(); +int mediatest_video2_back_test(); int mediatest_wifi_test(); int mediatest_worst_test(); int warptest_video_test(); From ee84befba57bdf8a5a4cc7726561e101ddf3fa8c Mon Sep 17 00:00:00 2001 From: Christian Huitema Date: Sat, 13 Apr 2024 15:40:57 -0700 Subject: [PATCH 2/3] Try understand the asan-ubsan error --- picoquictest/mediatest.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/picoquictest/mediatest.c b/picoquictest/mediatest.c index 991e41439..fc07a0a17 100644 --- a/picoquictest/mediatest.c +++ b/picoquictest/mediatest.c @@ -348,14 +348,20 @@ int mediatest_check_stats(mediatest_ctx_t* mt_ctx, mediatest_spec_t * spec, medi if (spec->latency_average == 0) { if (average > 25000 || sigma > 12500 || stats->max_delay > 100000) { + DBG_PRINTF("Latency average: %" PRIu64 ", sigma: %" PRIu64 ", max: %" PRIu64, + average, sigma, stats->max_delay); ret = -1; } } else { if (average > spec->latency_average) { + DBG_PRINTF("Average latency expected: %" PRIu64 ", got %" PRIu64, + spec->latency_average, average); ret = -1; } else if (spec->latency_max > 0 && stats->max_delay > spec->latency_max) { + DBG_PRINTF("Max latency expected: %" PRIu64 ", got %" PRIu64, + spec->latency_max, stats->max_delay); ret = -1; } } @@ -1320,8 +1326,8 @@ int mediatest_video2_down_test() spec.do_video2 = 1; spec.do_audio = 1; spec.data_size = 0; - spec.latency_average = 100000; - spec.latency_max = 450000; + spec.latency_average = 110000; + spec.latency_max = 600000; spec.do_not_check_video2 = 1; ret = mediatest_one(mediatest_video2_down, &spec); From f6eb300ce8fd4ff612a8845f25bdb9d141508a43 Mon Sep 17 00:00:00 2001 From: Christian Huitema Date: Sat, 13 Apr 2024 15:48:44 -0700 Subject: [PATCH 3/3] Account for variability in video_back test --- picoquictest/mediatest.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/picoquictest/mediatest.c b/picoquictest/mediatest.c index fc07a0a17..1d9c3a908 100644 --- a/picoquictest/mediatest.c +++ b/picoquictest/mediatest.c @@ -1344,8 +1344,8 @@ int mediatest_video2_back_test() spec.do_video2 = 1; spec.do_audio = 1; spec.data_size = 0; - spec.latency_average = 90000; - spec.latency_max = 500000; + spec.latency_average = 110000; + spec.latency_max = 600000; spec.do_not_check_video2 = 1; ret = mediatest_one(mediatest_video2_back, &spec);