Add packet concealment for opus decoder in audiobridge

[spscream]

I made clear changes for adding of plc to audiobridge, based on https://github.com/meetecho/janus-gateway/pull/3308 and to address issue with crack noises https://github.com/meetecho/janus-gateway/issues/3297

[lminiero]

we're basically ditching the FEC in favor of the PLC

I'd be against that. While it may be true it's useless for bursts > 1, it's still much better than concealment when loss is indeed 1.

[spscream]

we're basically ditching the FEC in favor of the PLC

I'd be against that. While it may be true it's useless for bursts > 1, it's still much better than concealment when loss is indeed 1.

I can add 1 fec packet recovery in case of first MISSING or INSERTION error, but It doesn't make sence too much. In our production environment fec recovery still leads to click sounds for some reason and I removed it completely. We use audiobridge with plc patch in production installs for our customers since last year and we had no issues with clicks at all since then.

[atoppi]

I'm in general in favor of this feature since its current absence is one evidence that make p2p calls slightly better than audiobridge. Nonetheless I agree with @lminiero with the FEC aspect. We should use FEC data when available and then switch to PLC for the rest.

In our production environment fec recovery still leads to click sounds for some reason and I removed it completely.

@spscream That might be due to several reasons:

• Was the sender really attaching fec data to the packets? Some browsers only do this when loss is being noticed
• Was the loss spotty or in bursts? As we already said, FEC will do nothing against bursts
• I'd not exclude bugs in the FEC decoding code, we already patched it a couple of times

[spscream]

I'm in general in favor of this feature since its current absence is one evidence that make p2p calls slightly better than audiobridge. Nonetheless I agree with @lminiero with the FEC aspect. We should use FEC data when available and then switch to PLC for the rest.

In our production environment fec recovery still leads to click sounds for some reason and I removed it completely.

@spscream That might be due to several reasons:

• Was the sender really attaching fec data to the packets? Some browsers only do this when loss is being noticed
• Was the loss spotty or in bursts? As we already said, FEC will do nothing against bursts
• I'd not exclude bugs in the FEC decoding code, we already patched it a couple of times

I'm sure fec were attached to packets, we use only latest chrome and electron clients. Losses were in bursts and spotty.

I'm not sure how to implement fec now, since if first loss occured we generate plc now and output for encoder is going monotonically. If we delay generation of plc until second MISSING or INSERTION error, output of encoders will contain click sound, since input isn't monotonically anymore and empty input on encode will cause click sound.

Is it encoder input has some sort of queue? If so I can try to postpone plc generation on first error until normal packet, if second MISSING or INSERTION error received, I will generate 2 plcs and don't use fec on subsequent JITTER_BUFFER_OK received.

[atoppi]

Code looks good now. Maybe we should add a configuration for a room: either use PLC or FEC. I agree that mixing the two things is not an easy task. Also @spscream mentioning a buffer for the encoder lead me to think about the effectiveness of FEC on "almost" empty queue-in buffers. What happens if we miss a packet, mixer mixes-in an empty spot, then both new packet and previous forward-corrected packet are put into the queue-in participant queue. I guess some audio disruption is the effect (the robotic/metallic voice mentioned here?).

[lminiero]

@spscream FYI, we just merged a couple of important fixes in the AudioBridge, plus the (optional) support for RNNoise that we had open for quite some time. I noticed this unfortunately caused some conflicts in your PR, could you rebase on the latest changes when you can? In the next few days we plan to make some tests of your effort too :v:

[spscream]

@spscream FYI, we just merged a couple of important fixes in the AudioBridge, plus the (optional) support for RNNoise that we had open for quite some time. I noticed this unfortunately caused some conflicts in your PR, could you rebase on the latest changes when you can? In the next few days we plan to make some tests of your effort too ✌️

rebased

[spscream]

@lminiero any updates on this?

[atoppi]

@spscream doing some tests with this PR. I noticed that a check for participant->muted is needed otherwise we'll fill the queue-in buffer with concealed packets when a participant is muted, e.g. adding a check if(!participant->muted)

[atoppi]

@spscream we are adding #3368 to the audiobridge in order to cleanup the buffers in muted/unmuted scenarios. However a patch to this PR is still needed in order to check for participant->muted otherwise a muted participant will trigger PLC until the max gap size is reached.

[atoppi]

I'm adding a "please test" tag since I think this PR is ready to be tested. I'd like to get some feedback from folks using audiobridge and interested in enhancing the perceived audio quality.

We've tested it a bit and did not find such a remarkable difference in respect to the FEC. Nevertheless I personally think this is a better approach overall, primarily because the current opus FEC is limited to a single packet.

[lminiero]

@spscream I wanted to merge this, but I just noticed a conflict in the AudioBridge, probably due to some fixes we made this morning. Could you rebase so that I can finally merge this upstream? Thanks!

[spscream]

@lminiero done, rebased

[lminiero]

Thanks! Merging then :+1: