Added bitrate slider for qTox

[ghost]

Also has code for a staging PR which would allow for (already tested) group calls (removed)

It also fixes group calls for #3359 with std::map

---

This change is 

[ghost]

Done. I also removed some commented out code that relies on a related PR in TokTok/c-toxcore (applying bit rate to group chats). As it stands now it should be ready.

[yurivict]

You also use tabs in places where in qTox they don't use tabs. And you also left empty space in the ends of some lines.

[ghost]

Electric indent must have been on accidentally, will fix that

[ghost]

I only found problems in audio/audio.cpp
let me know if I missed anything

[yurivict]

There is trailing space after this ToxGroupCall& call = groupCalls[groupId]; and subsequent lines.

[ghost]

Fixed that. Searched directory with rgrep and fixed two other occurances.

[yurivict]

grep -rn "[[:space:]]$" *

[ghost]

I used git grep -i -n -I -E '^.+[ ]+$' and it seemed to work fine, yours doesn't return any files I touched

[yurivict]

I am looking at it, and it isn't clear what it does.

• Why does it begin at 0-level? Does it mean that bitrate is zero by default?
• What is the value? I think you need to print the value that you limit bitrate to, like to the right of the slider.

Is the purpose to limit the bitrate? Does the quality drop when the slider is moved to the left? You need to explain this in the tooltip.

[sudden6]

Review status: 0 of 10 files reviewed at latest revision, 1 unresolved discussion.

---

src/audio/audio.cpp, line 94 at r1 (raw file):

    qreal   minInputGain;
    qreal   maxInputGain;
    qreal   minBitRate;

why are these floats?

---

Comments from Reviewable

[ghost]

I changed the tooltip. I have a concern about the audio namespace. The QMutexLock has to be disabled for bitRate(), otherwise there is a double lock situation with the frameAvailable signal and the bitRate() call. What would be a better way of implementing this, or does the function need to change at all?

[yurivict]

You need to say "6 to 64 kb/s" in the tooltip to be more informative.
You should print the current value to the right of the slider: NN kb/s
The value is also not persistent, unlike the "Gain" slider above it.

[ghost]

I fixed all of the double and int conflicts, and I modified the tooltip to be more informative. What would you recommend I do for the slider?

[yurivict]

• Fix persistence
• Fix tabstop (not a tabstop)
• Have a label to the right showing the current value.

[sudden6]

Reviewed 1 of 10 files at r1.
Review status: 1 of 12 files reviewed at latest revision, 7 unresolved discussions.

---

src/audio/audio.cpp, line 172 at r2 (raw file):

    connect(&playMono16Timer, &QTimer::timeout, this, &Audio::playMono16SoundCleanup);
    playMono16Timer.setSingleShot(true);
    setBitRate(64.0);

make this an int too

---

src/audio/audio.cpp, line 334 at r2 (raw file):

 */

void Audio::setMaxBitRate(qint32 maxBitRate)

min and max bitrates are limits of Opus and won't change, right? if so make the getters static and and remove the setters

---

src/audio/audio.cpp, line 354 at r2 (raw file):

 */

void Audio::setMinBitRate(qint32 minBitRate)

same

---

src/core/toxcall.cpp, line 195 at r2 (raw file):

}

ToxGroupCall::~ToxGroupCall()

this change doesn't seem to have anything to do with audio bitrates, please open a new PR if it was an intended change

---

src/core/toxcall.h, line 7 at r2 (raw file):

#include <QtGlobal>
#include <QMetaObject>
#include <map>

same as above

---

src/persistence/settings.h, line 119 at r2 (raw file):

    Q_PROPERTY(int outVolume READ getOutVolume WRITE setOutVolume
               NOTIFY outVolumeChanged FINAL)
    // new

this comment isn't really helpful, change or remove

---

src/widget/widget.cpp, line 1750 at r2 (raw file):

    else if (change == TOX_CONFERENCE_STATE_CHANGE_PEER_EXIT)
    {
        CoreAV::groupNameListChanged(groupnumber, peernumber);

should probably not be part of this PR

---

Comments from Reviewable

[ghost]

Those seemingly unrelated pieces was a fix for OpenAL to allow for multiple speakers which somebody pushed to my fork to help me out (in reference to the second line of the first post). I will ask him to take it out and to file it under a new PR himself.

[sudden6]

you mean #4138 ?

[ghost]

#4138 was created while we were talking about the fix in the qtox IRC. The code was already written at that time, so we didn't remove it.

[agrecascino]

@sudden6 similar purpose, different person(me)

[ghost]

I can confirm that agrecascino did the OpenAL work

[sudden6]

ok, because the comments and reactions are pretty confusing over there...

[yurivict]

The Opus website says "Bitrates from 6 kb/s to 510 kb/s" http://opus-codec.org
Why do you limit to 64 kb/s then?

[ghost]

I put the limit at 64kbps because qTox said somewhere in the source code that it was plenty. I forgot to change the avform.ui file, but the getters and setters for the bitRateSlider should override the file and allow for higher encoding rates. I will update avform.ui to have the proper default settings after agrecascino finishes with removing the OpenAL code (will rebase as well).

[ghost]

The code as it sits now should solve all concerns and questions posted before it. OpenAL code has been removed and the usable bitrate in qTox is the entire Opus spec (6kb/s to 510kb/s). There is also a number field next to the slider (not tested yet).

EDIT: oh darn, forgot the persistence, fixing that now

[sudden6]

Reviewed 5 of 11 files at r3.
Review status: 3 of 10 files reviewed at latest revision, 9 unresolved discussions.

---

src/audio/audio.cpp, line 95 at r3 (raw file):

    qreal   maxInputGain;
    qint32   minBitRate;
    qint32   maxBitRate;

Since there are static getter functions, these two are now unnecessary.

---

src/core/coreav.h, line 83 at r3 (raw file):

                                  void* core);

extra line

---

src/widget/form/settings/avform.cpp, line 84 at r3 (raw file):

    bitRateSlider->setToolTip(
                tr("Use slider to control Opus bitrate encoding from 6kb/s to"

Can you fill in the numbers with the actual min and max?

---

src/widget/form/settings/avform.cpp, line 94 at r3 (raw file):

    bitRateSpinBox->setToolTip(
                tr("Use slider to control Opus bitrate encoding from 6kb/s to"

same

---

Comments from Reviewable

[sudden6]

I think with the last problem fixed this PR is good. It will be on hold until TokTok/c-toxcore#464 is merged

---

Reviewed 2 of 11 files at r3, 4 of 5 files at r4.
Review status: 9 of 10 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

src/core/coreav.h, line 83 at r3 (raw file):

Previously, sudden6 wrote…

extra line

there's still an unnecessary extra line here

---

Comments from Reviewable

[ghost]

Sounds good to me. This code doesnt currently rely on that pull request, so i can push this after the persistence fix and apply it to groups later

[zetok]

PR needs to be rebased, since there are now some conflicts.

[sudden6]

Your last commit contains a lot of stuff that shouldn't get committed.

[ghost]

Yeah, I noticed that and i'm working on that now. I'll take down the last commit and put the final version up

[Diadlo]

Merge went wrong :)

[ghost]

whoops. everything should be fixed now and rebased. If something else needs to be done let me know

[Diadlo]

Well... 'Commits 119'
Something went wrong №2

[ghost]

This is the first time that I had to rebase after so many commits. Will look at this in about three hours.

[Diadlo]

@Dako300 Tip: create new branch for every new pull request to avoid problems like this.
Seems, you need to do:

git fetch origin master
git checkout master
git rebase -i origin/master

And change pick on drop for not yours commits