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

[Shengjiu Wang <shengjiu.wang@xxxxxxxxx>]

On Mon, Aug 12, 2024 at 9:44 PM Pierre-Louis Bossart
<pierre-louis.bossart@xxxxxxxxxxxxxxx> wrote:
>
>
>
> 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@xxxxxxxx> 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?
>

Yes, to go further, I think we can use SND_AUDIOCODEC_PCM, then
the SRC type will be dropped.

But my understanding of the control means the .set_metadata() API, right?
As I said, the output rate, output format, and ratio modifier are applied to
the instances of ASRC,  which is the snd_compr_stream in driver.
so only the .set_metadata() API can be used for these purposes.

Best regards
Shengjiu Wang