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

Incorrect access to tcp_info in puffer_ttp.cc #20

Closed meng72 closed 4 years ago

meng72 commented 4 years ago

curr_tcp_info.delivery_rate/cwnd/... is not consistent with client_.tcp_info().value().delivery_rate/cwnd/..., because of the wrong definition as below in puffer_ttp.cc

const auto & curr_tcp_info = client_.tcp_info().value();

NeverlMole commented 4 years ago

Hello ---------- Sorry, we didn't quite get your problem. Did you mean that the value curr_tcp_info.deliveryrate/cwnd/... might not equal to client.tcp_info().value().delivery_rate/cwnd/...?

meng72 commented 4 years ago

Yes.

Actually, in my experiments, I capped the network bandwidth to different values but the video format was always selected as the top one (1920x1080-22), even though there were frequent long rebuffering events happened, especially for the very first chunk. Then I printed out tcp-level information and found curr_tcp_info.deliveryrate/cwnd/... are not the expected tcp-level information (i.e. client.tcp_info().value().delivery_rate/cwnd/...). That's why I think TTP couldn't predict proper transmission time for next video chunks.

After I remove & from const auto & curr_tcp_info = client_.tcp_info().value();, now it seems that puffer could adapt to different network bandwidth.

Please correct me if I am wrong. Thanks!

francisyyan commented 4 years ago

I don't think & is the issue: Each media server of Puffer is single-threaded running a single process, so the execution is sequential and pretty straightforward to reason about. client_.tcp_info().value() returns a struct TCPInfo containing just five integers, and curr_tcp_info simply holds a reference to this struct. Removing & will cause an extra copy of five integers, no big deal, but shouldn't affect the correctness.

Could you please double check the symptom on your side? That is to output, say, curr_tcp_info.cwnd before and after removing the &, without making any other changes. Regardless of this, we definitely want to hear more about why Fugu doesn't work as expected! Although we can't guarantee Fugu working in simulation (re: our research paper), we still want to eliminate mysterious behaviors of it and understand it better.

meng72 commented 4 years ago

Thanks for your advice! Actually, I just pulled the latest Puffer and re-tried it once more. The error is gone now. I agree with what you said that removing & shouldn't make a difference. But I couldn't figure out the reason of the unexpected behavior previously. Anyways, good news that it's not there any more. Thanks again!