[pulseaudio-discuss] [PATCH 3/3] alsa-sink/source, sink, source: Consider sample format for avoid-resampling

[Tanu Kaskinen]

On Tue, 2018-07-10 at 23:48 +0900, Sangchul Lee wrote:
> Thanks for the comments.
> Before sharing new patchset, I have two questions about your comment. :)

Sorry for the delay in replying, I'm quite a bit behind on the mailing
list mails...

> > > @@ -113,11 +113,20 @@ struct userdata {
> > > 
> > >      pa_sample_format_t *supported_formats;
> > >      unsigned int *supported_rates;
> > > +    struct {
> > > +        pa_sample_spec sample_spec;
> > > +        size_t fragment_size;
> > > +        size_t nfrags;
> > > +        size_t tsched_size;
> > > +        size_t tsched_watermark;
> > > +        size_t rewind_safeguard;
> > > +    } initial_info;
> > 
> > I'm not sure this is needed. What breaks if you don't save the initial
> > info?
> > > +    if (u->initial_info.sample_spec.rate == ss->rate && u->initial_info.sample_spec.format == ss->format) {
> > > +        /* use initial values including module arguments */
> > > +        u->fragment_size = u->initial_info.fragment_size;
> > > +        u->hwbuf_size = u->initial_info.nfrags * u->fragment_size;
> > > +        u->tsched_size = u->initial_info.tsched_size;
> > > +        u->tsched_watermark = u->initial_info.tsched_watermark;
> > > +        u->rewind_safeguard = u->initial_info.rewind_safeguard;
> > > +    } else {
> > > +        u->fragment_size = pa_usec_to_bytes(u->core->default_fragment_size_msec * PA_USEC_PER_MSEC, ss);
> > > +        u->hwbuf_size = u->core->default_n_fragments * u->fragment_size;
> > > +        u->tsched_size = pa_usec_to_bytes(DEFAULT_TSCHED_BUFFER_USEC, ss);
> > > +        u->tsched_watermark = pa_usec_to_bytes(DEFAULT_TSCHED_WATERMARK_USEC, ss);
> > > +        u->rewind_safeguard = PA_MAX(DEFAULT_REWIND_SAFEGUARD_BYTES, pa_usec_to_bytes(DEFAULT_REWIND_SAFEGUARD_USEC, ss));
> > > +    }
> > 
> > The module arguments should be taken into account where applicable also
> > when changing the rate or format
> 
> I though these parameters are related to user preference(used for
> tuning) with a specific format and rate from module argument.
> So I intended to keep these values only for the initial format and
> rate from module argument. Therefore default variables are used
> when the format or rate is changed to another.
> As per your comment, does it need to be changed to keep using original
> variables(frag size, tsched size, and so on..) of module argument
> in case of the change of format or rate?

Yes, I think it's best to use the same logic when setting up the
initial parameters and when setting up the changed format parameters.
Your code can drastically change the latency, for example, which is not
good.

If the user has explicitly requested a particular sample format, then
it would make sense to not do any automatic format changes. If buffer
size related module arguments are used, those don't need to be kept
exactly the same, but we should try to keep the buffer size parameters
as close to the requested values as possible (as we already do during
the initialization).

> > > 
> > > -    } else if (avoid_resampling && (spec->rate >= default_rate || spec->rate >= alternate_rate)) {
> > > +    } else if (avoid_resampling && (spec->format != s->sample_spec.format ||
> > > +                spec->rate >= default_rate || spec->rate >= alternate_rate)) {
> > >          /* We just try to set the sink input's sample rate if it's not too low */
> > > +        desired_spec.format = spec->format;
> > >          desired_spec.rate = spec->rate;
> > 
> > desired_spec.rate shouldn't be set if spec->rate is less than
> > default_rate and alternate_rate. Now the rate will be set always if the
> > format needs changing.
> 
> If having the condition with default rate and alternate_rate, it can
> not be set with new sample format of a stream
> even if it is different with previous sample format of sink/source. I
> though it is no little limitation. For example, if the default
> sample rate is 48k and alternative sample rate is 48k or 96k, with a
> rate of 44.1k stream will be resampled anyway even though
> avoid-resampling feature is enabled and this condition also affects
> the case of different sample format similarly.
> Is there any reason for this..?

I'm not sure I understand what you're trying to say... Are you
wondering why we do resampling even if avoid_resampling is set, if the
stream rate is less than both default_rate and alternate_rate? The
reason is that if there's a stream with very low sample rate, for
example 8000 Hz, that can force other streams to be resampled to the
same low rate as well. To prevent that, we set a lower limit for when
avoid_resampling applies.

Regarding how to deal with the sample format: the desired format and
desired rate should be handled separately. You should add a separate if
block for setting the format:

    if (avoid_resampling)
        desired_spec.format = spec->format;

-- 
Tanu

https://www.patreon.com/tanuk
https://liberapay.com/tanuk