Alsaloop: sync mode PLAYSHIFT + Loopback on playback side

[Jaroslav Kysela]

On 04. 10. 21 13:24, Pavel Hofman wrote:
> 
> Dne 04. 10. 21 v 11:24 Jaroslav Kysela napsal(a):
>> On 01. 10. 21 10:35, Pavel Hofman wrote:
>>> Hi,
>>>
>>> I added support for "Playback Pitch 1000000" ctl elem to UAC2 gadget
>>> (not submitted to USB yet) and now I am working on alsaloop support for
>>> this ctl elem. The changes are simple (tested to work perfectly, patch
>>> to follow), but during the work I hit the following issue with playback
>>> Loopback "PCM Rate Shift 100000".
>>>
>>> If the snd-aloop device is on playback side (i.e. capture from soundcard
>>> -> Loopback), the required sync mode is PLAYSHIFT. That means Loopback
>>> ctl elem "PCM Rate Shift 100000" should be controlled (by a reciprocal).
>>> That is simple by a patch like this:
>>>
>>> diff --git a/alsaloop/pcmjob.c b/alsaloop/pcmjob.c
>>> index 845ab82..619bf35 100644
>>> --- a/alsaloop/pcmjob.c
>>> +++ b/alsaloop/pcmjob.c
>>> @@ -1061,7 +1061,13 @@ static int set_rate_shift(struct loopback_handle
>>> *lhandle, double pitch)
>>>            int err;
>>>
>>>            if (lhandle->ctl_rate_shift) {
>>> -               snd_ctl_elem_value_set_integer(lhandle->ctl_rate_shift,
>>> 0, pitch * 100000);
>>> +               long value;
>>> +               if (lhandle->loopback->play == lhandle)
>>> +                       // playback => reciprocal
>>> +                       value = 1/(pitch) * 100000;
>>> +               else
>>> +                       value = pitch * 100000;
>>> +               snd_ctl_elem_value_set_integer(lhandle->ctl_rate_shift,
>>> 0, value);
>>>                    err = snd_ctl_elem_write(lhandle->ctl,
>>> lhandle->ctl_rate_shift);
>>>            } else if (lhandle->capt_pitch) {
>>>                    snd_ctl_elem_value_set_integer(lhandle->capt_pitch, 0,
>>> (1 / pitch) * 1000000);
>>> @@ -1205,15 +1211,18 @@ static int openctl(struct loopback_handle
>>> *lhandle, int device, int subdevice)
>>>            int err;
>>>
>>>            lhandle->ctl_rate_shift = NULL;
>>> +       // both play and capture
>>> +       openctl_elem(lhandle, device, subdevice, "PCM Notify",
>>> +                       &lhandle->ctl_notify);
>>> +       openctl_elem(lhandle, device, subdevice, "PCM Rate Shift 100000",
>>> +                       &lhandle->ctl_rate_shift);
>>>            if (lhandle->loopback->play == lhandle) {
>>> +               // play only
>>>                    if (lhandle->loopback->controls)
>>>                            goto __events;
>>>                    return 0;
>>>            }
>>> -       openctl_elem(lhandle, device, subdevice, "PCM Notify",
>>> -                       &lhandle->ctl_notify);
>>> -       openctl_elem(lhandle, device, subdevice, "PCM Rate Shift 100000",
>>> -                       &lhandle->ctl_rate_shift);
>>> +       // capture only
>>>            openctl_elem(lhandle, device, subdevice, "Capture Pitch
>>> 1000000",
>>>                            &lhandle->capt_pitch);
>>>            set_rate_shift(lhandle, 1);
>>>
>>>
>>> However, IIUC how the Loopback device works, the "PCM Rate Shift 100000"
>>> ctl elem applicable to device=0 on playback side is that of the capture
>>> side, i.e. for device=1. The patch above would pick the playback-side
>>> device=0 ctl elem in pcmjob.c:openctl_elem. Hard-coding the device=0 ->
>>> device=1 is possible, but Loopback supports more devices.
>>>
>>> Please what solution for picking the correct "PCM Rate Shift 100000" ctl
>>> elem for the PLAYSHIFT sync mode would you recommend?
>>
>> Hi,
>>
>> I would not touch the controls associated to the capture PCM by default.
>> It would be possible to add another alsaloop option and code to
>> configure the rate shift control identifier separately for this use
>> case. The user should avoid the double pitch control (playback +
>> capture) for the loopback devices.
> 
> Thanks for your input. I am not sure I understand correctly your intention.
> 
> The current code already avoids running playback and capture pitch
> control concurrently by one alsaloop as the loop->sync is either
> SYNC_TYPE_CAPTRATESHIFT or SYNC_TYPE_PLAYRATESHIFT. IMO the current code
> ignores SYNC_TYPE_PLAYRATESHIFT because the code jumps to
> set_rate_shift(loop->play, pitch)
> https://github.com/alsa-project/alsa-utils/blob/master/alsaloop/pcmjob.c#L1103
> but the lhandle->ctl_rate_shift elem is never set for playback side in
> pcmjob.c:openctl
> https://github.com/alsa-project/alsa-utils/blob/master/alsaloop/pcmjob.c#L1208
> (lhandle->loopback->play == lhandle). I confirmed it using the debug
> patch you accepted today (thanks! :-)).
> 
> Therefore my patch of the openctl moves the "PCM Notify" and "PCM Rate
> Shift 100000" setup before the playback check, so that lhandles of both
> directions can have the two elems configured.
> 
> But the loopback->play lhandle should configure elements for the capture
> side of the snd-aloop pipe which I do not know how to locate nicely. I
> can always use some hack "if device==0 then use device = 1" for the
> playback side etc. but that does not look nice.

Please, assume that the 'PCM Rate Shift 100000' control is for the capture PCM 
only by default. The applications should not handle those driver specific mapping.

If the user wants to use this control for the playback side, a new command 
line option may be added to cope with the alternate playback rate shift 
control indentifier (it may be parsed using snd_ctl_ascii_elem_id_parse() for 
example). So the user may specify the correct (different) device number there.

					Jaroslav

-- 
Jaroslav Kysela <perex at perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.