[RFC PATCH v2 0/6] ALSA: compress: add support to change codec profile in gapless playback

[RFC PATCH v2 0/6] ALSA: compress: add support to change codec profile in gapless playback

[Srinivas Kandagatla]

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 patchset adds new SNDRV_COMPRESS_SET_CODEC_PARAMS IOCTL along with
flags in capablity structure to allow userspace to set this new
parameters required which switching codec profile, either for gapless
or cross fade usecase.

thanks,
srini


Changes since v1:
- split patch into smaller chuncks,
- bump up the version
- added flags in compress capablity structure 
- added user for this new functionality.
- add this new call in Documentation.


Srinivas Kandagatla (6):
  ALSA: compress: move codec parameter check to a function
  ALSA: compress: add new ioctl for setting codec parameters
  ALSA: compress: add flags to snd_compr_caps to expose dsp caps
  ASoC: compress: add snd_soc_dai_compr_set_codec_params()
  ALSA: compress: bump the version
  ASoC: q6asm-dai: add support to set_codec_params

 .../sound/designs/compress-offload.rst        |  6 ++
 include/sound/compress_driver.h               |  5 ++
 include/sound/soc-component.h                 |  3 +
 include/sound/soc-dai.h                       |  5 ++
 include/uapi/sound/compress_offload.h         | 10 ++-
 sound/core/compress_offload.c                 | 72 +++++++++++++++++--
 sound/soc/qcom/qdsp6/q6asm-dai.c              | 33 +++++++++
 sound/soc/soc-compress.c                      | 30 ++++++++
 sound/soc/soc-dai.c                           | 14 ++++
 9 files changed, 170 insertions(+), 8 deletions(-)

-- 
2.21.0

[RFC PATCH v2 0/6] ALSA: compress: add support to change codec profile in gapless playback

[Srinivas Kandagatla]

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 patchset adds new SNDRV_COMPRESS_SET_CODEC_PARAMS IOCTL along with
flags in capablity structure to allow userspace to set this new
parameters required which switching codec profile, either for gapless
or cross fade usecase.

thanks,
srini


Changes since v1:
- split patch into smaller chuncks,
- bump up the version
- added flags in compress capablity structure 
- added user for this new functionality.
- add this new call in Documentation.


Srinivas Kandagatla (6):
  ALSA: compress: move codec parameter check to a function
  ALSA: compress: add new ioctl for setting codec parameters
  ALSA: compress: add flags to snd_compr_caps to expose dsp caps
  ASoC: compress: add snd_soc_dai_compr_set_codec_params()
  ALSA: compress: bump the version
  ASoC: q6asm-dai: add support to set_codec_params

 .../sound/designs/compress-offload.rst        |  6 ++
 include/sound/compress_driver.h               |  5 ++
 include/sound/soc-component.h                 |  3 +
 include/sound/soc-dai.h                       |  5 ++
 include/uapi/sound/compress_offload.h         | 10 ++-
 sound/core/compress_offload.c                 | 72 +++++++++++++++++--
 sound/soc/qcom/qdsp6/q6asm-dai.c              | 33 +++++++++
 sound/soc/soc-compress.c                      | 30 ++++++++
 sound/soc/soc-dai.c                           | 14 ++++
 9 files changed, 170 insertions(+), 8 deletions(-)

-- 
2.21.0

[RFC PATCH v2 1/6] ALSA: compress: move codec parameter check to a function

[Srinivas Kandagatla]

Move codec parameter checking to dedicated function so that other
callers do not duplicate it. This is in preparedness for adding
snd_compr_set_codec_params() support.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 sound/core/compress_offload.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index 0e53f6f31916..af5824113246 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -566,6 +566,18 @@ static int snd_compr_allocate_buffer(struct snd_compr_stream *stream,
 	return 0;
 }
 
+static int snd_compress_check_codec_params(struct snd_codec *codec)
+{
+	/* now codec parameters */
+	if (codec->id == 0 || codec->id > SND_AUDIOCODEC_MAX)
+		return -EINVAL;
+
+	if (codec->ch_in == 0 || codec->ch_out == 0)
+		return -EINVAL;
+
+	return 0;
+}
+
 static int snd_compress_check_input(struct snd_compr_params *params)
 {
 	/* first let's check the buffer parameter's */
@@ -574,14 +586,8 @@ static int snd_compress_check_input(struct snd_compr_params *params)
 	    params->buffer.fragments == 0)
 		return -EINVAL;
 
-	/* now codec parameters */
-	if (params->codec.id == 0 || params->codec.id > SND_AUDIOCODEC_MAX)
-		return -EINVAL;
+	return snd_compress_check_codec_params(&params->codec);
 
-	if (params->codec.ch_in == 0 || params->codec.ch_out == 0)
-		return -EINVAL;
-
-	return 0;
 }
 
 static int
-- 
2.21.0

[RFC PATCH v2 1/6] ALSA: compress: move codec parameter check to a function

[Srinivas Kandagatla]

Move codec parameter checking to dedicated function so that other
callers do not duplicate it. This is in preparedness for adding
snd_compr_set_codec_params() support.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 sound/core/compress_offload.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index 0e53f6f31916..af5824113246 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -566,6 +566,18 @@ static int snd_compr_allocate_buffer(struct snd_compr_stream *stream,
 	return 0;
 }
 
+static int snd_compress_check_codec_params(struct snd_codec *codec)
+{
+	/* now codec parameters */
+	if (codec->id == 0 || codec->id > SND_AUDIOCODEC_MAX)
+		return -EINVAL;
+
+	if (codec->ch_in == 0 || codec->ch_out == 0)
+		return -EINVAL;
+
+	return 0;
+}
+
 static int snd_compress_check_input(struct snd_compr_params *params)
 {
 	/* first let's check the buffer parameter's */
@@ -574,14 +586,8 @@ static int snd_compress_check_input(struct snd_compr_params *params)
 	    params->buffer.fragments == 0)
 		return -EINVAL;
 
-	/* now codec parameters */
-	if (params->codec.id == 0 || params->codec.id > SND_AUDIOCODEC_MAX)
-		return -EINVAL;
+	return snd_compress_check_codec_params(&params->codec);
 
-	if (params->codec.ch_in == 0 || params->codec.ch_out == 0)
-		return -EINVAL;
-
-	return 0;
 }
 
 static int
-- 
2.21.0

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

[Srinivas Kandagatla]

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(+)

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;
-- 
2.21.0

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

[Srinivas Kandagatla]

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(+)

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;
-- 
2.21.0

[RFC PATCH v2 3/6] ALSA: compress: add flags to snd_compr_caps to expose dsp caps

[Srinivas Kandagatla]

Some of the DSPs like Qualcomm QDSP has ablity to change decoders
dynamically on a single stream, This could be used for features
like Cross Fade or gapless with different codec profile tracks.

Exposing such features in flags variable of compress caps descriptor
would provide userspace more flexibity in usings dsp features.

For now only one flag of switching decoders is added which is used
in subsequent patches.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 include/uapi/sound/compress_offload.h |  7 ++++++-
 sound/core/compress_offload.c         | 20 ++++++++++++++++++++
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h
index c46286113a4b..43c78cdf492c 100644
--- a/include/uapi/sound/compress_offload.h
+++ b/include/uapi/sound/compress_offload.h
@@ -88,6 +88,9 @@ enum snd_compr_direction {
 	SND_COMPRESS_CAPTURE
 };
 
+/* DSP can switch decoder in a single Stream while doing gapless */
+#define SND_COMPR_CAP_FLAGS_DSP_CAN_SWITCH_DECODER	((__u32) 0x00000001)
+
 /**
  * struct snd_compr_caps - caps descriptor
  * @codecs: pointer to array of codecs
@@ -97,6 +100,7 @@ enum snd_compr_direction {
  * @min_fragments: min fragments supported by DSP
  * @max_fragments: max fragments supported by DSP
  * @num_codecs: number of codecs supported
+ * @flags: flags to expose various DSP capabilities
  * @reserved: reserved field
  */
 struct snd_compr_caps {
@@ -107,7 +111,8 @@ struct snd_compr_caps {
 	__u32 min_fragments;
 	__u32 max_fragments;
 	__u32 codecs[MAX_NUM_CODECS];
-	__u32 reserved[11];
+	__u32 flags;
+	__u32 reserved[10];
 } __attribute__((packed, aligned(4)));
 
 /**
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index 184722643c97..abebd5fee2d2 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -590,12 +590,32 @@ static int snd_compress_check_input(struct snd_compr_params *params)
 
 }
 
+static bool snd_compr_can_dsp_switch_decoder(struct snd_compr_stream *stream)
+{
+	struct snd_compr_caps caps;
+
+	if (!stream->ops->get_caps)
+		return false;
+
+	memset(&caps, 0, sizeof(caps));
+	if (stream->ops->get_caps(stream, &caps))
+		return false;
+
+	if (caps.flags & SND_COMPR_CAP_FLAGS_DSP_CAN_SWITCH_DECODER)
+		return true;
+
+	return false;
+}
+
 static int snd_compr_set_codec_params(struct snd_compr_stream *stream,
 				      unsigned long arg)
 {
 	struct snd_codec *params;
 	int retval;
 
+	if (!snd_compr_can_dsp_switch_decoder(stream))
+		return -ENOTSUPP;
+
 	if (!stream->ops->set_codec_params)
 		return -EPERM;
 
-- 
2.21.0

[RFC PATCH v2 3/6] ALSA: compress: add flags to snd_compr_caps to expose dsp caps

[Srinivas Kandagatla]

Some of the DSPs like Qualcomm QDSP has ablity to change decoders
dynamically on a single stream, This could be used for features
like Cross Fade or gapless with different codec profile tracks.

Exposing such features in flags variable of compress caps descriptor
would provide userspace more flexibity in usings dsp features.

For now only one flag of switching decoders is added which is used
in subsequent patches.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 include/uapi/sound/compress_offload.h |  7 ++++++-
 sound/core/compress_offload.c         | 20 ++++++++++++++++++++
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h
index c46286113a4b..43c78cdf492c 100644
--- a/include/uapi/sound/compress_offload.h
+++ b/include/uapi/sound/compress_offload.h
@@ -88,6 +88,9 @@ enum snd_compr_direction {
 	SND_COMPRESS_CAPTURE
 };
 
+/* DSP can switch decoder in a single Stream while doing gapless */
+#define SND_COMPR_CAP_FLAGS_DSP_CAN_SWITCH_DECODER	((__u32) 0x00000001)
+
 /**
  * struct snd_compr_caps - caps descriptor
  * @codecs: pointer to array of codecs
@@ -97,6 +100,7 @@ enum snd_compr_direction {
  * @min_fragments: min fragments supported by DSP
  * @max_fragments: max fragments supported by DSP
  * @num_codecs: number of codecs supported
+ * @flags: flags to expose various DSP capabilities
  * @reserved: reserved field
  */
 struct snd_compr_caps {
@@ -107,7 +111,8 @@ struct snd_compr_caps {
 	__u32 min_fragments;
 	__u32 max_fragments;
 	__u32 codecs[MAX_NUM_CODECS];
-	__u32 reserved[11];
+	__u32 flags;
+	__u32 reserved[10];
 } __attribute__((packed, aligned(4)));
 
 /**
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index 184722643c97..abebd5fee2d2 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -590,12 +590,32 @@ static int snd_compress_check_input(struct snd_compr_params *params)
 
 }
 
+static bool snd_compr_can_dsp_switch_decoder(struct snd_compr_stream *stream)
+{
+	struct snd_compr_caps caps;
+
+	if (!stream->ops->get_caps)
+		return false;
+
+	memset(&caps, 0, sizeof(caps));
+	if (stream->ops->get_caps(stream, &caps))
+		return false;
+
+	if (caps.flags & SND_COMPR_CAP_FLAGS_DSP_CAN_SWITCH_DECODER)
+		return true;
+
+	return false;
+}
+
 static int snd_compr_set_codec_params(struct snd_compr_stream *stream,
 				      unsigned long arg)
 {
 	struct snd_codec *params;
 	int retval;
 
+	if (!snd_compr_can_dsp_switch_decoder(stream))
+		return -ENOTSUPP;
+
 	if (!stream->ops->set_codec_params)
 		return -EPERM;
 
-- 
2.21.0

[RFC PATCH v2 4/6] ASoC: compress: add snd_soc_dai_compr_set_codec_params()

[Srinivas Kandagatla]

Add corresponding ASoC changes to support gapless playback of tracks
which have different codec profile with or without same decoder.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 include/sound/soc-component.h |  3 +++
 include/sound/soc-dai.h       |  5 +++++
 sound/soc/soc-compress.c      | 30 ++++++++++++++++++++++++++++++
 sound/soc/soc-dai.c           | 14 ++++++++++++++
 4 files changed, 52 insertions(+)

diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h
index 5663891148e3..1e69c54ed0b9 100644
--- a/include/sound/soc-component.h
+++ b/include/sound/soc-component.h
@@ -36,6 +36,9 @@ struct snd_compress_ops {
 	int (*get_params)(struct snd_soc_component *component,
 			  struct snd_compr_stream *stream,
 			  struct snd_codec *params);
+	int (*set_codec_params)(struct snd_soc_component *component,
+			  struct snd_compr_stream *stream,
+			  struct snd_codec *params);
 	int (*set_metadata)(struct snd_soc_component *component,
 			    struct snd_compr_stream *stream,
 			    struct snd_compr_metadata *metadata);
diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
index 212257e84fac..526794ee555b 100644
--- a/include/sound/soc-dai.h
+++ b/include/sound/soc-dai.h
@@ -192,6 +192,9 @@ int snd_soc_dai_compr_trigger(struct snd_soc_dai *dai,
 int snd_soc_dai_compr_set_params(struct snd_soc_dai *dai,
 				 struct snd_compr_stream *cstream,
 				 struct snd_compr_params *params);
+int snd_soc_dai_compr_set_codec_params(struct snd_soc_dai *dai,
+				 struct snd_compr_stream *cstream,
+				 struct snd_codec *codec);
 int snd_soc_dai_compr_get_params(struct snd_soc_dai *dai,
 				 struct snd_compr_stream *cstream,
 				 struct snd_codec *params);
@@ -292,6 +295,8 @@ struct snd_soc_cdai_ops {
 			struct snd_soc_dai *);
 	int (*set_params)(struct snd_compr_stream *,
 			struct snd_compr_params *, struct snd_soc_dai *);
+	int (*set_codec_params)(struct snd_compr_stream *,
+			struct snd_codec *, struct snd_soc_dai *);
 	int (*get_params)(struct snd_compr_stream *,
 			struct snd_codec *, struct snd_soc_dai *);
 	int (*set_metadata)(struct snd_compr_stream *,
diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c
index 4984b6a2c370..e549e0197aca 100644
--- a/sound/soc/soc-compress.c
+++ b/sound/soc/soc-compress.c
@@ -518,6 +518,34 @@ static int soc_compr_set_params_fe(struct snd_compr_stream *cstream,
 	return ret;
 }
 
+static int soc_compr_set_codec_params(struct snd_compr_stream *cstream,
+				      struct snd_codec *codec)
+{
+	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
+	struct snd_soc_component *component;
+	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
+	int i, ret;
+
+	mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
+
+	ret = snd_soc_dai_compr_set_codec_params(cpu_dai, cstream, codec);
+	if (ret < 0)
+		goto err;
+
+	for_each_rtd_components(rtd, i, component) {
+		if (!component->driver->compress_ops ||
+		    !component->driver->compress_ops->set_codec_params)
+			continue;
+
+		ret = component->driver->compress_ops->set_codec_params(component, cstream,
+								     codec);
+		break;
+	}
+err:
+	mutex_unlock(&rtd->card->pcm_mutex);
+	return ret;
+}
+
 static int soc_compr_get_params(struct snd_compr_stream *cstream,
 				struct snd_codec *params)
 {
@@ -728,6 +756,7 @@ static struct snd_compr_ops soc_compr_ops = {
 	.open		= soc_compr_open,
 	.free		= soc_compr_free,
 	.set_params	= soc_compr_set_params,
+	.set_codec_params = soc_compr_set_codec_params,
 	.set_metadata   = soc_compr_set_metadata,
 	.get_metadata	= soc_compr_get_metadata,
 	.get_params	= soc_compr_get_params,
@@ -744,6 +773,7 @@ static struct snd_compr_ops soc_compr_dyn_ops = {
 	.free		= soc_compr_free_fe,
 	.set_params	= soc_compr_set_params_fe,
 	.get_params	= soc_compr_get_params,
+	.set_codec_params = soc_compr_set_codec_params,
 	.set_metadata   = soc_compr_set_metadata,
 	.get_metadata	= soc_compr_get_metadata,
 	.trigger	= soc_compr_trigger_fe,
diff --git a/sound/soc/soc-dai.c b/sound/soc/soc-dai.c
index b05e18b63a1c..06481d0278b8 100644
--- a/sound/soc/soc-dai.c
+++ b/sound/soc/soc-dai.c
@@ -594,6 +594,20 @@ int snd_soc_dai_compr_get_params(struct snd_soc_dai *dai,
 }
 EXPORT_SYMBOL_GPL(snd_soc_dai_compr_get_params);
 
+int snd_soc_dai_compr_set_codec_params(struct snd_soc_dai *dai,
+					struct snd_compr_stream *cstream,
+					struct snd_codec *codec)
+{	int ret = 0;
+
+	if (dai->driver->cops &&
+	    dai->driver->cops->set_codec_params)
+		ret = dai->driver->cops->set_codec_params(cstream, codec, dai);
+
+	return soc_dai_ret(dai, ret);
+
+}
+EXPORT_SYMBOL_GPL(snd_soc_dai_compr_set_codec_params);
+
 int snd_soc_dai_compr_ack(struct snd_soc_dai *dai,
 			  struct snd_compr_stream *cstream,
 			  size_t bytes)
-- 
2.21.0

[RFC PATCH v2 4/6] ASoC: compress: add snd_soc_dai_compr_set_codec_params()

[Srinivas Kandagatla]

Add corresponding ASoC changes to support gapless playback of tracks
which have different codec profile with or without same decoder.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 include/sound/soc-component.h |  3 +++
 include/sound/soc-dai.h       |  5 +++++
 sound/soc/soc-compress.c      | 30 ++++++++++++++++++++++++++++++
 sound/soc/soc-dai.c           | 14 ++++++++++++++
 4 files changed, 52 insertions(+)

diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h
index 5663891148e3..1e69c54ed0b9 100644
--- a/include/sound/soc-component.h
+++ b/include/sound/soc-component.h
@@ -36,6 +36,9 @@ struct snd_compress_ops {
 	int (*get_params)(struct snd_soc_component *component,
 			  struct snd_compr_stream *stream,
 			  struct snd_codec *params);
+	int (*set_codec_params)(struct snd_soc_component *component,
+			  struct snd_compr_stream *stream,
+			  struct snd_codec *params);
 	int (*set_metadata)(struct snd_soc_component *component,
 			    struct snd_compr_stream *stream,
 			    struct snd_compr_metadata *metadata);
diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
index 212257e84fac..526794ee555b 100644
--- a/include/sound/soc-dai.h
+++ b/include/sound/soc-dai.h
@@ -192,6 +192,9 @@ int snd_soc_dai_compr_trigger(struct snd_soc_dai *dai,
 int snd_soc_dai_compr_set_params(struct snd_soc_dai *dai,
 				 struct snd_compr_stream *cstream,
 				 struct snd_compr_params *params);
+int snd_soc_dai_compr_set_codec_params(struct snd_soc_dai *dai,
+				 struct snd_compr_stream *cstream,
+				 struct snd_codec *codec);
 int snd_soc_dai_compr_get_params(struct snd_soc_dai *dai,
 				 struct snd_compr_stream *cstream,
 				 struct snd_codec *params);
@@ -292,6 +295,8 @@ struct snd_soc_cdai_ops {
 			struct snd_soc_dai *);
 	int (*set_params)(struct snd_compr_stream *,
 			struct snd_compr_params *, struct snd_soc_dai *);
+	int (*set_codec_params)(struct snd_compr_stream *,
+			struct snd_codec *, struct snd_soc_dai *);
 	int (*get_params)(struct snd_compr_stream *,
 			struct snd_codec *, struct snd_soc_dai *);
 	int (*set_metadata)(struct snd_compr_stream *,
diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c
index 4984b6a2c370..e549e0197aca 100644
--- a/sound/soc/soc-compress.c
+++ b/sound/soc/soc-compress.c
@@ -518,6 +518,34 @@ static int soc_compr_set_params_fe(struct snd_compr_stream *cstream,
 	return ret;
 }
 
+static int soc_compr_set_codec_params(struct snd_compr_stream *cstream,
+				      struct snd_codec *codec)
+{
+	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
+	struct snd_soc_component *component;
+	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
+	int i, ret;
+
+	mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
+
+	ret = snd_soc_dai_compr_set_codec_params(cpu_dai, cstream, codec);
+	if (ret < 0)
+		goto err;
+
+	for_each_rtd_components(rtd, i, component) {
+		if (!component->driver->compress_ops ||
+		    !component->driver->compress_ops->set_codec_params)
+			continue;
+
+		ret = component->driver->compress_ops->set_codec_params(component, cstream,
+								     codec);
+		break;
+	}
+err:
+	mutex_unlock(&rtd->card->pcm_mutex);
+	return ret;
+}
+
 static int soc_compr_get_params(struct snd_compr_stream *cstream,
 				struct snd_codec *params)
 {
@@ -728,6 +756,7 @@ static struct snd_compr_ops soc_compr_ops = {
 	.open		= soc_compr_open,
 	.free		= soc_compr_free,
 	.set_params	= soc_compr_set_params,
+	.set_codec_params = soc_compr_set_codec_params,
 	.set_metadata   = soc_compr_set_metadata,
 	.get_metadata	= soc_compr_get_metadata,
 	.get_params	= soc_compr_get_params,
@@ -744,6 +773,7 @@ static struct snd_compr_ops soc_compr_dyn_ops = {
 	.free		= soc_compr_free_fe,
 	.set_params	= soc_compr_set_params_fe,
 	.get_params	= soc_compr_get_params,
+	.set_codec_params = soc_compr_set_codec_params,
 	.set_metadata   = soc_compr_set_metadata,
 	.get_metadata	= soc_compr_get_metadata,
 	.trigger	= soc_compr_trigger_fe,
diff --git a/sound/soc/soc-dai.c b/sound/soc/soc-dai.c
index b05e18b63a1c..06481d0278b8 100644
--- a/sound/soc/soc-dai.c
+++ b/sound/soc/soc-dai.c
@@ -594,6 +594,20 @@ int snd_soc_dai_compr_get_params(struct snd_soc_dai *dai,
 }
 EXPORT_SYMBOL_GPL(snd_soc_dai_compr_get_params);
 
+int snd_soc_dai_compr_set_codec_params(struct snd_soc_dai *dai,
+					struct snd_compr_stream *cstream,
+					struct snd_codec *codec)
+{	int ret = 0;
+
+	if (dai->driver->cops &&
+	    dai->driver->cops->set_codec_params)
+		ret = dai->driver->cops->set_codec_params(cstream, codec, dai);
+
+	return soc_dai_ret(dai, ret);
+
+}
+EXPORT_SYMBOL_GPL(snd_soc_dai_compr_set_codec_params);
+
 int snd_soc_dai_compr_ack(struct snd_soc_dai *dai,
 			  struct snd_compr_stream *cstream,
 			  size_t bytes)
-- 
2.21.0

[RFC PATCH v2 5/6] ALSA: compress: bump the version

[Srinivas Kandagatla]

Now that we have added support for new IOCTL and flags to
struct snd_compr_caps, bump up the version to 0,2,1
to help users find it.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 include/uapi/sound/compress_offload.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h
index 43c78cdf492c..0d407b57a1d4 100644
--- a/include/uapi/sound/compress_offload.h
+++ b/include/uapi/sound/compress_offload.h
@@ -31,7 +31,7 @@
 #include <sound/compress_params.h>
 
 
-#define SNDRV_COMPRESS_VERSION SNDRV_PROTOCOL_VERSION(0, 2, 0)
+#define SNDRV_COMPRESS_VERSION SNDRV_PROTOCOL_VERSION(0, 2, 1)
 /**
  * struct snd_compressed_buffer - compressed buffer
  * @fragment_size: size of buffer fragment in bytes
-- 
2.21.0

[RFC PATCH v2 5/6] ALSA: compress: bump the version

[Srinivas Kandagatla]

Now that we have added support for new IOCTL and flags to
struct snd_compr_caps, bump up the version to 0,2,1
to help users find it.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 include/uapi/sound/compress_offload.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h
index 43c78cdf492c..0d407b57a1d4 100644
--- a/include/uapi/sound/compress_offload.h
+++ b/include/uapi/sound/compress_offload.h
@@ -31,7 +31,7 @@
 #include <sound/compress_params.h>
 
 
-#define SNDRV_COMPRESS_VERSION SNDRV_PROTOCOL_VERSION(0, 2, 0)
+#define SNDRV_COMPRESS_VERSION SNDRV_PROTOCOL_VERSION(0, 2, 1)
 /**
  * struct snd_compressed_buffer - compressed buffer
  * @fragment_size: size of buffer fragment in bytes
-- 
2.21.0

[RFC PATCH v2 6/6] ASoC: q6asm-dai: add support to set_codec_params

[Srinivas Kandagatla]

Make use of new set_codec_params callback to allow decoder switching
during gapless playback.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 sound/soc/qcom/qdsp6/q6asm-dai.c | 33 ++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/sound/soc/qcom/qdsp6/q6asm-dai.c b/sound/soc/qcom/qdsp6/q6asm-dai.c
index b5c719682919..a8cfb1996614 100644
--- a/sound/soc/qcom/qdsp6/q6asm-dai.c
+++ b/sound/soc/qcom/qdsp6/q6asm-dai.c
@@ -876,6 +876,37 @@ static int __q6asm_dai_compr_set_codec_params(struct snd_soc_component *componen
 	return 0;
 }
 
+static int q6asm_dai_compr_set_codec_params(struct snd_soc_component *component,
+					    struct snd_compr_stream *stream,
+					    struct snd_codec *codec)
+{
+	struct snd_compr_runtime *runtime = stream->runtime;
+	struct q6asm_dai_rtd *prtd = runtime->private_data;
+	int ret;
+
+	ret = q6asm_open_write(prtd->audio_client, prtd->next_track_stream_id,
+			       codec->id, codec->profile, prtd->bits_per_sample,
+			       true);
+	if (ret < 0) {
+		pr_err("q6asm_open_write failed\n");
+		return ret;
+	}
+
+	ret = __q6asm_dai_compr_set_codec_params(component, stream, codec,
+						 prtd->next_track_stream_id);
+	if (ret < 0) {
+		pr_err("q6asm_open_write failed\n");
+		return ret;
+	}
+
+	ret = q6asm_stream_remove_initial_silence(prtd->audio_client,
+						   prtd->next_track_stream_id,
+						   prtd->initial_samples_drop);
+	prtd->next_track_stream_id = 0;
+
+	return ret;
+}
+
 static int q6asm_dai_compr_set_params(struct snd_soc_component *component,
 				      struct snd_compr_stream *stream,
 				      struct snd_compr_params *params)
@@ -1144,6 +1175,7 @@ static int q6asm_dai_compr_get_caps(struct snd_soc_component *component,
 	caps->max_fragment_size = COMPR_PLAYBACK_MAX_FRAGMENT_SIZE;
 	caps->min_fragments = COMPR_PLAYBACK_MIN_NUM_FRAGMENTS;
 	caps->max_fragments = COMPR_PLAYBACK_MAX_NUM_FRAGMENTS;
+	caps->flags = SND_COMPR_CAP_FLAGS_DSP_CAN_SWITCH_DECODER;
 	caps->num_codecs = 5;
 	caps->codecs[0] = SND_AUDIOCODEC_MP3;
 	caps->codecs[1] = SND_AUDIOCODEC_FLAC;
@@ -1173,6 +1205,7 @@ static struct snd_compress_ops q6asm_dai_compress_ops = {
 	.open		= q6asm_dai_compr_open,
 	.free		= q6asm_dai_compr_free,
 	.set_params	= q6asm_dai_compr_set_params,
+	.set_codec_params = q6asm_dai_compr_set_codec_params,
 	.set_metadata	= q6asm_dai_compr_set_metadata,
 	.pointer	= q6asm_dai_compr_pointer,
 	.trigger	= q6asm_dai_compr_trigger,
-- 
2.21.0

[RFC PATCH v2 6/6] ASoC: q6asm-dai: add support to set_codec_params

[Srinivas Kandagatla]

Make use of new set_codec_params callback to allow decoder switching
during gapless playback.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 sound/soc/qcom/qdsp6/q6asm-dai.c | 33 ++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/sound/soc/qcom/qdsp6/q6asm-dai.c b/sound/soc/qcom/qdsp6/q6asm-dai.c
index b5c719682919..a8cfb1996614 100644
--- a/sound/soc/qcom/qdsp6/q6asm-dai.c
+++ b/sound/soc/qcom/qdsp6/q6asm-dai.c
@@ -876,6 +876,37 @@ static int __q6asm_dai_compr_set_codec_params(struct snd_soc_component *componen
 	return 0;
 }
 
+static int q6asm_dai_compr_set_codec_params(struct snd_soc_component *component,
+					    struct snd_compr_stream *stream,
+					    struct snd_codec *codec)
+{
+	struct snd_compr_runtime *runtime = stream->runtime;
+	struct q6asm_dai_rtd *prtd = runtime->private_data;
+	int ret;
+
+	ret = q6asm_open_write(prtd->audio_client, prtd->next_track_stream_id,
+			       codec->id, codec->profile, prtd->bits_per_sample,
+			       true);
+	if (ret < 0) {
+		pr_err("q6asm_open_write failed\n");
+		return ret;
+	}
+
+	ret = __q6asm_dai_compr_set_codec_params(component, stream, codec,
+						 prtd->next_track_stream_id);
+	if (ret < 0) {
+		pr_err("q6asm_open_write failed\n");
+		return ret;
+	}
+
+	ret = q6asm_stream_remove_initial_silence(prtd->audio_client,
+						   prtd->next_track_stream_id,
+						   prtd->initial_samples_drop);
+	prtd->next_track_stream_id = 0;
+
+	return ret;
+}
+
 static int q6asm_dai_compr_set_params(struct snd_soc_component *component,
 				      struct snd_compr_stream *stream,
 				      struct snd_compr_params *params)
@@ -1144,6 +1175,7 @@ static int q6asm_dai_compr_get_caps(struct snd_soc_component *component,
 	caps->max_fragment_size = COMPR_PLAYBACK_MAX_FRAGMENT_SIZE;
 	caps->min_fragments = COMPR_PLAYBACK_MIN_NUM_FRAGMENTS;
 	caps->max_fragments = COMPR_PLAYBACK_MAX_NUM_FRAGMENTS;
+	caps->flags = SND_COMPR_CAP_FLAGS_DSP_CAN_SWITCH_DECODER;
 	caps->num_codecs = 5;
 	caps->codecs[0] = SND_AUDIOCODEC_MP3;
 	caps->codecs[1] = SND_AUDIOCODEC_FLAC;
@@ -1173,6 +1205,7 @@ static struct snd_compress_ops q6asm_dai_compress_ops = {
 	.open		= q6asm_dai_compr_open,
 	.free		= q6asm_dai_compr_free,
 	.set_params	= q6asm_dai_compr_set_params,
+	.set_codec_params = q6asm_dai_compr_set_codec_params,
 	.set_metadata	= q6asm_dai_compr_set_metadata,
 	.pointer	= q6asm_dai_compr_pointer,
 	.trigger	= q6asm_dai_compr_trigger,
-- 
2.21.0

Re: [RFC PATCH v2 1/6] ALSA: compress: move codec parameter check to a function

[Pierre-Louis Bossart]

On 7/21/20 12:00 PM, Srinivas Kandagatla wrote:
> Move codec parameter checking to dedicated function so that other
> callers do not duplicate it. This is in preparedness for adding

preparation?

> snd_compr_set_codec_params() support.
> 

Re: [RFC PATCH v2 1/6] ALSA: compress: move codec parameter check to a function

[Pierre-Louis Bossart]

On 7/21/20 12:00 PM, Srinivas Kandagatla wrote:
> Move codec parameter checking to dedicated function so that other
> callers do not duplicate it. This is in preparedness for adding

preparation?

> snd_compr_set_codec_params() support.
> 

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

[Pierre-Louis Bossart]

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.

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

[Pierre-Louis Bossart]

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.

Re: [RFC PATCH v2 6/6] ASoC: q6asm-dai: add support to set_codec_params

[Pierre-Louis Bossart]

On 7/21/20 12:00 PM, Srinivas Kandagatla wrote:
> Make use of new set_codec_params callback to allow decoder switching
> during gapless playback.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>   sound/soc/qcom/qdsp6/q6asm-dai.c | 33 ++++++++++++++++++++++++++++++++
>   1 file changed, 33 insertions(+)
> 
> diff --git a/sound/soc/qcom/qdsp6/q6asm-dai.c b/sound/soc/qcom/qdsp6/q6asm-dai.c
> index b5c719682919..a8cfb1996614 100644
> --- a/sound/soc/qcom/qdsp6/q6asm-dai.c
> +++ b/sound/soc/qcom/qdsp6/q6asm-dai.c
> @@ -876,6 +876,37 @@ static int __q6asm_dai_compr_set_codec_params(struct snd_soc_component *componen
>   	return 0;
>   }
>   
> +static int q6asm_dai_compr_set_codec_params(struct snd_soc_component *component,
> +					    struct snd_compr_stream *stream,
> +					    struct snd_codec *codec)
> +{
> +	struct snd_compr_runtime *runtime = stream->runtime;
> +	struct q6asm_dai_rtd *prtd = runtime->private_data;
> +	int ret;
> +
> +	ret = q6asm_open_write(prtd->audio_client, prtd->next_track_stream_id,
> +			       codec->id, codec->profile, prtd->bits_per_sample,
> +			       true);
> +	if (ret < 0) {
> +		pr_err("q6asm_open_write failed\n");
> +		return ret;
> +	}
> +
> +	ret = __q6asm_dai_compr_set_codec_params(component, stream, codec,
> +						 prtd->next_track_stream_id);
> +	if (ret < 0) {
> +		pr_err("q6asm_open_write failed\n");
> +		return ret;
> +	}
> +
> +	ret = q6asm_stream_remove_initial_silence(prtd->audio_client,
> +						   prtd->next_track_stream_id,
> +						   prtd->initial_samples_drop);
> +	prtd->next_track_stream_id = 0;

same comment as in the other patchset, the stream_id toggles between 1 
and 2, it's not clear to me what 0 means.

off-by-one bug or feature?

Re: [RFC PATCH v2 6/6] ASoC: q6asm-dai: add support to set_codec_params

[Pierre-Louis Bossart]

On 7/21/20 12:00 PM, Srinivas Kandagatla wrote:
> Make use of new set_codec_params callback to allow decoder switching
> during gapless playback.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>   sound/soc/qcom/qdsp6/q6asm-dai.c | 33 ++++++++++++++++++++++++++++++++
>   1 file changed, 33 insertions(+)
> 
> diff --git a/sound/soc/qcom/qdsp6/q6asm-dai.c b/sound/soc/qcom/qdsp6/q6asm-dai.c
> index b5c719682919..a8cfb1996614 100644
> --- a/sound/soc/qcom/qdsp6/q6asm-dai.c
> +++ b/sound/soc/qcom/qdsp6/q6asm-dai.c
> @@ -876,6 +876,37 @@ static int __q6asm_dai_compr_set_codec_params(struct snd_soc_component *componen
>   	return 0;
>   }
>   
> +static int q6asm_dai_compr_set_codec_params(struct snd_soc_component *component,
> +					    struct snd_compr_stream *stream,
> +					    struct snd_codec *codec)
> +{
> +	struct snd_compr_runtime *runtime = stream->runtime;
> +	struct q6asm_dai_rtd *prtd = runtime->private_data;
> +	int ret;
> +
> +	ret = q6asm_open_write(prtd->audio_client, prtd->next_track_stream_id,
> +			       codec->id, codec->profile, prtd->bits_per_sample,
> +			       true);
> +	if (ret < 0) {
> +		pr_err("q6asm_open_write failed\n");
> +		return ret;
> +	}
> +
> +	ret = __q6asm_dai_compr_set_codec_params(component, stream, codec,
> +						 prtd->next_track_stream_id);
> +	if (ret < 0) {
> +		pr_err("q6asm_open_write failed\n");
> +		return ret;
> +	}
> +
> +	ret = q6asm_stream_remove_initial_silence(prtd->audio_client,
> +						   prtd->next_track_stream_id,
> +						   prtd->initial_samples_drop);
> +	prtd->next_track_stream_id = 0;

same comment as in the other patchset, the stream_id toggles between 1 
and 2, it's not clear to me what 0 means.

off-by-one bug or feature?

Re: [RFC PATCH v2 6/6] ASoC: q6asm-dai: add support to set_codec_params

[Srinivas Kandagatla]

Thanks Pierre for quick review.

On 21/07/2020 21:09, Pierre-Louis Bossart wrote:
> 
> 
> On 7/21/20 12:00 PM, Srinivas Kandagatla wrote:
>> Make use of new set_codec_params callback to allow decoder switching
>> during gapless playback.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>   sound/soc/qcom/qdsp6/q6asm-dai.c | 33 ++++++++++++++++++++++++++++++++
>>   1 file changed, 33 insertions(+)
>>
>> diff --git a/sound/soc/qcom/qdsp6/q6asm-dai.c 
>> b/sound/soc/qcom/qdsp6/q6asm-dai.c
>> index b5c719682919..a8cfb1996614 100644
>> --- a/sound/soc/qcom/qdsp6/q6asm-dai.c
>> +++ b/sound/soc/qcom/qdsp6/q6asm-dai.c
>> @@ -876,6 +876,37 @@ static int 
>> __q6asm_dai_compr_set_codec_params(struct snd_soc_component *componen
>>       return 0;
>>   }
>> +static int q6asm_dai_compr_set_codec_params(struct snd_soc_component 
>> *component,
>> +                        struct snd_compr_stream *stream,
>> +                        struct snd_codec *codec)
>> +{
>> +    struct snd_compr_runtime *runtime = stream->runtime;
>> +    struct q6asm_dai_rtd *prtd = runtime->private_data;
>> +    int ret;
>> +
>> +    ret = q6asm_open_write(prtd->audio_client, 
>> prtd->next_track_stream_id,
>> +                   codec->id, codec->profile, prtd->bits_per_sample,
>> +                   true);
>> +    if (ret < 0) {
>> +        pr_err("q6asm_open_write failed\n");
>> +        return ret;
>> +    }
>> +
>> +    ret = __q6asm_dai_compr_set_codec_params(component, stream, codec,
>> +                         prtd->next_track_stream_id);
>> +    if (ret < 0) {
>> +        pr_err("q6asm_open_write failed\n");
>> +        return ret;
>> +    }
>> +
>> +    ret = q6asm_stream_remove_initial_silence(prtd->audio_client,
>> +                           prtd->next_track_stream_id,
>> +                           prtd->initial_samples_drop);
>> +    prtd->next_track_stream_id = 0;
> 
> same comment as in the other patchset, the stream_id toggles between 1 
> and 2, it's not clear to me what 0 means.

Valid stream ids start from 1. to achieve gapless we toggle between 1 and 2.

--srini


> 
> off-by-one bug or feature?

Re: [RFC PATCH v2 6/6] ASoC: q6asm-dai: add support to set_codec_params

[Srinivas Kandagatla]

Thanks Pierre for quick review.

On 21/07/2020 21:09, Pierre-Louis Bossart wrote:
> 
> 
> On 7/21/20 12:00 PM, Srinivas Kandagatla wrote:
>> Make use of new set_codec_params callback to allow decoder switching
>> during gapless playback.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>   sound/soc/qcom/qdsp6/q6asm-dai.c | 33 ++++++++++++++++++++++++++++++++
>>   1 file changed, 33 insertions(+)
>>
>> diff --git a/sound/soc/qcom/qdsp6/q6asm-dai.c 
>> b/sound/soc/qcom/qdsp6/q6asm-dai.c
>> index b5c719682919..a8cfb1996614 100644
>> --- a/sound/soc/qcom/qdsp6/q6asm-dai.c
>> +++ b/sound/soc/qcom/qdsp6/q6asm-dai.c
>> @@ -876,6 +876,37 @@ static int 
>> __q6asm_dai_compr_set_codec_params(struct snd_soc_component *componen
>>       return 0;
>>   }
>> +static int q6asm_dai_compr_set_codec_params(struct snd_soc_component 
>> *component,
>> +                        struct snd_compr_stream *stream,
>> +                        struct snd_codec *codec)
>> +{
>> +    struct snd_compr_runtime *runtime = stream->runtime;
>> +    struct q6asm_dai_rtd *prtd = runtime->private_data;
>> +    int ret;
>> +
>> +    ret = q6asm_open_write(prtd->audio_client, 
>> prtd->next_track_stream_id,
>> +                   codec->id, codec->profile, prtd->bits_per_sample,
>> +                   true);
>> +    if (ret < 0) {
>> +        pr_err("q6asm_open_write failed\n");
>> +        return ret;
>> +    }
>> +
>> +    ret = __q6asm_dai_compr_set_codec_params(component, stream, codec,
>> +                         prtd->next_track_stream_id);
>> +    if (ret < 0) {
>> +        pr_err("q6asm_open_write failed\n");
>> +        return ret;
>> +    }
>> +
>> +    ret = q6asm_stream_remove_initial_silence(prtd->audio_client,
>> +                           prtd->next_track_stream_id,
>> +                           prtd->initial_samples_drop);
>> +    prtd->next_track_stream_id = 0;
> 
> same comment as in the other patchset, the stream_id toggles between 1 
> and 2, it's not clear to me what 0 means.

Valid stream ids start from 1. to achieve gapless we toggle between 1 and 2.

--srini


> 
> off-by-one bug or feature?

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

[Srinivas Kandagatla]

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

> 

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

[Srinivas Kandagatla]

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

> 

Re: [RFC PATCH v2 6/6] ASoC: q6asm-dai: add support to set_codec_params

[Daniel Baluta]

On Tue, Jul 21, 2020 at 8:03 PM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
> Make use of new set_codec_params callback to allow decoder switching
> during gapless playback.
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  sound/soc/qcom/qdsp6/q6asm-dai.c | 33 ++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>
> diff --git a/sound/soc/qcom/qdsp6/q6asm-dai.c b/sound/soc/qcom/qdsp6/q6asm-dai.c
> index b5c719682919..a8cfb1996614 100644
> --- a/sound/soc/qcom/qdsp6/q6asm-dai.c
> +++ b/sound/soc/qcom/qdsp6/q6asm-dai.c
> @@ -876,6 +876,37 @@ static int __q6asm_dai_compr_set_codec_params(struct snd_soc_component *componen
>         return 0;
>  }
>
> +static int q6asm_dai_compr_set_codec_params(struct snd_soc_component *component,
> +                                           struct snd_compr_stream *stream,
> +                                           struct snd_codec *codec)
> +{
> +       struct snd_compr_runtime *runtime = stream->runtime;
> +       struct q6asm_dai_rtd *prtd = runtime->private_data;
> +       int ret;
> +
> +       ret = q6asm_open_write(prtd->audio_client, prtd->next_track_stream_id,
> +                              codec->id, codec->profile, prtd->bits_per_sample,
> +                              true);
> +       if (ret < 0) {
> +               pr_err("q6asm_open_write failed\n");

Since you have component->dev here I think it is worth it to use
dev_err instead of pr_err.

Same for the rest of the code.

Re: [RFC PATCH v2 6/6] ASoC: q6asm-dai: add support to set_codec_params

[Daniel Baluta]

On Tue, Jul 21, 2020 at 8:03 PM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
> Make use of new set_codec_params callback to allow decoder switching
> during gapless playback.
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  sound/soc/qcom/qdsp6/q6asm-dai.c | 33 ++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>
> diff --git a/sound/soc/qcom/qdsp6/q6asm-dai.c b/sound/soc/qcom/qdsp6/q6asm-dai.c
> index b5c719682919..a8cfb1996614 100644
> --- a/sound/soc/qcom/qdsp6/q6asm-dai.c
> +++ b/sound/soc/qcom/qdsp6/q6asm-dai.c
> @@ -876,6 +876,37 @@ static int __q6asm_dai_compr_set_codec_params(struct snd_soc_component *componen
>         return 0;
>  }
>
> +static int q6asm_dai_compr_set_codec_params(struct snd_soc_component *component,
> +                                           struct snd_compr_stream *stream,
> +                                           struct snd_codec *codec)
> +{
> +       struct snd_compr_runtime *runtime = stream->runtime;
> +       struct q6asm_dai_rtd *prtd = runtime->private_data;
> +       int ret;
> +
> +       ret = q6asm_open_write(prtd->audio_client, prtd->next_track_stream_id,
> +                              codec->id, codec->profile, prtd->bits_per_sample,
> +                              true);
> +       if (ret < 0) {
> +               pr_err("q6asm_open_write failed\n");

Since you have component->dev here I think it is worth it to use
dev_err instead of pr_err.

Same for the rest of the code.

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

[Pierre-Louis Bossart]

>>>    * 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?

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

[Pierre-Louis Bossart]

>>>    * 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?

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

[Vinod Koul]

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.

-- 
~Vinod

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

[Vinod Koul]

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.

-- 
~Vinod

Re: [RFC PATCH v2 0/6] ALSA: compress: add support to change codec profile in gapless playback

[Takashi Iwai]

On Tue, 21 Jul 2020 19:00:01 +0200,
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
> 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 patchset adds new SNDRV_COMPRESS_SET_CODEC_PARAMS IOCTL along with
> flags in capablity structure to allow userspace to set this new
> parameters required which switching codec profile, either for gapless
> or cross fade usecase.

One idea that came up at the previous audio conference regarding this
implementation was to just allow SET_PARAMS during the stream is
running (only if the driver sets the capability) instead of
introducing yet a new ioctl and an ops.
Would it make sense?

I have no big objection to add a new ioctl if other people agree,
though.


thanks,

Takashi

Re: [RFC PATCH v2 0/6] ALSA: compress: add support to change codec profile in gapless playback

[Takashi Iwai]

On Tue, 21 Jul 2020 19:00:01 +0200,
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
> 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 patchset adds new SNDRV_COMPRESS_SET_CODEC_PARAMS IOCTL along with
> flags in capablity structure to allow userspace to set this new
> parameters required which switching codec profile, either for gapless
> or cross fade usecase.

One idea that came up at the previous audio conference regarding this
implementation was to just allow SET_PARAMS during the stream is
running (only if the driver sets the capability) instead of
introducing yet a new ioctl and an ops.
Would it make sense?

I have no big objection to add a new ioctl if other people agree,
though.


thanks,

Takashi

Re: [RFC PATCH v2 0/6] ALSA: compress: add support to change codec profile in gapless playback

[Vinod Koul]

On 23-07-20, 14:38, Takashi Iwai wrote:
> On Tue, 21 Jul 2020 19:00:01 +0200,
> 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
> > 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 patchset adds new SNDRV_COMPRESS_SET_CODEC_PARAMS IOCTL along with
> > flags in capablity structure to allow userspace to set this new
> > parameters required which switching codec profile, either for gapless
> > or cross fade usecase.
> 
> One idea that came up at the previous audio conference regarding this
> implementation was to just allow SET_PARAMS during the stream is
> running (only if the driver sets the capability) instead of
> introducing yet a new ioctl and an ops.
> Would it make sense?

That does sound good but only issue would be that we need to somehow
mark/document that buffer info is useless and would be discarded, how do
we do that?

> I have no big objection to add a new ioctl if other people agree,
> though.
> 
> 
> thanks,
> 
> Takashi

-- 
~Vinod

Re: [RFC PATCH v2 0/6] ALSA: compress: add support to change codec profile in gapless playback

[Vinod Koul]

On 23-07-20, 14:38, Takashi Iwai wrote:
> On Tue, 21 Jul 2020 19:00:01 +0200,
> 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
> > 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 patchset adds new SNDRV_COMPRESS_SET_CODEC_PARAMS IOCTL along with
> > flags in capablity structure to allow userspace to set this new
> > parameters required which switching codec profile, either for gapless
> > or cross fade usecase.
> 
> One idea that came up at the previous audio conference regarding this
> implementation was to just allow SET_PARAMS during the stream is
> running (only if the driver sets the capability) instead of
> introducing yet a new ioctl and an ops.
> Would it make sense?

That does sound good but only issue would be that we need to somehow
mark/document that buffer info is useless and would be discarded, how do
we do that?

> I have no big objection to add a new ioctl if other people agree,
> though.
> 
> 
> thanks,
> 
> Takashi

-- 
~Vinod

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

[Pierre-Louis Bossart]

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...

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

[Pierre-Louis Bossart]

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...

Re: [RFC PATCH v2 0/6] ALSA: compress: add support to change codec profile in gapless playback

[Takashi Iwai]

On Thu, 23 Jul 2020 15:05:22 +0200,
Vinod Koul wrote:
> 
> On 23-07-20, 14:38, Takashi Iwai wrote:
> > On Tue, 21 Jul 2020 19:00:01 +0200,
> > 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
> > > 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 patchset adds new SNDRV_COMPRESS_SET_CODEC_PARAMS IOCTL along with
> > > flags in capablity structure to allow userspace to set this new
> > > parameters required which switching codec profile, either for gapless
> > > or cross fade usecase.
> > 
> > One idea that came up at the previous audio conference regarding this
> > implementation was to just allow SET_PARAMS during the stream is
> > running (only if the driver sets the capability) instead of
> > introducing yet a new ioctl and an ops.
> > Would it make sense?
> 
> That does sound good but only issue would be that we need to somehow
> mark/document that buffer info is useless and would be discarded, how do
> we do that?

Yes, the buffer and no_wake_mode can be ignored in the gapless
re-setup.  Is your concern only about the documentation?  Or something
else needs to be changed significantly?  It's a new scheme in anyway,
so the documentation update is required...


thanks,

Takashi

> 
> > I have no big objection to add a new ioctl if other people agree,
> > though.
> > 
> > 
> > thanks,
> > 
> > Takashi
> 
> -- 
> ~Vinod
> 

Re: [RFC PATCH v2 0/6] ALSA: compress: add support to change codec profile in gapless playback

[Takashi Iwai]

On Thu, 23 Jul 2020 15:05:22 +0200,
Vinod Koul wrote:
> 
> On 23-07-20, 14:38, Takashi Iwai wrote:
> > On Tue, 21 Jul 2020 19:00:01 +0200,
> > 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
> > > 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 patchset adds new SNDRV_COMPRESS_SET_CODEC_PARAMS IOCTL along with
> > > flags in capablity structure to allow userspace to set this new
> > > parameters required which switching codec profile, either for gapless
> > > or cross fade usecase.
> > 
> > One idea that came up at the previous audio conference regarding this
> > implementation was to just allow SET_PARAMS during the stream is
> > running (only if the driver sets the capability) instead of
> > introducing yet a new ioctl and an ops.
> > Would it make sense?
> 
> That does sound good but only issue would be that we need to somehow
> mark/document that buffer info is useless and would be discarded, how do
> we do that?

Yes, the buffer and no_wake_mode can be ignored in the gapless
re-setup.  Is your concern only about the documentation?  Or something
else needs to be changed significantly?  It's a new scheme in anyway,
so the documentation update is required...


thanks,

Takashi

> 
> > I have no big objection to add a new ioctl if other people agree,
> > though.
> > 
> > 
> > thanks,
> > 
> > Takashi
> 
> -- 
> ~Vinod
> 

Re: [RFC PATCH v2 0/6] ALSA: compress: add support to change codec profile in gapless playback

[Vinod Koul]

On 23-07-20, 15:17, Takashi Iwai wrote:
> On Thu, 23 Jul 2020 15:05:22 +0200,
> Vinod Koul wrote:
> > 
> > On 23-07-20, 14:38, Takashi Iwai wrote:
> > > On Tue, 21 Jul 2020 19:00:01 +0200,
> > > 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
> > > > 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 patchset adds new SNDRV_COMPRESS_SET_CODEC_PARAMS IOCTL along with
> > > > flags in capablity structure to allow userspace to set this new
> > > > parameters required which switching codec profile, either for gapless
> > > > or cross fade usecase.
> > > 
> > > One idea that came up at the previous audio conference regarding this
> > > implementation was to just allow SET_PARAMS during the stream is
> > > running (only if the driver sets the capability) instead of
> > > introducing yet a new ioctl and an ops.
> > > Would it make sense?
> > 
> > That does sound good but only issue would be that we need to somehow
> > mark/document that buffer info is useless and would be discarded, how do
> > we do that?
> 
> Yes, the buffer and no_wake_mode can be ignored in the gapless
> re-setup.  Is your concern only about the documentation?  Or something
> else needs to be changed significantly?  It's a new scheme in anyway,
> so the documentation update is required...

My concern is potential abuse of API down the road, if you feel it is
okay, I am okay with the proposal

Thanks
-- 
~Vinod

Re: [RFC PATCH v2 0/6] ALSA: compress: add support to change codec profile in gapless playback

[Vinod Koul]

On 23-07-20, 15:17, Takashi Iwai wrote:
> On Thu, 23 Jul 2020 15:05:22 +0200,
> Vinod Koul wrote:
> > 
> > On 23-07-20, 14:38, Takashi Iwai wrote:
> > > On Tue, 21 Jul 2020 19:00:01 +0200,
> > > 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
> > > > 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 patchset adds new SNDRV_COMPRESS_SET_CODEC_PARAMS IOCTL along with
> > > > flags in capablity structure to allow userspace to set this new
> > > > parameters required which switching codec profile, either for gapless
> > > > or cross fade usecase.
> > > 
> > > One idea that came up at the previous audio conference regarding this
> > > implementation was to just allow SET_PARAMS during the stream is
> > > running (only if the driver sets the capability) instead of
> > > introducing yet a new ioctl and an ops.
> > > Would it make sense?
> > 
> > That does sound good but only issue would be that we need to somehow
> > mark/document that buffer info is useless and would be discarded, how do
> > we do that?
> 
> Yes, the buffer and no_wake_mode can be ignored in the gapless
> re-setup.  Is your concern only about the documentation?  Or something
> else needs to be changed significantly?  It's a new scheme in anyway,
> so the documentation update is required...

My concern is potential abuse of API down the road, if you feel it is
okay, I am okay with the proposal

Thanks
-- 
~Vinod

Re: [RFC PATCH v2 0/6] ALSA: compress: add support to change codec profile in gapless playback

[Takashi Iwai]

On Thu, 23 Jul 2020 17:56:12 +0200,
Vinod Koul wrote:
> 
> On 23-07-20, 15:17, Takashi Iwai wrote:
> > On Thu, 23 Jul 2020 15:05:22 +0200,
> > Vinod Koul wrote:
> > > 
> > > On 23-07-20, 14:38, Takashi Iwai wrote:
> > > > On Tue, 21 Jul 2020 19:00:01 +0200,
> > > > 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
> > > > > 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 patchset adds new SNDRV_COMPRESS_SET_CODEC_PARAMS IOCTL along with
> > > > > flags in capablity structure to allow userspace to set this new
> > > > > parameters required which switching codec profile, either for gapless
> > > > > or cross fade usecase.
> > > > 
> > > > One idea that came up at the previous audio conference regarding this
> > > > implementation was to just allow SET_PARAMS during the stream is
> > > > running (only if the driver sets the capability) instead of
> > > > introducing yet a new ioctl and an ops.
> > > > Would it make sense?
> > > 
> > > That does sound good but only issue would be that we need to somehow
> > > mark/document that buffer info is useless and would be discarded, how do
> > > we do that?
> > 
> > Yes, the buffer and no_wake_mode can be ignored in the gapless
> > re-setup.  Is your concern only about the documentation?  Or something
> > else needs to be changed significantly?  It's a new scheme in anyway,
> > so the documentation update is required...
> 
> My concern is potential abuse of API down the road, if you feel it is
> okay, I am okay with the proposal

If this can be potentially dangerous, it shouldn't be used, of course.
What kind of scenario could it be?


thanks,

Takashi

Re: [RFC PATCH v2 0/6] ALSA: compress: add support to change codec profile in gapless playback

[Takashi Iwai]

On Thu, 23 Jul 2020 17:56:12 +0200,
Vinod Koul wrote:
> 
> On 23-07-20, 15:17, Takashi Iwai wrote:
> > On Thu, 23 Jul 2020 15:05:22 +0200,
> > Vinod Koul wrote:
> > > 
> > > On 23-07-20, 14:38, Takashi Iwai wrote:
> > > > On Tue, 21 Jul 2020 19:00:01 +0200,
> > > > 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
> > > > > 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 patchset adds new SNDRV_COMPRESS_SET_CODEC_PARAMS IOCTL along with
> > > > > flags in capablity structure to allow userspace to set this new
> > > > > parameters required which switching codec profile, either for gapless
> > > > > or cross fade usecase.
> > > > 
> > > > One idea that came up at the previous audio conference regarding this
> > > > implementation was to just allow SET_PARAMS during the stream is
> > > > running (only if the driver sets the capability) instead of
> > > > introducing yet a new ioctl and an ops.
> > > > Would it make sense?
> > > 
> > > That does sound good but only issue would be that we need to somehow
> > > mark/document that buffer info is useless and would be discarded, how do
> > > we do that?
> > 
> > Yes, the buffer and no_wake_mode can be ignored in the gapless
> > re-setup.  Is your concern only about the documentation?  Or something
> > else needs to be changed significantly?  It's a new scheme in anyway,
> > so the documentation update is required...
> 
> My concern is potential abuse of API down the road, if you feel it is
> okay, I am okay with the proposal

If this can be potentially dangerous, it shouldn't be used, of course.
What kind of scenario could it be?


thanks,

Takashi

Re: [RFC PATCH v2 0/6] ALSA: compress: add support to change codec profile in gapless playback

[Vinod Koul]

On 23-07-20, 22:33, Takashi Iwai wrote:
> On Thu, 23 Jul 2020 17:56:12 +0200,
> Vinod Koul wrote:
> > 
> > On 23-07-20, 15:17, Takashi Iwai wrote:
> > > On Thu, 23 Jul 2020 15:05:22 +0200,
> > > Vinod Koul wrote:
> > > > 
> > > > On 23-07-20, 14:38, Takashi Iwai wrote:
> > > > > On Tue, 21 Jul 2020 19:00:01 +0200,
> > > > > 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
> > > > > > 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 patchset adds new SNDRV_COMPRESS_SET_CODEC_PARAMS IOCTL along with
> > > > > > flags in capablity structure to allow userspace to set this new
> > > > > > parameters required which switching codec profile, either for gapless
> > > > > > or cross fade usecase.
> > > > > 
> > > > > One idea that came up at the previous audio conference regarding this
> > > > > implementation was to just allow SET_PARAMS during the stream is
> > > > > running (only if the driver sets the capability) instead of
> > > > > introducing yet a new ioctl and an ops.
> > > > > Would it make sense?
> > > > 
> > > > That does sound good but only issue would be that we need to somehow
> > > > mark/document that buffer info is useless and would be discarded, how do
> > > > we do that?
> > > 
> > > Yes, the buffer and no_wake_mode can be ignored in the gapless
> > > re-setup.  Is your concern only about the documentation?  Or something
> > > else needs to be changed significantly?  It's a new scheme in anyway,
> > > so the documentation update is required...
> > 
> > My concern is potential abuse of API down the road, if you feel it is
> > okay, I am okay with the proposal
> 
> If this can be potentially dangerous, it shouldn't be used, of course.
> What kind of scenario could it be?

I can think of users trying to invoke this incorrectly, right now we
would reject this.

Maybe, we can add checks like, if next_track is set and then copy the
codec params only to prevent any misuse.

Do you think that would be okay?

-- 
~Vinod

Re: [RFC PATCH v2 0/6] ALSA: compress: add support to change codec profile in gapless playback

[Vinod Koul]

On 23-07-20, 22:33, Takashi Iwai wrote:
> On Thu, 23 Jul 2020 17:56:12 +0200,
> Vinod Koul wrote:
> > 
> > On 23-07-20, 15:17, Takashi Iwai wrote:
> > > On Thu, 23 Jul 2020 15:05:22 +0200,
> > > Vinod Koul wrote:
> > > > 
> > > > On 23-07-20, 14:38, Takashi Iwai wrote:
> > > > > On Tue, 21 Jul 2020 19:00:01 +0200,
> > > > > 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
> > > > > > 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 patchset adds new SNDRV_COMPRESS_SET_CODEC_PARAMS IOCTL along with
> > > > > > flags in capablity structure to allow userspace to set this new
> > > > > > parameters required which switching codec profile, either for gapless
> > > > > > or cross fade usecase.
> > > > > 
> > > > > One idea that came up at the previous audio conference regarding this
> > > > > implementation was to just allow SET_PARAMS during the stream is
> > > > > running (only if the driver sets the capability) instead of
> > > > > introducing yet a new ioctl and an ops.
> > > > > Would it make sense?
> > > > 
> > > > That does sound good but only issue would be that we need to somehow
> > > > mark/document that buffer info is useless and would be discarded, how do
> > > > we do that?
> > > 
> > > Yes, the buffer and no_wake_mode can be ignored in the gapless
> > > re-setup.  Is your concern only about the documentation?  Or something
> > > else needs to be changed significantly?  It's a new scheme in anyway,
> > > so the documentation update is required...
> > 
> > My concern is potential abuse of API down the road, if you feel it is
> > okay, I am okay with the proposal
> 
> If this can be potentially dangerous, it shouldn't be used, of course.
> What kind of scenario could it be?

I can think of users trying to invoke this incorrectly, right now we
would reject this.

Maybe, we can add checks like, if next_track is set and then copy the
codec params only to prevent any misuse.

Do you think that would be okay?

-- 
~Vinod

Re: [RFC PATCH v2 0/6] ALSA: compress: add support to change codec profile in gapless playback

[Takashi Iwai]

On Thu, 06 Aug 2020 13:08:13 +0200,
Vinod Koul wrote:
> 
> On 23-07-20, 22:33, Takashi Iwai wrote:
> > On Thu, 23 Jul 2020 17:56:12 +0200,
> > Vinod Koul wrote:
> > > 
> > > On 23-07-20, 15:17, Takashi Iwai wrote:
> > > > On Thu, 23 Jul 2020 15:05:22 +0200,
> > > > Vinod Koul wrote:
> > > > > 
> > > > > On 23-07-20, 14:38, Takashi Iwai wrote:
> > > > > > On Tue, 21 Jul 2020 19:00:01 +0200,
> > > > > > 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
> > > > > > > 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 patchset adds new SNDRV_COMPRESS_SET_CODEC_PARAMS IOCTL along with
> > > > > > > flags in capablity structure to allow userspace to set this new
> > > > > > > parameters required which switching codec profile, either for gapless
> > > > > > > or cross fade usecase.
> > > > > > 
> > > > > > One idea that came up at the previous audio conference regarding this
> > > > > > implementation was to just allow SET_PARAMS during the stream is
> > > > > > running (only if the driver sets the capability) instead of
> > > > > > introducing yet a new ioctl and an ops.
> > > > > > Would it make sense?
> > > > > 
> > > > > That does sound good but only issue would be that we need to somehow
> > > > > mark/document that buffer info is useless and would be discarded, how do
> > > > > we do that?
> > > > 
> > > > Yes, the buffer and no_wake_mode can be ignored in the gapless
> > > > re-setup.  Is your concern only about the documentation?  Or something
> > > > else needs to be changed significantly?  It's a new scheme in anyway,
> > > > so the documentation update is required...
> > > 
> > > My concern is potential abuse of API down the road, if you feel it is
> > > okay, I am okay with the proposal
> > 
> > If this can be potentially dangerous, it shouldn't be used, of course.
> > What kind of scenario could it be?
> 
> I can think of users trying to invoke this incorrectly, right now we
> would reject this.
> 
> Maybe, we can add checks like, if next_track is set and then copy the
> codec params only to prevent any misuse.
> 
> Do you think that would be okay?

Yes, it's the same condition check that was proposed for the new
ioctl.


Takashi

Re: [RFC PATCH v2 0/6] ALSA: compress: add support to change codec profile in gapless playback

[Takashi Iwai]

On Thu, 06 Aug 2020 13:08:13 +0200,
Vinod Koul wrote:
> 
> On 23-07-20, 22:33, Takashi Iwai wrote:
> > On Thu, 23 Jul 2020 17:56:12 +0200,
> > Vinod Koul wrote:
> > > 
> > > On 23-07-20, 15:17, Takashi Iwai wrote:
> > > > On Thu, 23 Jul 2020 15:05:22 +0200,
> > > > Vinod Koul wrote:
> > > > > 
> > > > > On 23-07-20, 14:38, Takashi Iwai wrote:
> > > > > > On Tue, 21 Jul 2020 19:00:01 +0200,
> > > > > > 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
> > > > > > > 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 patchset adds new SNDRV_COMPRESS_SET_CODEC_PARAMS IOCTL along with
> > > > > > > flags in capablity structure to allow userspace to set this new
> > > > > > > parameters required which switching codec profile, either for gapless
> > > > > > > or cross fade usecase.
> > > > > > 
> > > > > > One idea that came up at the previous audio conference regarding this
> > > > > > implementation was to just allow SET_PARAMS during the stream is
> > > > > > running (only if the driver sets the capability) instead of
> > > > > > introducing yet a new ioctl and an ops.
> > > > > > Would it make sense?
> > > > > 
> > > > > That does sound good but only issue would be that we need to somehow
> > > > > mark/document that buffer info is useless and would be discarded, how do
> > > > > we do that?
> > > > 
> > > > Yes, the buffer and no_wake_mode can be ignored in the gapless
> > > > re-setup.  Is your concern only about the documentation?  Or something
> > > > else needs to be changed significantly?  It's a new scheme in anyway,
> > > > so the documentation update is required...
> > > 
> > > My concern is potential abuse of API down the road, if you feel it is
> > > okay, I am okay with the proposal
> > 
> > If this can be potentially dangerous, it shouldn't be used, of course.
> > What kind of scenario could it be?
> 
> I can think of users trying to invoke this incorrectly, right now we
> would reject this.
> 
> Maybe, we can add checks like, if next_track is set and then copy the
> codec params only to prevent any misuse.
> 
> Do you think that would be okay?

Yes, it's the same condition check that was proposed for the new
ioctl.


Takashi