[pulseaudio-discuss] [PATCH 1/2] udev-detect, alsa-card: Adopt avoid resampling option from daemon.conf

[Sangchul Lee]

2018-05-17 0:54 GMT+09:00 Tanu Kaskinen <tanuk at iki.fi>:
> 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.

Okay, I'll add more explanation.


> 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?

My intention is to set the avoid-resampling option to module-udev-detect or
module-alsa-card without setting it true of daemon.conf for applying it only to
external devices. With the patch 2/2, it can even be set only to
particular alsa-mapping.
A precondition of these patches is that avoid-resampling option of daemon.conf
is false. Because if it(daemon.conf's) is set to true, all other
options(udev-detect,alsa-card,
alsa-mapping) will get ignored. This was my intention.


>> -    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.)

Sure, I'll revise it too.


>>      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).

Yes, as your comments, it is impossible to "enable avoid-resampling by default
and disabling it on per-card basis". rather it is possible to "disable
avoid-resampling
by default and enabling on per-card basis". I think there is a different view.
I'm willing to revise it as per your comment. But actually it is
difficult to think the
options to be treated all together in daemon.conf. Could you let me know some
more advices on this?


>   Suspending sink %s due to changing format, desired rate = %u
It'll be revised. Thanks.


Best regards,
Sangchul