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

New setting "streaming_client.initial_volume" #1024

Merged
merged 4 commits into from
Jul 2, 2022
Merged

New setting "streaming_client.initial_volume" #1024

merged 4 commits into from
Jul 2, 2022

Conversation

nis65
Copy link
Contributor

@nis65 nis65 commented Jun 26, 2022

This resolves #910.

Testcases executed manually:

  • Snapclient connects to snapserver for the first time: initial volume is 100 if not configured in /etc/snapserver.conf (identical to behaviour without this change). Volume is set to configured value when set. When configuration value is invalid, initial value is set to 100 too.
  • Snapclient connects not for the first time (i.e. client already present in /var/lib/snapserver/server.json): Previous volume is unchanged.

Tested successfully both on current master and current develop.

Copy link
Owner

@badaix badaix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks great, but please add the new setting to a new config group, as in my comments.

@@ -156,6 +156,11 @@ source = pipe:///tmp/snapfifo?name=default
# Send audio to muted clients
#send_to_muted = false
#

# Volume assigned to new snapclients [percent]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please create a new config section streaming_client?
Because this is not really related to a stream, but to a client, and there are two kinds of clients: streaming and control.
Maybe this section will be extended later with e.g. black lists, white lists of client IPs or certificates and what not.

Also please rename the setting to initial_volume and keep the comment that the unit is percent, like this:

# streaming client options ####################################################
#
[streaming_client]

# Volume assigned to new snapclients [percent]
# Defaults to 100 if unset
#initial_volume = 100
#
###############################################################################

@@ -27,6 +27,7 @@ Within the `[stream]` section there are some global parameters valid for all `so
- `chunk_ms`: Default source stream read chunk size [ms]. The server will continously read this number of milliseconds from the source into a buffer, before this buffer is passed to the encoder (the `codec` above)
- `buffer`: Buffer [ms]. The end-to-end latency, from capturing a sample on the server until the sample is played-out on the client
- `send_to_muted`: `true` or `false`: Send audio to clients that are muted
- `initial_client_percent`: 0-100 [percent]: The volume an audio client gets assigned on very first connect (i.e. client not known to snapserver yet). Defaults to 100 if unset or invalid (e.g. out of bounds).
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change according to the requested changes for server/etc/snapserver.conf (below).

@@ -63,6 +63,7 @@ struct ServerSettings
size_t streamChunkMs{20};
bool sendAudioToMutedClients{false};
std::vector<std::string> bind_to_address{{"0.0.0.0"}};
uint16_t initialClientPercent{100};
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a new struct StreamingClient with uint16_t initialVolume

@nis65
Copy link
Contributor Author

nis65 commented Jul 1, 2022

Thanks a lot for your feedback and especially for the architecture clarification. Please have another look 😄

@nis65 nis65 changed the title New setting "stream.initial_client_percent" New setting "streaming_client.initial_volume" Jul 1, 2022
@badaix badaix merged commit afe6276 into badaix:develop Jul 2, 2022
@badaix
Copy link
Owner

badaix commented Jul 2, 2022

Tack så mycket! 👍

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

Successfully merging this pull request may close these issues.

2 participants