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

Fix opus #23

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix opus #23

wants to merge 2 commits into from

Conversation

anzz1
Copy link

@anzz1 anzz1 commented Feb 8, 2025

  • implement DTX correctly
  • fix frame size
  • fix packet loss

Sending 0 sized payload is not how DTX is implemented in Opus
Instead opus_encode can return 1 for maximum of MAX_CONSECUTIVE_DTX frames and it means to transmit nothing
After 10 frames not sent, the 11th frame is not empty but encoded silence frame. Then 10 frames not sent again.

ref: https://www.opus-codec.org/docs/html_api/group__opusencoder.html
If the return value is 1 byte, then the packet does not need to be transmitted (DTX).

Receiving end use opus_decode with packet loss concealment generating silence with (0, 0) parameters like already implemented, as the DTX packets are "lost".
So MAX_PACKET_LOSS must align with MAX_CONSECUTIVE_DTX

Also Opus Compress function was non-functioning, psRead needs to be advanced for FRAME_SIZE*2 when you read FRAME_SIZE*2, not MAX_FRAME_SIZE. But it currently isn't used.

Open Questions

There is currently a strange situation with codecs which doesn't really make sense
Currently only two types of client are detected, "speex" and "opus". Not silk, but its still used for compression but not decompression, for which opus is used.
"opus" clients are determined by sv_version cvar engine build >4554 (which also could be silk clients?).

Check the following chart

sender: "vct_speex"
-> for "vct_opus" clients transcoded to Silk -> can be heard by as long as support Silk
->passed as-is to "vct_speex" clients.

sender: "vct_silk"
-> for "opus" gets passed as-is -> can be heard by "vct_opus" client as long as support Silk
-> not transcoded, can't be heard by "vct_speex" clients.

sender: "vct_opus"
-> for "vct_opus" gets passed as-is -> can be heard by "vct_opus" client as long as support Opus_PLC
-> transcoded to Speex for "speex" clients.

So if aim was to use Silk for compatibility purposes, it doesn't quite hit the mark right now.
Because Silk clients are not detected (or detected as "opus"), nor are their packets transcoded to Speex.
Also I don't know if such client version exist with only Opus_PLC support, but those would only be able to speak to Speex/Opus, but only hear other Opus due to their packets received properly as Opus_PLC but when packets are sent to them, Silk is used.

sender →
receiver ↓
speex silk opus_plc
speex
silk
opus_plc

Again at least up-to-date Steam clients support both silk and opus_plc so it's not a problem for those clients, but it still doesn't make sense.
I think for maximum compatibility you should either always compress to Silk, which can be heard by all clients supporting Silk.
But if Silk is not to be used, like currently their speech is skipped for Speex clients, shouldn't the compression use opus_PLC like the decompression does?

@SergeyShorokhov SergeyShorokhov added Type: 🐞 bug An error that needs fixing. Status: 👀 needs review Requires code or changes review. Priority: 🕒 low Low priority tasks that can be postponed for the future. Difficulty: 🟡 medium Task of medium difficulty requiring moderate effort. labels Feb 9, 2025
@anzz1 anzz1 force-pushed the patch-1 branch 2 times, most recently from 276d483 to 0360f9f Compare February 9, 2025 19:40
- implement DTX correctly
- fix frame size
- fix packet loss
@anzz1
Copy link
Author

anzz1 commented Feb 9, 2025

I realize now that the wrong implementation of DTX is reverse-engineered from steam's, so it's their wrong implementation.
It does "work" meaning Opus doesn't spaz out because of it, when opus_encode returns 1 and you're not supposed to transmit it, it does write a single 0x48 byte to the buffer, which when received by opus_decode gets decoded as frame of silence.
It does render the whole point of DTX moot though.

The decoder does implement PLC correctly though, so when the DTX is corrected on the sender's side, even though it doesn't match steam's, it does work correctly.

@stamepicmorg stamepicmorg added OS: 💻 Independent Case do not refer to any OS. Engine: 📀 reHLDS Case refers to reHLDS engine. labels Feb 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Difficulty: 🟡 medium Task of medium difficulty requiring moderate effort. Engine: 📀 reHLDS Case refers to reHLDS engine. OS: 💻 Independent Case do not refer to any OS. Priority: 🕒 low Low priority tasks that can be postponed for the future. Status: 👀 needs review Requires code or changes review. Type: 🐞 bug An error that needs fixing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants