Its in a branch and I've given you svn access. Has all your testing been on linux? I'll compile it sometime today and promote it up to trunk if it doesn't cause anything bad to happen to my mythtv experiment.
All in all, handing true OOO behavior sounds like its worthwhile to take the extra buffer copy that I think this ends up adding. I suspect that my particular LAN case where I've seen dropped packets but not really much by way of OOO will fall into this code path
+ /* Call next packet in case it's just a single dropped packet.
+ * This behavior allows us to handle lossy data connections */
Note that
+ unsigned char packet_window[WINDOW_SIZE][HAVA_BUFFSIZE-16];
could probably be 1470-16 (or maybe 1.5k=1536) instead of HAVA_BUFFSIZE-16 (since video payload packets are bounded in size and HAVA_BUFFSIZE was a dumb guess from a long time ago.)
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Yes, all testing was done on linux. I haven't even tried a Windows compile at this point in time. I also didn't give it a try in a LAN environment as I'm currently using a Windows machine at home, and the linux VM on it just wasn't responsive enough for the video streaming. The code is in use on a linux client machine with a quality setting of 8 (~1.6mbps) and about 60ms of latency to a Hava on a connection with 2mbps of upstream. Because I didn't run it on a LAN, I didn't get a chance to run the code with a higher quality setting to see if the WINDOW_SIZE of 100 is enough, but I imagine it should be. I also didn't experiment with the number of retries, and just blindly re-used your define that's used to timeout on incoming data.
You're right that this code adds in an extry buffer copy. I wasn't even thinking along this lines while implementing so I just wrote it up the first way that came to mind. It could probably be refactored in the future though to eliminate the extra copy and instead store a list of pointers to the next data in the sequence, and pass a different pointer to recvfrom() every call..
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Maybe I'm just missing something in the logic... Can you say a bit more about this particular computation?
consider receiving the following packets (0xfffd and 0x0000 are OOO)
0xfffe,0xffff,0xfffd,0x0001,0x0000
You would hit the seq<hava->last_seq_num 3x.
my first question: I'd think you'd only want to hit it once (when the 0x0001 kicks in) and the diff between the seq and the last seq is bigger than the window size. Maybe I'm wrong though...
my second question: Even then, I'd think the write index computation would include (x10000, seq, and last_seq_num) and not just (x10000 and last_seq_num). something like (x10000-last_seq_num+seq instead of just x10000-last_seq_num).
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Its in a branch and I've given you svn access. Has all your testing been on linux? I'll compile it sometime today and promote it up to trunk if it doesn't cause anything bad to happen to my mythtv experiment.
All in all, handing true OOO behavior sounds like its worthwhile to take the extra buffer copy that I think this ends up adding. I suspect that my particular LAN case where I've seen dropped packets but not really much by way of OOO will fall into this code path
+ /* Call next packet in case it's just a single dropped packet.
+ * This behavior allows us to handle lossy data connections */
Note that
+ unsigned char packet_window[WINDOW_SIZE][HAVA_BUFFSIZE-16];
could probably be 1470-16 (or maybe 1.5k=1536) instead of HAVA_BUFFSIZE-16 (since video payload packets are bounded in size and HAVA_BUFFSIZE was a dumb guess from a long time ago.)
Yes, all testing was done on linux. I haven't even tried a Windows compile at this point in time. I also didn't give it a try in a LAN environment as I'm currently using a Windows machine at home, and the linux VM on it just wasn't responsive enough for the video streaming. The code is in use on a linux client machine with a quality setting of 8 (~1.6mbps) and about 60ms of latency to a Hava on a connection with 2mbps of upstream. Because I didn't run it on a LAN, I didn't get a chance to run the code with a higher quality setting to see if the WINDOW_SIZE of 100 is enough, but I imagine it should be. I also didn't experiment with the number of retries, and just blindly re-used your define that's used to timeout on incoming data.
You're right that this code adds in an extry buffer copy. I wasn't even thinking along this lines while implementing so I just wrote it up the first way that came to mind. It could probably be refactored in the future though to eliminate the extra copy and instead store a list of pointers to the next data in the sequence, and pass a different pointer to recvfrom() every call..
The only thing I'm not really understanding is this block of code:
if (seq < hava->last_seq_num)
{
/* Handle sequence number rollover */
write_index = (unsigned int)0x10000 - hava->last_seq_num;
}
else
{
write_index = seq - hava->last_seq_num;
}
write_index = (write_index + hava->last_write_index) % WINDOW_SIZE;
Maybe I'm just missing something in the logic... Can you say a bit more about this particular computation?
consider receiving the following packets (0xfffd and 0x0000 are OOO)
0xfffe,0xffff,0xfffd,0x0001,0x0000
You would hit the seq<hava->last_seq_num 3x.
my first question: I'd think you'd only want to hit it once (when the 0x0001 kicks in) and the diff between the seq and the last seq is bigger than the window size. Maybe I'm wrong though...
my second question: Even then, I'd think the write index computation would include (x10000, seq, and last_seq_num) and not just (x10000 and last_seq_num). something like (x10000-last_seq_num+seq instead of just x10000-last_seq_num).