From 9452bda8d7d1b335b99e6066c0f0c9b735908725 Mon Sep 17 00:00:00 2001 From: Christian Becker Date: Fri, 3 Aug 2018 13:21:21 +0200 Subject: [PATCH] Improved interpretation of DTMF tones in DtmfTransformEngine This change addresses a number of issues that led to incorrect interpretations of DTMF tones, especially tone sequences. It introduces a queue to mitigate previous thread-safety issues and handle more than one RTP packet at a time (a single tone is associated with at least three RTP packets). It also adds support to recognize shorter tones (<120 ms) that are typically represented by one self-contained RTP packet containing both start and end indications. --- .../transform/dtmf/DtmfTransformEngine.java | 146 +++++++++++------- 1 file changed, 90 insertions(+), 56 deletions(-) diff --git a/src/org/jitsi/impl/neomedia/transform/dtmf/DtmfTransformEngine.java b/src/org/jitsi/impl/neomedia/transform/dtmf/DtmfTransformEngine.java index 6c053d335..a6be989b8 100644 --- a/src/org/jitsi/impl/neomedia/transform/dtmf/DtmfTransformEngine.java +++ b/src/org/jitsi/impl/neomedia/transform/dtmf/DtmfTransformEngine.java @@ -16,6 +16,7 @@ package org.jitsi.impl.neomedia.transform.dtmf; import java.util.*; +import java.util.concurrent.*; import javax.media.*; @@ -25,6 +26,7 @@ import org.jitsi.service.neomedia.*; import org.jitsi.service.neomedia.codec.*; import org.jitsi.service.neomedia.format.*; +import org.jitsi.util.*; /** * The class is responsible for sending DTMF tones in an RTP audio stream as @@ -38,6 +40,13 @@ public class DtmfTransformEngine extends SinglePacketTransformer implements TransformEngine { + /** + * The Logger used by the DtmfTransformEngine class and + * its instances for logging output. + */ + private static final Logger logger + = Logger.getLogger(DtmfTransformEngine.class); + /** * The AudioMediaStreamImpl that this transform engine was created * by and that it's going to deliver DTMF packets for. @@ -500,10 +509,11 @@ private void checkIfCurrentToneMustBeStopped() } /** - * A simple thread that waits for new tones to be reported from incoming - * RTP packets and then delivers them to the AudioMediaStream - * associated with this engine. The reason we need to do this in a separate - * thread is of course the time sensitive nature of incoming RTP packets. + * A simple thread that waits for new tone events to be reported from + * incoming RTP packets and then delivers them as start / end events to the + * AudioMediaStream associated with this engine. + * The reason we need to do this in a separate thread is of course the + * time sensitive nature of incoming RTP packets. */ private class DTMFDispatcher implements Runnable @@ -511,61 +521,96 @@ private class DTMFDispatcher /** Indicates whether this thread is supposed to be running */ private boolean isRunning = false; - /** The tone that we last received from the reverseTransform thread*/ - private DTMFRtpTone lastReceivedTone = null; - - /** The tone that we last received from the reverseTransform thread*/ + /** The tone that we last reported to the listener */ private DTMFRtpTone lastReportedTone = null; /** - * Have we received end of the currently started tone. + * Timestamp the last reported tone start; used to identify starts + * when the marker bit is lost + */ + private long lastReportedStart = 0; + + /** Timestamp the last reported tone end; used to prevent duplicates */ + private long lastReportedEnd = 0; + + /** + * The maximum number of DTMF events that are queued for processing */ - private boolean toEnd = false; + private int QUEUE_SIZE = 100; /** - * Waits for new tone to be reported via the addTonePacket() - * method and then delivers them to the AudioMediaStream that - * we are associated with. + * The queue of DtmfRawPackets pending processing + */ + private final LinkedBlockingQueue queue + = new LinkedBlockingQueue<>(QUEUE_SIZE); + + /** + * Waits for new tone events to be reported via the + * addTonePacket() method and delivers them as start / end + * events to the AudioMediaStream that we are associated with. */ public void run() { isRunning = true; - DTMFRtpTone temp = null; - while(isRunning) { - synchronized(this) + DtmfRawPacket pkt; + DTMFRtpTone tone; + + try + { + pkt = queue.poll(500, TimeUnit.MILLISECONDS); + } + catch (InterruptedException iex) { - if(lastReceivedTone == null) - { - try - { - wait(); - } - catch (InterruptedException ie) {} - } - - temp = lastReceivedTone; - // make lastReportedLevels to null - // so we will wait for the next tone on next iteration - lastReceivedTone = null; + continue; } - if(temp != null - && ((lastReportedTone == null && !toEnd) - || (lastReportedTone != null && toEnd))) + // The current thread has potentially waited. + if (!isRunning) { - //now notify our listener - if (mediaStream != null) - { - mediaStream.fireDTMFEvent(temp, toEnd); - if(toEnd) - lastReportedTone = null; - else - lastReportedTone = temp; - toEnd = false; - } + break; + } + + if (pkt == null) + { + continue; + } + + tone = getToneFromPacket(pkt); + + /* + * Detect DTMF tone start by looking for new tones + * It doesn't make sense to look at the 'marked' flag as those + * packets may be re-sent multiple times if they also contain + * the 'end' bit. + */ + if (lastReportedTone == null + && pkt.getTimestamp() != lastReportedStart) + { + logger.info("Delivering DTMF tone start: " + + tone.getValue()); + // now notify our listener + mediaStream.fireDTMFEvent(tone, false); + lastReportedStart = pkt.getTimestamp(); + lastReportedTone = tone; + } + + /* + * Detect DTMF tone end via the explicit 'end' flag. + * End packets are repeated for redundancy. To filter out + * duplicates, we track them by their timestamp. + * Start and end may be present in the same packet, typically + * for durations below 120 ms. + */ + if (pkt.isEnd() && pkt.getTimestamp() != lastReportedEnd + && tone == lastReportedTone) { + logger.info("Delivering DTMF tone end: " + tone.getValue()); + // now notify our listener + mediaStream.fireDTMFEvent(tone, true); + lastReportedEnd = pkt.getTimestamp(); + lastReportedTone = null; } } } @@ -578,13 +623,7 @@ public void run() */ public void addTonePacket(DtmfRawPacket p) { - synchronized(this) - { - this.lastReceivedTone = getToneFromPacket(p); - this.toEnd = p.isEnd(); - - notifyAll(); - } + queue.offer(p); } /** @@ -593,13 +632,8 @@ public void addTonePacket(DtmfRawPacket p) */ public void stop() { - synchronized(this) - { - this.lastReceivedTone = null; - isRunning = false; - - notifyAll(); - } + isRunning = false; + queue.clear(); } /**