[pulseaudio-discuss] An raop2 support

[Tanu Kaskinen]

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.

> >> 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);

-- 
Tanu