Reverse RTCRtpSender<->RTCDtmfSender composition #446

Closed
murillo128 opened this Issue Mar 31, 2016 · 11 comments

Projects

None yet

3 participants

@murillo128
murillo128 commented Mar 31, 2016 edited

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.

@aboba aboba added 1.1 1.0 labels Apr 6, 2016
@robin-raymond
Contributor

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
Contributor

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
Contributor

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
Contributor

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
Contributor

@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
Contributor

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;
@aboba aboba removed the 1.1 label Apr 26, 2016
@robin-raymond robin-raymond was assigned by aboba Jul 13, 2016
@aboba aboba added a commit that referenced this issue Jul 14, 2016
@aboba aboba RTCDtmfSender exceptions
Fix for Issue #446
d76659d
@robin-raymond
Contributor

The issues found that could be resolved (e.g. exceptions) were added into latest spec. This is now resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment