Issue 688: FEC estimates block duration #717

baranovmv · 2024-04-28T17:35:48Z

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

github-actions · 2024-04-28T17:41:33Z

🤖 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.

github-actions · 2024-05-10T19:31:15Z

🤖 The latest upstream change made this pull request unmergeable. Please resolve the merge conflicts.

gavv · 2024-05-12T14:01:32Z

src/internal_modules/roc_audio/latency_monitor.cpp

 bool LatencyMonitor::pre_process_(const Frame& frame) {
+    if (fec_reader_) {
+        latency_metrics_.fec_block_duration =
+            packet_sample_spec_.stream_timestamp_2_ns(fec_reader_->max_block_duration());
+    } else {
+        latency_metrics_.fec_block_duration = 0;
+    }


In LatencyMonitor::read, we have:

compute_niq_latency_(); query_link_meter_(); if (!pre_process_(frame)) { alive_ = false; }

All metrics except fec_block_duration are filled before calling pre-process. So for consistency, I suggest to add a method like query_fec_reader() and also call it there.

gavv · 2024-05-12T14:13:57Z

src/internal_modules/roc_fec/reader.cpp

+    if (next_packet_ == 0) {
+        if (pp) {
+            update_block_duration_(pp);
+        } else {
+            prev_block_timestamp_valid_ = false;
+        }
+    }


So this is triggered when the very first packet of a block is received.

What about moving this logic to next_block_()?

It is triggered when we're going to switch to the next block.

Inside next_block_(), even if the first packet was lost, there is the maximum chance that it was repaired.

gavv · 2024-05-12T14:16:16Z

src/internal_modules/roc_fec/reader.cpp

+    if (!ptr->rtp()) {
+        return;
+    }


Let's use Packet::stream_timestamp() instead of Packet::rtp(), this way fec::Reader will continue to be RTP-independent.

gavv · 2024-05-12T14:18:50Z

src/internal_modules/roc_fec/reader.cpp

+        block_dur = packet::stream_timestamp_diff(ptr->rtp()->stream_timestamp,
+                                                  prev_block_timestamp_);
+    }
+    roc_panic_if_msg(block_dur < 0, "fec reader: negative block duration");


I think panic is not appropriate here, because corrupted/malformed packets would lead to a crash?

Instead we can skip the check. Probably also log to LogTrace.

gavv · 2024-05-12T14:21:05Z

src/internal_modules/roc_fec/reader.cpp

+                                                  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);


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.

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.

Fair enough.

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?

gavv · 2024-05-12T14:23:03Z

src/internal_modules/roc_fec/writer.cpp

@@ -109,6 +114,7 @@ status::StatusCode Writer::write(const packet::PacketPtr& pp) {
    }

    if (cur_packet_ == 0) {
+        update_block_duration_(pp);


I suggest to call this from begin_block_ or end_block_ - so that everything called on block boundary would be in same place.

gavv · 2024-05-12T14:23:14Z

src/internal_modules/roc_fec/writer.cpp

+    if (!ptr->rtp()) {
+        return;
+    }


Same comment about rtp().

gavv · 2024-05-12T14:40:48Z

src/tests/roc_fec/test_writer_reader.cpp

Can we create a separate set of tests for block_duration feature?

This feature feels pretty isolated from the main "purpose" of fec::Reader (repairing packets) and I think the story of its test is a bit different from story of the rest tests.

The story of original tests goes from simpler to more complicated, starting from one block, then introducing a loss, then switching to multiple blocks, introducing more losses, reordering, etc.

The story of block_duration tests I think should include:

good case

lost first packet of first block

lost first packet of non-first block

lost packet inside block (maybe even everything except first packet)

lost whole block (in other words, switched to other block)

resizing block (look at other resize_xxx tests for an example)

Sorry for complicating your life, but fec reader & writer are so complicated that I think having so many tests for every feature is worth it.

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.

baranovmv requested a review from gavv April 28, 2024 17:35

github-actions bot added the ready for review Pull request can be reviewed label Apr 28, 2024

baranovmv changed the title ~~FEC estimates block duration~~ Issue 688: FEC estimates block duration May 6, 2024

github-actions bot added the needs rebase Pull request has conflicts and should be rebased label May 10, 2024

gavv reviewed May 12, 2024

View reviewed changes

gavv added needs revision Pull request should be revised by its author and removed ready for review Pull request can be reviewed labels May 12, 2024

FEC estimates block duration

3f2a94f

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.

baranovmv force-pushed the feature/fec_block_duration branch from 6584217 to 3f2a94f Compare May 26, 2024 09:53

github-actions bot removed the needs rebase Pull request has conflicts and should be rebased label May 26, 2024

Fec Block Duration: refactor

549f677

baranovmv force-pushed the feature/fec_block_duration branch from 0693040 to 549f677 Compare May 29, 2024 16:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 688: FEC estimates block duration #717

Issue 688: FEC estimates block duration #717

baranovmv commented Apr 28, 2024 •

edited

github-actions bot commented Apr 28, 2024

github-actions bot commented May 10, 2024

gavv May 12, 2024

gavv May 12, 2024

gavv May 12, 2024

gavv May 12, 2024

gavv May 12, 2024

baranovmv May 29, 2024

gavv May 29, 2024

gavv May 30, 2024

gavv May 12, 2024

gavv May 12, 2024

gavv May 12, 2024

Issue 688: FEC estimates block duration #717

Are you sure you want to change the base?

Issue 688: FEC estimates block duration #717

Conversation

baranovmv commented Apr 28, 2024 • edited

github-actions bot commented Apr 28, 2024

github-actions bot commented May 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baranovmv commented Apr 28, 2024 •

edited