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
