Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't drop leading packets in FEC reader #186

Open
gavv opened this issue May 16, 2019 · 2 comments
Open

Don't drop leading packets in FEC reader #186

gavv opened this issue May 16, 2019 · 2 comments
Labels
defect Something isn't working

Comments

@gavv
Copy link
Member

gavv commented May 16, 2019

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

This is bad for two reasons:

  • It may unintentionally decrease the latency up to SBL-1 packets, because we first accumulate the desired latency in the queue and than start the FEC reader which drops these packets.

  • It may unintentionally increase the start time if several blocks are missing the first packet.

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:

  • either take FEC block boundaries into account in packet::Delayer, before fec::Reader;
  • or zeroize leading frames (if they are incomplete) in the frame pipeline after fec::Reader and audio::Depacketizer.

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 gavv changed the title FEC: don't drop leading packets in FEC reader Don't drop leading packets in FEC reader May 16, 2019
@dshil dshil self-assigned this May 31, 2019
@gavv gavv modified the milestones: 0.2, 0.1, 0.1.0, 0.1.1 May 31, 2019
@gavv
Copy link
Member Author

gavv commented Jun 6, 2019

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 gavv modified the milestones: 0.1.1, 0.1.2 Jun 17, 2019
@gavv
Copy link
Member Author

gavv commented Jun 21, 2019

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.

@gavv gavv unassigned dshil Jun 27, 2019
@gavv gavv removed this from the 0.1.2 milestone Jun 27, 2019
@gavv gavv added the help wanted An important and awaited task but we have no human resources for it yet label Dec 22, 2022
@gavv gavv removed the help wanted An important and awaited task but we have no human resources for it yet label Sep 21, 2023
@gavv gavv added defect Something isn't working and removed enhancement labels Jun 27, 2024
@gavv gavv added this to Roc Toolkit Jul 6, 2024
@gavv gavv moved this to Backlog in Roc Toolkit Jul 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defect Something isn't working
Projects
Status: Backlog
Development

No branches or pull requests

2 participants