Reverse RTCRtpSender<->RTCDtmfSender composition

[murillo128]

Currently we create a new RTCDtmfSender by calling it's constructor and passing the RTCRtpSender as attribute. Then we have to inspect canInsertDTMF to check whether the RTCDtmfSender is capable of sending DTMF.

var sender = new RTCDtmfSender(sendObject);
if (sender.canInsertDTMF) {
  var duration = 500;
  sender.insertDTMF("1234", duration);
} else {
  trace("DTMF function not available");
}

This seems weird to me, as canInsertDTMF refers to te ability to of the RTCRtpSender to send or not DTMFs and has nothing to do with the RTCDtmfSender object it self.

The natural way of doing that would be by querying the RTCRtpSender for it's RTCDtmfSender object as the sender MUST know if it supports or not dtmfs based on the media type/send codecs (removing then the canInsertDTMF property of RTCDtmfSender)

var sender = sendObject.getDtmfSender();
if (sender) {
  var duration = 500;
  sender.insertDTMF("1234", duration);
} else {
  trace("DTMF function not available");
}

We can refine the pattern later in order to remove the telephony-event as a codec.

[robin-raymond]

Ok, upon further review, I'm less in favour of such a change. The ability to do DTMF is dependent upon the DTMF codec being defined with a distinct PT.

If you create a DTMF transport via a getter (factory), at the moment of construction a check for the DTMF codec can be done and thus allow or deny the DTMF sender's construction. The trouble is that subsequent calls to send() can strip out the codec. Thus you are left with a sender that cannot send anymore.

While I still like the pattern, that is a bit of a problem. The previous solution allows for the toggle on/off of the DTMF sender's ability to do it's job hence the canInsertDTMF check.

So, if we wanted to switch to this pattern a few things would need to happen IMHO:

1. Reject any send() calls where the DTMF was stripped out where a DTMF sender was already constructed via an invalid params exception, or,
2. Reject any calls to insertDTMF with an invalid state exception if the sender stripped out the DTMF codec after the DTMS sender was created.

...or we leave things as they are since it does work...

[murillo128]

Same could happen today if you try to insert stmt when canInsertDTMF is false.

I like 2.

[robin-raymond]

Given the above, would returning null still be relevant? i.e. the object can exist if the dtmf codec is present or not but we get an exception upon insert dtmf?

[murillo128]

No, it would not be relevant.

We could return always an object and the check the canInsertDTMF
El 8/4/2016 20:06, "Robin Raymond" notifications@github.com escribió:

Given the above, would returning null still be relevant? i.e. the object
can exist if the dtmf codec is present or not but we get an exception upon
insert dtmf?

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#446 (comment)

[robin-raymond]

Then is it better to have a getter for the object since it cannot be null vs a constructor? The CG had a meeting and I argue in favour of factories but others argued in favour of constructors. I lost that argument...

So if we made a getter "factory" that would fit less the pattern than constructors. Given it can't be null, I'm not sure we need to/should change it or not.

Also, what happens if someone attempts to create this on a video media sender? I would say that should cause an invalid state exception.

[murillo128]

Also, what happens if someone attempts to create this on a video media sender? I would say that should cause an invalid state exception.

That would still happen with constructors anyway

[robin-raymond]

That would still happen with constructors anyway

Correct, it would be identical.

Given this exchange, I'm not sure at this point that a factory getter versus a constructor is really a meaningful a difference given that:

1. CG voted against factory pattern in favour of constructors (unless really required);
2. The behaviour proposed of returning null when DTMF is not supported turns out to not to be the means to test if dtmf sending is supported or not.

But there may be some fixes to be had out of this exchange:

1. constructing DTMF against a video sender should throw an exception;
2. attempting to send a dtmf when the codec is stripped off the sender should throw an exception (it might already, need to double check this).

[murillo128]

Ok, I agree. I prefer it the other way, but it is just a matter of
personal preferences.

If we find a better way of handling non-media codecs (dtmf,cn,etc..), the
composition direction will be much obvious and natural..
El 14/4/2016 20:47, "Robin Raymond" notifications@github.com escribió:

That would still happen with constructors anyway

Correct, it would be identical.

Given this exchange, I'm not sure at this point that a factory getter
versus a constructor is really a meaningful a difference given that:

1. CG voted against factory pattern in favour of constructors (unless
   really required);
2. The behaviour proposed of returning null when DTMF is not supported
   turns out to not to be the means to test if dtmf sending is supported or
   not.

But there may be some fixes to be had out of this exchange:

1. constructing DTMF against a video sender should throw an exception;
2. attempting to send a dtmf when the codec is stripped off the sender
   should throw an exception (it might already, need to double check this).

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#446 (comment)

[robin-raymond]

@murillo128 .. FYI - I prefer that too, but alas, I agree, it is a personal preference so I defer to the will of the CG on the overall pattern desired.

I'll verify any fixes needed and get a PR issued.

[robin-raymond]

Verified, summary of fixes needed:

1. constructing DTMF against a video sender should throw an invalid usage exception;
2. attempting to send a dtmf while the DTMF codec is stripped off the linked sender should throw an invalid state exception;