Re: [RFC PATCH 1/6] ALSA: compress: add Sample Rate Converter codec support

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

On 8/12/24 15:31, Jaroslav Kysela wrote:
> On 12. 08. 24 12:24, Shengjiu Wang wrote:
>> On Fri, Aug 9, 2024 at 10:01 PM Jaroslav Kysela <perex@perex.cz> wrote:
>>>
>>> On 09. 08. 24 14:52, Pierre-Louis Bossart wrote:
>>>
>>>>> And metadata
>>>>> ioctl can be called many times which can meet the ratio modifier
>>>>> requirement (ratio may be drift on the fly)
>>>>
>>>> Interesting, that's yet another way of handling the drift with
>>>> userspace
>>>> modifying the ratio dynamically. That's different to what I've seen
>>>> before.
>>>
>>> Note that the "timing" is managed by the user space with this scheme.
>>>
>>>>> And compress API uses codec as the unit for capability query and
>>>>> parameter setting,  So I think need to define "SND_AUDIOCODEC_SRC'
>>>>> and 'struct snd_dec_src',  for the 'snd_dec_src' just defined output
>>>>> format and output rate, channels definition just reuse the
>>>>> snd_codec.ch_in.
>>>>
>>>> The capability query is an interesting point as well, it's not clear
>>>> how
>>>> to expose to userspace what this specific implementation can do, while
>>>> at the same time *requiring* userpace to update the ratio dynamically.
>>>> For something like this to work, userspace needs to have pre-existing
>>>> information on how the SRC works.
>>>
>>> Yes, it's about abstraction. The user space wants to push data, read
>>> data back
>>> converted to the target rate and eventually modify the drift using a
>>> control
>>> managing clocks using own way. We can eventually assume, that if this
>>> control
>>> does not exist, the drift cannot be controlled. Also, nice thing is
>>> that the
>>> control has min and max values (range), so driver can specify the
>>> drift range,
>>> too.
>>>
>>> And again, look to "PCM Rate Shift 100000" control implementation in
>>> sound/drivers/aloop.c. It would be nice to have the base offset for the
>>> shift/drift/pitch value standardized.
>>
>> Thanks.
>>
>> But the ASRC driver I implemented is different, I just register one sound
>> card, one device/subdevice.  but the ASRC hardware support 4 instances
>> together, so user can open the card device 4 times to create 4 instances
>> then the controls can only bind with compress streams.
> 
> It's just a reason to add the subdevice code for the compress offload
> layer like we have in other APIs for overall consistency. I'll try to
> work on this.

I thought this was supported already? I remember there was a request to
enable more than one compressed stream for enhanced cross-fade support
with different formats? That isn't supported with the single-device +
PARTIAL_DRAIN method.

Vinod?

>> I think I can remove the 'SNDRV_COMPRESS_SRC_RATIO_MOD',
> 
> Yes.
> 
>> Only define a private type for driver,  which means only the ASRC driver
>> and its user application know the type.
> 
> The control API should be used for this IMHO.

Agree, this would be a 'clean' split where the compress API is used for
the data parts and the control parts used otherwise to alter the ratio
or whatever else is needed.

>> For the change in 'include/uapi/sound/compress_params.h",  should I
>> keep them,  is there any other suggestion for them?

You can add the SRC type but if you use a control for the parameters you
don't need to add anything for the encoder options, do you?