Expose api functions for enabling and disabling AV in AV groups

[zugz]

see #1303, #1304

---

This change is 

[anthonybilinski]

Reviewable status: 0 of 1 approvals obtained (waiting on @zugz)

---

toxav/groupav.c, line 444 at r1 (raw file):

                        audio_data_cb *audio_callback, void *userdata)
{
    if (group_get_type(g_c, groupnumber) != GROUPCHAT_TYPE_AV

didn't this API use to support converting a text groupchat to an AV groupchat? If so, doesn't this check that the groupchat is GROUPCHAT_TYPE_AV break that functionality?

[anthonybilinski]

This is working well from qTox (but note all groups qTox creates are AV groups with our own audio-disablement logic, so my review comment above may still be valid). Thanks for the speedy fix, zugz :D

No, there's no way in the api to change the type of a group. Since nothing else does, I think we should take the type to in particular dictate the meaning of the group object and the peer objects, which future types may use for different purposes, so then it's important to check it before casting the object as a Group_AV*.

[codecov]

Codecov Report

Merging #1305 into master will increase coverage by 1.6%.
The diff coverage is 97.9%.

@@           Coverage Diff            @@
##           master   #1305     +/-   ##
========================================
+ Coverage    83.3%   84.9%   +1.6%     
========================================
  Files          82      84      +2     
  Lines       14978   15057     +79     
========================================
+ Hits        12484   12798    +314     
+ Misses       2494    2259    -235

Impacted Files	Coverage Δ
auto_tests/run_auto_test.h	100% <100%> (ø)	⬆️
auto_tests/conference_test.c	99.3% <100%> (ø)	⬆️
toxcore/group.c	91.7% <100%> (+12.9%)	⬆️
toxav/toxav_old.c	100% <100%> (ø)
toxav/groupav.c	82% <100%> (ø)
auto_tests/conference_av_test.c	97.1% <97.1%> (ø)
auto_tests/toxav_many_test.c	92.5% <0%> (-4.1%)	⬇️
toxav/msi.c	64.5% <0%> (-4%)	⬇️
toxav/toxav.c	67.6% <0%> (-2.5%)	⬇️
toxcore/friend_connection.c	94.6% <0%> (-2.3%)	⬇️
... and 9 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0aad180...d14989f. Read the comment docs.

[sudden6]

What's the argumentation behind 5980e8e0c192b7e72c4bfec474946055b84b63d5? Why would we want to forward a packet for which we didn't have a handler? Won't this open up a lot of attack vectors?

[sudden6]

Reviewed 3 of 4 files at r1, 5 of 8 files at r2.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @anthonybilinski and @zugz)

---

auto_tests/conference_av_test.c, line 258 at r2 (raw file):

{
    uint64_t start = state[0].clock;
    do {} while (!(test_audio(toxes, state, disabled, true)

This doesn't look very readable to me, can you maybe put this in a more standard loop structure?

---

toxav/groupav.c, line 444 at r1 (raw file):

Previously, anthonybilinski (Anthony Bilinski) wrote…

didn't this API use to support converting a text groupchat to an AV groupchat? If so, doesn't this check that the groupchat is GROUPCHAT_TYPE_AV break that functionality?

I second this issue.

[zugz]

* Sunday, 2019-02-03 at 15:16 -0800 - sudden6 <notifications@github.com>:

What's the argumentation behind 5980e8e0c192b7e72c4bfec474946055b84b63d5? Why would we want to forward a packet for which we didn't have a handler? Won't this open up a lot of attack vectors?

Interesting point. The reason for this change was to ensure that members of an audio group who do not have av enabled (either because they explicitly disabled it, or because they loaded the group from a save file) will still forward audio packets. If they don't, connectivity of the group is broken. The only potential for abuse I see is that it could be used to flood a group with lossy packets, saturating connections without any client-visible sign of the attack. That's already possible in an audio group, because the audio callback doesn't return an error if the packet fails to decode, but it would be new in a text group. These conferences are intended for use by mutually trusting parties, and there are already plenty of ways a malicious member can screw things up, but I agree it's better not to add one. So what I suggest as a fix is simply to check the type of the group, and error out if it's of a type which doesn't expect lossy packets (which currently just means text groups).

[zugz]

* Sunday, 2019-02-03 at 15:16 -0800 - sudden6 <notifications@github.com>:

<details><summary><i>Previously, anthonybilinski (Anthony Bilinski) wrote…</i></summary><blockquote> didn't this API use to support converting a text groupchat to an AV groupchat? If so, doesn't this check that the groupchat is GROUPCHAT_TYPE_AV break that functionality? </blockquote></details> I second this issue.

Is there something I'm missing here? How can you turn a text group into an AV one?

[sudden6]

Is there something I'm missing here? How can you turn a text group into an AV one?

There might really be a misunderstanding here. I thought this was about making text and AV groups equal, by allowing to switch on/off audio, which would convert the group to AV/text respectively. I guess @anthonybilinski also followed this train of thought.

I think you are implementing just enabling/disabling audio for an AV group, without touching text groups at all?

On a second thought your approach also makes sense, because then if you join a group you already know if there will ever be audio or not.

What I'm now also missing is, that one can join an AV group only as listener/speaker. But maybe that could be implemented on the client side.

The only potential for abuse I see is that it could be used to flood a group with lossy packets, saturating connections without any client-visible sign of the attack. That's already possible in an audio group, because the audio callback doesn't return an error if the packet fails to decode, but it would be new in a text group.

I think there might be other possibilities to abuse forwarding arbitrary data through clients, but your argument about the trusting group members also makes sense, so I'm fine with your solution.

Sorry, wrote this already yesterday, but forgot to send...

[zugz]

* Tuesday, 2019-02-05 at 11:13 -0800 - sudden6 <notifications@github.com>:

I thought this was about making text and AV groups equal

Aha! No, not at all.

I think you are implementing just enabling/disabling audio for an AV group, without touching text groups at all?

Yes, precisely.

What I'm now also missing is, that one can join an AV group only as listener/speaker. But maybe that could be implemented on the client side.

Yes. The only advantage I see to doing it in toxcore would be to prevent wasting cycles on decoding audio which isn't going to be played. It wouldn't be so hard to add api calls for doing that, i.e. having a concept in toxcore of an av group being "muted". Do you think it makes sense to do this? I think it would be best left to a separate PR, anyway.

[sudden6]

I think it would be best left to a separate PR, anyway.

I agree. Just to note though, I would extend the API to just introduce a parameter specifying which direction(s) to enable/disable.

[sudden6]

@zugz what about the loop construction?

[zugz]

* Wednesday, 2019-02-06 at 00:23 +0000 - sudden6 <notifications@github.com>:

@zugz what about the loop construction?

Yes, it was a mess. I've cleaned it up, and added the test we discussed of group type when handling lossy packets.

[sudden6]

Reviewed 2 of 8 files at r2, 2 of 2 files at r3.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @anthonybilinski)

---

auto_tests/conference_av_test.c, line 258 at r2 (raw file):

Previously, sudden6 wrote…

This doesn't look very readable to me, can you maybe put this in a more standard loop structure?

ok, much better now

[sudden6]

Reviewed 7 of 7 files at r4.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @anthonybilinski)

[sudden6]

@anthonybilinski do you want to add anything here? If not I'll go ahead and merge this.

[anthonybilinski]

Reviewable status: complete! 1 of 1 approvals obtained

---

toxav/groupav.c, line 444 at r1 (raw file):

Previously, sudden6 wrote…

I second this issue.

you are right about my interpretation, I thought new_group_av was creating a group_av out of a non-av group before. I've got no further issue.