Closed niekschoemaker closed 2 weeks ago
When a channel doesn't exist on a mumble server, the call to MUMBLE_ADD_VOICE_TARGET_CHANNEL is ignored (see MumbleClient.cpp#L356).
This causes people not to be able to hear eachother, since the target doesn't get added.
This seems like intended functionality, if you're expecting other functionality maybe try the BY_SERVER_ID variant
Another "solution" would be to simply constantly Call MUMBLE_ADD_VOICE_TARGET_CHANNEL, this however would cause a lot of packets to be sent to the server for no reason, and would probably hit the "MAX_VOICE_TARGET_CHANNELS" limit on the mumble server which is 256, since there's no check if the target is already added anywhere as far a I could tell.
This is the solution I use in pma-voice currently I eventually ™️ plan to swap to the ByServerId to get around some of the aforementioned issues here, relying on channels in the first place just seems like a bad move. They're pretty inflexible and look to have bad perf issues on higher player counts.
Looking at all the code, the check if a channel actually exists doesn't actually seem necessary for correct behavior (see client.cpp#L1011 so removing the check would also fix the issue.
I'm curious on how this is not necessary?
Would this not be a null pointer deref?
When a channel doesn't exist on a mumble server, the call to MUMBLE_ADD_VOICE_TARGET_CHANNEL is ignored (see MumbleClient.cpp#L356).
This causes people not to be able to hear eachother, since the target doesn't get added.
This seems like intended functionality, if you're expecting other functionality maybe try the BY_SERVER_ID variant
The specific problem I have here is that I have a grid system, in which I add the current grid you're in to the voice target and the neighboring grids, but it's not guaranteed that there's any players in that grid, so the channel might not exist.
This then causes the update to be ignored and the plugin itself thinking it already added the channel so not re-adding it anymore.
And both solutions in this case (either re-adding it causing duplicates which causes unecessarry bandwidth, or removing and re-adding) are not the cleanest solutions.
Another "solution" would be to simply constantly Call MUMBLE_ADD_VOICE_TARGET_CHANNEL, this however would cause a lot of packets to be sent to the server for no reason, and would probably hit the "MAX_VOICE_TARGET_CHANNELS" limit on the mumble server which is 256, since there's no check if the target is already added anywhere as far a I could tell.
This is the solution I use in pma-voice currently I eventually ™️ plan to swap to the ByServerId to get around some of the aforementioned issues here, relying on channels in the first place just seems like a bad move. They're pretty inflexible and look to have bad perf issues on higher player counts.
I use the channels only for the grid system, since this requires far less voice target updates and in my experience this increased performance quite a bit over only using player based targets. And speaking of player count, used it succesfully for around 700 players so appears to be working fine that way.
Looking at all the code, the check if a channel actually exists doesn't actually seem necessary for correct behavior (see client.cpp#L1011 so removing the check would also fix the issue.
I'm curious on how this is not necessary?
Would this not be a null pointer deref?
Not as far as I can tell, it would already cause a null pointer deref anyways if that was the case since channels are all temporary so get deleted when no one is in them.
https://github.com/citizenfx/fivem/blob/e8e15540b6ffbea6b1f1c20c4b382d8f4da2c19e/code/components/voip-server-mumble/src/client.cpp#L1010-L1012 And as can be seen here it does a null check right there.
A solution I did figure out is simply removing the target (either channel or player) and then re-adding it again, since this doesn't cause any side effects since updates are only sent every 500 ms and are batched, so updating the same voice target twice only causes one update, in which it sends the entire voice target data.
This then causes the update to be ignored and the plugin itself thinking it already added the channel so not re-adding it anymore.
And both solutions in this case (either re-adding it causing duplicates which causes unnecessary bandwidth, or removing and re-adding) are not the cleanest solutions.
Constantly listening to channels when you don't need to uses a lot more unnecessary bandwidth than re-sending voice target requests, by an order of magnitude. This is the primary reason that pma-voice swapped from grid -> targeting system.
See:
Not as far as I can tell, it would already cause a null pointer deref anyways if that was the case since channels are all temporary so get deleted when no one is in them.
I'm very confused here, in your original post you said
client.cpp#L1011 so removing the check would also fix the issue.
But this gets used on the very next line to add to targeted_channels
, which would be a null pointer deref and crash.
With the current way that it is done this will not cause a null pointer deref because its checked, but you're saying to remove this check to fix this issue
This then causes the update to be ignored and the plugin itself thinking it already added the channel so not re-adding it anymore.
And both solutions in this case (either re-adding it causing duplicates which causes unnecessary bandwidth, or removing and re-adding) are not the cleanest solutions.
Constantly listening to channels when you don't need to uses a lot more unnecessary bandwidth than re-sending voice target requests, by an order of magnitude. This is the primary reason that pma-voice swapped from grid -> targeting system.
See:
Not as far as I can tell, it would already cause a null pointer deref anyways if that was the case since channels are all temporary so get deleted when no one is in them.
I'm very confused here, in your original post you said
client.cpp#L1011 so removing the check would also fix the issue.
But this gets used on the very next line to add to
targeted_channels
, which would be a null pointer deref and crash.With the current way that it is done this will not cause a null pointer deref because its checked, but you're saying to remove this check to fix this issue
Yes, I stand corrected at the removing the check, I got the channel ID parameter completely wrong. So I'll remove that one as an option to begin with.
What happened?
When a channel doesn't exist on a mumble server, the call to MUMBLE_ADD_VOICE_TARGET_CHANNEL is ignored (see MumbleClient.cpp#L356).
This causes people not to be able to hear eachother, since the target doesn't get added.
Expected result
The target should be added or there should be a way to validate the result of the target update.
Reproduction steps
Importancy
Slight inconvenience
Area(s)
FXServer
Specific version(s)
Server 9073 linux/windows
Additional information
For some context, I internally cache which voice targets I have added, to avoid network overhead, this works fine, except for the case given above. I did think of some possible solutions, which I listed below. I'm willing to implement them myself, I'm just not sure which one would be the "most correct" one.
Possible solution:
5. Looking at all the code, the check if a channel actually exists doesn't actually seem necessary for correct behavior (see client.cpp#L1011 so removing the check would also fix the issue.I think option 1 or 4 would be the easiest to implement, but would require some testing to see if there's any implications to this.