On Tue, 2018-05-08 at 01:54 +0900, Sangchul Lee wrote:
> Previously, the "avoid-resampling" option of daemon.conf is to make
> the daemon try to use the stream sample rate if possible for all the
> audio device regardless of built-in or not.
>
> This patch applies this option to audio devices - generally external
> devices - which are detected by module-udev-detect.
>
> To set it, use "avoid_resampling=true" as the module argument.
>
> Signed-off-by: Sangchul Lee <sc11.lee at samsung.com>
The commit message doesn't really explain why you want to set the
option for external devices. You explained the rationale to me in IRC,
but it needs to get to the commit message as well.
Also, the commit message says that the intention is to use the module
argument with external devices, but I don't understand how you're
really planning to use the option. The option for module-udev-detect
isn't very useful, because it will apply to all detected sound cards,
so it's basically the same as the option in daemon.conf. If you're not
using module-udev-detect, how do you load module-alsa-card instances?
You could have a static module-alsa-card in default.pa for the internal
sound card, but how do you load module-alsa-card for USB sound cards
without using module-udev-detect?
> ---
> src/modules/alsa/alsa-sink.c | 9 ++++++++-
> src/modules/alsa/alsa-source.c | 9 ++++++++-
> src/modules/alsa/module-alsa-card.c | 2 ++
> src/modules/module-udev-detect.c | 16 ++++++++++++++--
> src/pulsecore/sink.c | 6 ++++--
> src/pulsecore/sink.h | 2 ++
> src/pulsecore/source.c | 6 ++++--
> src/pulsecore/source.h | 2 ++
> 8 files changed, 44 insertions(+), 8 deletions(-)
>
> diff --git a/src/modules/alsa/alsa-sink.c b/src/modules/alsa/alsa-sink.c
> index 4568a19..464eb17 100644
> --- a/src/modules/alsa/alsa-sink.c
> +++ b/src/modules/alsa/alsa-sink.c
> @@ -2100,7 +2100,7 @@ pa_sink *pa_alsa_sink_new(pa_module *m, pa_modargs *ma, const char*driver, pa_ca
> uint32_t nfrags, frag_size, buffer_size, tsched_size, tsched_watermark, rewind_safeguard;
> snd_pcm_uframes_t period_frames, buffer_frames, tsched_frames;
> size_t frame_size;
> - bool use_mmap = true, b, use_tsched = true, d, ignore_dB = false, namereg_fail = false, deferred_volume = false, set_formats = false, fixed_latency_range = false;
> + bool use_mmap = true, b, use_tsched = true, d, ignore_dB = false, namereg_fail = false, deferred_volume = false, set_formats = false, fixed_latency_range = false, avoid_resampling = false;
This line was too long already, could you put each variable on its own
line? (Same for the code in alsa-source.c.)
The initial value of the variable should be set to m->core-
>avoid_resampling, because otherwise the daemon.conf option gets
ignored. You can't set the initial value here, because you'll need to
dereference m, but pa_assert(m) is located some lines below, and
dereferencing should be avoided before the assertion.
> pa_sink_new_data data;
> bool volume_is_set;
> bool mute_is_set;
> @@ -2368,6 +2368,13 @@ pa_sink *pa_alsa_sink_new(pa_module *m, pa_modargs *ma, const char*driver, pa_ca
> }
> data.namereg_fail = namereg_fail;
>
> + if (pa_modargs_get_value_boolean(ma, "avoid_resampling", &avoid_resampling) < 0) {
> + pa_log("Failed to parse avoid_resampling argument.");
> + pa_sink_new_data_done(&data);
> + goto fail;
> + }
> + data.avoid_resampling = avoid_resampling;
> +
> pa_sink_new_data_set_sample_spec(&data, &ss);
> pa_sink_new_data_set_channel_map(&data, &map);
> pa_sink_new_data_set_alternate_sample_rate(&data, alternate_sample_rate);
> diff --git a/src/modules/alsa/alsa-source.c b/src/modules/alsa/alsa-source.c
> index 8014bc0..aa54eaa 100644
> --- a/src/modules/alsa/alsa-source.c
> +++ b/src/modules/alsa/alsa-source.c
> @@ -1794,7 +1794,7 @@ pa_source *pa_alsa_source_new(pa_module *m, pa_modargs *ma, const char*driver, p
> uint32_t nfrags, frag_size, buffer_size, tsched_size, tsched_watermark;
> snd_pcm_uframes_t period_frames, buffer_frames, tsched_frames;
> size_t frame_size;
> - bool use_mmap = true, b, use_tsched = true, d, ignore_dB = false, namereg_fail = false, deferred_volume = false, fixed_latency_range = false;
> + bool use_mmap = true, b, use_tsched = true, d, ignore_dB = false, namereg_fail = false, deferred_volume = false, fixed_latency_range = false, avoid_resampling = false;
> pa_source_new_data data;
> bool volume_is_set;
> bool mute_is_set;
> @@ -2045,6 +2045,13 @@ pa_source *pa_alsa_source_new(pa_module *m, pa_modargs *ma, const char*driver, p
> }
> data.namereg_fail = namereg_fail;
>
> + if (pa_modargs_get_value_boolean(ma, "avoid_resampling", &avoid_resampling) < 0) {
> + pa_log("Failed to parse avoid_resampling argument.");
> + pa_source_new_data_done(&data);
> + goto fail;
> + }
> + data.avoid_resampling = avoid_resampling;
> +
> pa_source_new_data_set_sample_spec(&data, &ss);
> pa_source_new_data_set_channel_map(&data, &map);
> pa_source_new_data_set_alternate_sample_rate(&data, alternate_sample_rate);
> diff --git a/src/modules/alsa/module-alsa-card.c b/src/modules/alsa/module-alsa-card.c
> index b193d40..d15eaf6 100644
> --- a/src/modules/alsa/module-alsa-card.c
> +++ b/src/modules/alsa/module-alsa-card.c
> @@ -68,6 +68,7 @@ PA_MODULE_USAGE(
> "profile_set=<profile set configuration file> "
> "paths_dir=<directory containing the path configuration files> "
> "use_ucm=<load use case manager> "
> + "avoid_resampling=<use stream original sample rate if possible?> "
> );
>
> static const char* const valid_modargs[] = {
> @@ -95,6 +96,7 @@ static const char* const valid_modargs[] = {
> "profile_set",
> "paths_dir",
> "use_ucm",
> + "avoid_resampling",
> NULL
> };
>
> diff --git a/src/modules/module-udev-detect.c b/src/modules/module-udev-detect.c
> index beac9d4..32c4369 100644
> --- a/src/modules/module-udev-detect.c
> +++ b/src/modules/module-udev-detect.c
> @@ -46,7 +46,8 @@ PA_MODULE_USAGE(
> "fixed_latency_range=<disable latency range changes on underrun?> "
> "ignore_dB=<ignore dB information from the device?> "
> "deferred_volume=<syncronize sw and hw volume changes in IO-thread?> "
> - "use_ucm=<use ALSA UCM for card configuration?>");
> + "use_ucm=<use ALSA UCM for card configuration?> "
> + "avoid_resampling=<use stream original sample rate if possible?>");
>
> struct device {
> char *path;
> @@ -67,6 +68,7 @@ struct userdata {
> bool ignore_dB:1;
> bool deferred_volume:1;
> bool use_ucm:1;
> + bool avoid_resampling:1;
>
> uint32_t tsched_buffer_size;
>
> @@ -85,6 +87,7 @@ static const char* const valid_modargs[] = {
> "ignore_dB",
> "deferred_volume",
> "use_ucm",
> + "avoid_resampling",
> NULL
> };
>
> @@ -401,6 +404,7 @@ static void card_changed(struct userdata *u, struct udev_device *dev) {
> "ignore_dB=%s "
> "deferred_volume=%s "
> "use_ucm=%s "
> + "avoid_resampling=%s "
> "card_properties=\"module-udev-detect.discovered=1\"",
> path_get_card_id(path),
> n,
> @@ -409,7 +413,8 @@ static void card_changed(struct userdata *u, struct udev_device *dev) {
> pa_yes_no(u->fixed_latency_range),
> pa_yes_no(u->ignore_dB),
> pa_yes_no(u->deferred_volume),
> - pa_yes_no(u->use_ucm));
> + pa_yes_no(u->use_ucm),
> + pa_yes_no(u->avoid_resampling));
> pa_xfree(n);
>
> if (u->tsched_buffer_size_valid)
> @@ -682,6 +687,7 @@ int pa__init(pa_module *m) {
> int fd;
> bool use_tsched = true, fixed_latency_range = false, ignore_dB = false, deferred_volume = m->core->deferred_volume;
> bool use_ucm = true;
> + bool avoid_resampling = false;
Again, you shouldn't initialize the variable here. It should be
initialized later to the value of m->core->avoid_resampling.
>
> pa_assert(m);
>
> @@ -734,6 +740,12 @@ int pa__init(pa_module *m) {
> }
> u->use_ucm = use_ucm;
>
> + if (pa_modargs_get_value_boolean(ma, "avoid_resampling", &avoid_resampling) < 0) {
> + pa_log("Failed to parse avoid_resampling= argument.");
> + goto fail;
> + }
> + u->avoid_resampling = avoid_resampling;
> +
> if (!(u->udev = udev_new())) {
> pa_log("Failed to initialize udev library.");
> goto fail;
> diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
> index 38e8e50..40ad14f 100644
> --- a/src/pulsecore/sink.c
> +++ b/src/pulsecore/sink.c
> @@ -271,6 +271,8 @@ pa_sink* pa_sink_new(
> else
> s->alternate_sample_rate = s->core->alternate_sample_rate;
>
> + s->avoid_resampling = data->avoid_resampling;
> +
> s->inputs = pa_idxset_new(NULL, NULL);
> s->n_corked = 0;
> s->input_to_master = NULL;
> @@ -1444,7 +1446,7 @@ int pa_sink_reconfigure(pa_sink *s, pa_sample_spec *spec, bool passthrough) {
> pa_sink_input *i;
> bool default_rate_is_usable = false;
> bool alternate_rate_is_usable = false;
> - bool avoid_resampling = s->core->avoid_resampling;
> + bool avoid_resampling = s->core->avoid_resampling || s->avoid_resampling;
Oh, this is how you planned to solve the "daemon.conf gets ignored"
problem. I think it's better to treat the daemon.conf option as a
default and override it with more fine-grained options, rather than
merging all options together. Your approach makes it impossible to
enable avoid-resampling by default and disabling it on per-card basis
(which is what I thought you wanted to do in the first place).
>
> /* We currently only try to reconfigure the sample rate */
>
> @@ -1512,7 +1514,7 @@ int pa_sink_reconfigure(pa_sink *s, pa_sample_spec *spec, bool passthrough) {
> if (!passthrough && pa_sink_used_by(s) > 0)
> return -1;
>
> - pa_log_debug("Suspending sink %s due to changing format.", s->name);
> + pa_log_debug("Suspending sink %s due to changing format, desired rate(%u)", s->name, desired_spec.rate);
Nitpicking: I think this would look better:
Suspending sink %s due to changing format, desired rate = %u
--
Tanu
https://liberapay.com/tanuk
https://www.patreon.com/tanuk
