[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e89a56bf-c377-43d8-bba8-6a09e571ed64@linux.intel.com>
Date: Tue, 6 Aug 2024 13:16:38 +0200
From: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To: Shengjiu Wang <shengjiu.wang@....com>, vkoul@...nel.org, perex@...ex.cz,
tiwai@...e.com, alsa-devel@...a-project.org, linux-sound@...r.kernel.org,
linux-kernel@...r.kernel.org, shengjiu.wang@...il.com, Xiubo.Lee@...il.com,
festevam@...il.com, nicoleotsuka@...il.com, lgirdwood@...il.com,
broonie@...nel.org, linuxppc-dev@...ts.ozlabs.org
Subject: Re: [RFC PATCH 1/6] ALSA: compress: add Sample Rate Converter codec
support
On 8/6/24 12:26, Shengjiu Wang wrote:
> Add Sample Rate Converter(SRC) codec support, define the output
> format and rate for SRC.
>
> Signed-off-by: Shengjiu Wang <shengjiu.wang@....com>
> ---
> include/uapi/sound/compress_offload.h | 2 ++
> include/uapi/sound/compress_params.h | 9 ++++++++-
> 2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h
> index 98772b0cbcb7..8b2b72f94e26 100644
> --- a/include/uapi/sound/compress_offload.h
> +++ b/include/uapi/sound/compress_offload.h
> @@ -112,10 +112,12 @@ struct snd_compr_codec_caps {
> * end of the track
> * @SNDRV_COMPRESS_ENCODER_DELAY: no of samples inserted by the encoder at the
> * beginning of the track
> + * @SNDRV_COMPRESS_SRC_RATIO_MOD: Resampling Ratio Modifier for sample rate converter
> */
> enum sndrv_compress_encoder {
> SNDRV_COMPRESS_ENCODER_PADDING = 1,
> SNDRV_COMPRESS_ENCODER_DELAY = 2,
> + SNDRV_COMPRESS_SRC_RATIO_MOD = 3,
> };
this sounds wrong to me. The sample rate converter is not an "encoder",
and the properties for padding/delay are totally specific to an encoder
function.
The other point is that I am not sure how standard this ratio_mod
parameter is. This could be totally specific to a specific
implementation, and another ASRC might have different parameters.
>
> /**
> diff --git a/include/uapi/sound/compress_params.h b/include/uapi/sound/compress_params.h
> index ddc77322d571..0843773ea6b4 100644
> --- a/include/uapi/sound/compress_params.h
> +++ b/include/uapi/sound/compress_params.h
> @@ -43,7 +43,8 @@
> #define SND_AUDIOCODEC_BESPOKE ((__u32) 0x0000000E)
> #define SND_AUDIOCODEC_ALAC ((__u32) 0x0000000F)
> #define SND_AUDIOCODEC_APE ((__u32) 0x00000010)
> -#define SND_AUDIOCODEC_MAX SND_AUDIOCODEC_APE
> +#define SND_AUDIOCODEC_SRC ((__u32) 0x00000011)
> +#define SND_AUDIOCODEC_MAX SND_AUDIOCODEC_SRC
I am not sure this is wise to change such definitions?
>
> /*
> * Profile and modes are listed with bit masks. This allows for a
> @@ -324,6 +325,11 @@ struct snd_dec_ape {
> __u32 seek_table_present;
> } __attribute__((packed, aligned(4)));
>
> +struct snd_dec_src {
> + __u32 format_out;
> + __u32 rate_out;
> +} __attribute__((packed, aligned(4)));
Again I am not sure how standard those parameters are, and even if they
were if their representation is reusable.
And the compressed API has a good split between encoders and decoders, I
am not sure how an SRC can be classified as either.
> union snd_codec_options {
> struct snd_enc_wma wma;
> struct snd_enc_vorbis vorbis;
> @@ -334,6 +340,7 @@ union snd_codec_options {
> struct snd_dec_wma wma_d;
> struct snd_dec_alac alac_d;
> struct snd_dec_ape ape_d;
> + struct snd_dec_src src;
> } __attribute__((packed, aligned(4)));
>
> /** struct snd_codec_desc - description of codec capabilities
Powered by blists - more mailing lists