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

Implement & auto-select Opus' low delay mode #3753

Merged
merged 1 commit into from Sep 20, 2019
Merged

Implement & auto-select Opus' low delay mode #3753

merged 1 commit into from Sep 20, 2019

Conversation

trudnorx
Copy link
Contributor

@trudnorx trudnorx commented Aug 21, 2019

Partially implements #3502. This pull request implements Opus' low delay mode selection, which should result in a big decrease in latency.
The bUseOpusMusicEncoding option has been removed, because now the Opus mode is selected depending on the bitrate. The automatic selection aims to decrease the latency while maintaining acceptable quality.
The exact values may have to be tuned further for a smarter auto-selection because the low delay mode, while causing a strong decrease in latency, requires an higher bitrate to maintain the same quality; in order to compensate, the maximum bitrate in the UI has been increased to 192 kbit/s.

The automatic selection sets:

  • Low-delay mode when the bitrate is >= 64 kbit/s.
  • Music mode when the bitrate is >= 32 kbit/s.
  • VOIP mode when the bitrate is < 32 kbit/s.

@trudnorx
Copy link
Contributor Author

So should this be accepted? @davidebeatrici ?
Just to clarify I did test it and it seemed to work.

@davidebeatrici
Copy link
Member

From #mumble@freenode:

<tom> some interesting tests
<tom> at extremely low bitrates low delay can sound more intelligible than voip
<tom> albeit more robotic
<tom> this is interesting
<tom> music application being the highest quality, even above low delay
<tom> but determining if that still matters at higher bitrates needs more testing
<tom> https://cloud.nuegia.net/s/XSWxmD9fAEHPt9a
<tom> I was curious what kind of artifacting I should be looking for so I recorded a standalone PCM wav 32bit little endian float and compressed them standalone at a very low bitrate
<tom> in this case 4k
<tom> the size difference between music and voip is only 2kb
<tom> ok here's what I think
<tom> unless you're REALLY getting a large improvement in latency at higher bitrates (64k and above) it may not be worth going from music to lowdelay. it's very subtle but it does reduce the emphasis on sections of speech with a lot of dynamic range.
<tom> which can matter if you're doing a studio recording
<tom> one thing that may be worth looking at through is disabling phase inversion since microphones are always going to be mono devices
<tom> but I have not tested this yet

@davidebeatrici
Copy link
Member

From https://wiki.xiph.org/OpusFAQ#Does_Opus_support_higher_sampling_rates.2C_such_as_96_kHz_or_192_kHz.3F:

Does Opus support higher sampling rates, such as 96 kHz or 192 kHz?

Yes and no.

Opus encoding tools like opusenc will happily encode input files that are sampled at 96 or 192 kHz.

However, files at these rates are internally converted to 48 kHz and then only frequencies up to 20 kHz are encoded.

The reason is simple: lossy codecs are designed to preserve audible details while discarding irrelevant information. Since the human ear can only hear up to 20 kHz at best (usually lower than that), frequency content above 20 kHz is the first thing to go.

See Monty's article for more details.

If you want a codec to handle high sampling rates losslessly, use FLAC!

@trudnorx
Copy link
Contributor Author

unless you're REALLY getting a large improvement in latency at higher bitrates (64k and above) it may not be worth going from music to lowdelay. it's very subtle but it does reduce the emphasis on sections of speech with a lot of dynamic range.

It is a large improvement in latency. It decreases 15ms from the round trip. It is auto enabled only for high bitrate in which the difference in latency definitely outweighs the difference in quality. In any case we could change the bitrate minimum at which it is auto enabled to a higher bitrate in order to preserve even more quality.

which can matter if you're doing a studio recording

People aren't using Mumble for studio recording, they're using Mumble for video games in which latency matters a lot. In any case we can always make an option to force disable it.

From https://wiki.xiph.org/OpusFAQ#Does_Opus_support_higher_sampling_rates.2C_such_as_96_kHz_or_192_kHz.3F:

Does Opus support higher sampling rates, such as 96 kHz or 192 kHz?

Yes and no.

How is this relevant?

@davidebeatrici
Copy link
Member

It is a large improvement in latency. It decreases 15ms from the round trip. It is auto enabled only for high bitrate in which the difference in latency definitely outweighs the difference in quality. In any case we could change the bitrate minimum at which it is auto enabled to a higher bitrate in order to preserve even more quality.

I agree, a 15 ms difference is significant.

People aren't using Mumble for studio recording, they're using Mumble for video games in which latency matters a lot. In any case we can always make an option to force disable it.

Most people are using Mumble for gaming, however it's also used for podcasts, for example.

Adding a new option to disable the low-latency encoding is probably the best solution.

How is this relevant?

From #3753 (comment):

In order to compensate, max bitrate in UI has increased to 192kbit/s. I didn't test whether the current max setting of 96kbit/s was enough for virtually perfect quality with low delay mode or not so I am unsure if this change was even truly needed, or whether it is too much or too little. However I did it just in case.

@trudnorx
Copy link
Contributor Author

trudnorx commented Sep 18, 2019

Adding a new option to disable the low-latency encoding is probably the best solution.

Checkbox in "Audio Input"?

How is this relevant?

From #3753 (comment):

In order to compensate, max bitrate in UI has increased to 192kbit/s. I didn't test whether the current max setting of 96kbit/s was enough for virtually perfect quality with low delay mode or not so I am unsure if this change was even truly needed, or whether it is too much or too little. However I did it just in case.

I think you're confusing sampling rate with bitrate. It's unrelated. Mumble always uses 48KHz sampling rate internally but supports up to 96kbit/s bitrate currently (from UI, higher bitrate can be forced from other places). With my changes it still always uses 48KHz sampling rate, but bumps UI-selectable bitrate up to 192kbit/s. Opus also always uses 48KHz sampling rate, but supports up to 510kbit/s bitrate.

@davidebeatrici
Copy link
Member

davidebeatrici commented Sep 18, 2019

Checkbox in "Audio Input"?

Yes

I think you're confusing sampling rate with bitrate. It's unrelated. Mumble always uses 48KHz sampling rate internally but supports up to 96kbit/s bitrate currently (from UI, higher bitrate can be forced from other places). With my changes it still always uses 48KHz sampling rate, but bumps UI-selectable bitrate up to 192kbit/s. Opus also always uses 48KHz sampling rate, but supports up to 510kbit/s bitrate.

Huge mistake on my side, sorry. I read "96kbit/s" as 96 KHz and didn't think about the fact that we always use 48000 Hz. I got confused by the numbers being equal to the most common frequencies.

If Opus supports up to 510 Kb/s, let's set that as the maximum in the UI.

@trudnorx
Copy link
Contributor Author

You don't need to be sorry for something like that :D

If Opus supports up to 510 Kb/s, let's set that as the maximum in the UI.

I thought about that. But maybe the bar would be ugly and harder to use? I also thought about an idea to enable up to 510kbit/s in the UI only if "Advanced" option is checked, and use 96 or 192kbit/s as maximum otherwise.

@davidebeatrici
Copy link
Member

Perhaps we should add a QLineEdit near the slider.

@trudnorx
Copy link
Contributor Author

trudnorx commented Sep 19, 2019

Ok there's now an "allow low delay" option, enabled by default. I think I'm leaving the bitrate limit at 192kbit/s for now for reasons of practicality, QLineEdit could be a good idea but shouldn't we handle that in another issue?

Tell me what you think. Do you think this is ready or needs further refinement?

src/mumble/AudioInput.ui Outdated Show resolved Hide resolved
src/mumble/AudioInput.cpp Outdated Show resolved Hide resolved
src/mumble/AudioInput.cpp Outdated Show resolved Hide resolved
src/mumble/AudioInput.cpp Outdated Show resolved Hide resolved
@davidebeatrici
Copy link
Member

Ok there's now an "allow low delay" option, enabled by default. I think I'm leaving the bitrate limit at 192kbit/s for now for reasons of practicality, QLineEdit could be a good idea but shouldn't we handle that in another issue?

Yes.

Tell me what you think. Do you think this is ready or needs further refinement?

Please add a message for each commit, explaining the changes.

@trudnorx
Copy link
Contributor Author

I accepted your changes.

Please add a message for each commit, explaining the changes.

What do you mean? There's already commit messages.

@davidebeatrici
Copy link
Member

I would squash all the commits into a single one and apply the pull request's message.

@trudnorx
Copy link
Contributor Author

I would squash all the commits into a single one and apply the pull request's message.

However, my branch is both commits ahead and commits behind. I suppose I have to fix that first.
I have the Git label "myrepo" for my fork and "origin" for Mumble repository.
So I tried "git pull origin lowdelay1" (to update my branch), but it didn't work. Says "can't find remote ref lowdelay1". So I tried to do "myrepo/lowdelay1" instead. Still says the same thing.
Why is that?

@davidebeatrici
Copy link
Member

davidebeatrici commented Sep 19, 2019

The remote branch should already be set as upstream, git pull --force should work.

That command will discard your local changes.

@trudnorx
Copy link
Contributor Author

trudnorx commented Sep 20, 2019

The remote branch should already be set as upstream, git pull --force should work.

So what's gone wrong here?
https://github.com/trudnorx/mumble/commits/lowdelay1
How do I fix that?
Seems that some commits got duplicated.

@davidebeatrici
Copy link
Member

I see you pulled from origin/master, which is fine.

The issue is that you don't have Git set to rebase by default, you can do that with: git config --global pull.rebase true

@trudnorx
Copy link
Contributor Author

The issue is that you don't have Git set to rebase by default, you can do that with: git config --global pull.rebase true

But doesn't that affect future pulls only? What should I do right now that this already happened?

@davidebeatrici
Copy link
Member

Assuming that cae8c282c643d9a65437f2dc857bf922fe225d2e is the commit before the merge:

git checkout cae8c282c643d9a65437f2dc857bf922fe225d2e
git pull origin master

@trudnorx
Copy link
Contributor Author

trudnorx commented Sep 20, 2019

So now it's showing a rebase screen with the contents:

pick 05b7580 fix vertical resizing of user local volume dialog
pick daf1997 Auto selects Opus low delay mode
pick 5eb26d4 UI max bitrate set to 192kbit/s
pick d8d6e01 add allow low delay option
pick 7737e6b Clarified "ms" string
pick 71eb446 Syntax
pick 7bfac3a Clarify kb/s
pick cae8c28 Fix typo
pick fd51215 Auto selects Opus low delay mode
pick b52c5de UI max bitrate set to 192kbit/s
pick f8cc043 add allow low delay option
pick 06b4d5a Clarified "ms" string
pick f30a442 Syntax
pick cd288f9 Clarify kb/s
pick a4fa549 Fix typo

Do I just delete the duplicated ones? Which of the two sets? Just at random?
And why is it showing commit 05b7580 if that belongs to some other issue? Do I delete or keep that?

@davidebeatrici
Copy link
Member

It shouldn't show a rebase screen after running the two commands, it should pull from origin/master cleanly.

Was your local branch diverged from the remote one?

@trudnorx
Copy link
Contributor Author

Was your local branch diverged from the remote one?

My branch was commits ahead and commits behind when you told me to squash, as I said, if that's what you mean.

My knowledge of Git is very low, if what you said one year ago about you offering to do the Git part instead of me still applies you're quite welcome to solve this mess yourself if you want.

@davidebeatrici
Copy link
Member

Done.

I assumed you have origin set to the master repository and another remote set to your forked one, is that not the case?

You can check with git remote -v.

@trudnorx
Copy link
Contributor Author

Thank you. This is the output:

$ git remote -v
myrepo https://github.com/trudnorx/mumble (fetch)
myrepo https://github.com/trudnorx/mumble (push)
origin https://github.com/mumble-voip/mumble (fetch)
origin https://github.com/mumble-voip/mumble (push)
upstream https://github.com/mumble-voip/mumble (fetch)
upstream https://github.com/mumble-voip/mumble (push)

@davidebeatrici
Copy link
Member

You're welcome.

Your local repository is configured correctly, next time you want to rebase against master you can simply execute git pull origin master now that Git is configured correctly as well.

@davidebeatrici
Copy link
Member

g.s.bUseOpusMusicEncoding option is made obsolete but not deleted from the SAVELOAD part of the code. Now it selects between VOIP/"Music"/Low Delay mode depending on bitrate. So there are 3 modes now. Before there was only 2 modes with music on/off option. Pull request #3747 still preserved 2 modes but with a better default option. As I stated in #3747, I am unsure whether the VOIP mode is even needed at all for low bitrate cases or should be deleted it completely, but just to play it safe I auto-enabled it with <32kbit/s bitrate.

So just to be clear:
Before #3747: VOIP/"Music" modes are available. VOIP on by default, "Music" can be enabled with hidden option.
After #3747: VOIP/"Music" modes are available. "Music" on by default, VOIP can be enabled hidden option.
After THIS pull request: Low delay/"Music"/VOIP modes available'. If >64kbit/s bitrate, enable Low Delay, else if >32kbit/s bitrate enable "Music", else enable VOIP.

This part is not valid anymore now that the option is removed, right?

@trudnorx
Copy link
Contributor Author

trudnorx commented Sep 20, 2019

The "After THIS pull request" part is no longer valid. Now there is no option to toggle Music/VOIP, so an accurate description is: Low delay/"Music"/VOIP modes available'. If (>64kbit/s bitrate && low delay allowed), enable Low Delay, else if >32kbit/s bitrate enable "Music", else enable VOIP.

Can this be accepted now?

@davidebeatrici
Copy link
Member

I squashed the commits and updated the message.

@davidebeatrici davidebeatrici changed the title Implements & Auto selects Opus low delay mode Implement & auto-select Opus' low delay mode Sep 20, 2019
This commit implements Opus' low delay mode selection, which should result in a big decrease in latency.
The bUseOpusMusicEncoding option has been removed, because now the Opus mode is selected depending on the bitrate. The automatic selection aims to decrease the latency while maintaining acceptable quality.
The exact values may have to be tuned further for a smarter auto-selection because the low delay mode, while causing a strong decrease in latency, requires an higher bitrate to maintain the same quality; in order to compensate, the maximum bitrate in the UI has been increased to 192 kbit/s.

The automatic selection sets:

Low-delay mode when the bitrate is >= 64 kbit/s.
Music mode when the bitrate is >= 32 kbit/s.
VOIP mode when the bitrate is < 32 kbit/s.
@davidebeatrici davidebeatrici merged commit cb7244a into mumble-voip:master Sep 20, 2019
@davidebeatrici
Copy link
Member

Thank you very much for your contribution!

@trudnorx
Copy link
Contributor Author

@davidebeatrici It seems that now there's a duplicate commit in the Mumble repository

@davidebeatrici
Copy link
Member

c8f365e is the pull request's commit, cb7244a is the merge commit.

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

Successfully merging this pull request may close these issues.

None yet

2 participants