search

sipwise / rtpengine

The Sipwise media proxy for Kamailio
GNU General Public License v3.0
763 stars 360 forks source link

Some DTMF injection fixes #1809

Closed tombriden closed 3 months ago

tombriden commented 3 months ago

A few scenarios are fixed for me with these changes.

packet_encoded_tx: add a ts delay when transmitting DTMF event packets

This fixes not passing a ts_delay into codec_output_rtp resulting in packets not being scheduled correctly (like already happens in codec_add_dtmf_packet) and an, eg, 100ms event has all packets transmitted in as few as 20ms

dtmf_event_payload: canonicalise DTMF end event ts if start packet send was delayed

in some scenarios DTMF injected with, eg, 100ms duration are transmitted much shorter due to the start time being adjusted but not the end time

dtmf_inject: fix generating one too many event packets

the starting value used to add num_samples to is that of the current packet, which is already increased from the previous packet. The result is a 100ms injection coming through as 6 packets with a total event-duration of 960 dtmf_inject: adjust start_pts if last_event + pause is later than it

codec_last_dtmf_event: return ts of dtmf_state if handler queue is empty dtmf_inject: adjust start_pts if last_event + pause is later than it

DTMF requires an inter-digit pause, which is allowed for with the pause parameter. However, this is only accounted for if an injection request comes in during an insertion, as its the event queue used to return the last event end timestamp. If there's nothing on the queue, we can fall back to the dtmf_state var as that seems to always be the most recent. The second commit can now use this value to calculate a minimum start ts for the injected event to ensure the pause is accounted for. Similar to the previous change, the actual time is increased by one packet's worth of samples to avoid an 100ms pause only having 80ms worth of packets

dtmf: only update recv list if not injected and send list if injected, delayed or not blocked

when the DTMF-security mode is one of the values between PCM_REPLACE_START/END, the event packets are transcoded to PCM DTMF. Injected DTMF gets added to the dtmf_recv list and can cause out of order timestamp in the list, resulting in is_in_dtmf incorrectly returning NULL, resulting in those PCM DTMF frames leaking through. I'm not sure if its needed, but to me it seems logical that maintaining separate lists for recv and send shouldn't result in them being basically identical by always adding all events to it (other than an adjusted ts for dtmf-delay). So have included a change only add to the send list if its injected, unblocked or being delayed. This way each list contains only contains the relevant events.

rfuchs commented 3 months ago

Looks like this is breaking some of the test cases. I'll try to adapt.

rfuchs commented 3 months ago

I've pushed the final rebased series of commits (plus an extra one that was needed) including the updated tests to https://github.com/sipwise/rtpengine/tree/rfuchs/gh1809

Have a look if these look OK to you, then I will merge.

tombriden commented 3 months ago

I've pushed the final rebased series of commits (plus an extra one that was needed) including the updated tests to https://github.com/sipwise/rtpengine/tree/rfuchs/gh1809

Have a look if these look OK to you, then I will merge.

I don't have a scenario that makes use of the extra commit you've added, but it looks sane to me. Tests are passing here now too. Thanks a lot!

rfuchs commented 3 months ago

Everything merged and backported, thanks!