would like to take this up!

@gagankonana Thanks, welcome!

Hey @gavv, I have made some updates relating to the refactoring but I am not able to figure out how to run or test the changes locally.

Can you please point me in the direction of documentation on how to build and run tests? I am new to the repo and trying to understand how to set up the project locally and run tests.

Can you please point me in the direction of documentation on how to build and run tests?

Hi, thank you for your PR

You can start from this page and then all others are also nice to observe.

This line works for me:

 scons -Q --build-3rdparty=openfec,speexdsp,cpputest --enable-werror --enable-tests test

It builds everything and runs tests.

As @gavv has written, all the tests should pass except those for DelayReader itself (which could be found here: src/tests/roc_packet/test_delayed_reader.cpp) -- you can adjust them so that they align with your changes. Pipeline tests are testing this functionality, which implementation you are modifying, so they are point of your interest, and they could be found here: src/tests/roc_pipeline/. E.g. in src/tests/roc_pipeline/test_loopback_sink_2_source.cpp this line is responsible to postpone the checks for initial delay.

@baranovmv Awesome, thank you!

Hi @baranovmv, I was able to run the tests with your instructions but some of the tests are failing. I think the way I have implemented can_start() is wrong. Below is my implementation which I think is wrong. Can you please help me understand how we should be calculating actual latency to compare it with target_latency here?

bool LatencyTuner::can_start() const {
    roc_panic_if(!is_valid());

    packet::stream_timestamp_diff_t latency = 0;
    switch (backend_) {
    case audio::LatencyTunerBackend_Niq:
        latency = niq_latency_;
        break;

    case audio::LatencyTunerBackend_E2e:
        latency = e2e_latency_;
        break;

    default:
        break;
    }
    return latency >= target_latency_;

}

Hi @gagankonana That's hard to say, could you please point me to a valid commit I could experiment with myself? Your recent master branch refuses to be built (f91e74)

HI @baranovmv, sorry about that. I did not have the changes pushed up. Just pushed the updates to my master. I think my implementation of can_start() is wrong.

Haha, the issue is a bit complicated, so the changes should be more involving. In short, LatencyMonitor stays at the very end of the pipeline, and it stats getting niq_latency when at least one packet passes through Depacketizer, but here, in order to enable this feature, we need some value like the_first_received_pkt_ts - the_last_received_pkt_ts which is different to what LatencyMonitor calculates. So I suppose @gavv could reformulate this task a bit so that you (we) could proceed

Okay, thank you for looking into it!

Hi @gavv, I was wondering if you were able to look into it and give me more info on how to calculate the actual delay

@gagankonana I'm sorry, I'm being sick last weeks, but I keep in mind this PR and will take a look as soon as I'll recover. Thanks for the patch!

@gavv no worries, take care!

Hi, sorry again for delay, and thank a lot you both for looking into it.

So, LatencyMonitor needs a way to calculate NIQ latency before depacketizer is started.

An obvious solution would be to get the very first packet written to incoming queue (we currently don't have method for that), as well as the very last (returned by latest() method), and calculate the difference.

But the problem is that fec::BlockReader usually drops leading packets until it meets first packet in FEC block (i.e. packet with ESI=0). In result, we'll always start too early - we'll see that incoming queue have enough length and start, but then FEC reader will drop part of the packets.

I found an old task related to this behavior: gh-186. It discusses various possible approaches. Originally I was trying to avoid dropping packets at all, but so far I don't see any good way to do that.

Suggested solution

However, we can do a simpler change that will fix problems both for that task and for the current one:

1. Move waiting for the first packet in FEC block from fec::BlockReader to DelayedReader. If FEC is enabled, DelayedReader will drop incoming packets until it meets packet with ESI=0.

2. Add has_first() and first_timestamp() methods to DelayedReader. They report whether we've got the first non-dropped packet, and its timestamp.

3. In LatencyMonitor, if Depacketizer is not started yet, then we should wait until DelayedReader gets first packet, and then use its first_timestamp() instead of Depacketizer's next_timestamp() to calculate NIQ latency.

This will not solve increased "cold" latency (i.e. waiting time before we meet FEC block boundary), but this will solve problems with incorrect start time. Cold latency is not so important.

Implementation notes

About FEC blocks, ESI, etc: https://roc-streaming.org/toolkit/docs/internals/fec.html

Currently waiting for FEC block boundary is implemented in BlockReader::try_start_(). It's not necessary to remove that feature from there. If DelayedReader will do the dropping, the dropping code in FEC reader just won't be executed.

Note that DelayedReader should take into account possible packet reordering when waiting for the block boundary. The algorithm can be roughly the following:

• Until we see packet with ESI=0, we accumulate all packets to queue.
• While doing this, we also track current FEC block number.
• If FEC block number increases, we should remove all packets from previous block from the queue, to release unneeded packets to memory pool.
• When we meet packet with ESI=0, we put it to the queue, and stop dropping packets. But we continue collecting packets in the queue until LatencyMonitor tells us to start.

If FEC is disabled, DelayedReader should set has_first() to true as soon as it gets a packet first time. In this case, first_timestamp() should return timestamp of the oldest packet. Note that due to packet reordering, this timestamp can change until DelayedReader is started (e.g. if 1st packet is delayed and is arrived after the 2nd).