[pulseaudio-discuss] An raop2 support

[Hajime Fujita]

Tanu Kaskinen wrote:
> On Thu, 2013-09-19 at 22:47 -0500, Hajime Fujita wrote:
>> Hi Tanu,
>>
>> Tanu Kaskinen wrote:
>>> On Wed, 2013-09-18 at 23:09 -0500, Hajime Fujita wrote:
>>>> Hi Tanu,
>>>>
>>>> Thank you so much for your advice.
>>>>
>>>> Tanu Kaskinen wrote:
>>>>> On Sun, 2013-09-15 at 00:24 -0500, Hajime Fujita wrote:
>>>>>> For those who are interested in raop2 experiments,
>>>>>>
>>>>>> As Matthias pointed out [1], current volume calculation is incorrect.
>>>>>> https://bugs.freedesktop.org/show_bug.cgi?id=42804#c43
>>>>>>
>>>>>> I have uploaded a patch which calculates volume in more reasonable way.
>>>>>> https://github.com/hfujita/pulseaudio-raop2/commit/69f3ce883dfb93eb7b24f98b9664e14071672475
>>>>>>
>>>>>> First I have to confess that this is the first time for me to deal with
>>>>>> the unit 'dB', so maybe I'm doing totally stupid. I'd be very happy if I
>>>>>> could have any suggestions from who is more experienced in this field.
>>>>>>
>>>>>> I started looking at pa_raop_client_set_volume() in
>>>>>> src/modules/raop/raop_client.c. It was using pa_sw_volume_to_dB to
>>>>>> calculate dB from a linear scale.
>>>>>
>>>>> Correction: pa_volume_t doesn't use a linear scale, it uses a cubic
>>>>> scale (see pa_sw_volume_from_linear() and pa_sw_volume_to_linear()).
>>>>
>>>> I see. I should have noticed some comments about cubic scale.
>>>> http://cgit.freedesktop.org/pulseaudio/pulseaudio/tree/src/pulse/volume.c#n251
>>>>
>>>>>
>>>>>> Then I learned from code reading that pa_sw_volume_to_dB essentially
>>>>>> calculates the following:
>>>>>>   20 * log10((v/MAXVOLUME)^3) = 60 * log10(v/MAXVOLUME)
>>>>>> This does not fit into the volume scale which RAOP is expecting, where
>>>>>> -30.0 db is a kind of minimum [2].
>>>>>>
>>>>>> [2]: http://git.zx2c4.com/Airtunes2/about/#id29
>>>>>>
>>>>>> Also I looked at packet dumps from iPad and guessed how it calculated dB
>>>>>> from a linear scale. It looked like iPad was using the following formula.
>>>>>>   db = 10 * log10(volume/MAXVOLUME/30)
>>>>>> So for the time being I decided to follow this way.
>>>>>
>>>>> The way to convert pa_volume_t to dB is to use pa_sw_volume_to_dB().
>>>>> Doing anything else means that the result is not dB, it's something
>>>>> different.
>>>>>
>>>>> The way to handle this is to ensure that the volume that is given to
>>>>> pa_raop_client_set_volume() is never outside the supported range. In
>>>>> module-raop-sink.c there's sink_set_volume_cb(), which calls
>>>>> pa_raop_client_set_volume(). In sink_set_volume_cb() there's this line:
>>>>>
>>>>>     pa_cvolume_set(&hw, s->sample_spec.channels, v);
>>>>>
>>>>> Before that line you should check what dB value v corresponds to, and if
>>>>> it's outside the supported range, then you should modify v to fall into
>>>>> the supported range. Any modification that you make to v will be
>>>>> compensated with software volume, so the modifications shouldn't affect
>>>>> the resulting volume at all, assuming that the RAOP device actually
>>>>> attenuates the signal by the dB amount that you send to it.
>>>>>
>>>>
>>>> According to your advice, I inserted an adjustment to v before
>>>> pa_cvolume_set. Here's the patch.
>>>> https://github.com/hfujita/pulseaudio-raop2/commit/c6337c0fef7710ee20b42ed3fb87ec12910ce35a
>>>>
>>>> Adjustment goes like this (pa_raop_client_adjust_volume()):
>>>>   adj(v) = v * (1 - minv/maxv) + minv;
>>>> where pa_sw_volume_to_dB(minv) = -30.0,
>>>> and pa_sw_volume_to_dB(maxv) = 0.0
>>>>
>>>> The idea is that when the original v is 0, adj(0) = minv. And when the
>>>> original v is maxv, adj(maxv) = maxv;
>>>>
>>>> Does this make sense?
>>>
>>> The idea was to simply adjust v to -30 dB if the original v is less than
>>> that. There's no need to do any complicated calculations.
>>
>> Do you mean something like
>>   v = max(minv, v); /* where minv corresponds to -30.0dB */ ?
> 
> Yes.
> 
>> If I do that, resulted volume will be almost muted in the range of 0% <=
>> v <= 31%, then suddenly goes up beyond 31%. (31% is where dB becomes -30.0)
>> I was afraid that this was not an intuitive behavior to users.
> 
> It sounds like the RAOP device doesn't apply the attenuation as it
> should. Could you verify this? If you don't apply any software volume
> (set s->soft_volume to PA_VOLUME_NORM), and you tell the RAOP device to
> use volume -30.0, do you get very quiet (but not silent) audio? And if
> you then tell the RAOP device to use volume -29.9, do you get much
> louder audio? This shouldn't happen. A change of 0.1 dB should have very
> small effect.

I actually did it. At -30.0, the device shows "MIN" in the volume
indicator and I did not hear any sound. Same in -29.9.
If it becomes -29.79, I recognize sound from the speaker. At this moment
the volume indicator shows "1".

>>>> As you noted, this also affects the software volume, and in my
>>>> environment, the actual volume (volume heard from the speaker) may be
>>>> way too small.
>>>
>>> What do you mean by way too small? The maximum volume will be the same
>>> as before, but the minimum volume will be quieter than before. With the
>>> original code you complained that you need to keep the volume at rather
>>> low levels, so extending the scale towards low volumes should help with
>>> this.
>>
>> Well, this could be just my personal feeling. Since it was a bit loud
>> before (however it was almost the same volume level as iPad), this time
>> I felt it was way too small.
>>
>> I wonder if it makes sense to 1) adjust v as
>>   adj(v) = v * (1 - minv/maxv) + minv;
>> and 2) use the original v for pa_cvolume_set(), then 3) use adj(v) for
>> setting the actual RAOP device volume.
>> Under this condition, the calculated software volume always becomes
>> 100%, as in the current (mainline) raop module.
> 
> Yes, that sounds like a reasonable approach, if you can't trust that the
> RAOP device will handle the volume correctly. Note that the the current
> raop module does use software volume when all channels don't have the
> same volume. With the adjustment that you suggest, you can't do this any
> more, because the volume is not any more convertible to dB, and thus you
> can't use pa_sw_volume_divide(). You should then force all channels to
> have the same volume, which can be done by modifying s->real_volume:
> 
>     pa_cvolume_set(&s->real_volume, s->sample_spec.channels, v_orig);
> 

Thanks. I'll also try this.


Thanks,
Hajime