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

SimpleConsumer: Opus DTX ignore capabilities #846

Merged
merged 4 commits into from
Jun 29, 2022
Merged

SimpleConsumer: Opus DTX ignore capabilities #846

merged 4 commits into from
Jun 29, 2022

Conversation

jmillan
Copy link
Member

@jmillan jmillan commented Jun 29, 2022

Tested with mediasoup-demo. Clearly shows ignore logs when there is silence.

RTP sequence numbers are properly handled, no audio cuts are appreciated.

@jmillan jmillan requested a review from ibc June 29, 2022 10:34
@jmillan
Copy link
Member Author

jmillan commented Jun 29, 2022

@nazar-pc, I have added the changes to Rust. Would you please check it? I'm having issues when running cargo test. I didn't have any few days ago.

meson.build:162:0: ERROR: Subproject exists but has no meson.build file

It's pointing to openssl subproject, which does not have the meson.build file.

Update: Please ignore. I've removed the openssl subprojects and build it again. The meson.build file is there.

@jmillan
Copy link
Member Author

jmillan commented Jun 29, 2022

Checking those CI failures..

@douglaseel
Copy link
Contributor

douglaseel commented Jun 29, 2022

@jmillan FYI you can also discard the next packet after a DTX if the RTP Marker bit is not set :)))

@ibc
Copy link
Member

ibc commented Jun 29, 2022

Why would the encoder send a "real" Opus audio packet after a DTX if such a packet should be discarded?

@douglaseel
Copy link
Contributor

I don't know why, but it never sends two consecutive DTX packets.
For example: if we have 1 second of silence, it will send a DTX + opus frame + DTX + opus frame + DTX.... I suppose those frames between DTX are used for comfort noise.
The first "real" voice frame after a DTX has the RTP Marker bit set.

@douglaseel
Copy link
Contributor

Just saying, if the goal is to decrease the bandwidth consumption, maybe it might be a good idea to drop those frames too (they are much bigger than DTX)

@ibc
Copy link
Member

ibc commented Jun 29, 2022

How standard is that behavior? It looks a bit dangerous to me if we assume that.

@jmillan
Copy link
Member Author

jmillan commented Jun 29, 2022

@jmillan FYI you can also discard the next packet after a DTX if the RTP Marker bit is not set :)))

Any reference to the specification?

marker bit in audio is used to indicate the first packet after a silence period. Sending no marked packet after a DTX is not even specified.

@douglaseel
Copy link
Contributor

There is no reference in the specs about this..
But this is exaclty what I've observed with opus/dtx when I've worked with it in the recent past.

@douglaseel
Copy link
Contributor

douglaseel commented Jun 29, 2022

marker bit in audio is used to indicate the first packet after a silence period. Sending no marked packet after a DTX is not even specified.

BTW,
DTX is a silence period, right?
So, the first voice packet after a silence period should have marker bit set..

The main problem is that they send a non-DTX frame between consecutive DTX without marker bit set. And if the marker bit is not set, it's not the first voice frame after a silence period :))))

@jmillan
Copy link
Member Author

jmillan commented Jun 29, 2022

From https://datatracker.ietf.org/doc/html/rfc3551#section-4.1

 For applications which send either no packets or occasional comfort-
   noise packets during silence, the first packet of a talkspurt, that
   is, the first packet after a silence period during which packets have
   not been transmitted contiguously, SHOULD be distinguished by setting
   the marker bit in the RTP data header to one.

It is RECOMMENDED and hence we should not discard packets after a DTX if they come with marker bit unset, otherwise we could be dropping legitimate audio.

@jmillan jmillan requested a review from nazar-pc June 29, 2022 15:21
@jmillan
Copy link
Member Author

jmillan commented Jun 29, 2022

@nazar-pc I mostly assigned you for reviewing the Rust side, in case you don't have the time for a full review.

@nazar-pc
Copy link
Collaborator

nazar-pc commented Jun 29, 2022

Rust side is trivial, looks good to me. I can proper review, but probably not until weekend, very busy.

UPD: No idea why it closed with comment 👀

@nazar-pc nazar-pc closed this Jun 29, 2022
@nazar-pc nazar-pc reopened this Jun 29, 2022
@jmillan jmillan merged commit f6d1284 into v3 Jun 29, 2022
@jmillan jmillan deleted the opus-dtx-ignore branch June 29, 2022 15:56
@smoke
Copy link
Contributor

smoke commented Sep 6, 2022

@jmillan Do you have a branch / repo with the mediasoup-demo setup with Opus DTX? I am looking for some examples / demo to experience what this flag does exactly.
Did you test with https://github.com/versatica/mediasoup-demo or was it with https://github.com/Kurento/mediasoup-demos/tree/master/mediasoup-recording? Do you use / test with ffmpeg only or with gstreamer as well?

@ibc
Copy link
Member

ibc commented Sep 6, 2022

@smoke plesse don't pollute this already merged PR with questions and direct mentions. Use the public forum instead.

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

Successfully merging this pull request may close these issues.

5 participants