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

Update switch_core_media.c - Patch for #2226 #2623

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

shaunjstokes
Copy link
Contributor

@shaunjstokes shaunjstokes commented Oct 14, 2024

We've found that the patch #2582 has the side effect of causing calls that fail to negotiate the same codec as Leg A for Leg B to fail with SIP 488 INCOMPATIBLE_DESTINATION. That's not an option for us, we need backwards compatibility with other codecs.

Rather than apply this for all codecs it should only apply for Opus. Now calls using Opus and other codecs establish correctly on both legs, and there are no issues with codec negotiation when the codecs on Leg A and Leg B don't match.

@celliso1
Copy link

It appears this PR has revealed a bug in the opus encoder affecting the timestamp increment. The RFC 7587 states opus always increments at 48kHz regardless of the payload sample rate, but mod_opus.c is using 16 kHz or 8kHz samples per packet. See comments in #2226 (comment).

I can't say if this has any other impact, but it does fix the timestamp increment:

diff --git a/src/mod/codecs/mod_opus/mod_opus.c b/src/mod/codecs/mod_opus/mod_opus.c
index 96933e6ea5..5f7972724a 100644
--- a/src/mod/codecs/mod_opus/mod_opus.c
+++ b/src/mod/codecs/mod_opus/mod_opus.c
@@ -1487,7 +1487,7 @@ SWITCH_MODULE_LOAD_FUNCTION(mod_opus_load)
        }

        /* 16 khz */
-       samples = 160;
+       samples = 480; /* Increment RTP timestamp as if 48 kHz */
        bytes = 320;
        mss = 10000;
        rate = 16000;
@@ -1584,7 +1584,7 @@ SWITCH_MODULE_LOAD_FUNCTION(mod_opus_load)
        }

        /* 8 khz */
-       samples = 80;
+       samples = 480; /* Increment RTP timestamp as if 48 kHz */
        bytes = 160;
        mss = 10000;
        rate = 8000;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants