Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarification: insertDTMF replaces the current tone buffer #1338

Merged
merged 3 commits into from
Jun 15, 2017

Conversation

aboba
Copy link
Contributor

@aboba aboba commented Jun 5, 2017

Fix for Issue #1260

@aboba aboba requested a review from fluffy June 5, 2017 20:33
webrtc.html Outdated
<p>Since <code>insertDTMF</code> replaces the tone
buffer, in order to add to the DTMF tones to be played,
it is necessary to append additional tones to the
tone buffer. Calling <code><a data-for=
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: "in order to add to the DTMF tones being played, it is necessary to call insertDTMF with a string containing both the remaining tones (stored in toneBuffer) and the new tones appended together."

@stefhak
Copy link
Contributor

stefhak commented Jun 15, 2017

LGTM.

it is necessary to call <code>insertDTMF</code> with a
string containing both the remaining tones (stored in
<code>toneBuffer</code>) and the new tones appended
together. Calling <code><a data-for=
Copy link
Contributor

@taylor-b taylor-b Jun 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this actually safe? A tone could be sent in between toneBuffer being read and insertDTMF being called with the additional tones, resulting in one being sent twice.

Is it too late to change this behavior? I wonder how many applications are actually reliant on it, or what the intention was.

EDIT: Nevermind. toneBuffer is only drained from a queued "playout task" so this is safe.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we have playout tasks and is on main loop this should be safe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants