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
