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

Add packet concealment for opus decoder in audiobridge #3349

Merged
merged 1 commit into from
Jun 21, 2024

Conversation

spscream
Copy link
Contributor

@spscream spscream commented Apr 2, 2024

I made clear changes for adding of plc to audiobridge, based on #3308 and to address issue with crack noises #3297

@lminiero lminiero added the multistream Related to Janus 1.x label Apr 3, 2024
Copy link
Member

@atoppi atoppi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @spscream for preparing a clean PR for this feature. I've left some notes.

The main point of the discussion here is that we're basically ditching the FEC in favor of the PLC. There a couple of reasons for doing this:

  1. Packet concealment is something browsers already do (and it definitely helps when there is loss), however audiobridge is terminating the RTP, so having a mechanism like this will greatly help to match audio quality experienced in sfu/p2p.
  2. opus FEC is currently limited to a single packet, so it's totally useless in case of bursts of losses. Since bursts are probably the more frequent and disruptive case, PLC might have an advantage over FEC.

src/plugins/janus_audiobridge.c Outdated Show resolved Hide resolved
src/plugins/janus_audiobridge.c Outdated Show resolved Hide resolved
src/plugins/janus_audiobridge.c Show resolved Hide resolved
src/plugins/janus_audiobridge.c Outdated Show resolved Hide resolved
@lminiero
Copy link
Member

lminiero commented Apr 4, 2024

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
Copy link
Contributor Author

spscream commented Apr 4, 2024

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
Copy link
Member

atoppi commented Apr 5, 2024

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
Copy link
Contributor Author

spscream commented Apr 6, 2024

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
Copy link
Member

atoppi commented Apr 8, 2024

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
Copy link
Member

lminiero commented Apr 8, 2024

@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 ✌️

@spscream
Copy link
Contributor Author

spscream commented Apr 9, 2024

@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
Copy link
Contributor Author

spscream commented May 3, 2024

@lminiero any updates on this?

@atoppi
Copy link
Member

atoppi commented May 3, 2024

@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
Copy link
Member

atoppi commented May 6, 2024

@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 atoppi self-requested a review May 14, 2024 09:42
src/plugins/janus_audiobridge.c Outdated Show resolved Hide resolved
@atoppi
Copy link
Member

atoppi commented May 17, 2024

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
Copy link
Member

@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 spscream force-pushed the packets-concealment branch 2 times, most recently from 68eeb81 to 60c907f Compare June 21, 2024 08:02
@spscream
Copy link
Contributor Author

@lminiero done, rebased

@lminiero
Copy link
Member

Thanks! Merging then 👍

@lminiero lminiero merged commit d98a584 into meetecho:master Jun 21, 2024
8 checks passed
@spscream spscream deleted the packets-concealment branch June 21, 2024 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multistream Related to Janus 1.x please-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants