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

Change SipPlugin's dtmf pt hardcoded to get from sdp #3299

Merged
merged 3 commits into from
Dec 5, 2023

Conversation

ywmoyue
Copy link
Contributor

@ywmoyue ywmoyue commented Nov 29, 2023

Fix bug raised by @ycherniavskyi from #3280 (comment)

#3280 set a hardcoded payload type of 101 for the telephone-event RTP payload format.

This PR Change dtmf payload hardcoded to get the telephone-event payload value from sdp

Copy link
Member

@lminiero lminiero left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! I've added a couple of notes inline.

As a side note, while doing some unrelated tests today, I noticed that your DTMF detection only seems to be working when using "master" sessions, and not when using "helper" sessions instead (that is, the additional call lines you can associate to the main registration). I didn't have time to investigate yet, but I just thought I'd raise it to your attention, in case you have cycles to debug that too.

src/plugins/janus_sip.c Show resolved Hide resolved
src/plugins/janus_sip.c Outdated Show resolved Hide resolved
@lminiero lminiero added the multistream Related to Janus 1.x label Nov 29, 2023
@ywmoyue
Copy link
Contributor Author

ywmoyue commented Nov 30, 2023

First of all, thank you for notes, and secondly, thank's for the reminder about the 'helper' session. I haven't used 'helper' session before, I will use and test this dtmf detection later.

For the first note,did you mean the janus_sip_media_reset function? I have just modified it. I am not sure whether I need to add the dtmf_pt field to janus_no_sip_media struct.

@lminiero
Copy link
Member

did you mean the janus_sip_media_reset function?

Yes, apologies for the typo!

@lminiero
Copy link
Member

I haven't used 'helper' session before, I will use and test this dtmf detection later.

It may also be a bug in the demos, as that's what I was using to test a call between two users via the SIP plugin and injecting DTMF digits. I'll make some more tests on that later.

@lminiero
Copy link
Member

lminiero commented Dec 4, 2023

It may also be a bug in the demos

That's indeed it: there's a bug when using the DTMF controls from the SIP demo on helper calls. If I manually invoke the DTMF sending methods, DTMF tones are correctly intercepted on the other end, and correctly sent as events on the right helper handle. I'll fix that demo bug separately.

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

lminiero commented Dec 5, 2023

Looks good to me, merging then!

@lminiero lminiero merged commit ba92d97 into meetecho:master Dec 5, 2023
8 checks passed
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants