diff mbox series

[RFC,v2,2/6] ALSA: compress: add new ioctl for setting codec parameters

Message ID 20200721170007.4554-3-srinivas.kandagatla@linaro.org (mailing list archive)
State New, archived
Headers show
Series ALSA: compress: add support to change codec profile in gapless playback | expand

Commit Message

Srinivas Kandagatla July 21, 2020, 5 p.m. UTC 
For gapless playback it is possible that each track can have different
codec profile with same decoder, for example we have WMA album,
we may have different tracks as WMA v9, WMA v10 and so on

Or if DSP's like QDSP have abililty to switch decoders on single stream
for each track, then this call could be used to set new codec parameters.

Existing code does not allow to change this profile while doing gapless
playback.

This patch adds new SNDRV_COMPRESS_SET_CODEC_PARAMS IOCTL to allow
userspace to set this new parameters required for new codec profile.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 .../sound/designs/compress-offload.rst        |  6 ++++
 include/sound/compress_driver.h               |  5 +++
 include/uapi/sound/compress_offload.h         |  1 +
 sound/core/compress_offload.c                 | 34 +++++++++++++++++++
 4 files changed, 46 insertions(+)

Comments

Pierre-Louis Bossart July 21, 2020, 8:05 p.m. UTC | #1 
On 7/21/20 12:00 PM, Srinivas Kandagatla wrote:
> For gapless playback it is possible that each track can have different
> codec profile with same decoder, for example we have WMA album,
> we may have different tracks as WMA v9, WMA v10 and so on
> 
> Or if DSP's like QDSP have abililty to switch decoders on single stream

ability

> for each track, then this call could be used to set new codec parameters.
> 
> Existing code does not allow to change this profile while doing gapless
> playback.
> 
> This patch adds new SNDRV_COMPRESS_SET_CODEC_PARAMS IOCTL to allow
> userspace to set this new parameters required for new codec profile.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>   .../sound/designs/compress-offload.rst        |  6 ++++
>   include/sound/compress_driver.h               |  5 +++
>   include/uapi/sound/compress_offload.h         |  1 +
>   sound/core/compress_offload.c                 | 34 +++++++++++++++++++
>   4 files changed, 46 insertions(+)
> 
> diff --git a/Documentation/sound/designs/compress-offload.rst b/Documentation/sound/designs/compress-offload.rst
> index 935f325dbc77..305ccc7bfdd9 100644
> --- a/Documentation/sound/designs/compress-offload.rst
> +++ b/Documentation/sound/designs/compress-offload.rst
> @@ -128,6 +128,12 @@ set_params
>     cases decoders will ignore other fields, while encoders will strictly
>     comply to the settings
>   
> +set_codec_params
> +  This routine is very much simillar to set_params but exculding stream

typos: similar, excluding

> +  information. Only codec related information is set as part of this.
> +  It is used in gapless playback where its required to change decoder
> +  or its parameters for next track. This is optional.
> +
>   get_params
>     This routines returns the actual settings used by the DSP. Changes to
>     the settings should remain the exception.
> diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
> index 70cbc5095e72..d9c00bcfce9b 100644
> --- a/include/sound/compress_driver.h
> +++ b/include/sound/compress_driver.h
> @@ -93,6 +93,9 @@ struct snd_compr_stream {
>    * @set_params: Sets the compressed stream parameters, mandatory
>    * This can be called in during stream creation only to set codec params
>    * and the stream properties
> + * @set_codec_params: Sets the compressed stream codec parameters, Optional
> + * This can be called in during gapless next track codec change only to set
> + * codec params

Would it be clearer if this was called set_next_codec_params()? or 
set_next_track_codec_params()?

Having set_params() and set_codec_params() is a bit confusing since the 
semantic difference is not captured in the callback name.
Srinivas Kandagatla July 22, 2020, 8:59 a.m. UTC | #2 
On 21/07/2020 21:05, Pierre-Louis Bossart wrote:
> 
> 
> On 7/21/20 12:00 PM, Srinivas Kandagatla wrote:
>> For gapless playback it is possible that each track can have different
>> codec profile with same decoder, for example we have WMA album,
>> we may have different tracks as WMA v9, WMA v10 and so on
>>
>> Or if DSP's like QDSP have abililty to switch decoders on single stream
> 
> ability
> 
>> for each track, then this call could be used to set new codec parameters.
>>
>> Existing code does not allow to change this profile while doing gapless
>> playback.
>>
>> This patch adds new SNDRV_COMPRESS_SET_CODEC_PARAMS IOCTL to allow
>> userspace to set this new parameters required for new codec profile.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>   .../sound/designs/compress-offload.rst        |  6 ++++
>>   include/sound/compress_driver.h               |  5 +++
>>   include/uapi/sound/compress_offload.h         |  1 +
>>   sound/core/compress_offload.c                 | 34 +++++++++++++++++++
>>   4 files changed, 46 insertions(+)
>>
>> diff --git a/Documentation/sound/designs/compress-offload.rst 
>> b/Documentation/sound/designs/compress-offload.rst
>> index 935f325dbc77..305ccc7bfdd9 100644
>> --- a/Documentation/sound/designs/compress-offload.rst
>> +++ b/Documentation/sound/designs/compress-offload.rst
>> @@ -128,6 +128,12 @@ set_params
>>     cases decoders will ignore other fields, while encoders will strictly
>>     comply to the settings
>> +set_codec_params
>> +  This routine is very much simillar to set_params but exculding stream
> 
> typos: similar, excluding
> 
>> +  information. Only codec related information is set as part of this.
>> +  It is used in gapless playback where its required to change decoder
>> +  or its parameters for next track. This is optional.
>> +
>>   get_params
>>     This routines returns the actual settings used by the DSP. Changes to
>>     the settings should remain the exception.
>> diff --git a/include/sound/compress_driver.h 
>> b/include/sound/compress_driver.h
>> index 70cbc5095e72..d9c00bcfce9b 100644
>> --- a/include/sound/compress_driver.h
>> +++ b/include/sound/compress_driver.h
>> @@ -93,6 +93,9 @@ struct snd_compr_stream {
>>    * @set_params: Sets the compressed stream parameters, mandatory
>>    * This can be called in during stream creation only to set codec 
>> params
>>    * and the stream properties
>> + * @set_codec_params: Sets the compressed stream codec parameters, 
>> Optional
>> + * This can be called in during gapless next track codec change only 
>> to set
>> + * codec params
> 
> Would it be clearer if this was called set_next_codec_params()? or 
> set_next_track_codec_params()?
> 
> Having set_params() and set_codec_params() is a bit confusing since the 
> semantic difference is not captured in the callback name.

set_next_track_codec_params seems more sensible as its next track params.
Will change this in next version!

--srini

>
Pierre-Louis Bossart July 22, 2020, 3:36 p.m. UTC | #3 
>>>    * and the stream properties
>>> + * @set_codec_params: Sets the compressed stream codec parameters, 
>>> Optional
>>> + * This can be called in during gapless next track codec change only 
>>> to set
>>> + * codec params
>>
>> Would it be clearer if this was called set_next_codec_params()? or 
>> set_next_track_codec_params()?
>>
>> Having set_params() and set_codec_params() is a bit confusing since 
>> the semantic difference is not captured in the callback name.
> 
> set_next_track_codec_params seems more sensible as its next track params.
> Will change this in next version!

maybe set_params() and set_next_track_params() are enough, not sure if 
the codec reference helps?
Vinod Koul July 23, 2020, 4:47 a.m. UTC | #4 
On 22-07-20, 10:36, Pierre-Louis Bossart wrote:
> 
> > > >    * and the stream properties
> > > > + * @set_codec_params: Sets the compressed stream codec
> > > > parameters, Optional
> > > > + * This can be called in during gapless next track codec change
> > > > only to set
> > > > + * codec params
> > > 
> > > Would it be clearer if this was called set_next_codec_params()? or
> > > set_next_track_codec_params()?
> > > 
> > > Having set_params() and set_codec_params() is a bit confusing since
> > > the semantic difference is not captured in the callback name.
> > 
> > set_next_track_codec_params seems more sensible as its next track params.
> > Will change this in next version!
> 
> maybe set_params() and set_next_track_params() are enough, not sure if the
> codec reference helps?

params typically refers to whole set of compress parameters which
includes buffer information and codec parameters, so codec reference
would help.
Pierre-Louis Bossart July 23, 2020, 1:17 p.m. UTC | #5 
On 7/22/20 11:47 PM, Vinod Koul wrote:
> On 22-07-20, 10:36, Pierre-Louis Bossart wrote:
>>
>>>>>     * and the stream properties
>>>>> + * @set_codec_params: Sets the compressed stream codec
>>>>> parameters, Optional
>>>>> + * This can be called in during gapless next track codec change
>>>>> only to set
>>>>> + * codec params
>>>>
>>>> Would it be clearer if this was called set_next_codec_params()? or
>>>> set_next_track_codec_params()?
>>>>
>>>> Having set_params() and set_codec_params() is a bit confusing since
>>>> the semantic difference is not captured in the callback name.
>>>
>>> set_next_track_codec_params seems more sensible as its next track params.
>>> Will change this in next version!
>>
>> maybe set_params() and set_next_track_params() are enough, not sure if the
>> codec reference helps?
> 
> params typically refers to whole set of compress parameters which
> includes buffer information and codec parameters, so codec reference
> would help.

then add the codec reference to both...
diff mbox series

Patch

diff --git a/Documentation/sound/designs/compress-offload.rst b/Documentation/sound/designs/compress-offload.rst
index 935f325dbc77..305ccc7bfdd9 100644
--- a/Documentation/sound/designs/compress-offload.rst
+++ b/Documentation/sound/designs/compress-offload.rst
@@ -128,6 +128,12 @@  set_params
   cases decoders will ignore other fields, while encoders will strictly
   comply to the settings
 
+set_codec_params
+  This routine is very much simillar to set_params but exculding stream
+  information. Only codec related information is set as part of this.
+  It is used in gapless playback where its required to change decoder
+  or its parameters for next track. This is optional.
+
 get_params
   This routines returns the actual settings used by the DSP. Changes to
   the settings should remain the exception.
diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
index 70cbc5095e72..d9c00bcfce9b 100644
--- a/include/sound/compress_driver.h
+++ b/include/sound/compress_driver.h
@@ -93,6 +93,9 @@  struct snd_compr_stream {
  * @set_params: Sets the compressed stream parameters, mandatory
  * This can be called in during stream creation only to set codec params
  * and the stream properties
+ * @set_codec_params: Sets the compressed stream codec parameters, Optional
+ * This can be called in during gapless next track codec change only to set
+ * codec params
  * @get_params: retrieve the codec parameters, mandatory
  * @set_metadata: Set the metadata values for a stream
  * @get_metadata: retrieves the requested metadata values from stream
@@ -112,6 +115,8 @@  struct snd_compr_ops {
 	int (*free)(struct snd_compr_stream *stream);
 	int (*set_params)(struct snd_compr_stream *stream,
 			struct snd_compr_params *params);
+	int (*set_codec_params)(struct snd_compr_stream *stream,
+			struct snd_codec *params);
 	int (*get_params)(struct snd_compr_stream *stream,
 			struct snd_codec *params);
 	int (*set_metadata)(struct snd_compr_stream *stream,
diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h
index 7184265c0b0d..c46286113a4b 100644
--- a/include/uapi/sound/compress_offload.h
+++ b/include/uapi/sound/compress_offload.h
@@ -172,6 +172,7 @@  struct snd_compr_metadata {
 						 struct snd_compr_metadata)
 #define SNDRV_COMPRESS_GET_METADATA	_IOWR('C', 0x15,\
 						 struct snd_compr_metadata)
+#define SNDRV_COMPRESS_SET_CODEC_PARAMS	_IOW('C', 0x16, struct snd_codec)
 #define SNDRV_COMPRESS_TSTAMP		_IOR('C', 0x20, struct snd_compr_tstamp)
 #define SNDRV_COMPRESS_AVAIL		_IOR('C', 0x21, struct snd_compr_avail)
 #define SNDRV_COMPRESS_PAUSE		_IO('C', 0x30)
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index af5824113246..184722643c97 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -590,6 +590,37 @@  static int snd_compress_check_input(struct snd_compr_params *params)
 
 }
 
+static int snd_compr_set_codec_params(struct snd_compr_stream *stream,
+				      unsigned long arg)
+{
+	struct snd_codec *params;
+	int retval;
+
+	if (!stream->ops->set_codec_params)
+		return -EPERM;
+
+	if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING)
+		return -EPERM;
+
+	/* codec params can be only set when next track has been signalled */
+	if (stream->next_track == false)
+		return -EPERM;
+
+	params = memdup_user((void __user *)arg, sizeof(*params));
+	if (IS_ERR(params))
+		return PTR_ERR(params);
+
+	retval = snd_compress_check_codec_params(params);
+	if (retval)
+		goto out;
+
+	retval = stream->ops->set_codec_params(stream, params);
+
+out:
+	kfree(params);
+	return retval;
+}
+
 static int
 snd_compr_set_params(struct snd_compr_stream *stream, unsigned long arg)
 {
@@ -973,6 +1004,9 @@  static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
 	case _IOC_NR(SNDRV_COMPRESS_GET_PARAMS):
 		retval = snd_compr_get_params(stream, arg);
 		break;
+	case _IOC_NR(SNDRV_COMPRESS_SET_CODEC_PARAMS):
+		retval = snd_compr_set_codec_params(stream, arg);
+		break;
 	case _IOC_NR(SNDRV_COMPRESS_SET_METADATA):
 		retval = snd_compr_set_metadata(stream, arg);
 		break;