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

change ptime may cause AudioLevel don't work #1022

Open
buptlsp opened this issue Mar 10, 2023 · 12 comments
Open

change ptime may cause AudioLevel don't work #1022

buptlsp opened this issue Mar 10, 2023 · 12 comments
Labels

Comments

@buptlsp
Copy link

buptlsp commented Mar 10, 2023

Bug Report

In AudioLevelObserver.cpp, the interval was between 250ms and 5000ms, as follow.
image

and the DBovs.count should >= 10.
image

ptime is the audio length of an rtp packet. Therefore, ptime * DBovs.count must be less than interval in the time of interval.
The default ptime is 20, so 20 * 10 < 250ms, it is working properly. But users may set different ptime, maxptime and minptime depending on the actual situation, so if they are larger, it may cause audioLevel not to work. For example if I set the ptime to 30ms, then audioLevel may not receive 10 packets in 250 ms. Even if I speak, audioLevel will think I am not speaking.

Issue description

Therefore, I suggest to make maxInterval, minInterval, maxCount optional configuration items.

@buptlsp buptlsp added the bug label Mar 10, 2023
@ibc
Copy link
Member

ibc commented Mar 13, 2023

Therefore, I suggest to make maxInterval, minInterval, maxCount optional configuration items.

What do you mean with "maxInterval" and "minInterval"? It doesn't make sense. We already have a customizable "interval" setting.

Regarding "maxCount", how can it be friendly at all? Different clients with different ptime settings may exist in the same meeting.

Wouldn't it be enough if we replace hardcoded max count 10 to 5 or some other value?

@buptlsp
Copy link
Author

buptlsp commented Mar 14, 2023

let's calculate the avgtime, if we use audiolevel to control the stream. Assuming that the user speaks continuously, it may speak at any point x in the period T. y represents count*ptime.
image

Then, we can derive the following equation.
If 0=< x <= T - y , we can determine whether the user speaks in the current cycle , the user's waiting time is:
f(x) = T - x
If T - y < x <= T, at this time, in the current cycle the AudioLevel will think the user did not speak. it will be recognize till the next cycle end. so the user's waiting time is:
f(x) = 2*T - x

the average waiting time is :
image

so, the current setting is T=800ms, y = 20 * 10 = 200ms, g(t) = 600ms. means the person begin to speak , his voice will lost data of 600ms. it's too long! when many people in the room ,i use AudioLevel to control (using producer.pause or consumer.pause), the people resume need 600ms to speaking, This is unacceptable. even the min T = 250 ms, g(t) = 325ms, it's also too long!

In fact, what I need is to be able to dynamically adjust parameters, including interval, mincount of packets, like router.updateAudioLevelConfig(). Because the number of people in the room is always changing, and the adjustment of these parameters will bring more computation (cpu consumption).

Thanks for your reply, I think it's a bug just because the current settings of the parameters don't really fit and are in code that can't be changed. If you don't think it's appropriate, please ignore it.

@ibc
Copy link
Member

ibc commented Mar 14, 2023

Do you feel brave to write a PR implementing this? That mathematics is too hard for me and honestly I don't think we will be able to spend too much time on it.

@buptlsp
Copy link
Author

buptlsp commented Mar 14, 2023

I don't know c++. I write a pr about another audiolevel bug last week, and you guys rewrote it. The math just calculates the average time we need to wait if we use audiolevel to control the sound. If we follow the parameters in mediasoup-demo, it will take 600ms to hear the sound, which will actually cause many users to report lagging sound when using audiolevel to pause&resume.

Now the minimum interval of audiolevel is 250ms, and the calculated average wait time is 325ms, but we actually need around 120ms (a friend of mine recommended me other algorithms that might work). So I just suggest you make minInterval and minCount configurable. Of course, it's up to you to change them or not

@buptlsp buptlsp closed this as completed Mar 14, 2023
@ibc
Copy link
Member

ibc commented Mar 14, 2023

No need to close the issue, @buptlsp.

@ibc ibc reopened this Mar 14, 2023
@jmillan
Copy link
Member

jmillan commented Mar 14, 2023

So I just suggest you make minInterval and minCount configurable. Of course, it's up to you to change them or not

Minimum interval could be set to 100 or any other value. It's just a lower limit. It does not need to be a configuration parameter.

That being said, would it be enough to just be able to update the count value? which of course should be given another name like: samplesCountThreshold or alike.

@buptlsp
Copy link
Author

buptlsp commented Mar 15, 2023

If the minimum interval is set to 100, samplesCountThreshold should be less than 5. I recommend setting to 4, and now I'm in testing, I'm not sure too.

The smaller the interval, the smaller the samplesCountThreshold. If the interval is larger, the samplesCountThreshold should also be larger.

@ibc
Copy link
Member

ibc commented Mar 15, 2023

@buptlsp I'd appreciate if you could change those values in your local fork, test them and, if possible, come with appropriate ones (or write a PR directly if you wish).

@nazar-pc
Copy link
Collaborator

Please don't feel discouraged if PR wasn't merged as is, it is still useful and appreciated!

@buptlsp
Copy link
Author

buptlsp commented Mar 15, 2023

thanks! maybe I need study some c++ first, I will try next time. if the setting is ok, I will tell you.

I appreciate for all of your works

@ibc
Copy link
Member

ibc commented Mar 15, 2023

Please don't feel discouraged if PR wasn't merged as is, it is still useful and appreciated!

Yes @buptlsp, even if you PR was not merged it was conceptually right and made it easy for us to implement the intended changes, so please don't give up :)

@buptlsp
Copy link
Author

buptlsp commented Mar 17, 2023

I'm sorry. in my tests, if I change the interval to 100ms and set samplesCountThreshold to 4, the voice stuttering problem gets worse instead of better. Due to the smaller interval, it will bring more frequent user switching (pause&consume), Although less data will be lost in the interval.

This problem has been bothering me and I'm currently looking for other solutions to solve it. It seems that modifying the parameters doesn't help much, sorry for wasting your time and thanks for your replies.

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

No branches or pull requests

4 participants