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

[Georg Chini]

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.