Some DTMF injection fixes #1809
Conversation
|
Looks like this is breaking some of the test cases. I'll try to adapt. |
daemon/dtmf.c
Outdated
| // get the last event end time, and increase by the required pause | ||
| // conversely to the above, we need to add the last packet num samples to its TS before adding | ||
| // a pause so we dont generate one packet too few | ||
| // if that's later than start_pts, we need to adjust it | ||
| uint64_t last_end_pts = codec_last_dtmf_event(csh) + ((pause + ch->dest_pt.ptime) * ch->dest_pt.clock_rate / 1000); | ||
| if (last_end_pts > start_pts) | ||
| start_pts = last_end_pts; | ||
|
|
There was a problem hiding this comment.
Are you sure this is correct? This seems to add the pause even if this is the first DTMF event. Is this intended?
There was a problem hiding this comment.
ah, good spot. have fixed that to only adjust last_end_pts if it isn't 0
tombriden
force-pushed
the
dtmf-injection-fixes
branch
from
0cafa6f
to
6b43180
Compare
|
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! |
daemon/codec.c
Outdated
| struct telephone_event_payload *ev_pt = (void *) inout->s; | ||
| ts_delay = ntohs(ev_pt->duration) - (ch->handler->dest_pt.ptime * ch->handler->dest_pt.clock_rate / 1000); | ||
| if (is_dtmf == 3) | ||
| repeats = 2; // DTMF end event | ||
| } |
There was a problem hiding this comment.
Do you think it would make sense to always do this TS adjustment, and not just for non-start event packets? I assume in your case the delay probably comes out as zero for the initial packet, but it's possible the first packet (triggering the start event) uses a different duration.
There was a problem hiding this comment.
I don't think so. the first event packet has had its ts increased from the previous packet already so it gets scheduled correctly; in the pcaps I made during testing, the first packet always looked right (~20ms from the previous non-event packet). Then, because the ts_delay is applied on top of that first event packet TS, this adjustment has to be one ptime less than the current event duration so that they don't end up being scheduled starting 40ms after the first event.
There was a problem hiding this comment.
I don't think so. the first event packet has had its ts increased from the previous packet already so it gets scheduled correctly; in the pcaps I made during testing, the first packet always looked right (~20ms from the previous non-event packet). Then, because the
ts_delayis applied on top of that first event packet TS, this adjustment has to be one ptime less than the current event duration so that they don't end up being scheduled starting 40ms after the first event.
AFAICT the first event packet usually has a duration of 20 ms or 160 samples, so that would come out as a delay of 160 - 20 * 8 = 0. But what if that packet got lost, and the packet triggering the start event (i.e. the state change) is actually the second one, using a duration of 40 ms or 320 samples?
There was a problem hiding this comment.
oh i see what you mean. In which case that second-now-first event packet wouldn't be scheduled correctly as it should have had a ts_delay of 160 but ended up with 0. Yep, makes sense. you want me to amend that from your branch and repush here?
There was a problem hiding this comment.
i gone ahead and done that anyway :)
…nt packets when DTMF is being transmitted using codec_output_rtp, the scheduled time to send the packet is calculated using the timestamp difference from the last transmitted applied on top of the realtime time the last packet was sent. However, the timestamp is only updated on the first event packet so a delay needs to be passed in to codec_output_rtp to ensure the subsequent packets are scheduled based on the event duration of the event packets Change-Id: I5a2f6cf67b5f570f6099d201592d9a6fc01d60a5
…packet send was delayed in some scenarios the start event ts can be before the *pts value, which will result in a shortened DTMF event being transmitted than expected during injection as the end event ts is calculated based on that initial dtmf start value. This change updates the end event ts by the amount the start ts was behind, so that the resulting event has the right duration Change-Id: Ia637d1e1c5d92de8b35317ec552c22eae23c0645
Change-Id: Ia16ffefc0791d01575248ac5d8025eb30ccaec67
the num_samples was added to the start_pts, which is the first event packet timestamp, which has already increased its ts by its event duration. so, the total duration of events ends up being one packet more than intended. Change-Id: I423bb222a81c5bd78e570ff2026c72dd4dd1b100
…eue is empty this function is used to determine if a pause is needed on a new injected DTMF's start ts to ensure a gap between the events. However, if an inject request comes in after the end of the previous event but before it would have been offset due to pause, no pause is added This change returns the ts value from dtmf_state if the queue is empty as that will always be the ts of the last DTMF transmitted Change-Id: I4f3cf5115d1a8e26c0ca1bc9570c46e29391e0d0
… than it now that the returned last_event_ts is always that of the previous DTMF, we can ensure that the next one isn't transmitted until that time plus the required pause. Like the num_samples calculation, the actual time needs to be increased by 1 packets worth of samples so tha pause lasts the full duration required Change-Id: I6da1dd7cbcf49f7f0431a5123df2cdc382fe3dba
… injected, delayed or not blocked having injected events on the recv list can cause out of order TS values which results in is_in_dtmf incorrectly returning NULL and letting the transcoded PCM frames through. It also doesn't make sense to add DTMF to the send list unless they're actually being sent, so injected delayed or unblocked Change-Id: I07e2a35e27142715a5257f199326b7a3d133e2a8
tombriden
force-pushed
the
dtmf-injection-fixes
branch
from
6b43180
to
e484a17
Compare
|
Everything merged and backported, thanks! |
A few scenarios are fixed for me with these changes.
This fixes not passing a
ts_delayintocodec_output_rtpresulting in packets not being scheduled correctly (like already happens incodec_add_dtmf_packet) and an, eg, 100ms event has all packets transmitted in as few as 20msin some scenarios DTMF injected with, eg, 100ms duration are transmitted much shorter due to the start time being adjusted but not the end time
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
DTMF requires an inter-digit pause, which is allowed for with the
pauseparameter. 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 thedtmf_statevar 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
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 thedtmf_recvlist and can cause out of order timestamp in the list, resulting inis_in_dtmfincorrectly returningNULL, 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
recvandsendshouldn'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.