[pulseaudio-discuss] [PATCH] sink, source: Add a mode to avoid resampling if possible

[Arun Raghavan]

On Mon, 30 Jan 2017, at 01:14 PM, Tanu Kaskinen wrote:
> On Sun, 2017-01-29 at 23:23 +0530, Arun Raghavan wrote:
> > 
> > On Sun, 29 Jan 2017, at 02:29 PM, Tanu Kaskinen wrote:
> > > On Sun, 2017-01-29 at 10:29 +0530, Arun Raghavan wrote:
> > > > This adds an "avoid-resampling" option to daemon.conf that makes the
> > > > daemon try to use the stream sample rate if possible (the device needs
> > > > to support it, which currently only ALSA does), and there should not be
> > > > any other stream connected).
> > > > 
> > > > This should enable some of the "audiophile" use-cases where users wish
> > > > to play high sample rate audio files without resampling.
> > > > 
> > > > We still will do conversion if sample formats don't match, though. This
> > > > means that if you want to play 96 kHz/24 bit audio without any
> > > > modification the default format will need to be set to be 24-bit as
> > > > well. This will force all streams to be upconverted, which, other than
> > > > the wasted resources, should be relatively harmless.
> > > > ---
> > > >  man/pulse-daemon.conf.5.xml.in |  9 +++++++++
> > > >  src/daemon/daemon-conf.c       |  3 +++
> > > >  src/daemon/daemon-conf.h       |  1 +
> > > >  src/daemon/daemon.conf.in      |  1 +
> > > >  src/daemon/main.c              |  1 +
> > > >  src/pulsecore/core.h           |  1 +
> > > >  src/pulsecore/sink.c           | 10 ++++++++--
> > > >  src/pulsecore/source.c         |  9 +++++++--
> > > >  8 files changed, 31 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/man/pulse-daemon.conf.5.xml.in b/man/pulse-daemon.conf.5.xml.in
> > > > index b81a549..cb09b97 100644
> > > > --- a/man/pulse-daemon.conf.5.xml.in
> > > > +++ b/man/pulse-daemon.conf.5.xml.in
> > > > @@ -124,6 +124,15 @@ License along with PulseAudio; if not, see <http://www.gnu.org/licenses/>.
> > > >      </option>
> > > >  
> > > >      <option>
> > > > +      <p><opt>avoid-resampling=</opt> If set, try to configure the
> > > > +      device to avoid resampling. This only works if on devices that
> > > 
> > > The extra "if" is still here.
> > > 
> > > > @@ -1442,7 +1443,12 @@ int pa_sink_update_rate(pa_sink *s, uint32_t rate, bool passthrough) {
> > > >      if (PA_UNLIKELY(!pa_sample_rate_valid(desired_rate)))
> > > >          return -1;
> > > >  
> > > > -    if (!passthrough && default_rate != desired_rate && alternate_rate != desired_rate) {
> > > > +    if (avoid_resampling && (rate >= default_rate || rate >= alternate_rate)) {
> > > > +        /* We just try to set the sink input's sample rate if it's not too low */
> > > > +        if (rate > default_rate || rate > alternate_rate)
> > > > +            desired_rate = rate;
> > > 
> > > Now the code works, which is nice. However, you said that the
> > 
> > I don't know what you're talking about. It was working before, this
> > makes the condition cause the alternate resample rate calculation to be
> > used in the case of avoid_resampling=true and the desired rate being too
> > low. .
> 
> I meant that your original patch didn't do what you intended it to do.
> I believe your intention was to not allow the sink rate to be changed
> to a value smaller than default_rate or alternate_rate, but the
> original patch did allow that.

Ah, okay.

> > > "desired_rate = rate" assignment makes the code clearer, but I
> > > disagree. The code looks like the intention is to do something if the
> > > "rate > default_rate || rate > alternate_rate" condition is true, but
> > > actually the code never does anything, regardless of whether the
> > > condition is true or false.
> > 
> > That part was wrong, the second if was a duplicate (I fixed it in
> > source.c but not sink.c).
> 
> Ok, I didn't notice that source.c was different. (BTW, the indentation
> in source.c is off.)

Fixed.

> > > If you think leaving the code block empty or merging the "if" and "else
> > > if" branches makes the code unclear, then I think a comment is more
> > > appropriate way to clarify things than adding code that doesn't do
> > > anything.
> > 
> > I'm not sure I understand -- are you saying the code in source.c is
> > fine, or that it needs to not do anything in the avoid_resampling case
> > (whereas now we have a duplicated "desired_rate = rate")?
> 
> The code in source.c is definitely less confusing than the code in
> sink.c. I'd prefer not to have the redundant assignment, but I don't
> mind if you leave it there.

We spoke about this on IRC, and I'm pushing a second patch to make all
the conditions more explicit in this path (and thus remove the early
initialisation).

Cheers,
Arun