[pulseaudio-discuss] Freezing master, release notes, blocker bugs

[Tanu Kaskinen]

On Thu, 2017-04-27 at 14:12 +0200, Georg Chini wrote:
> On 24.04.2017 15:58, Tanu Kaskinen wrote:
> > On Sat, 2017-04-22 at 20:37 +0200, Georg Chini wrote:
> > > On 21.04.2017 15:32, Tanu Kaskinen wrote:
> > > > Hi all,
> > > > 
> > > > It's been three months since the last release, which means that it's
> > > > time to freeze the master branch in preparation for the next release.
> > > > 
> > > > 
> > > > Here's a review of the current blockers:
> > > > 
> > > > frequent crash in pa_alsa_path_set_volume
> > > > https://bugs.freedesktop.org/show_bug.cgi?id=65520
> > > > 
> > > >       Needs investigation, might be difficult to fix.
> > > > 
> > > 
> > > I took a look into this one and at the log at
> > > https://launchpadlibrarian.net/308478962/pulseverbose.log
> > > 
> > > The only possibility it can fail this way is when the channel volumes
> > > of s->real_volume or s->thread_info.current_hw_volume are larger
> > > than PA_VOLUME_MAX / 10.
> > > 
> > > There are only two calls to pa_alsa_path_set_volume() and each of
> > > them is preceded by a call to pa_sw_cvolume_divide_scalar(). This
> > > call succeeds, otherwise it would return NULL and
> > > pa_alsa_path_set_volume() would crash at another assertion.
> > > pa_sw_cvolume_divide_scalar() also checks the volumes on entry,
> > > so we can be sure the volumes are valid at that point in time.
> > > A base volume of -60dB corresponds to
> > > s->base_volume = 0.1 * PA_VOLUME_NORM.
> > > 
> > > This means that pa_sw_volume_divide() which is called by
> > > pa_sw_cvolume_divide_scalar() roughly multiplies the input channel
> > > values by 10, which leads to the statement above.
> > > 
> > > Can we simply clip pa_sw_volume_divide() at PA_VOLUME_MAX?
> > > 
> > > Or will we need further investigation?
> > 
> > I wouldn't add clipping to pa_sw_volume_divide(). In principle the
> > function should apply clipping for correctness, but if it has to clip,
> > the volume value is humongous, so big that it indicates a bug
> > somewhere. 0.1 * PA_VOLUME_MAX corresponds to about 211 dB.
> > 
> > If your analysis is correct, and the input volume is indeed over 200
> > dB, then I think we should try to figure out how such volume could end
> > up being used. It looks like this happens during initialization, so the
> > initialization code should be reviewed.
> > 
> > If no bugs get catched by reviewing, I think we should add some extra
> > logging and/or assertions somewhere so that we have more clues when
> > this happens again.
> > 
> 
> I did some further investigation and I have to admit that I don't really
> understand what is done with the volumes. There are so many of them ...
> 
> Nevertheless I think I have figured out what causes the crash, although
> I have no idea how to fix it properly.
> 
> What happens in the bug report is that the volume of the source is
> initially set to 42952294 by module-device-restore. This seems much too
> high but should not cause a crash.

It's possible to set device volumes insanely high, and module-device-
restore will happily save any volume. Perhaps the user accidentally set
some silly volume at some point with pactl. I wonder if we should
define some upper limit for device volumes, much less than
PA_VOLUME_MAX. If the upper limit isn't configurable, I think it has to
be pretty high, but if it's e.g. +120dB, I think it should get rid of
this crash.

> But when the source is created source_set_volume_cb() is called.
> There the hardware volume is set to 65540 and the soft_volume
> to 42949673. So after the call, soft volume is 42949673 and real_volume
> is 42952294. This can be seen from the log attached to the bug.
> 
> During pa_source_put(), thread_info.current_hw_volume is set like this:
> pa_sw_cvolume_multiply(&s->thread_info.current_hw_volume, 
> &s->soft_volume, &s->real_volume);
> This sets thread_info.current_hw_volume to PA_VOLUME_MAX.
> 
> When the port changes, source_write_volume_cb() is called. The
> pa_sw_cvolume_divide_scalar() call multiplies thread_info.current_hw_volume
> by 10 which then leads to the crash in pa_alsa_path_set_volume()
> 
> Any idea how to fix this? I don't even understand what the
> pa_sw_cvolume_multiply() in pa_source_put() is for.

I think it's supposed to calculate the hw volume, but the way it's
calculated seems wrong to me. One would think that

real volume (dB) = hw volume (dB) + soft volume (dB)

and therefore

hw volume (dB) = real volume (dB) - soft volume (dB)

So the pa_sw_cvolume_multiply() call should be replaced with a
pa_sw_cvolume_divide() call (and the argument order should be swapped).

-- 
Tanu

https://www.patreon.com/tanuk