Refactor DelayedReader startup

[gavv]

DelayedReader is pipeline elements that inserts initial delay in the stream, by accumulating packets until there is enough of them, and only then forwarding them.

LatencyTuner is a class that monitors and adjusts latency on fly.

Currently, they work independently. DelayedReader unconditionally inserts initial delay and then enters "normal" mode (just forwards packets through it). We want to change it, so that LatencyTuner will decide when initial delay is done and tell DelayedReader to finish delay and enter normal mode.

This refactoring would allow us (in future) to control initial delay dynamically (#712, #127).

Steps

• Rework DelayedReader. Remove target_delay parameter and instead add new methods is_started() and start(). Until start() is called, DelayedReader accumulates packets. When it's called, it enters normal mode.

  • Unit tests for DelayedReader should be updated as well.

• Update LatencyTuner. Add new method can_start(). It starts returning true when actual_latency calculated in update_stream() became >= target_latency.

• Update LatencyMonitor. It is a class that glues multiple latency-related classes together. Pass a reference to DelayedReader to LatencyMonitor. In pre_read_(), check if DelayedReader is_started(). If not, ask LatencyTuner if we can_start(). If yes, start() Delay reader.

After this, all tests (except unit tests for DelayedReader) should continue working without modification. roc_pipeline tests cover initial delay, so if they pass, we've done everything right.

[gagankonana]

would like to take this up!

[gavv]

@gagankonana Thanks, welcome!

[gagankonana]

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.

[baranovmv]

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.

[gagankonana]

@baranovmv Awesome, thank you!

[gagankonana]

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_;

}

[baranovmv]

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)

[gagankonana]

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.

[baranovmv]

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

[gagankonana]

Okay, thank you for looking into it!

[gagankonana]

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

[gavv]

@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!

[gagankonana]

@gavv no worries, take care!