Add client setting to toggle internet sound streams

[patrikjuvonen]

Summary

• Add new enabled reason to onClientSoundStarted event, which is returned on a sound element if the setting gets enabled
• Add new disabled reason to onClientSoundStopped event, which is returned on a sound element if the setting gets disabled
• Add new allow_internet_sound_streams client setting
  • Enabled by default
  • Reasoning
    • Decreases network bandwidth consumption when scripts use high quality internet streams by default
    • Should primarily be the choice of the client to allow 3rd party connections, and not make it too confusing or time-consuming
  • Upon enable will create missing sound streams bound to sound elements
  • Upon disable will destroy existing sound streams bound to sound elements
  • Sound elements themselves will not be destroyed, meaning no change is required in scripts for this to work out of the box. If a script needs to know whether a sound is stopped or setting gets disabled, they can use the new function or started/stopped event reason parameter.

Requesting client feedback

1. Should this be changed to work with the existing whitelisted URLs setting, rather than (only) introduce a "whitelist all" setting for internet streams (some servers have radio lists of well over 100 internet radios and they keep changing)
2. If we don't want this to be mixed with the whitelisted URLs setting, or we want to introduce a global setting on top of adding whitelisted URLs to this (point above), what would be the best location for the global setting in the settings menu? Right now I've put it under the default Multiplayer tab, but it could make more sense in Audio tab, but there is not enough space. I would have to create a new tab panel inside the Audio tab to make everything fit (considering localization...)

Requesting scripting feedback

1. Should onClientSoundStarted be triggered if the sound is not actually started upon playSound or playSound3D? Right now 3D sounds that are outside the streaming range, or sounds that have been disabled using this new setting trigger the event and reason given is play, although neither are actually playing. Changing this behavior might have a chance of breaking some script that makes use of this event despite if it plays or not.
2. I assume this breaks if the stream has the game server address / IP, which I think is still a valid stream to allow since it is "trusted" by the client upon join. Maybe the server IP should still be whitelisted? I believe this point is relevant to point 1 of client feedback above...

cc @Dutchman101

[Dutchman101]

I think some things in here have been overthinked, although the results of that mean integration has been done properly for once, and offering good ways for scripters to notice a client disabled streams.
Perhaps though, this stalled review.

Client feedback:

1. I don't think a whitelist like we use for CEF should be used at all.
   The main use for this change are bandwidth control and opt-in hardened security for conscious users (I won't elaborate, but certain unspecified factors also drive this PR).
   Those that are interested in using it for bandwidth control, generally are so limited that they have no flexibility (not a real benefit in picking 'low usage' streams to whitelist).
   When it comes to this, perhaps in future we could sample the bitrate of streams and add options to block "medium, high" usage categories.

2. I believe it fits in nicely next to options like 'Allow screen upload'. It also has more to do with networking than sound, so I think it's fine like you did it.

Scripting feedback:

1. Let's not risk breaking scripts, I think the functionality as per your draft is just fine.
2. This would stand in the way of players interested in ensuring that no more bandwidth is used than they can be certain of (game, sync, resource download).
   Servers can just use a local sound file (in resource) instead of a remote stream in that case.

I also think the lack of other views means your draft's technical concept is fine. I propose that this gets merged if no other opinions get posted in the next couple of days. There's a certain priority to it as well (read: part of my first point 1)

Great job @patrikjuvonen

[qaisjp]

Add new allow_internet_sound_streams client setting

• Enabled by default

• Reasoning

  • Decreases network bandwidth consumption when scripts use high quality internet streams by default
  • Should primarily be the choice of the client to allow 3rd party connections, and not make it too confusing or time-consuming

Enabled by default sounds good.

Reducing bandwidth consumption has its own unique set of challenges, and I think we should stick to improving user privacy in this pull request. After all, nothing stops the server from sending a large amount of data through other means.

For simplicity's sake, I think we should call this "allow_external_sounds".

• Add new enabled reason to onClientSoundStarted event, which is returned on a sound element if the setting gets enabled
• Add new disabled reason to onClientSoundStopped event, which is returned on a sound element if the setting gets disabled

This could eliminated by only apply the setting on reconnect.

• Add new isSoundStopped client audio function, which returns a boolean representing whether or not the sound is actually playing

MTA currently destroys a sound when it stops, and I think it would be nice for us to have more control over this behaviour — i.e. to have handles to audio sources regardless of whether or not it is currently playing.

Instead of overlapping with that "potential future feature" in this PR, maybe the client setting should just be exposed via a generic function like getClientSetting/s (name is an example). Or areExternalSoundsEnabled()... or something.

• Sound elements themselves will not be destroyed, meaning no change is required in scripts for this to work out of the box.

What if we just swapped audio with a 30s (or other appropriately sized) blank audio file with volume=0?

Also having paused=true may be desirable, but would stray from the defaults and might cause problems.

Should this be changed to work with the existing whitelisted URLs setting, rather than (only) introduce a "whitelist all" setting for internet streams (some servers have radio lists of well over 100 internet radios and they keep changing)

Personally I am currently indifferent to having a (sound) whitelist. This can always be explored later.

I assume this breaks if the stream has the game server address / IP, which I think is still a valid stream to allow since it is "trusted" by the client upon join. Maybe the server IP should still be whitelisted? I believe this point is relevant to point 1 of client feedback above...

I think this should be permitted as it's not an "external" file, and therefore not a privacy risk.

[Dutchman101]

Reducing bandwidth consumption has its own unique set of challenges, and I think we should stick to improving user privacy in this pull request. After all, nothing stops the server from sending a large amount of data through other means.

I don't agree, remote streams are the main reason MTA's bandwidth usage often exceeds the base game's usage (game, sync, resource download) at a continuous pace, as a lot of streams can play infinitely, even without being noticed (lowered MTA volume), or on a lot of servers practically at the hands of other nearby players. This is speaking from the reality of common gamemodes found on servers these days.

It's not said that the new option guarantees that nothing else takes up bandwidth (like what the server sends), but it's a tool for the user to decrease the majority of the surface it happens through.
Also, adding controls (or measures) for further bandwidth optimization not related to sound remains possible in future, but not being able to cover it all right now doesn't mean we shouldn't advertise a toggle like this as a good way to lower bandwidth consumption.

I think that bandwidth and security (privacy) should both matter.

I assume this breaks if the stream has the game server address / IP, which I think is still a valid stream to allow since it is "trusted" by the client upon join. Maybe the server IP should still be whitelisted? I believe this point is relevant to point 1 of client feedback above...

I think this should be permitted as it's not an "external" file, and therefore not a privacy risk.

because I disagree with your opinion that only privacy matters in this PR, I think that remote streams (not local sound files) should also be covered, as I clarified in my previous comment otherwise users that toggled it to spare bandwidth can still be drained despite their efforts to stop that from happening through remote streams.

• Add new enabled reason to onClientSoundStarted event, which is returned on a sound element if the setting gets enabled
• Add new disabled reason to onClientSoundStopped event, which is returned on a sound element if the setting gets disabled

This could eliminated by only apply the setting on reconnect.

Good point

• Add new isSoundStopped client audio function, which returns a boolean representing whether or not the sound is actually playing

MTA currently destroys a sound when it stops, and I think it would be nice for us to have more control over this behaviour — i.e. to have handles to audio sources regardless of whether or not it is currently playing.

Instead of overlapping with that "potential future feature" in this PR, maybe the client setting should just be exposed via a generic function like getClientSetting/s (name is an example). Or areExternalSoundsEnabled()... or something.

• Sound elements themselves will not be destroyed, meaning no change is required in scripts for this to work out of the box.

What if we just swapped audio with a 30s (or other appropriately sized) blank audio file with volume=0?

Also having paused=true may be desirable, but would stray from the defaults and might cause problems.

My thoughts on these 3 quotes:
I think that, due to present concerns, the last thing we need to do now is shove this onto the long run, by setting all kinds of possible goals that also cover things unrelated to streams. Everyone wants an ideal world, or a project where everything integrates seamlessly with everything else, but who is going to do it? There's not a lot of development capacity available lately.

[patrikjuvonen]

For simplicity's sake, I think we should call this "allow_external_sounds".
~@qaisjp

I agree, I've changed the name.

---

Add new enabled reason to onClientSoundStarted event, which is returned on a sound element if the setting gets enabled
Add new disabled reason to onClientSoundStopped event, which is returned on a sound element if the setting gets disabled

This could eliminated by only apply the setting on reconnect.
~@qaisjp

Indeed, however, I think the fact that scripts and their purpose, users and their networks come in so many different shapes and sizes and everything keeps changing constantly I think making this setting have instantaneous effect is a welcome user-friendly feature and this current proposal supports scripts well using already existing events and parameters. Also I must remind that the reason parameter itself is not new to these events, just two new values that have been added to support this new setting.

---

MTA currently destroys a sound when it stops, and I think it would be nice for us to have more control over this behaviour — i.e. to have handles to audio sources regardless of whether or not it is currently playing.
~@qaisjp

Yes, but a sound element may exist but not actually be playing if not streamed in, or some other odd reason. In this proposal changing this new setting does not affect the sound element; only the audio handle, which is extremely nice to have since it doesn't require any change in any scripts to work dynamically.

---

Instead of overlapping with that "potential future feature" in this PR, maybe the client setting should just be exposed via a generic function like getClientSetting/s (name is an example). Or areExternalSoundsEnabled()... or something.
~@qaisjp

I believe we have to look at this as it is right now:

Adding a new getClientSettings function has been discussed many times on Discord, and I also support such function. The points for such new function have been more or less as follows:

• to maybe eventually get rid of all the separate client setting getter functions since they become more or less redundant, keeping in mind however having them is scripting-friendly, so just needs consensus
• to just provide a generic function for client settings, especially new ones so we won't necessarily have to write alias functions for every single (small) one

We have yet to develop such feature, and is not exactly in scope of this pull request. Also it wouldn't really affect the functionality of this new feature, but would rather make scripting cases where external sounds are disabled slightly more convenient. Should we decide to merge this PR in, I believe implementing such function afterwards is just fine, but whichever is ok imho.

---

Sound elements themselves will not be destroyed, meaning no change is required in scripts for this to work out of the box.

What if we just swapped audio with a 30s (or other appropriately sized) blank audio file with volume=0?
Also having paused=true may be desirable, but would stray from the defaults and might cause problems.
~@qaisjp

I believe you must've misunderstood my point. My point was meant as a good effect: client doesn't want sound played -> audio handle will not be created (or will be destroyed). Playing a blank audio file would consume resources for no reason and sounds like a hack (no pun intended :D)

---

I assume this breaks if the stream has the game server address / IP, which I think is still a valid stream to allow since it is "trusted" by the client upon join. Maybe the server IP should still be whitelisted? I believe this point is relevant to point 1 of client feedback above...
~@patrikjuvonen

@Dutchman101 pointed out that bandwidth consumption would still be an issue.
Let's keep it simple and not make any exceptions.

---

Here is the summary of the previous discussion.

Summary

• Rename the setting from allow_internet_sound_streams to allow_external_sounds and update related texts to reflect the change
• We still don't have a getClientSetting function in MTA
• All external sounds will be affected by the setting, game server is no exception (bandwidth consumption)

[qaisjp]

[... ] but not being able to cover it all right now doesn't mean we shouldn't advertise a toggle like this as a good way to lower bandwidth consumption.

I think that bandwidth and security (privacy) should both matter.

I think that, due to present concerns, the last thing we need to do now is shove this onto the long run, by setting all kinds of possible goals that also cover things unrelated to streams. Everyone wants an ideal world, or a project where everything integrates seamlessly with everything else, but who is going to do it? There's not a lot of development capacity available lately.
~ @Dutchman101

Both bandwidth and security do matter, but my reasoning is that we should carefully evaluate how we want to optimise bandwidth consumption, instead of just applying changes to achieve this. I agree that being able to toggle sound streams is one of the solutions, but it should be discussed and decided upon separately.

Please refer to #887 for (a little) more explanatory information about why future planning is important.

Since this is literally the difference between a stream connecting to the gameserver vs. not connecting to the gameserver, I'll yield! It doesn't really matter either way and is ultimately a minor difference.

instantaneous effect is a welcome user-friendly feature and this current proposal supports scripts well using already existing events and parameters. Also I must remind that the reason parameter itself is not new to these events, just two new values that have been added to support this new setting.
~ @patrikjuvonen

Fair! And my bad - forgot about the reason parameter. enabled and disabled sounds like it'll work.

Adding a new getClientSettings function has been discussed many times on Discord, and I also support such function.
Should we decide to merge this PR in, I believe implementing such function afterwards is just fine, but whichever is ok imho.
~ @patrikjuvonen

Merging this before the introduction of getClientSettings is not ideal, but is not the end of the world. (But there's also no reason for this PR to not wait, since most players are only supposed to get this feature when the next MTA version is released.)

I strongly believe that isSoundStopped (the naming) may cause confusion/problems in the future, however, and would strongly advise against keeping it in its current form.

[patrikjuvonen]

Only thing left in this PR is to rename the isSoundStopped to something else or remove it maybe.

[patrikjuvonen]

I have decided to remove the isSoundStopped function. Let's rather make a generic client config getter function that lists whether external sounds are enabled or not!

[qaisjp]

Causes #1096