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

On Mon, Aug 12, 2024 at 9:44 PM Pierre-Louis Bossart
[off-list ref] wrote:

On 8/12/24 15:31, Jaroslav Kysela wrote:

quoted

On 12. 08. 24 12:24, Shengjiu Wang wrote:

quoted

On Fri, Aug 9, 2024 at 10:01 PM Jaroslav Kysela [off-list ref] wrote:

quoted

On 09. 08. 24 14:52, Pierre-Louis Bossart wrote:

quoted

quoted

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.

quoted

quoted

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?

quoted

quoted

I think I can remove the 'SNDRV_COMPRESS_SRC_RATIO_MOD',

Yes.

quoted

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.

quoted

quoted

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