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