Issue 688: FEC estimates block duration #717
Conversation
|
🤖 Upon creation, pull request description does not have a link to an issue. If there is a related issue, please add it to the description using any of the supported formats. |
baranovmv
changed the title
github-actions
bot
added
the
needs rebase
|
🤖 The latest upstream change made this pull request unmergeable. Please resolve the merge conflicts. |
| prev_block_timestamp_); | ||
| } | ||
| roc_panic_if_msg(block_dur < 0, "fec reader: negative block duration"); | ||
| block_max_duration_ = std::max(block_max_duration_, block_dur); |
There was a problem hiding this comment.
Shouldn't we compute maximum in LatencyTuner instead of fec::Reader? And report only immediate value from fec::Reader?
Why:
-
Most likely we want running maximum instead of total maxium. Maybe at least in later versions. But this kind of details (running window, etc) should belong to LatencyTuner?
-
This logic (running max) would probably be the same for sender & receiver.
There was a problem hiding this comment.
With the manner the metrics are passed into tuner, it would be difficult to be sure that it has updates of fec block duration on every new block arrived. And this could potentially lead to wrong estimation of maximum.
There was a problem hiding this comment.
Hm, given it one more thought, this is probably not the case.
While latency tuner updates FE with some big period, like 200ms, latency monitor and feedback monitor are invoked every frame.
And, while user is allowed to read and write big frames, top-level pipeline code will anyway split them in smaller frames (to ensure that we don't block in processing for long time and that frames can fit max size of pool buffer).
So in practice all pipeline components are invoked with small frames (e.g. 1ms), much lower than FEC block size, and I think in our design (in general) we can assume that it's always true, because it simplifies many things.
For example, we already do the same with incoming queue - we're sampling it's size every frame. I think we in general can sample anything per frame.
So I guess you can just sample FEC block duration in LatencyMonitor::read?
There was a problem hiding this comment.
It is not an easy task to reproduce your last reasoning based on source code only, so my concern could potentially be raised during some debug session and it would be difficult to confirm or deny, so I'd rather keep it as it is
gavv
added
needs revision
baranovmv
force-pushed
the
feature/fec_block_duration
branch
from
6584217 to
3f2a94f
Compare
github-actions
bot
removed
the
needs rebase
baranovmv
force-pushed
the
feature/fec_block_duration
branch
from
0693040 to
549f677
Compare
baranovmv
force-pushed
the
feature/fec_block_duration
branch
3 times, most recently
from
d1b901b to
4a4b3e9
Compare
github-actions
bot
added
ready for review
baranovmv
force-pushed
the
feature/fec_block_duration
branch
from
4a4b3e9 to
8c88f61
Compare
gavv
force-pushed
the
feature/fec_block_duration
branch
from
8c88f61 to
0f4b187
Compare
|
Thanks!! Follow-up: 5fedaf4 |
Reader and writer esimate duration of
FEC block in ns as this value could vary
even though number of packets is known.
For now it is decided to keep the greates
value seen in the current session, not
average.
#688