Fix big TWCC time deltas #524
Conversation
src/rtp/rtcp/twcc.rs
Outdated
| let Ok(diff_time) = diff_time else { | ||
| // This time diff was so large we can't even reliably represent it. Treat the same | ||
| // as the more sane out-of-range case below. | ||
| break; |
There was a problem hiding this comment.
Duration::as_micros() gives us as 128 bit number. I think it is cleaner to cast to an i64 and rely on the rule below rather than introducing this Result.
Another idea is to keep the i128, however I have a hunch it's faster to cast to something that has native support on the CPU. In fact, that's probably why we are here to start off with.
There was a problem hiding this comment.
It's not clear to me that an i64 would solve the issue - my impression is that it would just push the problematic range further out. It would be great if we could just as i128 and be done with it, but since as_micros() specifically returns u128 there's a danger zone around u128::MAX.
Of course, as we go from i32 to i64 to i128, the problematic cases get more and more unrealistic 😅 But despite the cleanliness hit, if nothing else I think there's value in the explicitness, signaling to readers that we've thought about this case.
There was a problem hiding this comment.
As far as performance concerns of an explicitly checked cast, I'm confident that this is an easy case for a branch predictor, making it basically free.
There was a problem hiding this comment.
the problematic cases get more and more unrealistic 😅
This is the point, yes. Microseconds represented as 32 bit will handle a duration of ~4300 seconds or ~70 minutes. Having such a delta in this context (twcc) would be madness, but let's assume it can happen if you sleep the laptop or something. For 64 bit we get a duration of ~580 thousand years, at which point the battery of my laptop will be drained.
As far as performance concerns of an explicitly checked cast
I wasn't thinking about the performance of your solve, but rather about casting to 64 bit and then perform those divisions. Dividing 128 bit on hardware without 128-bit support will be more expensive, hence I propose to cast to 64.
There was a problem hiding this comment.
Okay then, I'll change the cast to 64.
…er wraparound issues
OxleyS
force-pushed
the
fix-big-twcc-deltas
branch
from
f4f5c35
to
2b28a22
Compare
|
I've rewritten the second commit to just promote the integer width to |
|
Thanks! |
Resolves #523.
While working on this, I found another issue when you have deltas that are big enough to overflow when cast down to an
i32. This wasn't panicking, but would nevertheless cause strange results. The second commit contains a fix for this as well.