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
