Improved interpretation of DTMF tones in DtmfTransformEngine#433
Improved interpretation of DTMF tones in DtmfTransformEngine#433cbeckr wants to merge 1 commit intojitsi:masterfrom
Conversation
|
Hi, thanks for your contribution! |
|
|
||
| notifyAll(); | ||
| } | ||
| queue.offer(p); |
There was a problem hiding this comment.
I haven't read the diff carefully, but: PacketTransformers should not keep the RawPacket/DtmfRawPacket instances, because these instances and their buffers are re-used. I don't expect many DTMF packets, so a simple clone() should be enough.
There was a problem hiding this comment.
Thanks for your feedback. In e982394, I added a clone() implementation for DtmfRawPacket and added the call on line 626.
cbeckr
force-pushed
the
dtmf-improvements
branch
from
9452bda to
e982394
Compare
|
Is there still interest in this PR? If so, I'd be happy to update it. I see some files were moved but the underlying code hasn't changed much. |
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.
cbeckr
force-pushed
the
dtmf-improvements
branch
from
e982394 to
4ab8d48
Compare
|
Rebased - this is ready for review once more. Tested with The Test Call:
|
3 participants
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 for recognizing shorter tones (<120 ms) that are typically
represented by one self-contained RTP packet containing both start and end
indications.
Note: Corporate CLA was completed on 2018-08-03.