[jitsi-dev] DTMF functionality


#1

Hi devs,

Thanks to Lubo and Damencho help, the inband DTMF is ready. However, there is still some points I would like to discuss with you before committing it:

1) I made the choice to send inband DTMF signals even if the audio channel is mute. Does someone knows if RFC4733 DTMF have the same behavior: are telephone event still sent when the audio stream is muted?

2) As inband DTMF is implemented following the mute interface, I have incorporated it into the MuteDataSource interface. Does we keep this name of MuteDataSource interface and all implementing classes, or can we rename it into a "MutableDataSource" or "RewritableDataSource"?

3) For the GUI, I have created a new string placed into the "resources/languages/resources.properties" file. Is it sufficient to upload this string at the http://translate.jitsi.org/ web site (at least for English language)?

4) Another topic, found while previously looking at DTMF RTP transform. I have found (and this patch propose to remove) unused functions in AudioMediaStream interface and its implementation, relative functions and attributes in AudioMediaStreamImpl class:

(src/net/java/sip/communicator/service/neomedia/AudioMediaStream.java)
public void addDTMFListener(DTMFListener listener);
public void removeDTMFListener(DTMFListener listener);

(src/net/java/sip/communicator/impl/neomedia/AudioMediaStreamImpl.java)
private List<DTMFListener> dtmfListeners;
public void addDTMFListener(DTMFListener listener);
public void removeDTMFListener(DTMFListener listener);
public void fireDTMFEvent(DTMFTone tone, boolean end);

5) Always while looking at DTMF RTP transform, I have found (and this patch propose to remove) an unused thread and relative attributes from DtmfTransformEngine class:

(src/net/java/sip/communicator/impl/neomedia/transform/dtmf/DtmfTransformEngine.java)
private static final DTMFTone[] supportedTones
private DTMFDispatcher dtmfDispatcher
private class DTMFDispatcher implements Runnable

See you at FOSDEM,
Chenzo

patch_inband_DTMF.patch (51.7 KB)

···

--
Vincent Lucas, Ph.D. Jitsi developer
chenzo@jitsi.org http://jitsi.org


#2

Hey Vincent,

I think that it'd be better to (1) keep the MuteDataSource interface
unmodified and introduce a separate interface for in-band DTMF and (2)
implement the in-band DTMF interface in the same classes as the
MuteDataSource interface (e.g. in MutePushBufferDataSource and
MutePullBufferDataSource) because wrapping a DataSource takes a lot of
code and effort.

I agree MutePushBufferDataSource and the likes will then probably
benefit from renaming. I'm not sold on Mutable or Rewritable but I
don't have a suggestion at this time which sounds better to me.

Regards,
Lyubomir

···

2012/2/2 Vincent Lucas <chenzo@jitsi.org>:

2) As inband DTMF is implemented following the mute interface, I have
incorporated it into the MuteDataSource interface. Does we keep this name of
MuteDataSource interface and all implementing classes, or can we rename it
into a "MutableDataSource" or "RewritableDataSource"?


#3

Vincent, it's me again.

4) Another topic, found while previously looking at DTMF RTP transform. I
have found (and this patch propose to remove) unused functions in
AudioMediaStream interface and its implementation, relative functions and
attributes in AudioMediaStreamImpl class:

(src/net/java/sip/communicator/service/neomedia/AudioMediaStream.java)
public void addDTMFListener(DTMFListener listener);
public void removeDTMFListener(DTMFListener listener);

(src/net/java/sip/communicator/impl/neomedia/AudioMediaStreamImpl.java)
private List<DTMFListener> dtmfListeners;
public void addDTMFListener(DTMFListener listener);
public void removeDTMFListener(DTMFListener listener);
public void fireDTMFEvent(DTMFTone tone, boolean end);

If the DTMF arrives in the audio stream, then notifications about its
arrival will be fired to the world outside neomedia by
AudioMediaStream. There may currently be no actual listeners but we've
written the support code so let's leave it there for now.

5) Always while looking at DTMF RTP transform, I have found (and this patch
propose to remove) an unused thread and relative attributes from
DtmfTransformEngine class:

(src/net/java/sip/communicator/impl/neomedia/transform/dtmf/DtmfTransformEngine.java)
private static final DTMFTone[] supportedTones
private DTMFDispatcher dtmfDispatcher
private class DTMFDispatcher implements Runnable

I'm not sure I understand. DtmfTransformEngine is another DTMF
implementation which you're not substituting, you're adding a new one.
Could you please clarify?

···

2012/2/2 Vincent Lucas <chenzo@jitsi.org>:


#4

Hi Lyubomir,

See comments below:

2) As inband DTMF is implemented following the mute interface, I have
incorporated it into the MuteDataSource interface. Does we keep this name of
MuteDataSource interface and all implementing classes, or can we rename it
into a "MutableDataSource" or "RewritableDataSource"?

Hey Vincent,

I think that it'd be better to (1) keep the MuteDataSource interface
unmodified and introduce a separate interface for in-band DTMF and (2)
implement the in-band DTMF interface in the same classes as the
MuteDataSource interface (e.g. in MutePushBufferDataSource and
MutePullBufferDataSource) because wrapping a DataSource takes a lot of
code and effort.

Ok, I have corrected this part.

I agree MutePushBufferDataSource and the likes will then probably
benefit from renaming. I'm not sold on Mutable or Rewritable but I
don't have a suggestion at this time which sounds better to me.

The class MutePushBufferDataSource and MutePullBufferDataSource are then respectively renamed into RewritablePushBufferDataSource and RewritablePullBufferDataSource.

Regards,
Vincent

···

On 02/02/2012 02:25 PM, Lyubomir Marinov wrote:

2012/2/2 Vincent Lucas<chenzo@jitsi.org>:

Regards,
Lyubomir

--
Vincent Lucas, Ph.D. Jitsi developer
chenzo@jitsi.org http://jitsi.org


#5

Hi Lyubomir again,

(More details below)

Vincent, it's me again.

4) Another topic, found while previously looking at DTMF RTP transform. I
have found (and this patch propose to remove) unused functions in
AudioMediaStream interface and its implementation, relative functions and
attributes in AudioMediaStreamImpl class:

(src/net/java/sip/communicator/service/neomedia/AudioMediaStream.java)
public void addDTMFListener(DTMFListener listener);
public void removeDTMFListener(DTMFListener listener);

(src/net/java/sip/communicator/impl/neomedia/AudioMediaStreamImpl.java)
private List<DTMFListener> dtmfListeners;
public void addDTMFListener(DTMFListener listener);
public void removeDTMFListener(DTMFListener listener);
public void fireDTMFEvent(DTMFTone tone, boolean end);

If the DTMF arrives in the audio stream, then notifications about its
arrival will be fired to the world outside neomedia by
AudioMediaStream. There may currently be no actual listeners but we've
written the support code so let's leave it there for now.

Ok, I have reverted this part.

5) Always while looking at DTMF RTP transform, I have found (and this patch
propose to remove) an unused thread and relative attributes from
DtmfTransformEngine class:

(src/net/java/sip/communicator/impl/neomedia/transform/dtmf/DtmfTransformEngine.java)
private static final DTMFTone[] supportedTones
private DTMFDispatcher dtmfDispatcher
private class DTMFDispatcher implements Runnable

I'm not sure I understand. DtmfTransformEngine is another DTMF
implementation which you're not substituting, you're adding a new one.
Could you please clarify?

Yes, you are right, the DtmfTransformerEngine is another DTMF: it used telephone-event RTP packets to send DMTF tones as defined in RFC4733. And there is no substitution: the user must choose in the account configuration if he wants to used the "RFC4733/SIP-INFO DTMF" (default choice) or the "in-band DTMF".

However, the DtmfTransformerEngine generates the RTP-DTMF packets via the transform() function and receives DTMF tones via the reverseTransform(RawPacket pkt) function. This reverseTransform is the single function calling the DTMFDispatcher, which after a thread loop calls the fireDTMFEvent from the AudioMediaStreamImpl, which finally notifies the recorded listeners. When inspecting the code more precisely, each time a DTMF packet is received a fireDTMFEvent is called. Thereby, the thread is unnecessary and can be easily replaced by a "direct" function call.
To this end, as I have re-activated (cf. above discussion) the DTMFListener mechanism, I will remove the DTMFDispatcher class, but keeping the addTonePacket(DtmfRawPacket p) function modified to call the fireDTMFevent:

private void addTonePacket(DtmfRawPacket p)
{
     DTMFTone tone = getToneFromPacket(p);
     boolean toEnd = p.isEnd();

     mediaStream.fireDTMFEvent(tone, toEnd);
}

Thank you for the review.

Regards,
Vincent

···

On 02/02/2012 03:10 PM, Lyubomir Marinov wrote:

2012/2/2 Vincent Lucas<chenzo@jitsi.org>:

--
Vincent Lucas, Ph.D. Jitsi developer
chenzo@jitsi.org http://jitsi.org