search

StanfordSNR / puffer

Puffer is a free live TV streaming website and a research study at Stanford using machine learning to improve video streaming
https://puffer.stanford.edu
835 stars 131 forks source link

Bug with getting tcp_info #29

Closed ofirbarak closed 2 years ago

ofirbarak commented 2 years ago

https://github.com/StanfordSNR/puffer/blob/b1ccbe8655bfdcbe040110d4a72e9758e9a30dad/src/abr/puffer_ttp.cc#L135

I think you need to remove the &. I don't know why but with the reference, the values of the curr_tcp_info are just changing every time you touching raw_input and curr_tcp_info.

francisyyan commented 2 years ago

Hi Ofir -- How did you conclude that the values in current_tcp_info changed unexpectedly?

In the function reinit_sending_time where the issue was opened, client_.tcp_info() will not get updated, so there will be no difference to grab a (const) reference or make a copy. To verify this, feel free to print out curr_tcp_info three times in a row anywhere in that function but make sure you are printing for the same client_.

Additionally, before every video chunk is served to a client, the tcp_info of the client will be updated with new values from the kernel, so when Line 135 is executed again next time, the tcp_info will probably have different values.

ofirbarak commented 2 years ago

I know it sound strange but this is exactly what I tried: If I'm printing right after declaration, 3 times in a row, I'm getting good results (I've printed all its properties - delivery_rate, cwnd, in_flight, min_rtt, rtt): 

0,sdsdfsdf: Firefox on Linux, 127.0.0.1:44938
0,sdsdfsdf: connection initialized
0,sdsdfsdf: channel nbc, audio 0 32k
936228571 11 0 17 19
936228571 11 0 17 19
936228571 11 0 17 19

But if I'm printing right after declaration and after the if-block (line 176), curr_tcp_info changed: For referenced curr_tcp_info (original code):

0,sdsdfsdf: Firefox on Linux, 127.0.0.1:44774
0,sdsdfsdf: connection initialized
0,sdsdfsdf: channel nbc, audio 0 32k
1129931034 11 0 18 19
1129931034 616078304 21854 18 19
0,sdsdfsdf: channel nbc, video 0 1920x1080-22 0.987653

4365533333 18 0 5 570
93862818819104 603531808 21854 603531808 21854
0,sdsdfsdf: channel nbc, video 180180 1920x1080-24 0.987683

But For copied curr_tcp_info:

0,sdsdfsdf: Firefox on Linux, 127.0.0.1:44510
0,sdsdfsdf: connection initialized
0,sdsdfsdf: channel nbc, audio 0 32k
1057032258 12 0 24 25
1057032258 12 0 24 25
0,sdsdfsdf: channel nbc, video 0 1920x1080-22 0.987653

4092687500 10 0 12 113
4092687500 10 0 12 113
0,sdsdfsdf: channel nbc, video 180180 1920x1080-24 0.987683
francisyyan commented 2 years ago

Sorry for the delay! I finally got a chance to test it and decided to remove the ampersand for the following reason.

With the ampersand, capturing client_.tcp_info().value(); by reference might lead to undefined behavior depending on the compiler. I am not sure if the lifetime of the returned optional object will be guaranteed to extend, so I decided to play it safe and capture by value instead, as you suggested.

Thanks a lot for catching that!