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

opusrtp: Implement DTX support #46

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

phil-lavin
Copy link

Implemented DTX support into opusrtp
Also had to properly implement the --rate parameter as 48000 was hard coded in 2 places

Phil Lavin added 2 commits June 27, 2019 14:18
DTX is signified by an increase in timestamp by a multiple of the number of samples.
Silence can be added to an OPUS stream by inserting 1 byte packets at the correct granule positions.
Copy link
Collaborator

@mark4o mark4o left a comment

Choose a reason for hiding this comment

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

Opus streams are sample rate independent, and the audio bandwidth can change in the middle of the stream, so Opus in RTP normalizes all timestamps to 48000 Hz (https://tools.ietf.org/html/rfc7587#section-6.1). Therefore these changes are not correct. The only purpose of the opusrtp sample rate option is when writing an Ogg Opus output file, there is a field that can optionally record the original sample rate, and this option allows that field to be filled in.

@mark4o
Copy link
Collaborator

mark4o commented Aug 24, 2019

Ideally, timestamp gaps due to either DTX or packet loss would be handled according to https://tools.ietf.org/html/rfc7845#section-4.1 (it is the same basic idea as this but a little more sophisticated). The main reason that this is not implemented is because currently opusrtp does not handle out-of-order packets. Instead of a small glitch, this change would cause huge gaps to be inserted whenever packets are received out of order. In addition to handling out-of-order packets, there should be some kind of sanity checks to ensure that the data is reasonable before using it to generate a large number of packets, to protect against bad or malicious data. Also, it is possible that multiple streams were inadvertently captured, which would cause this change to produce an explosion of output with a huge number of packets written for each packet received, since each received packet would be on a different stream with a different time base. Before this is implemented there should be some checks to ensure that the data is from a single stream, to avoid this issue. Alternatively, different streams could be written to different files. A while back I did add checks for matching port number and RTP payload type, which catches some common reasons for mixed up streams, however that is not enough to ensure that all packets are from the same stream.

If you are interested in adding support for out-of-order packets, sanity checks, and same-stream checks then that would be terrific and this could be added as well, but I think it is important that those be added first, and at least the out-of-order packet support is probably much more work. Therefore I am leaving this pull request open for now.

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

2 participants