roc-streaming / roc-toolkit

Real-time audio streaming over the network.
https://roc-streaming.org
Mozilla Public License 2.0
1.09k stars 213 forks source link

Don't drop leading packets in FEC reader #186

Open gavv opened 5 years ago

gavv commented 5 years ago

Currently FEC reader drops leading packets until it receives the first packet in a block.

This is bad for two reasons:

We should replace this code in reader:

    if (!pp || pp->fec()->encoding_symbol_id > 0) {
        return source_queue_.read();
    }

with:

    if (!pp) {
        return NULL;
    }

On the other hand, it may make sense to zeroize the first (incomplete) FEC block because it potentially has lower quality of service than other blocks (because FEC decoder has less packets that can be used to restore losses).

However, the right way to do it is one of the following:

Both of this approaches will not affect the latency.

I think we should first fix fec::Reader and then test the new behavior for a while. If we'll find problems with it, we can implement one of these approaches.

gavv commented 5 years ago

or zeroize leading frames (if they are incomplete) in the frame pipeline after fec::Reader and audio::Depacketizer.

After some though, it appears that this is a bad option. An unrecovered loss in the first block does not always lead to incomplete leading frame. First few frames may be complete, and then we can get an incomplete one.

Thus, it seems that we can't zeroize bad first block at the frame level (after Depacketizer) without introducing additional latency. So we need to do it at the packet level (before Depacketizer and likely before fec::Reader).

gavv commented 5 years ago

One idea is to make DelayedReader to drop packets until it meets the block boundary. More exactly, DelayedReader should drop the very first packet until it is either the first packet in a block (esi=0), or it belongs to the next block after the block of the previously dropped packet (cur.sbn > prev.sbn). This way we'll increase the cold latency (session startup time), but won't affect the runtime latency.

The "dropping" part should be done smart enough. We should take into account that when a delayed packet is received, it can change our decision where the block boundary is. Thus we likely should not just blindly drop packets until the session startup; instead, we should somehow mark them as dropped (e.g. store in a separate queue) and exclude from latency calculations; but we should reconsider whether they should be really dropped whenever a delayed packet is received.

This idea require more thinking though. It is based on a simple and incorrect assumption that if we have received the very first non-lost packet in a block, all other packets of that block were not received yet. Obviously this is not always true, depending on how the network may reorder packets. Probably DelayedReader need even smarter approach and should manage some window corresponding to the allowed network jitter.