* [PATCH] compress: add support for gapless playback
@ 2013-02-12 18:31 Vinod Koul
2013-02-13 6:23 ` Takashi Iwai
0 siblings, 1 reply; 25+ messages in thread
From: Vinod Koul @ 2013-02-12 18:31 UTC (permalink / raw)
To: alsa-devel; +Cc: tiwai, Jeeja KP, broonie, Vinod Koul, liam.r.girdwood
From: Jeeja KP <jeeja.kp@intel.com>
this add new API for sound compress to support gapless playback.
As noted in Documentation change, we add API to send metadata of encoder and
padding delay to DSP. Also add API for indicating EOF and switching to
subsequent track
Also bump the compress API version
Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
v3:
- Change back to ioctl struct with padding
- adding states for next_track and metadata for proper transistion
v2:
- Make it a patch, not RFC
- split metadata to key/value pairs, and send multiple keys
- add get_metadata api
- split partial_drain to next_data & partial_drain
- add stream states for transistion
- update documentation
---
Documentation/sound/alsa/compress_offload.txt | 46 ++++++++++
include/sound/compress_driver.h | 8 ++
include/uapi/sound/compress_offload.h | 23 +++++-
sound/core/compress_offload.c | 116 +++++++++++++++++++++++++
4 files changed, 192 insertions(+), 1 deletions(-)
diff --git a/Documentation/sound/alsa/compress_offload.txt b/Documentation/sound/alsa/compress_offload.txt
index 90e9b3a..0bcc551 100644
--- a/Documentation/sound/alsa/compress_offload.txt
+++ b/Documentation/sound/alsa/compress_offload.txt
@@ -145,6 +145,52 @@ Modifications include:
- Addition of encoding options when required (derived from OpenMAX IL)
- Addition of rateControlSupported (missing in OpenMAX AL)
+Gapless Playback
+================
+When playing thru an album, the decoders have the ability to skip the encoder
+delay and padding and directly move from one track content to another. The end
+user can perceive this as gapless playback as we dont have silence while
+switching from one track to another
+
+Also, there might be low-intensity noises due to encoding. Perfect gapless is
+difficult to reach with all types of compressed data, but works fine with most
+music content. The decoder needs to know the encoder delay and encoder padding.
+So we need to pass this to DSP. This metadata is extracted from ID3/MP4 headers
+and are not present by default in the bitstream, hence the need for a new
+interface to pass this information to the DSP. Also DSP and userspace needs to
+switch from one track to another and start using data for second track.
+
+The main additions are:
+
+- set_metadata
+This routine sets the encoder delay and encoder padding. This can be used by
+decoder to strip the silence. This needs to be set before the data in the track
+is written.
+
+- set_next_track
+This routine tells DSP that metadata and write operation sent after this would
+correspond to subsequent track
+
+- partial drain
+This is called when end of file is reached. The userspace can inform DSP that
+EOF is reached and now DSP can start skipping padding delay. Also next write
+data would belong to next track
+
+Sequence flow for gapless would be:
+- Open
+- Get caps / codec caps
+- Set params
+- Set metadata of the first track
+- Fill data of the first track
+- Trigger start
+- User-space finished sending all,
+- Indicaite next track data by sending set_next_track
+- Set metadata of the next track
+- then call partial_drain to flush most of buffer in DSP
+- Fill data of the next track
+- DSP switches to second track
+(note: order for partial_drain and write for next track can be reversed as well)
+
Not supported:
- Support for VoIP/circuit-switched calls is not the target of this
diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
index f2912ab..ff6c741 100644
--- a/include/sound/compress_driver.h
+++ b/include/sound/compress_driver.h
@@ -71,6 +71,8 @@ struct snd_compr_runtime {
* @runtime: pointer to runtime structure
* @device: device pointer
* @direction: stream direction, playback/recording
+ * @metadata_set: metadata set flag, true when set
+ * @next_track: has userspace signall next track transistion, true when set
* @private_data: pointer to DSP private data
*/
struct snd_compr_stream {
@@ -79,6 +81,8 @@ struct snd_compr_stream {
struct snd_compr_runtime *runtime;
struct snd_compr *device;
enum snd_compr_direction direction;
+ bool metadata_set;
+ bool next_track;
void *private_data;
};
@@ -110,6 +114,10 @@ struct snd_compr_ops {
struct snd_compr_params *params);
int (*get_params)(struct snd_compr_stream *stream,
struct snd_codec *params);
+ int (*set_metadata)(struct snd_compr_stream *stream,
+ struct snd_compr_metadata *metadata);
+ int (*get_metadata)(struct snd_compr_stream *stream,
+ struct snd_compr_metadata *metadata);
int (*trigger)(struct snd_compr_stream *stream, int cmd);
int (*pointer)(struct snd_compr_stream *stream,
struct snd_compr_tstamp *tstamp);
diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h
index 05341a4..8903fe4 100644
--- a/include/uapi/sound/compress_offload.h
+++ b/include/uapi/sound/compress_offload.h
@@ -30,7 +30,7 @@
#include <sound/compress_params.h>
-#define SNDRV_COMPRESS_VERSION SNDRV_PROTOCOL_VERSION(0, 1, 0)
+#define SNDRV_COMPRESS_VERSION SNDRV_PROTOCOL_VERSION(0, 1, 1)
/**
* struct snd_compressed_buffer: compressed buffer
* @fragment_size: size of buffer fragment in bytes
@@ -122,6 +122,19 @@ struct snd_compr_codec_caps {
};
/**
+ * struct snd_compr_metadata: compressed stream metadata
+ * @encoder_delay: no of samples inserted by the encoder at the beginning
+ * of the track
+ * @encoder_padding: no of samples appended by the encoder at the end
+ * of the track
+ */
+struct snd_compr_metadata {
+ __u32 encoder_delay;
+ __u32 encoder_padding;
+ __u32 reserved[12];
+};
+
+/**
* compress path ioctl definitions
* SNDRV_COMPRESS_GET_CAPS: Query capability of DSP
* SNDRV_COMPRESS_GET_CODEC_CAPS: Query capability of a codec
@@ -145,6 +158,10 @@ struct snd_compr_codec_caps {
struct snd_compr_codec_caps)
#define SNDRV_COMPRESS_SET_PARAMS _IOW('C', 0x12, struct snd_compr_params)
#define SNDRV_COMPRESS_GET_PARAMS _IOR('C', 0x13, struct snd_codec)
+#define SNDRV_COMPRESS_SET_METADATA _IOW('C', 0x14,\
+ struct snd_compr_metadata)
+#define SNDRV_COMPRESS_GET_METADATA _IOW('C', 0x15,\
+ struct snd_compr_metadata)
#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)
@@ -152,10 +169,14 @@ struct snd_compr_codec_caps {
#define SNDRV_COMPRESS_START _IO('C', 0x32)
#define SNDRV_COMPRESS_STOP _IO('C', 0x33)
#define SNDRV_COMPRESS_DRAIN _IO('C', 0x34)
+#define SNDRV_COMPRESS_NEXT_TRACK _IO('C', 0x35)
+#define SNDRV_COMPRESS_PARTIAL_DRAIN _IO('C', 0x36)
/*
* TODO
* 1. add mmap support
*
*/
#define SND_COMPR_TRIGGER_DRAIN 7 /*FIXME move this to pcm.h */
+#define SND_COMPR_TRIGGER_NEXT_TRACK 8
+#define SND_COMPR_TRIGGER_PARTIAL_DRAIN 9
#endif
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index ad11dc9..ba2f0ad 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -514,6 +514,69 @@ out:
return retval;
}
+static int
+snd_compr_get_metadata(struct snd_compr_stream *stream, unsigned long arg)
+{
+ struct snd_compr_metadata *metadata;
+ int retval;
+
+ if (!stream->ops->get_metadata)
+ return -ENXIO;
+
+ metadata = kmalloc(sizeof(*metadata), GFP_KERNEL);
+ if (!metadata)
+ return -ENOMEM;
+ if (copy_from_user(metadata, (void __user *)arg, sizeof(*metadata))) {
+ retval = -EFAULT;
+ goto out;
+ }
+
+ retval = stream->ops->get_metadata(stream, metadata);
+ if (retval != 0)
+ goto out;
+
+ if (copy_to_user((void __user *)arg, metadata, sizeof(*metadata))) {
+ retval = -EFAULT;
+ goto out;
+ }
+
+out:
+ kfree(metadata);
+ return retval;
+}
+
+static int
+snd_compr_set_metadata(struct snd_compr_stream *stream, unsigned long arg)
+{
+ struct snd_compr_metadata *metadata;
+ int retval;
+
+ if (!stream->ops->set_metadata)
+ return -ENXIO;
+ /*
+ * we should allow parameter change only when stream has been
+ * opened not in other cases
+ */
+ metadata = kmalloc(sizeof(*metadata), GFP_KERNEL);
+ if (!metadata)
+ return -ENOMEM;
+ if (copy_from_user(metadata, (void __user *)arg,
+ sizeof(*metadata))) {
+ retval = -EFAULT;
+ goto out;
+ }
+
+ pr_debug("metadata encoder delay=%x encoder padding=%x\n",
+ metadata->encoder_delay, metadata->encoder_padding);
+
+ retval = stream->ops->set_metadata(stream, metadata);
+ stream->metadata_set = true;
+
+out:
+ kfree(metadata);
+ return retval;
+}
+
static inline int
snd_compr_tstamp(struct snd_compr_stream *stream, unsigned long arg)
{
@@ -594,6 +657,46 @@ static int snd_compr_drain(struct snd_compr_stream *stream)
return retval;
}
+static int snd_compr_next_track(struct snd_compr_stream *stream)
+{
+ int retval;
+
+ /* only a running stream can transition to next track */
+ if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING)
+ return -EPERM;
+
+ /* you can signal next track isf this is intended to be a gapless stream
+ * and current track metadata is set
+ */
+ if (stream->metadata_set == false)
+ return -EPERM;
+
+ retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_NEXT_TRACK);
+ if (retval != 0)
+ return retval;
+ stream->metadata_set = false;
+ stream->next_track = true;
+ return 0;
+}
+
+static int snd_compr_partial_drain(struct snd_compr_stream *stream)
+{
+ int retval;
+ if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
+ stream->runtime->state == SNDRV_PCM_STATE_SETUP)
+ return -EPERM;
+ /* stream can be drained only when next track has been signalled */
+ if (stream->next_track == false)
+ return -EPERM;
+
+ retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_PARTIAL_DRAIN);
+ if (retval)
+ pr_err("Partial drain returned failure\n");
+
+ stream->next_track = false;
+ return retval;
+}
+
static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
{
struct snd_compr_file *data = f->private_data;
@@ -623,6 +726,12 @@ 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_METADATA):
+ retval = snd_compr_set_metadata(stream, arg);
+ break;
+ case _IOC_NR(SNDRV_COMPRESS_GET_METADATA):
+ retval = snd_compr_get_metadata(stream, arg);
+ break;
case _IOC_NR(SNDRV_COMPRESS_TSTAMP):
retval = snd_compr_tstamp(stream, arg);
break;
@@ -644,6 +753,13 @@ static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
case _IOC_NR(SNDRV_COMPRESS_DRAIN):
retval = snd_compr_drain(stream);
break;
+ case _IOC_NR(SNDRV_COMPRESS_PARTIAL_DRAIN):
+ retval = snd_compr_partial_drain(stream);
+ break;
+ case _IOC_NR(SNDRV_COMPRESS_NEXT_TRACK):
+ retval = snd_compr_next_track(stream);
+ break;
+
}
mutex_unlock(&stream->device->lock);
return retval;
--
1.7.0.4
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH] compress: add support for gapless playback
2013-02-12 18:31 [PATCH] compress: add support for gapless playback Vinod Koul
@ 2013-02-13 6:23 ` Takashi Iwai
2013-02-13 6:35 ` Vinod Koul
0 siblings, 1 reply; 25+ messages in thread
From: Takashi Iwai @ 2013-02-13 6:23 UTC (permalink / raw)
To: Vinod Koul; +Cc: Jeeja KP, alsa-devel, broonie, liam.r.girdwood
At Tue, 12 Feb 2013 10:31:55 -0800,
Vinod Koul wrote:
>
> From: Jeeja KP <jeeja.kp@intel.com>
>
> this add new API for sound compress to support gapless playback.
> As noted in Documentation change, we add API to send metadata of encoder and
> padding delay to DSP. Also add API for indicating EOF and switching to
> subsequent track
>
> Also bump the compress API version
>
> Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> ---
> v3:
> - Change back to ioctl struct with padding
But this makes difficult to know which parameter is unavailable in
this kernel, as already mentioned in the thread. You need to add
another ioctl or add a mask in the struct. Or, take back to single
key/value pairs, so user can see the -ENOENT or such error for
non-existing parameters.
Takashi
> - adding states for next_track and metadata for proper transistion
>
> v2:
> - Make it a patch, not RFC
> - split metadata to key/value pairs, and send multiple keys
> - add get_metadata api
> - split partial_drain to next_data & partial_drain
> - add stream states for transistion
> - update documentation
> ---
> Documentation/sound/alsa/compress_offload.txt | 46 ++++++++++
> include/sound/compress_driver.h | 8 ++
> include/uapi/sound/compress_offload.h | 23 +++++-
> sound/core/compress_offload.c | 116 +++++++++++++++++++++++++
> 4 files changed, 192 insertions(+), 1 deletions(-)
>
> diff --git a/Documentation/sound/alsa/compress_offload.txt b/Documentation/sound/alsa/compress_offload.txt
> index 90e9b3a..0bcc551 100644
> --- a/Documentation/sound/alsa/compress_offload.txt
> +++ b/Documentation/sound/alsa/compress_offload.txt
> @@ -145,6 +145,52 @@ Modifications include:
> - Addition of encoding options when required (derived from OpenMAX IL)
> - Addition of rateControlSupported (missing in OpenMAX AL)
>
> +Gapless Playback
> +================
> +When playing thru an album, the decoders have the ability to skip the encoder
> +delay and padding and directly move from one track content to another. The end
> +user can perceive this as gapless playback as we dont have silence while
> +switching from one track to another
> +
> +Also, there might be low-intensity noises due to encoding. Perfect gapless is
> +difficult to reach with all types of compressed data, but works fine with most
> +music content. The decoder needs to know the encoder delay and encoder padding.
> +So we need to pass this to DSP. This metadata is extracted from ID3/MP4 headers
> +and are not present by default in the bitstream, hence the need for a new
> +interface to pass this information to the DSP. Also DSP and userspace needs to
> +switch from one track to another and start using data for second track.
> +
> +The main additions are:
> +
> +- set_metadata
> +This routine sets the encoder delay and encoder padding. This can be used by
> +decoder to strip the silence. This needs to be set before the data in the track
> +is written.
> +
> +- set_next_track
> +This routine tells DSP that metadata and write operation sent after this would
> +correspond to subsequent track
> +
> +- partial drain
> +This is called when end of file is reached. The userspace can inform DSP that
> +EOF is reached and now DSP can start skipping padding delay. Also next write
> +data would belong to next track
> +
> +Sequence flow for gapless would be:
> +- Open
> +- Get caps / codec caps
> +- Set params
> +- Set metadata of the first track
> +- Fill data of the first track
> +- Trigger start
> +- User-space finished sending all,
> +- Indicaite next track data by sending set_next_track
> +- Set metadata of the next track
> +- then call partial_drain to flush most of buffer in DSP
> +- Fill data of the next track
> +- DSP switches to second track
> +(note: order for partial_drain and write for next track can be reversed as well)
> +
> Not supported:
>
> - Support for VoIP/circuit-switched calls is not the target of this
> diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
> index f2912ab..ff6c741 100644
> --- a/include/sound/compress_driver.h
> +++ b/include/sound/compress_driver.h
> @@ -71,6 +71,8 @@ struct snd_compr_runtime {
> * @runtime: pointer to runtime structure
> * @device: device pointer
> * @direction: stream direction, playback/recording
> + * @metadata_set: metadata set flag, true when set
> + * @next_track: has userspace signall next track transistion, true when set
> * @private_data: pointer to DSP private data
> */
> struct snd_compr_stream {
> @@ -79,6 +81,8 @@ struct snd_compr_stream {
> struct snd_compr_runtime *runtime;
> struct snd_compr *device;
> enum snd_compr_direction direction;
> + bool metadata_set;
> + bool next_track;
> void *private_data;
> };
>
> @@ -110,6 +114,10 @@ struct snd_compr_ops {
> struct snd_compr_params *params);
> int (*get_params)(struct snd_compr_stream *stream,
> struct snd_codec *params);
> + int (*set_metadata)(struct snd_compr_stream *stream,
> + struct snd_compr_metadata *metadata);
> + int (*get_metadata)(struct snd_compr_stream *stream,
> + struct snd_compr_metadata *metadata);
> int (*trigger)(struct snd_compr_stream *stream, int cmd);
> int (*pointer)(struct snd_compr_stream *stream,
> struct snd_compr_tstamp *tstamp);
> diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h
> index 05341a4..8903fe4 100644
> --- a/include/uapi/sound/compress_offload.h
> +++ b/include/uapi/sound/compress_offload.h
> @@ -30,7 +30,7 @@
> #include <sound/compress_params.h>
>
>
> -#define SNDRV_COMPRESS_VERSION SNDRV_PROTOCOL_VERSION(0, 1, 0)
> +#define SNDRV_COMPRESS_VERSION SNDRV_PROTOCOL_VERSION(0, 1, 1)
> /**
> * struct snd_compressed_buffer: compressed buffer
> * @fragment_size: size of buffer fragment in bytes
> @@ -122,6 +122,19 @@ struct snd_compr_codec_caps {
> };
>
> /**
> + * struct snd_compr_metadata: compressed stream metadata
> + * @encoder_delay: no of samples inserted by the encoder at the beginning
> + * of the track
> + * @encoder_padding: no of samples appended by the encoder at the end
> + * of the track
> + */
> +struct snd_compr_metadata {
> + __u32 encoder_delay;
> + __u32 encoder_padding;
> + __u32 reserved[12];
> +};
> +
> +/**
> * compress path ioctl definitions
> * SNDRV_COMPRESS_GET_CAPS: Query capability of DSP
> * SNDRV_COMPRESS_GET_CODEC_CAPS: Query capability of a codec
> @@ -145,6 +158,10 @@ struct snd_compr_codec_caps {
> struct snd_compr_codec_caps)
> #define SNDRV_COMPRESS_SET_PARAMS _IOW('C', 0x12, struct snd_compr_params)
> #define SNDRV_COMPRESS_GET_PARAMS _IOR('C', 0x13, struct snd_codec)
> +#define SNDRV_COMPRESS_SET_METADATA _IOW('C', 0x14,\
> + struct snd_compr_metadata)
> +#define SNDRV_COMPRESS_GET_METADATA _IOW('C', 0x15,\
> + struct snd_compr_metadata)
> #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)
> @@ -152,10 +169,14 @@ struct snd_compr_codec_caps {
> #define SNDRV_COMPRESS_START _IO('C', 0x32)
> #define SNDRV_COMPRESS_STOP _IO('C', 0x33)
> #define SNDRV_COMPRESS_DRAIN _IO('C', 0x34)
> +#define SNDRV_COMPRESS_NEXT_TRACK _IO('C', 0x35)
> +#define SNDRV_COMPRESS_PARTIAL_DRAIN _IO('C', 0x36)
> /*
> * TODO
> * 1. add mmap support
> *
> */
> #define SND_COMPR_TRIGGER_DRAIN 7 /*FIXME move this to pcm.h */
> +#define SND_COMPR_TRIGGER_NEXT_TRACK 8
> +#define SND_COMPR_TRIGGER_PARTIAL_DRAIN 9
> #endif
> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> index ad11dc9..ba2f0ad 100644
> --- a/sound/core/compress_offload.c
> +++ b/sound/core/compress_offload.c
> @@ -514,6 +514,69 @@ out:
> return retval;
> }
>
> +static int
> +snd_compr_get_metadata(struct snd_compr_stream *stream, unsigned long arg)
> +{
> + struct snd_compr_metadata *metadata;
> + int retval;
> +
> + if (!stream->ops->get_metadata)
> + return -ENXIO;
> +
> + metadata = kmalloc(sizeof(*metadata), GFP_KERNEL);
> + if (!metadata)
> + return -ENOMEM;
> + if (copy_from_user(metadata, (void __user *)arg, sizeof(*metadata))) {
> + retval = -EFAULT;
> + goto out;
> + }
> +
> + retval = stream->ops->get_metadata(stream, metadata);
> + if (retval != 0)
> + goto out;
> +
> + if (copy_to_user((void __user *)arg, metadata, sizeof(*metadata))) {
> + retval = -EFAULT;
> + goto out;
> + }
> +
> +out:
> + kfree(metadata);
> + return retval;
> +}
> +
> +static int
> +snd_compr_set_metadata(struct snd_compr_stream *stream, unsigned long arg)
> +{
> + struct snd_compr_metadata *metadata;
> + int retval;
> +
> + if (!stream->ops->set_metadata)
> + return -ENXIO;
> + /*
> + * we should allow parameter change only when stream has been
> + * opened not in other cases
> + */
> + metadata = kmalloc(sizeof(*metadata), GFP_KERNEL);
> + if (!metadata)
> + return -ENOMEM;
> + if (copy_from_user(metadata, (void __user *)arg,
> + sizeof(*metadata))) {
> + retval = -EFAULT;
> + goto out;
> + }
> +
> + pr_debug("metadata encoder delay=%x encoder padding=%x\n",
> + metadata->encoder_delay, metadata->encoder_padding);
> +
> + retval = stream->ops->set_metadata(stream, metadata);
> + stream->metadata_set = true;
> +
> +out:
> + kfree(metadata);
> + return retval;
> +}
> +
> static inline int
> snd_compr_tstamp(struct snd_compr_stream *stream, unsigned long arg)
> {
> @@ -594,6 +657,46 @@ static int snd_compr_drain(struct snd_compr_stream *stream)
> return retval;
> }
>
> +static int snd_compr_next_track(struct snd_compr_stream *stream)
> +{
> + int retval;
> +
> + /* only a running stream can transition to next track */
> + if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING)
> + return -EPERM;
> +
> + /* you can signal next track isf this is intended to be a gapless stream
> + * and current track metadata is set
> + */
> + if (stream->metadata_set == false)
> + return -EPERM;
> +
> + retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_NEXT_TRACK);
> + if (retval != 0)
> + return retval;
> + stream->metadata_set = false;
> + stream->next_track = true;
> + return 0;
> +}
> +
> +static int snd_compr_partial_drain(struct snd_compr_stream *stream)
> +{
> + int retval;
> + if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
> + stream->runtime->state == SNDRV_PCM_STATE_SETUP)
> + return -EPERM;
> + /* stream can be drained only when next track has been signalled */
> + if (stream->next_track == false)
> + return -EPERM;
> +
> + retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_PARTIAL_DRAIN);
> + if (retval)
> + pr_err("Partial drain returned failure\n");
> +
> + stream->next_track = false;
> + return retval;
> +}
> +
> static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
> {
> struct snd_compr_file *data = f->private_data;
> @@ -623,6 +726,12 @@ 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_METADATA):
> + retval = snd_compr_set_metadata(stream, arg);
> + break;
> + case _IOC_NR(SNDRV_COMPRESS_GET_METADATA):
> + retval = snd_compr_get_metadata(stream, arg);
> + break;
> case _IOC_NR(SNDRV_COMPRESS_TSTAMP):
> retval = snd_compr_tstamp(stream, arg);
> break;
> @@ -644,6 +753,13 @@ static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
> case _IOC_NR(SNDRV_COMPRESS_DRAIN):
> retval = snd_compr_drain(stream);
> break;
> + case _IOC_NR(SNDRV_COMPRESS_PARTIAL_DRAIN):
> + retval = snd_compr_partial_drain(stream);
> + break;
> + case _IOC_NR(SNDRV_COMPRESS_NEXT_TRACK):
> + retval = snd_compr_next_track(stream);
> + break;
> +
> }
> mutex_unlock(&stream->device->lock);
> return retval;
> --
> 1.7.0.4
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] compress: add support for gapless playback
2013-02-13 6:23 ` Takashi Iwai
@ 2013-02-13 6:35 ` Vinod Koul
2013-02-13 7:37 ` Takashi Iwai
0 siblings, 1 reply; 25+ messages in thread
From: Vinod Koul @ 2013-02-13 6:35 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Jeeja KP, alsa-devel, broonie, liam.r.girdwood
On Wed, Feb 13, 2013 at 07:23:32AM +0100, Takashi Iwai wrote:
> At Tue, 12 Feb 2013 10:31:55 -0800,
> Vinod Koul wrote:
> >
> > From: Jeeja KP <jeeja.kp@intel.com>
> >
> > this add new API for sound compress to support gapless playback.
> > As noted in Documentation change, we add API to send metadata of encoder and
> > padding delay to DSP. Also add API for indicating EOF and switching to
> > subsequent track
> >
> > Also bump the compress API version
> >
> > Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
> > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > ---
> > v3:
> > - Change back to ioctl struct with padding
>
> But this makes difficult to know which parameter is unavailable in
> this kernel, as already mentioned in the thread. You need to add
> another ioctl or add a mask in the struct. Or, take back to single
> key/value pairs, so user can see the -ENOENT or such error for
> non-existing parameters.
Right, but we have the API version. Wouldnt it be okay to use API version to
find what kernel supports, of course it doesnt scale well as many more params
are added.
--
~Vinod
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] compress: add support for gapless playback
2013-02-13 6:35 ` Vinod Koul
@ 2013-02-13 7:37 ` Takashi Iwai
2013-02-13 11:34 ` Mark Brown
2013-02-13 13:39 ` Vinod Koul
0 siblings, 2 replies; 25+ messages in thread
From: Takashi Iwai @ 2013-02-13 7:37 UTC (permalink / raw)
To: Vinod Koul; +Cc: Jeeja KP, alsa-devel, broonie, liam.r.girdwood
At Tue, 12 Feb 2013 22:35:47 -0800,
Vinod Koul wrote:
>
> On Wed, Feb 13, 2013 at 07:23:32AM +0100, Takashi Iwai wrote:
> > At Tue, 12 Feb 2013 10:31:55 -0800,
> > Vinod Koul wrote:
> > >
> > > From: Jeeja KP <jeeja.kp@intel.com>
> > >
> > > this add new API for sound compress to support gapless playback.
> > > As noted in Documentation change, we add API to send metadata of encoder and
> > > padding delay to DSP. Also add API for indicating EOF and switching to
> > > subsequent track
> > >
> > > Also bump the compress API version
> > >
> > > Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
> > > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > > ---
> > > v3:
> > > - Change back to ioctl struct with padding
> >
> > But this makes difficult to know which parameter is unavailable in
> > this kernel, as already mentioned in the thread. You need to add
> > another ioctl or add a mask in the struct. Or, take back to single
> > key/value pairs, so user can see the -ENOENT or such error for
> > non-existing parameters.
> Right, but we have the API version.
But what if a driver doesn't support a particular metadata?
Or would you mandate for every driver to support all metadata?
Takashi
> Wouldnt it be okay to use API version to
> find what kernel supports, of course it doesnt scale well as many more params
> are added.
>
> --
> ~Vinod
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] compress: add support for gapless playback
2013-02-13 7:37 ` Takashi Iwai
@ 2013-02-13 11:34 ` Mark Brown
2013-02-13 13:39 ` Vinod Koul
1 sibling, 0 replies; 25+ messages in thread
From: Mark Brown @ 2013-02-13 11:34 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Vinod Koul, Jeeja KP, alsa-devel, liam.r.girdwood
[-- Attachment #1.1: Type: text/plain, Size: 797 bytes --]
On Wed, Feb 13, 2013 at 08:37:15AM +0100, Takashi Iwai wrote:
> Vinod Koul wrote:
> > On Wed, Feb 13, 2013 at 07:23:32AM +0100, Takashi Iwai wrote:
> > > But this makes difficult to know which parameter is unavailable in
> > > this kernel, as already mentioned in the thread. You need to add
> > > another ioctl or add a mask in the struct. Or, take back to single
> > > key/value pairs, so user can see the -ENOENT or such error for
> > > non-existing parameters.
> > Right, but we have the API version.
> But what if a driver doesn't support a particular metadata?
> Or would you mandate for every driver to support all metadata?
I tend to agree with Takashi here, it seems better if we can have
drivers able to implement only a subset of features especially if the
API does get extended.
[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] compress: add support for gapless playback
2013-02-13 7:37 ` Takashi Iwai
2013-02-13 11:34 ` Mark Brown
@ 2013-02-13 13:39 ` Vinod Koul
1 sibling, 0 replies; 25+ messages in thread
From: Vinod Koul @ 2013-02-13 13:39 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Jeeja KP, alsa-devel, broonie, liam.r.girdwood
On Wed, Feb 13, 2013 at 08:37:15AM +0100, Takashi Iwai wrote:
> At Tue, 12 Feb 2013 22:35:47 -0800,
> Vinod Koul wrote:
> >
> > On Wed, Feb 13, 2013 at 07:23:32AM +0100, Takashi Iwai wrote:
> > > But this makes difficult to know which parameter is unavailable in
> > > this kernel, as already mentioned in the thread. You need to add
> > > another ioctl or add a mask in the struct. Or, take back to single
> > > key/value pairs, so user can see the -ENOENT or such error for
> > > non-existing parameters.
> > Right, but we have the API version.
>
> But what if a driver doesn't support a particular metadata?
> Or would you mandate for every driver to support all metadata?
yes you have point. We cant expect everyone to support all metadate.
So I think going back to simple single key/value is easy.
Driver cna implement what it supports and return error for ones it doesn't. This
becomes scalable as well.
But we can't keep them in core as we dont know which ones will change...
Expect updated patch soon :)
--
~Vinod
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH] compress: add support for gapless playback
@ 2013-02-14 11:22 Vinod Koul
2013-02-14 11:31 ` Takashi Iwai
0 siblings, 1 reply; 25+ messages in thread
From: Vinod Koul @ 2013-02-14 11:22 UTC (permalink / raw)
To: alsa-devel; +Cc: tiwai, Jeeja KP, broonie, Vinod Koul, liam.r.girdwood
From: Jeeja KP <jeeja.kp@intel.com>
this add new API for sound compress to support gapless playback.
As noted in Documentation change, we add API to send metadata of encoder and
padding delay to DSP. Also add API for indicating EOF and switching to
subsequent track
Also bump the compress API version
Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
----
v6:
- update ioctls GET_METADATA to IOWR
- add documenetation of enum
v5:
- update metadata value with 8 words
- add reset of new states in set_params
- use statically allocated structs
v4:
- update metadata struct with single key/value
v3:
- Change back to ioctl struct with padding
- adding states for next_track and metadata for proper transistion
v2:
- Make it a patch, not RFC
- split metadata to key/value pairs, and send multiple keys
- add get_metadata api
- split partial_drain to next_data & partial_drain
- add stream states for transistion
- update documentation
-----
Documentation/sound/alsa/compress_offload.txt | 46 ++++++++++++
include/sound/compress_driver.h | 8 ++
include/uapi/sound/compress_offload.h | 31 ++++++++-
sound/core/compress_offload.c | 96 +++++++++++++++++++++++++
4 files changed, 180 insertions(+), 1 deletions(-)
diff --git a/Documentation/sound/alsa/compress_offload.txt b/Documentation/sound/alsa/compress_offload.txt
index 90e9b3a..0bcc551 100644
--- a/Documentation/sound/alsa/compress_offload.txt
+++ b/Documentation/sound/alsa/compress_offload.txt
@@ -145,6 +145,52 @@ Modifications include:
- Addition of encoding options when required (derived from OpenMAX IL)
- Addition of rateControlSupported (missing in OpenMAX AL)
+Gapless Playback
+================
+When playing thru an album, the decoders have the ability to skip the encoder
+delay and padding and directly move from one track content to another. The end
+user can perceive this as gapless playback as we dont have silence while
+switching from one track to another
+
+Also, there might be low-intensity noises due to encoding. Perfect gapless is
+difficult to reach with all types of compressed data, but works fine with most
+music content. The decoder needs to know the encoder delay and encoder padding.
+So we need to pass this to DSP. This metadata is extracted from ID3/MP4 headers
+and are not present by default in the bitstream, hence the need for a new
+interface to pass this information to the DSP. Also DSP and userspace needs to
+switch from one track to another and start using data for second track.
+
+The main additions are:
+
+- set_metadata
+This routine sets the encoder delay and encoder padding. This can be used by
+decoder to strip the silence. This needs to be set before the data in the track
+is written.
+
+- set_next_track
+This routine tells DSP that metadata and write operation sent after this would
+correspond to subsequent track
+
+- partial drain
+This is called when end of file is reached. The userspace can inform DSP that
+EOF is reached and now DSP can start skipping padding delay. Also next write
+data would belong to next track
+
+Sequence flow for gapless would be:
+- Open
+- Get caps / codec caps
+- Set params
+- Set metadata of the first track
+- Fill data of the first track
+- Trigger start
+- User-space finished sending all,
+- Indicaite next track data by sending set_next_track
+- Set metadata of the next track
+- then call partial_drain to flush most of buffer in DSP
+- Fill data of the next track
+- DSP switches to second track
+(note: order for partial_drain and write for next track can be reversed as well)
+
Not supported:
- Support for VoIP/circuit-switched calls is not the target of this
diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
index f2912ab..ff6c741 100644
--- a/include/sound/compress_driver.h
+++ b/include/sound/compress_driver.h
@@ -71,6 +71,8 @@ struct snd_compr_runtime {
* @runtime: pointer to runtime structure
* @device: device pointer
* @direction: stream direction, playback/recording
+ * @metadata_set: metadata set flag, true when set
+ * @next_track: has userspace signall next track transistion, true when set
* @private_data: pointer to DSP private data
*/
struct snd_compr_stream {
@@ -79,6 +81,8 @@ struct snd_compr_stream {
struct snd_compr_runtime *runtime;
struct snd_compr *device;
enum snd_compr_direction direction;
+ bool metadata_set;
+ bool next_track;
void *private_data;
};
@@ -110,6 +114,10 @@ struct snd_compr_ops {
struct snd_compr_params *params);
int (*get_params)(struct snd_compr_stream *stream,
struct snd_codec *params);
+ int (*set_metadata)(struct snd_compr_stream *stream,
+ struct snd_compr_metadata *metadata);
+ int (*get_metadata)(struct snd_compr_stream *stream,
+ struct snd_compr_metadata *metadata);
int (*trigger)(struct snd_compr_stream *stream, int cmd);
int (*pointer)(struct snd_compr_stream *stream,
struct snd_compr_tstamp *tstamp);
diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h
index 05341a4..d630163 100644
--- a/include/uapi/sound/compress_offload.h
+++ b/include/uapi/sound/compress_offload.h
@@ -30,7 +30,7 @@
#include <sound/compress_params.h>
-#define SNDRV_COMPRESS_VERSION SNDRV_PROTOCOL_VERSION(0, 1, 0)
+#define SNDRV_COMPRESS_VERSION SNDRV_PROTOCOL_VERSION(0, 1, 1)
/**
* struct snd_compressed_buffer: compressed buffer
* @fragment_size: size of buffer fragment in bytes
@@ -122,6 +122,27 @@ struct snd_compr_codec_caps {
};
/**
+ * @SNDRV_COMPRESS_ENCODER_PADDING: no of samples appended by the encoder at the
+ * end of the track
+ * @SNDRV_COMPRESS_ENCODER_DELAY: no of samples inserted by the encoder at the
+ * beginning of the track
+ */
+enum {
+ SNDRV_COMPRESS_ENCODER_PADDING = 1,
+ SNDRV_COMPRESS_ENCODER_DELAY = 2,
+};
+
+/**
+ * struct snd_compr_metadata: compressed stream metadata
+ * @key: key id
+ * @value: key value
+ */
+struct snd_compr_metadata {
+ __u32 key;
+ __u32 value[8];
+};
+
+/**
* compress path ioctl definitions
* SNDRV_COMPRESS_GET_CAPS: Query capability of DSP
* SNDRV_COMPRESS_GET_CODEC_CAPS: Query capability of a codec
@@ -145,6 +166,10 @@ struct snd_compr_codec_caps {
struct snd_compr_codec_caps)
#define SNDRV_COMPRESS_SET_PARAMS _IOW('C', 0x12, struct snd_compr_params)
#define SNDRV_COMPRESS_GET_PARAMS _IOR('C', 0x13, struct snd_codec)
+#define SNDRV_COMPRESS_SET_METADATA _IOW('C', 0x14,\
+ struct snd_compr_metadata)
+#define SNDRV_COMPRESS_GET_METADATA _IOWR('C', 0x15,\
+ struct snd_compr_metadata)
#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)
@@ -152,10 +177,14 @@ struct snd_compr_codec_caps {
#define SNDRV_COMPRESS_START _IO('C', 0x32)
#define SNDRV_COMPRESS_STOP _IO('C', 0x33)
#define SNDRV_COMPRESS_DRAIN _IO('C', 0x34)
+#define SNDRV_COMPRESS_NEXT_TRACK _IO('C', 0x35)
+#define SNDRV_COMPRESS_PARTIAL_DRAIN _IO('C', 0x36)
/*
* TODO
* 1. add mmap support
*
*/
#define SND_COMPR_TRIGGER_DRAIN 7 /*FIXME move this to pcm.h */
+#define SND_COMPR_TRIGGER_NEXT_TRACK 8
+#define SND_COMPR_TRIGGER_PARTIAL_DRAIN 9
#endif
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index ad11dc9..cbb592f 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -483,6 +483,8 @@ snd_compr_set_params(struct snd_compr_stream *stream, unsigned long arg)
if (retval)
goto out;
stream->runtime->state = SNDRV_PCM_STATE_SETUP;
+ stream->metadata_set = false;
+ stream->next_track = false;
} else {
return -EPERM;
}
@@ -514,6 +516,49 @@ out:
return retval;
}
+static int
+snd_compr_get_metadata(struct snd_compr_stream *stream, unsigned long arg)
+{
+ struct snd_compr_metadata metadata;
+ int retval;
+
+ if (!stream->ops->get_metadata)
+ return -ENXIO;
+
+ if (copy_from_user(&metadata, (void __user *)arg, sizeof(metadata)))
+ return -EFAULT;
+
+ retval = stream->ops->get_metadata(stream, &metadata);
+ if (retval != 0)
+ return retval;
+
+ if (copy_to_user((void __user *)arg, &metadata, sizeof(metadata)))
+ return -EFAULT;
+
+ return 0;
+}
+
+static int
+snd_compr_set_metadata(struct snd_compr_stream *stream, unsigned long arg)
+{
+ struct snd_compr_metadata metadata;
+ int retval;
+
+ if (!stream->ops->set_metadata)
+ return -ENXIO;
+ /*
+ * we should allow parameter change only when stream has been
+ * opened not in other cases
+ */
+ if (copy_from_user(&metadata, (void __user *)arg, sizeof(metadata)))
+ return -EFAULT;
+
+ retval = stream->ops->set_metadata(stream, &metadata);
+ stream->metadata_set = true;
+
+ return retval;
+}
+
static inline int
snd_compr_tstamp(struct snd_compr_stream *stream, unsigned long arg)
{
@@ -594,6 +639,44 @@ static int snd_compr_drain(struct snd_compr_stream *stream)
return retval;
}
+static int snd_compr_next_track(struct snd_compr_stream *stream)
+{
+ int retval;
+
+ /* only a running stream can transition to next track */
+ if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING)
+ return -EPERM;
+
+ /* you can signal next track isf this is intended to be a gapless stream
+ * and current track metadata is set
+ */
+ if (stream->metadata_set == false)
+ return -EPERM;
+
+ retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_NEXT_TRACK);
+ if (retval != 0)
+ return retval;
+ stream->metadata_set = false;
+ stream->next_track = true;
+ return 0;
+}
+
+static int snd_compr_partial_drain(struct snd_compr_stream *stream)
+{
+ int retval;
+ if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
+ stream->runtime->state == SNDRV_PCM_STATE_SETUP)
+ return -EPERM;
+ /* stream can be drained only when next track has been signalled */
+ if (stream->next_track == false)
+ return -EPERM;
+
+ retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_PARTIAL_DRAIN);
+
+ stream->next_track = false;
+ return retval;
+}
+
static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
{
struct snd_compr_file *data = f->private_data;
@@ -623,6 +706,12 @@ 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_METADATA):
+ retval = snd_compr_set_metadata(stream, arg);
+ break;
+ case _IOC_NR(SNDRV_COMPRESS_GET_METADATA):
+ retval = snd_compr_get_metadata(stream, arg);
+ break;
case _IOC_NR(SNDRV_COMPRESS_TSTAMP):
retval = snd_compr_tstamp(stream, arg);
break;
@@ -644,6 +733,13 @@ static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
case _IOC_NR(SNDRV_COMPRESS_DRAIN):
retval = snd_compr_drain(stream);
break;
+ case _IOC_NR(SNDRV_COMPRESS_PARTIAL_DRAIN):
+ retval = snd_compr_partial_drain(stream);
+ break;
+ case _IOC_NR(SNDRV_COMPRESS_NEXT_TRACK):
+ retval = snd_compr_next_track(stream);
+ break;
+
}
mutex_unlock(&stream->device->lock);
return retval;
--
1.7.0.4
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH] compress: add support for gapless playback
2013-02-14 11:22 Vinod Koul
@ 2013-02-14 11:31 ` Takashi Iwai
2013-02-14 13:37 ` Vinod Koul
0 siblings, 1 reply; 25+ messages in thread
From: Takashi Iwai @ 2013-02-14 11:31 UTC (permalink / raw)
To: Vinod Koul; +Cc: Jeeja KP, alsa-devel, broonie, liam.r.girdwood
At Thu, 14 Feb 2013 16:52:51 +0530,
Vinod Koul wrote:
>
> From: Jeeja KP <jeeja.kp@intel.com>
>
> this add new API for sound compress to support gapless playback.
> As noted in Documentation change, we add API to send metadata of encoder and
> padding delay to DSP. Also add API for indicating EOF and switching to
> subsequent track
>
> Also bump the compress API version
>
> Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
Thanks, I'm going to apply it to for-next branch.
Is there any driver requesting this new API for 3.9 kernel?
Takashi
> ----
> v6:
> - update ioctls GET_METADATA to IOWR
> - add documenetation of enum
>
> v5:
> - update metadata value with 8 words
> - add reset of new states in set_params
> - use statically allocated structs
>
> v4:
> - update metadata struct with single key/value
>
> v3:
> - Change back to ioctl struct with padding
> - adding states for next_track and metadata for proper transistion
>
> v2:
> - Make it a patch, not RFC
> - split metadata to key/value pairs, and send multiple keys
> - add get_metadata api
> - split partial_drain to next_data & partial_drain
> - add stream states for transistion
> - update documentation
> -----
> Documentation/sound/alsa/compress_offload.txt | 46 ++++++++++++
> include/sound/compress_driver.h | 8 ++
> include/uapi/sound/compress_offload.h | 31 ++++++++-
> sound/core/compress_offload.c | 96 +++++++++++++++++++++++++
> 4 files changed, 180 insertions(+), 1 deletions(-)
>
> diff --git a/Documentation/sound/alsa/compress_offload.txt b/Documentation/sound/alsa/compress_offload.txt
> index 90e9b3a..0bcc551 100644
> --- a/Documentation/sound/alsa/compress_offload.txt
> +++ b/Documentation/sound/alsa/compress_offload.txt
> @@ -145,6 +145,52 @@ Modifications include:
> - Addition of encoding options when required (derived from OpenMAX IL)
> - Addition of rateControlSupported (missing in OpenMAX AL)
>
> +Gapless Playback
> +================
> +When playing thru an album, the decoders have the ability to skip the encoder
> +delay and padding and directly move from one track content to another. The end
> +user can perceive this as gapless playback as we dont have silence while
> +switching from one track to another
> +
> +Also, there might be low-intensity noises due to encoding. Perfect gapless is
> +difficult to reach with all types of compressed data, but works fine with most
> +music content. The decoder needs to know the encoder delay and encoder padding.
> +So we need to pass this to DSP. This metadata is extracted from ID3/MP4 headers
> +and are not present by default in the bitstream, hence the need for a new
> +interface to pass this information to the DSP. Also DSP and userspace needs to
> +switch from one track to another and start using data for second track.
> +
> +The main additions are:
> +
> +- set_metadata
> +This routine sets the encoder delay and encoder padding. This can be used by
> +decoder to strip the silence. This needs to be set before the data in the track
> +is written.
> +
> +- set_next_track
> +This routine tells DSP that metadata and write operation sent after this would
> +correspond to subsequent track
> +
> +- partial drain
> +This is called when end of file is reached. The userspace can inform DSP that
> +EOF is reached and now DSP can start skipping padding delay. Also next write
> +data would belong to next track
> +
> +Sequence flow for gapless would be:
> +- Open
> +- Get caps / codec caps
> +- Set params
> +- Set metadata of the first track
> +- Fill data of the first track
> +- Trigger start
> +- User-space finished sending all,
> +- Indicaite next track data by sending set_next_track
> +- Set metadata of the next track
> +- then call partial_drain to flush most of buffer in DSP
> +- Fill data of the next track
> +- DSP switches to second track
> +(note: order for partial_drain and write for next track can be reversed as well)
> +
> Not supported:
>
> - Support for VoIP/circuit-switched calls is not the target of this
> diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
> index f2912ab..ff6c741 100644
> --- a/include/sound/compress_driver.h
> +++ b/include/sound/compress_driver.h
> @@ -71,6 +71,8 @@ struct snd_compr_runtime {
> * @runtime: pointer to runtime structure
> * @device: device pointer
> * @direction: stream direction, playback/recording
> + * @metadata_set: metadata set flag, true when set
> + * @next_track: has userspace signall next track transistion, true when set
> * @private_data: pointer to DSP private data
> */
> struct snd_compr_stream {
> @@ -79,6 +81,8 @@ struct snd_compr_stream {
> struct snd_compr_runtime *runtime;
> struct snd_compr *device;
> enum snd_compr_direction direction;
> + bool metadata_set;
> + bool next_track;
> void *private_data;
> };
>
> @@ -110,6 +114,10 @@ struct snd_compr_ops {
> struct snd_compr_params *params);
> int (*get_params)(struct snd_compr_stream *stream,
> struct snd_codec *params);
> + int (*set_metadata)(struct snd_compr_stream *stream,
> + struct snd_compr_metadata *metadata);
> + int (*get_metadata)(struct snd_compr_stream *stream,
> + struct snd_compr_metadata *metadata);
> int (*trigger)(struct snd_compr_stream *stream, int cmd);
> int (*pointer)(struct snd_compr_stream *stream,
> struct snd_compr_tstamp *tstamp);
> diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h
> index 05341a4..d630163 100644
> --- a/include/uapi/sound/compress_offload.h
> +++ b/include/uapi/sound/compress_offload.h
> @@ -30,7 +30,7 @@
> #include <sound/compress_params.h>
>
>
> -#define SNDRV_COMPRESS_VERSION SNDRV_PROTOCOL_VERSION(0, 1, 0)
> +#define SNDRV_COMPRESS_VERSION SNDRV_PROTOCOL_VERSION(0, 1, 1)
> /**
> * struct snd_compressed_buffer: compressed buffer
> * @fragment_size: size of buffer fragment in bytes
> @@ -122,6 +122,27 @@ struct snd_compr_codec_caps {
> };
>
> /**
> + * @SNDRV_COMPRESS_ENCODER_PADDING: no of samples appended by the encoder at the
> + * end of the track
> + * @SNDRV_COMPRESS_ENCODER_DELAY: no of samples inserted by the encoder at the
> + * beginning of the track
> + */
> +enum {
> + SNDRV_COMPRESS_ENCODER_PADDING = 1,
> + SNDRV_COMPRESS_ENCODER_DELAY = 2,
> +};
> +
> +/**
> + * struct snd_compr_metadata: compressed stream metadata
> + * @key: key id
> + * @value: key value
> + */
> +struct snd_compr_metadata {
> + __u32 key;
> + __u32 value[8];
> +};
> +
> +/**
> * compress path ioctl definitions
> * SNDRV_COMPRESS_GET_CAPS: Query capability of DSP
> * SNDRV_COMPRESS_GET_CODEC_CAPS: Query capability of a codec
> @@ -145,6 +166,10 @@ struct snd_compr_codec_caps {
> struct snd_compr_codec_caps)
> #define SNDRV_COMPRESS_SET_PARAMS _IOW('C', 0x12, struct snd_compr_params)
> #define SNDRV_COMPRESS_GET_PARAMS _IOR('C', 0x13, struct snd_codec)
> +#define SNDRV_COMPRESS_SET_METADATA _IOW('C', 0x14,\
> + struct snd_compr_metadata)
> +#define SNDRV_COMPRESS_GET_METADATA _IOWR('C', 0x15,\
> + struct snd_compr_metadata)
> #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)
> @@ -152,10 +177,14 @@ struct snd_compr_codec_caps {
> #define SNDRV_COMPRESS_START _IO('C', 0x32)
> #define SNDRV_COMPRESS_STOP _IO('C', 0x33)
> #define SNDRV_COMPRESS_DRAIN _IO('C', 0x34)
> +#define SNDRV_COMPRESS_NEXT_TRACK _IO('C', 0x35)
> +#define SNDRV_COMPRESS_PARTIAL_DRAIN _IO('C', 0x36)
> /*
> * TODO
> * 1. add mmap support
> *
> */
> #define SND_COMPR_TRIGGER_DRAIN 7 /*FIXME move this to pcm.h */
> +#define SND_COMPR_TRIGGER_NEXT_TRACK 8
> +#define SND_COMPR_TRIGGER_PARTIAL_DRAIN 9
> #endif
> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> index ad11dc9..cbb592f 100644
> --- a/sound/core/compress_offload.c
> +++ b/sound/core/compress_offload.c
> @@ -483,6 +483,8 @@ snd_compr_set_params(struct snd_compr_stream *stream, unsigned long arg)
> if (retval)
> goto out;
> stream->runtime->state = SNDRV_PCM_STATE_SETUP;
> + stream->metadata_set = false;
> + stream->next_track = false;
> } else {
> return -EPERM;
> }
> @@ -514,6 +516,49 @@ out:
> return retval;
> }
>
> +static int
> +snd_compr_get_metadata(struct snd_compr_stream *stream, unsigned long arg)
> +{
> + struct snd_compr_metadata metadata;
> + int retval;
> +
> + if (!stream->ops->get_metadata)
> + return -ENXIO;
> +
> + if (copy_from_user(&metadata, (void __user *)arg, sizeof(metadata)))
> + return -EFAULT;
> +
> + retval = stream->ops->get_metadata(stream, &metadata);
> + if (retval != 0)
> + return retval;
> +
> + if (copy_to_user((void __user *)arg, &metadata, sizeof(metadata)))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> +static int
> +snd_compr_set_metadata(struct snd_compr_stream *stream, unsigned long arg)
> +{
> + struct snd_compr_metadata metadata;
> + int retval;
> +
> + if (!stream->ops->set_metadata)
> + return -ENXIO;
> + /*
> + * we should allow parameter change only when stream has been
> + * opened not in other cases
> + */
> + if (copy_from_user(&metadata, (void __user *)arg, sizeof(metadata)))
> + return -EFAULT;
> +
> + retval = stream->ops->set_metadata(stream, &metadata);
> + stream->metadata_set = true;
> +
> + return retval;
> +}
> +
> static inline int
> snd_compr_tstamp(struct snd_compr_stream *stream, unsigned long arg)
> {
> @@ -594,6 +639,44 @@ static int snd_compr_drain(struct snd_compr_stream *stream)
> return retval;
> }
>
> +static int snd_compr_next_track(struct snd_compr_stream *stream)
> +{
> + int retval;
> +
> + /* only a running stream can transition to next track */
> + if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING)
> + return -EPERM;
> +
> + /* you can signal next track isf this is intended to be a gapless stream
> + * and current track metadata is set
> + */
> + if (stream->metadata_set == false)
> + return -EPERM;
> +
> + retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_NEXT_TRACK);
> + if (retval != 0)
> + return retval;
> + stream->metadata_set = false;
> + stream->next_track = true;
> + return 0;
> +}
> +
> +static int snd_compr_partial_drain(struct snd_compr_stream *stream)
> +{
> + int retval;
> + if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
> + stream->runtime->state == SNDRV_PCM_STATE_SETUP)
> + return -EPERM;
> + /* stream can be drained only when next track has been signalled */
> + if (stream->next_track == false)
> + return -EPERM;
> +
> + retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_PARTIAL_DRAIN);
> +
> + stream->next_track = false;
> + return retval;
> +}
> +
> static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
> {
> struct snd_compr_file *data = f->private_data;
> @@ -623,6 +706,12 @@ 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_METADATA):
> + retval = snd_compr_set_metadata(stream, arg);
> + break;
> + case _IOC_NR(SNDRV_COMPRESS_GET_METADATA):
> + retval = snd_compr_get_metadata(stream, arg);
> + break;
> case _IOC_NR(SNDRV_COMPRESS_TSTAMP):
> retval = snd_compr_tstamp(stream, arg);
> break;
> @@ -644,6 +733,13 @@ static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
> case _IOC_NR(SNDRV_COMPRESS_DRAIN):
> retval = snd_compr_drain(stream);
> break;
> + case _IOC_NR(SNDRV_COMPRESS_PARTIAL_DRAIN):
> + retval = snd_compr_partial_drain(stream);
> + break;
> + case _IOC_NR(SNDRV_COMPRESS_NEXT_TRACK):
> + retval = snd_compr_next_track(stream);
> + break;
> +
> }
> mutex_unlock(&stream->device->lock);
> return retval;
> --
> 1.7.0.4
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] compress: add support for gapless playback
2013-02-14 11:31 ` Takashi Iwai
@ 2013-02-14 13:37 ` Vinod Koul
0 siblings, 0 replies; 25+ messages in thread
From: Vinod Koul @ 2013-02-14 13:37 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Jeeja KP, alsa-devel, broonie, liam.r.girdwood
On Thu, Feb 14, 2013 at 12:31:33PM +0100, Takashi Iwai wrote:
> At Thu, 14 Feb 2013 16:52:51 +0530,
> Vinod Koul wrote:
> >
> > From: Jeeja KP <jeeja.kp@intel.com>
> >
> > this add new API for sound compress to support gapless playback.
> > As noted in Documentation change, we add API to send metadata of encoder and
> > padding delay to DSP. Also add API for indicating EOF and switching to
> > subsequent track
> >
> > Also bump the compress API version
> >
> > Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
> > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
>
> Thanks, I'm going to apply it to for-next branch.
> Is there any driver requesting this new API for 3.9 kernel?
Thanks, I will send the ASoC and driver update soon. Hopefully they will be
before merge window.
--
~Vinod
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH] compress: add support for gapless playback
@ 2013-02-14 8:42 Vinod Koul
2013-02-14 9:13 ` Takashi Iwai
0 siblings, 1 reply; 25+ messages in thread
From: Vinod Koul @ 2013-02-14 8:42 UTC (permalink / raw)
To: alsa-devel; +Cc: tiwai, Vinod Koul, broonie, liam.r.girdwood, Jeeja KP
this add new API for sound compress to support gapless playback.
As noted in Documentation change, we add API to send metadata of encoder and
padding delay to DSP. Also add API for indicating EOF and switching to
subsequent track
Also bump the compress API version
Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
v5:
- update metadata value with 8 words
- add reset of new states in set_params
- use statically allocated structs
v4:
- update metadata struct with single key/value
v3:
- Change back to ioctl struct with padding
- adding states for next_track and metadata for proper transistion
v2:
- Make it a patch, not RFC
- split metadata to key/value pairs, and send multiple keys
- add get_metadata api
- split partial_drain to next_data & partial_drain
- add stream states for transistion
- update documentation
---
Documentation/sound/alsa/compress_offload.txt | 46 ++++++++++++
include/sound/compress_driver.h | 8 ++
include/uapi/sound/compress_offload.h | 25 ++++++-
sound/core/compress_offload.c | 96 +++++++++++++++++++++++++
4 files changed, 174 insertions(+), 1 deletions(-)
diff --git a/Documentation/sound/alsa/compress_offload.txt b/Documentation/sound/alsa/compress_offload.txt
index 90e9b3a..0bcc551 100644
--- a/Documentation/sound/alsa/compress_offload.txt
+++ b/Documentation/sound/alsa/compress_offload.txt
@@ -145,6 +145,52 @@ Modifications include:
- Addition of encoding options when required (derived from OpenMAX IL)
- Addition of rateControlSupported (missing in OpenMAX AL)
+Gapless Playback
+================
+When playing thru an album, the decoders have the ability to skip the encoder
+delay and padding and directly move from one track content to another. The end
+user can perceive this as gapless playback as we dont have silence while
+switching from one track to another
+
+Also, there might be low-intensity noises due to encoding. Perfect gapless is
+difficult to reach with all types of compressed data, but works fine with most
+music content. The decoder needs to know the encoder delay and encoder padding.
+So we need to pass this to DSP. This metadata is extracted from ID3/MP4 headers
+and are not present by default in the bitstream, hence the need for a new
+interface to pass this information to the DSP. Also DSP and userspace needs to
+switch from one track to another and start using data for second track.
+
+The main additions are:
+
+- set_metadata
+This routine sets the encoder delay and encoder padding. This can be used by
+decoder to strip the silence. This needs to be set before the data in the track
+is written.
+
+- set_next_track
+This routine tells DSP that metadata and write operation sent after this would
+correspond to subsequent track
+
+- partial drain
+This is called when end of file is reached. The userspace can inform DSP that
+EOF is reached and now DSP can start skipping padding delay. Also next write
+data would belong to next track
+
+Sequence flow for gapless would be:
+- Open
+- Get caps / codec caps
+- Set params
+- Set metadata of the first track
+- Fill data of the first track
+- Trigger start
+- User-space finished sending all,
+- Indicaite next track data by sending set_next_track
+- Set metadata of the next track
+- then call partial_drain to flush most of buffer in DSP
+- Fill data of the next track
+- DSP switches to second track
+(note: order for partial_drain and write for next track can be reversed as well)
+
Not supported:
- Support for VoIP/circuit-switched calls is not the target of this
diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
index f2912ab..ff6c741 100644
--- a/include/sound/compress_driver.h
+++ b/include/sound/compress_driver.h
@@ -71,6 +71,8 @@ struct snd_compr_runtime {
* @runtime: pointer to runtime structure
* @device: device pointer
* @direction: stream direction, playback/recording
+ * @metadata_set: metadata set flag, true when set
+ * @next_track: has userspace signall next track transistion, true when set
* @private_data: pointer to DSP private data
*/
struct snd_compr_stream {
@@ -79,6 +81,8 @@ struct snd_compr_stream {
struct snd_compr_runtime *runtime;
struct snd_compr *device;
enum snd_compr_direction direction;
+ bool metadata_set;
+ bool next_track;
void *private_data;
};
@@ -110,6 +114,10 @@ struct snd_compr_ops {
struct snd_compr_params *params);
int (*get_params)(struct snd_compr_stream *stream,
struct snd_codec *params);
+ int (*set_metadata)(struct snd_compr_stream *stream,
+ struct snd_compr_metadata *metadata);
+ int (*get_metadata)(struct snd_compr_stream *stream,
+ struct snd_compr_metadata *metadata);
int (*trigger)(struct snd_compr_stream *stream, int cmd);
int (*pointer)(struct snd_compr_stream *stream,
struct snd_compr_tstamp *tstamp);
diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h
index 05341a4..5081a66 100644
--- a/include/uapi/sound/compress_offload.h
+++ b/include/uapi/sound/compress_offload.h
@@ -30,7 +30,7 @@
#include <sound/compress_params.h>
-#define SNDRV_COMPRESS_VERSION SNDRV_PROTOCOL_VERSION(0, 1, 0)
+#define SNDRV_COMPRESS_VERSION SNDRV_PROTOCOL_VERSION(0, 1, 1)
/**
* struct snd_compressed_buffer: compressed buffer
* @fragment_size: size of buffer fragment in bytes
@@ -121,6 +121,21 @@ struct snd_compr_codec_caps {
struct snd_codec_desc descriptor[MAX_NUM_CODEC_DESCRIPTORS];
};
+enum {
+ SNDRV_COMPRESS_ENCODER_PADDING = 1,
+ SNDRV_COMPRESS_ENCODER_DELAY = 2,
+};
+
+/**
+ * struct snd_compr_metadata: compressed stream metadata
+ * @key: key id
+ * @value: key value
+ */
+struct snd_compr_metadata {
+ __u32 key;
+ __u32 value[8];
+};
+
/**
* compress path ioctl definitions
* SNDRV_COMPRESS_GET_CAPS: Query capability of DSP
@@ -145,6 +160,10 @@ struct snd_compr_codec_caps {
struct snd_compr_codec_caps)
#define SNDRV_COMPRESS_SET_PARAMS _IOW('C', 0x12, struct snd_compr_params)
#define SNDRV_COMPRESS_GET_PARAMS _IOR('C', 0x13, struct snd_codec)
+#define SNDRV_COMPRESS_SET_METADATA _IOW('C', 0x14,\
+ struct snd_compr_metadata)
+#define SNDRV_COMPRESS_GET_METADATA _IOW('C', 0x15,\
+ struct snd_compr_metadata)
#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)
@@ -152,10 +171,14 @@ struct snd_compr_codec_caps {
#define SNDRV_COMPRESS_START _IO('C', 0x32)
#define SNDRV_COMPRESS_STOP _IO('C', 0x33)
#define SNDRV_COMPRESS_DRAIN _IO('C', 0x34)
+#define SNDRV_COMPRESS_NEXT_TRACK _IO('C', 0x35)
+#define SNDRV_COMPRESS_PARTIAL_DRAIN _IO('C', 0x36)
/*
* TODO
* 1. add mmap support
*
*/
#define SND_COMPR_TRIGGER_DRAIN 7 /*FIXME move this to pcm.h */
+#define SND_COMPR_TRIGGER_NEXT_TRACK 8
+#define SND_COMPR_TRIGGER_PARTIAL_DRAIN 9
#endif
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index ad11dc9..cbb592f 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -483,6 +483,8 @@ snd_compr_set_params(struct snd_compr_stream *stream, unsigned long arg)
if (retval)
goto out;
stream->runtime->state = SNDRV_PCM_STATE_SETUP;
+ stream->metadata_set = false;
+ stream->next_track = false;
} else {
return -EPERM;
}
@@ -514,6 +516,49 @@ out:
return retval;
}
+static int
+snd_compr_get_metadata(struct snd_compr_stream *stream, unsigned long arg)
+{
+ struct snd_compr_metadata metadata;
+ int retval;
+
+ if (!stream->ops->get_metadata)
+ return -ENXIO;
+
+ if (copy_from_user(&metadata, (void __user *)arg, sizeof(metadata)))
+ return -EFAULT;
+
+ retval = stream->ops->get_metadata(stream, &metadata);
+ if (retval != 0)
+ return retval;
+
+ if (copy_to_user((void __user *)arg, &metadata, sizeof(metadata)))
+ return -EFAULT;
+
+ return 0;
+}
+
+static int
+snd_compr_set_metadata(struct snd_compr_stream *stream, unsigned long arg)
+{
+ struct snd_compr_metadata metadata;
+ int retval;
+
+ if (!stream->ops->set_metadata)
+ return -ENXIO;
+ /*
+ * we should allow parameter change only when stream has been
+ * opened not in other cases
+ */
+ if (copy_from_user(&metadata, (void __user *)arg, sizeof(metadata)))
+ return -EFAULT;
+
+ retval = stream->ops->set_metadata(stream, &metadata);
+ stream->metadata_set = true;
+
+ return retval;
+}
+
static inline int
snd_compr_tstamp(struct snd_compr_stream *stream, unsigned long arg)
{
@@ -594,6 +639,44 @@ static int snd_compr_drain(struct snd_compr_stream *stream)
return retval;
}
+static int snd_compr_next_track(struct snd_compr_stream *stream)
+{
+ int retval;
+
+ /* only a running stream can transition to next track */
+ if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING)
+ return -EPERM;
+
+ /* you can signal next track isf this is intended to be a gapless stream
+ * and current track metadata is set
+ */
+ if (stream->metadata_set == false)
+ return -EPERM;
+
+ retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_NEXT_TRACK);
+ if (retval != 0)
+ return retval;
+ stream->metadata_set = false;
+ stream->next_track = true;
+ return 0;
+}
+
+static int snd_compr_partial_drain(struct snd_compr_stream *stream)
+{
+ int retval;
+ if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
+ stream->runtime->state == SNDRV_PCM_STATE_SETUP)
+ return -EPERM;
+ /* stream can be drained only when next track has been signalled */
+ if (stream->next_track == false)
+ return -EPERM;
+
+ retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_PARTIAL_DRAIN);
+
+ stream->next_track = false;
+ return retval;
+}
+
static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
{
struct snd_compr_file *data = f->private_data;
@@ -623,6 +706,12 @@ 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_METADATA):
+ retval = snd_compr_set_metadata(stream, arg);
+ break;
+ case _IOC_NR(SNDRV_COMPRESS_GET_METADATA):
+ retval = snd_compr_get_metadata(stream, arg);
+ break;
case _IOC_NR(SNDRV_COMPRESS_TSTAMP):
retval = snd_compr_tstamp(stream, arg);
break;
@@ -644,6 +733,13 @@ static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
case _IOC_NR(SNDRV_COMPRESS_DRAIN):
retval = snd_compr_drain(stream);
break;
+ case _IOC_NR(SNDRV_COMPRESS_PARTIAL_DRAIN):
+ retval = snd_compr_partial_drain(stream);
+ break;
+ case _IOC_NR(SNDRV_COMPRESS_NEXT_TRACK):
+ retval = snd_compr_next_track(stream);
+ break;
+
}
mutex_unlock(&stream->device->lock);
return retval;
--
1.7.0.4
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH] compress: add support for gapless playback
2013-02-14 8:42 Vinod Koul
@ 2013-02-14 9:13 ` Takashi Iwai
2013-02-14 9:32 ` Vinod Koul
0 siblings, 1 reply; 25+ messages in thread
From: Takashi Iwai @ 2013-02-14 9:13 UTC (permalink / raw)
To: Vinod Koul; +Cc: Jeeja KP, alsa-devel, broonie, liam.r.girdwood
At Thu, 14 Feb 2013 14:12:51 +0530,
Vinod Koul wrote:
>
> this add new API for sound compress to support gapless playback.
> As noted in Documentation change, we add API to send metadata of encoder and
> padding delay to DSP. Also add API for indicating EOF and switching to
> subsequent track
>
> Also bump the compress API version
>
> Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> ---
> v5:
> - update metadata value with 8 words
What's the reason behind this change? I can think of metadata like EQ
parameters, but the magic number 8 is in question :)
I'm no enthusiastic over minimalism like Apple, so I don't mind this
change, but just out of curiosity...
Also, another spot:
> @@ -145,6 +160,10 @@ struct snd_compr_codec_caps {
> struct snd_compr_codec_caps)
> #define SNDRV_COMPRESS_SET_PARAMS _IOW('C', 0x12, struct snd_compr_params)
> #define SNDRV_COMPRESS_GET_PARAMS _IOR('C', 0x13, struct snd_codec)
> +#define SNDRV_COMPRESS_SET_METADATA _IOW('C', 0x14,\
> + struct snd_compr_metadata)
> +#define SNDRV_COMPRESS_GET_METADATA _IOW('C', 0x15,\
> + struct snd_compr_metadata)
Isn't it _IOWR()?
Takashi
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] compress: add support for gapless playback
2013-02-14 9:13 ` Takashi Iwai
@ 2013-02-14 9:32 ` Vinod Koul
2013-02-14 9:45 ` Takashi Iwai
0 siblings, 1 reply; 25+ messages in thread
From: Vinod Koul @ 2013-02-14 9:32 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Jeeja KP, alsa-devel, broonie, liam.r.girdwood
On Thu, Feb 14, 2013 at 10:13:57AM +0100, Takashi Iwai wrote:
> At Thu, 14 Feb 2013 14:12:51 +0530,
> Vinod Koul wrote:
> >
> > this add new API for sound compress to support gapless playback.
> > As noted in Documentation change, we add API to send metadata of encoder and
> > padding delay to DSP. Also add API for indicating EOF and switching to
> > subsequent track
> >
> > Also bump the compress API version
> >
> > Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
> > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > ---
> > v5:
> > - update metadata value with 8 words
>
> What's the reason behind this change? I can think of metadata like EQ
> parameters, but the magic number 8 is in question :)
Yes the concern was that there might be an issue with parameters which are not
single dwords, so 8 dwords was what came to my mind. 8 why, i picked the number
:). Seems decent enough and not too big. For even bigger pparameters I was
thinking to split..
If you think we should have 16/32 dwrods, am okay with it.
> I'm no enthusiastic over minimalism like Apple, so I don't mind this
> change, but just out of curiosity...
>
> Also, another spot:
>
> > @@ -145,6 +160,10 @@ struct snd_compr_codec_caps {
> > struct snd_compr_codec_caps)
> > #define SNDRV_COMPRESS_SET_PARAMS _IOW('C', 0x12, struct snd_compr_params)
> > #define SNDRV_COMPRESS_GET_PARAMS _IOR('C', 0x13, struct snd_codec)
> > +#define SNDRV_COMPRESS_SET_METADATA _IOW('C', 0x14,\
> > + struct snd_compr_metadata)
> > +#define SNDRV_COMPRESS_GET_METADATA _IOW('C', 0x15,\
> > + struct snd_compr_metadata)
Yes!
>
> Isn't it _IOWR()?
If this is only thing, let me send the update or you want a beverage :)
--
~Vinod
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] compress: add support for gapless playback
2013-02-14 9:32 ` Vinod Koul
@ 2013-02-14 9:45 ` Takashi Iwai
2013-02-14 11:06 ` Vinod Koul
0 siblings, 1 reply; 25+ messages in thread
From: Takashi Iwai @ 2013-02-14 9:45 UTC (permalink / raw)
To: Vinod Koul; +Cc: Jeeja KP, alsa-devel, broonie, liam.r.girdwood
At Thu, 14 Feb 2013 15:02:45 +0530,
Vinod Koul wrote:
>
> On Thu, Feb 14, 2013 at 10:13:57AM +0100, Takashi Iwai wrote:
> > At Thu, 14 Feb 2013 14:12:51 +0530,
> > Vinod Koul wrote:
> > >
> > > this add new API for sound compress to support gapless playback.
> > > As noted in Documentation change, we add API to send metadata of encoder and
> > > padding delay to DSP. Also add API for indicating EOF and switching to
> > > subsequent track
> > >
> > > Also bump the compress API version
> > >
> > > Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
> > > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > > ---
> > > v5:
> > > - update metadata value with 8 words
> >
> > What's the reason behind this change? I can think of metadata like EQ
> > parameters, but the magic number 8 is in question :)
> Yes the concern was that there might be an issue with parameters which are not
> single dwords, so 8 dwords was what came to my mind. 8 why, i picked the number
> :). Seems decent enough and not too big. For even bigger pparameters I was
> thinking to split..
>
> If you think we should have 16/32 dwrods, am okay with it.
OK, I don't mind this change, so I can take it as is.
> > I'm no enthusiastic over minimalism like Apple, so I don't mind this
> > change, but just out of curiosity...
> >
> > Also, another spot:
> >
> > > @@ -145,6 +160,10 @@ struct snd_compr_codec_caps {
> > > struct snd_compr_codec_caps)
> > > #define SNDRV_COMPRESS_SET_PARAMS _IOW('C', 0x12, struct snd_compr_params)
> > > #define SNDRV_COMPRESS_GET_PARAMS _IOR('C', 0x13, struct snd_codec)
> > > +#define SNDRV_COMPRESS_SET_METADATA _IOW('C', 0x14,\
> > > + struct snd_compr_metadata)
> > > +#define SNDRV_COMPRESS_GET_METADATA _IOW('C', 0x15,\
> > > + struct snd_compr_metadata)
> Yes!
> >
> > Isn't it _IOWR()?
> If this is only thing, let me send the update or you want a beverage :)
Another favor would be to have a bit more comments about the existing
metadata parameter definitions, namely, what's exactly expected, in
which unit, and how many values are used (now we have an array).
This can be put in either compress API document or a short comment in
the enum item definition.
Takashi
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] compress: add support for gapless playback
2013-02-14 9:45 ` Takashi Iwai
@ 2013-02-14 11:06 ` Vinod Koul
0 siblings, 0 replies; 25+ messages in thread
From: Vinod Koul @ 2013-02-14 11:06 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Jeeja KP, alsa-devel, broonie, liam.r.girdwood
On Thu, Feb 14, 2013 at 10:45:54AM +0100, Takashi Iwai wrote:
> Another favor would be to have a bit more comments about the existing
> metadata parameter definitions, namely, what's exactly expected, in
> which unit, and how many values are used (now we have an array).
> This can be put in either compress API document or a short comment in
> the enum item definition.
I had that in one of the rev... let me update it.
I feel header is best suited...
You lost on the beverage :)
--
~Vinod
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH] compress: add support for gapless playback
@ 2013-02-13 16:00 Vinod Koul
2013-02-13 16:40 ` Takashi Iwai
0 siblings, 1 reply; 25+ messages in thread
From: Vinod Koul @ 2013-02-13 16:00 UTC (permalink / raw)
To: alsa-devel; +Cc: tiwai, Vinod Koul, broonie, liam.r.girdwood, Jeeja KP
this add new API for sound compress to support gapless playback.
As noted in Documentation change, we add API to send metadata of encoder and
padding delay to DSP. Also add API for indicating EOF and switching to
subsequent track
Also bump the compress API version
Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
----
v4:
- update metadata struct witj single key/value
v3:
- Change back to ioctl struct with padding
- adding states for next_track and metadata for proper transistion
v2:
- Make it a patch, not RFC
- split metadata to key/value pairs, and send multiple keys
- add get_metadata api
- split partial_drain to next_data & partial_drain
- add stream states for transistion
- update documentation
--
Documentation/sound/alsa/compress_offload.txt | 46 ++++++++++
include/sound/compress_driver.h | 8 ++
include/uapi/sound/compress_offload.h | 25 +++++-
sound/core/compress_offload.c | 113 +++++++++++++++++++++++++
4 files changed, 191 insertions(+), 1 deletions(-)
diff --git a/Documentation/sound/alsa/compress_offload.txt b/Documentation/sound/alsa/compress_offload.txt
index 90e9b3a..0bcc551 100644
--- a/Documentation/sound/alsa/compress_offload.txt
+++ b/Documentation/sound/alsa/compress_offload.txt
@@ -145,6 +145,52 @@ Modifications include:
- Addition of encoding options when required (derived from OpenMAX IL)
- Addition of rateControlSupported (missing in OpenMAX AL)
+Gapless Playback
+================
+When playing thru an album, the decoders have the ability to skip the encoder
+delay and padding and directly move from one track content to another. The end
+user can perceive this as gapless playback as we dont have silence while
+switching from one track to another
+
+Also, there might be low-intensity noises due to encoding. Perfect gapless is
+difficult to reach with all types of compressed data, but works fine with most
+music content. The decoder needs to know the encoder delay and encoder padding.
+So we need to pass this to DSP. This metadata is extracted from ID3/MP4 headers
+and are not present by default in the bitstream, hence the need for a new
+interface to pass this information to the DSP. Also DSP and userspace needs to
+switch from one track to another and start using data for second track.
+
+The main additions are:
+
+- set_metadata
+This routine sets the encoder delay and encoder padding. This can be used by
+decoder to strip the silence. This needs to be set before the data in the track
+is written.
+
+- set_next_track
+This routine tells DSP that metadata and write operation sent after this would
+correspond to subsequent track
+
+- partial drain
+This is called when end of file is reached. The userspace can inform DSP that
+EOF is reached and now DSP can start skipping padding delay. Also next write
+data would belong to next track
+
+Sequence flow for gapless would be:
+- Open
+- Get caps / codec caps
+- Set params
+- Set metadata of the first track
+- Fill data of the first track
+- Trigger start
+- User-space finished sending all,
+- Indicaite next track data by sending set_next_track
+- Set metadata of the next track
+- then call partial_drain to flush most of buffer in DSP
+- Fill data of the next track
+- DSP switches to second track
+(note: order for partial_drain and write for next track can be reversed as well)
+
Not supported:
- Support for VoIP/circuit-switched calls is not the target of this
diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
index f2912ab..ff6c741 100644
--- a/include/sound/compress_driver.h
+++ b/include/sound/compress_driver.h
@@ -71,6 +71,8 @@ struct snd_compr_runtime {
* @runtime: pointer to runtime structure
* @device: device pointer
* @direction: stream direction, playback/recording
+ * @metadata_set: metadata set flag, true when set
+ * @next_track: has userspace signall next track transistion, true when set
* @private_data: pointer to DSP private data
*/
struct snd_compr_stream {
@@ -79,6 +81,8 @@ struct snd_compr_stream {
struct snd_compr_runtime *runtime;
struct snd_compr *device;
enum snd_compr_direction direction;
+ bool metadata_set;
+ bool next_track;
void *private_data;
};
@@ -110,6 +114,10 @@ struct snd_compr_ops {
struct snd_compr_params *params);
int (*get_params)(struct snd_compr_stream *stream,
struct snd_codec *params);
+ int (*set_metadata)(struct snd_compr_stream *stream,
+ struct snd_compr_metadata *metadata);
+ int (*get_metadata)(struct snd_compr_stream *stream,
+ struct snd_compr_metadata *metadata);
int (*trigger)(struct snd_compr_stream *stream, int cmd);
int (*pointer)(struct snd_compr_stream *stream,
struct snd_compr_tstamp *tstamp);
diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h
index 05341a4..d7544b5 100644
--- a/include/uapi/sound/compress_offload.h
+++ b/include/uapi/sound/compress_offload.h
@@ -30,7 +30,7 @@
#include <sound/compress_params.h>
-#define SNDRV_COMPRESS_VERSION SNDRV_PROTOCOL_VERSION(0, 1, 0)
+#define SNDRV_COMPRESS_VERSION SNDRV_PROTOCOL_VERSION(0, 1, 1)
/**
* struct snd_compressed_buffer: compressed buffer
* @fragment_size: size of buffer fragment in bytes
@@ -121,6 +121,21 @@ struct snd_compr_codec_caps {
struct snd_codec_desc descriptor[MAX_NUM_CODEC_DESCRIPTORS];
};
+enum {
+ SNDRV_COMPRESS_ENCODER_PADDING = 1,
+ SNDRV_COMPRESS_ENCODER_DELAY = 2,
+};
+
+/**
+ * struct snd_compr_metadata: compressed stream metadata
+ * @key: key id
+ * @value: key value
+ */
+struct snd_compr_metadata {
+ __u32 key;
+ __u32 value;
+};
+
/**
* compress path ioctl definitions
* SNDRV_COMPRESS_GET_CAPS: Query capability of DSP
@@ -145,6 +160,10 @@ struct snd_compr_codec_caps {
struct snd_compr_codec_caps)
#define SNDRV_COMPRESS_SET_PARAMS _IOW('C', 0x12, struct snd_compr_params)
#define SNDRV_COMPRESS_GET_PARAMS _IOR('C', 0x13, struct snd_codec)
+#define SNDRV_COMPRESS_SET_METADATA _IOW('C', 0x14,\
+ struct snd_compr_metadata)
+#define SNDRV_COMPRESS_GET_METADATA _IOW('C', 0x15,\
+ struct snd_compr_metadata)
#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)
@@ -152,10 +171,14 @@ struct snd_compr_codec_caps {
#define SNDRV_COMPRESS_START _IO('C', 0x32)
#define SNDRV_COMPRESS_STOP _IO('C', 0x33)
#define SNDRV_COMPRESS_DRAIN _IO('C', 0x34)
+#define SNDRV_COMPRESS_NEXT_TRACK _IO('C', 0x35)
+#define SNDRV_COMPRESS_PARTIAL_DRAIN _IO('C', 0x36)
/*
* TODO
* 1. add mmap support
*
*/
#define SND_COMPR_TRIGGER_DRAIN 7 /*FIXME move this to pcm.h */
+#define SND_COMPR_TRIGGER_NEXT_TRACK 8
+#define SND_COMPR_TRIGGER_PARTIAL_DRAIN 9
#endif
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index ad11dc9..8d6812e 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -514,6 +514,66 @@ out:
return retval;
}
+static int
+snd_compr_get_metadata(struct snd_compr_stream *stream, unsigned long arg)
+{
+ struct snd_compr_metadata *metadata;
+ int retval;
+
+ if (!stream->ops->get_metadata)
+ return -ENXIO;
+
+ metadata = kmalloc(sizeof(*metadata), GFP_KERNEL);
+ if (!metadata)
+ return -ENOMEM;
+ if (copy_from_user(metadata, (void __user *)arg, sizeof(*metadata))) {
+ retval = -EFAULT;
+ goto out;
+ }
+
+ retval = stream->ops->get_metadata(stream, metadata);
+ if (retval != 0)
+ goto out;
+
+ if (copy_to_user((void __user *)arg, metadata, sizeof(*metadata))) {
+ retval = -EFAULT;
+ goto out;
+ }
+
+out:
+ kfree(metadata);
+ return retval;
+}
+
+static int
+snd_compr_set_metadata(struct snd_compr_stream *stream, unsigned long arg)
+{
+ struct snd_compr_metadata *metadata;
+ int retval;
+
+ if (!stream->ops->set_metadata)
+ return -ENXIO;
+ /*
+ * we should allow parameter change only when stream has been
+ * opened not in other cases
+ */
+ metadata = kmalloc(sizeof(*metadata), GFP_KERNEL);
+ if (!metadata)
+ return -ENOMEM;
+ if (copy_from_user(metadata, (void __user *)arg,
+ sizeof(*metadata))) {
+ retval = -EFAULT;
+ goto out;
+ }
+
+ retval = stream->ops->set_metadata(stream, metadata);
+ stream->metadata_set = true;
+
+out:
+ kfree(metadata);
+ return retval;
+}
+
static inline int
snd_compr_tstamp(struct snd_compr_stream *stream, unsigned long arg)
{
@@ -594,6 +654,46 @@ static int snd_compr_drain(struct snd_compr_stream *stream)
return retval;
}
+static int snd_compr_next_track(struct snd_compr_stream *stream)
+{
+ int retval;
+
+ /* only a running stream can transition to next track */
+ if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING)
+ return -EPERM;
+
+ /* you can signal next track isf this is intended to be a gapless stream
+ * and current track metadata is set
+ */
+ if (stream->metadata_set == false)
+ return -EPERM;
+
+ retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_NEXT_TRACK);
+ if (retval != 0)
+ return retval;
+ stream->metadata_set = false;
+ stream->next_track = true;
+ return 0;
+}
+
+static int snd_compr_partial_drain(struct snd_compr_stream *stream)
+{
+ int retval;
+ if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
+ stream->runtime->state == SNDRV_PCM_STATE_SETUP)
+ return -EPERM;
+ /* stream can be drained only when next track has been signalled */
+ if (stream->next_track == false)
+ return -EPERM;
+
+ retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_PARTIAL_DRAIN);
+ if (retval)
+ pr_err("Partial drain returned failure\n");
+
+ stream->next_track = false;
+ return retval;
+}
+
static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
{
struct snd_compr_file *data = f->private_data;
@@ -623,6 +723,12 @@ 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_METADATA):
+ retval = snd_compr_set_metadata(stream, arg);
+ break;
+ case _IOC_NR(SNDRV_COMPRESS_GET_METADATA):
+ retval = snd_compr_get_metadata(stream, arg);
+ break;
case _IOC_NR(SNDRV_COMPRESS_TSTAMP):
retval = snd_compr_tstamp(stream, arg);
break;
@@ -644,6 +750,13 @@ static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
case _IOC_NR(SNDRV_COMPRESS_DRAIN):
retval = snd_compr_drain(stream);
break;
+ case _IOC_NR(SNDRV_COMPRESS_PARTIAL_DRAIN):
+ retval = snd_compr_partial_drain(stream);
+ break;
+ case _IOC_NR(SNDRV_COMPRESS_NEXT_TRACK):
+ retval = snd_compr_next_track(stream);
+ break;
+
}
mutex_unlock(&stream->device->lock);
return retval;
--
1.7.0.4
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH] compress: add support for gapless playback
2013-02-13 16:00 Vinod Koul
@ 2013-02-13 16:40 ` Takashi Iwai
2013-02-13 17:03 ` Vinod Koul
0 siblings, 1 reply; 25+ messages in thread
From: Takashi Iwai @ 2013-02-13 16:40 UTC (permalink / raw)
To: Vinod Koul; +Cc: Jeeja KP, alsa-devel, broonie, liam.r.girdwood
At Wed, 13 Feb 2013 08:00:34 -0800,
Vinod Koul wrote:
>
> +static int
> +snd_compr_get_metadata(struct snd_compr_stream *stream, unsigned long arg)
> +{
> + struct snd_compr_metadata *metadata;
> + int retval;
> +
> + if (!stream->ops->get_metadata)
> + return -ENXIO;
> +
> + metadata = kmalloc(sizeof(*metadata), GFP_KERNEL);
There is no merit to use kmalloc just for two integers at all.
Simply put on stack:
struct snd_compr_metadata metdata;
if (copy_from_user(&metadata, (void __user *)arg, sizeof(metadata)))
...
Ditto for snd_compr_set_metadata().
> +static int snd_compr_partial_drain(struct snd_compr_stream *stream)
> +{
> + int retval;
> + if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
> + stream->runtime->state == SNDRV_PCM_STATE_SETUP)
> + return -EPERM;
> + /* stream can be drained only when next track has been signalled */
> + if (stream->next_track == false)
> + return -EPERM;
> +
> + retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_PARTIAL_DRAIN);
> + if (retval)
> + pr_err("Partial drain returned failure\n");
I don't think it's good to spew errors.
Also, the initialization of new stream fields is missing in other
places like prepare callback? Otherwise the values will retain.
thanks,
Takashi
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] compress: add support for gapless playback
2013-02-13 16:40 ` Takashi Iwai
@ 2013-02-13 17:03 ` Vinod Koul
2013-02-13 17:15 ` Takashi Iwai
0 siblings, 1 reply; 25+ messages in thread
From: Vinod Koul @ 2013-02-13 17:03 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Jeeja KP, alsa-devel, broonie, liam.r.girdwood
On Wed, Feb 13, 2013 at 05:40:11PM +0100, Takashi Iwai wrote:
> At Wed, 13 Feb 2013 08:00:34 -0800,
> Vinod Koul wrote:
> >
> > +static int
> > +snd_compr_get_metadata(struct snd_compr_stream *stream, unsigned long arg)
> > +{
> > + struct snd_compr_metadata *metadata;
> > + int retval;
> > +
> > + if (!stream->ops->get_metadata)
> > + return -ENXIO;
> > +
> > + metadata = kmalloc(sizeof(*metadata), GFP_KERNEL);
>
> There is no merit to use kmalloc just for two integers at all.
> Simply put on stack:
> struct snd_compr_metadata metdata;
> if (copy_from_user(&metadata, (void __user *)arg, sizeof(metadata)))
> ...
while doing this I had thought on this, but my laziness got better of me :(
>
> Ditto for snd_compr_set_metadata().
>
> > +static int snd_compr_partial_drain(struct snd_compr_stream *stream)
> > +{
> > + int retval;
> > + if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
> > + stream->runtime->state == SNDRV_PCM_STATE_SETUP)
> > + return -EPERM;
> > + /* stream can be drained only when next track has been signalled */
> > + if (stream->next_track == false)
> > + return -EPERM;
> > +
> > + retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_PARTIAL_DRAIN);
> > + if (retval)
> > + pr_err("Partial drain returned failure\n");
>
> I don't think it's good to spew errors.
ok
>
> Also, the initialization of new stream fields is missing in other
> places like prepare callback? Otherwise the values will retain.
The stream is created at open. Since it used kzalloc, these variables would be
init to false.
--
~Vinod
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] compress: add support for gapless playback
2013-02-13 17:03 ` Vinod Koul
@ 2013-02-13 17:15 ` Takashi Iwai
2013-02-13 17:21 ` Vinod Koul
0 siblings, 1 reply; 25+ messages in thread
From: Takashi Iwai @ 2013-02-13 17:15 UTC (permalink / raw)
To: Vinod Koul; +Cc: Jeeja KP, alsa-devel, broonie, liam.r.girdwood
At Wed, 13 Feb 2013 22:33:22 +0530,
Vinod Koul wrote:
>
> On Wed, Feb 13, 2013 at 05:40:11PM +0100, Takashi Iwai wrote:
> > At Wed, 13 Feb 2013 08:00:34 -0800,
> > Vinod Koul wrote:
> > >
> > > +static int
> > > +snd_compr_get_metadata(struct snd_compr_stream *stream, unsigned long arg)
> > > +{
> > > + struct snd_compr_metadata *metadata;
> > > + int retval;
> > > +
> > > + if (!stream->ops->get_metadata)
> > > + return -ENXIO;
> > > +
> > > + metadata = kmalloc(sizeof(*metadata), GFP_KERNEL);
> >
> > There is no merit to use kmalloc just for two integers at all.
> > Simply put on stack:
> > struct snd_compr_metadata metdata;
> > if (copy_from_user(&metadata, (void __user *)arg, sizeof(metadata)))
> > ...
> while doing this I had thought on this, but my laziness got better of me :(
> >
> > Ditto for snd_compr_set_metadata().
> >
> > > +static int snd_compr_partial_drain(struct snd_compr_stream *stream)
> > > +{
> > > + int retval;
> > > + if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
> > > + stream->runtime->state == SNDRV_PCM_STATE_SETUP)
> > > + return -EPERM;
> > > + /* stream can be drained only when next track has been signalled */
> > > + if (stream->next_track == false)
> > > + return -EPERM;
> > > +
> > > + retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_PARTIAL_DRAIN);
> > > + if (retval)
> > > + pr_err("Partial drain returned failure\n");
> >
> > I don't think it's good to spew errors.
> ok
> >
> > Also, the initialization of new stream fields is missing in other
> > places like prepare callback? Otherwise the values will retain.
> The stream is created at open. Since it used kzalloc, these variables would be
> init to false.
But you can restart the stream after stopping it without closing the
device, right?
Takashi
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] compress: add support for gapless playback
2013-02-13 17:15 ` Takashi Iwai
@ 2013-02-13 17:21 ` Vinod Koul
0 siblings, 0 replies; 25+ messages in thread
From: Vinod Koul @ 2013-02-13 17:21 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Jeeja KP, alsa-devel, broonie, liam.r.girdwood
On Wed, Feb 13, 2013 at 06:15:49PM +0100, Takashi Iwai wrote:
> > The stream is created at open. Since it used kzalloc, these variables would be
> > init to false.
>
> But you can restart the stream after stopping it without closing the
> device, right?
In theory yes. Let me update this in set_params always
--
~Vinod
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH] compress: add support for gapless playback
@ 2013-02-11 12:22 Vinod Koul
2013-02-11 13:41 ` Takashi Iwai
0 siblings, 1 reply; 25+ messages in thread
From: Vinod Koul @ 2013-02-11 12:22 UTC (permalink / raw)
To: alsa-devel; +Cc: tiwai, Vinod Koul, broonie, liam.r.girdwood, Jeeja KP
this adds new API for sound compress to support gapless playback.
As noted in Documentation change, we add API to send metadata of encoder and
padding delay to DSP. Also add API for indicating EOF and switching to
subsequent track
Also bump the compress API version
Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
v2:
- Make it a patch, not RFC
- split metadata to key/value pairs, and send multiple keys
- add get_metadata api
- split partial_drain to next_data & partial_drain
- add stream states for transistion
- update documentation
---
Documentation/sound/alsa/compress_offload.txt | 46 +++++++++
include/sound/compress_driver.h | 4 +
include/uapi/sound/asound.h | 4 +-
include/uapi/sound/compress_offload.h | 36 +++++++-
sound/core/compress_offload.c | 129 ++++++++++++++++++++++++-
5 files changed, 215 insertions(+), 4 deletions(-)
diff --git a/Documentation/sound/alsa/compress_offload.txt b/Documentation/sound/alsa/compress_offload.txt
index 90e9b3a..0bcc551 100644
--- a/Documentation/sound/alsa/compress_offload.txt
+++ b/Documentation/sound/alsa/compress_offload.txt
@@ -145,6 +145,52 @@ Modifications include:
- Addition of encoding options when required (derived from OpenMAX IL)
- Addition of rateControlSupported (missing in OpenMAX AL)
+Gapless Playback
+================
+When playing thru an album, the decoders have the ability to skip the encoder
+delay and padding and directly move from one track content to another. The end
+user can perceive this as gapless playback as we dont have silence while
+switching from one track to another
+
+Also, there might be low-intensity noises due to encoding. Perfect gapless is
+difficult to reach with all types of compressed data, but works fine with most
+music content. The decoder needs to know the encoder delay and encoder padding.
+So we need to pass this to DSP. This metadata is extracted from ID3/MP4 headers
+and are not present by default in the bitstream, hence the need for a new
+interface to pass this information to the DSP. Also DSP and userspace needs to
+switch from one track to another and start using data for second track.
+
+The main additions are:
+
+- set_metadata
+This routine sets the encoder delay and encoder padding. This can be used by
+decoder to strip the silence. This needs to be set before the data in the track
+is written.
+
+- set_next_track
+This routine tells DSP that metadata and write operation sent after this would
+correspond to subsequent track
+
+- partial drain
+This is called when end of file is reached. The userspace can inform DSP that
+EOF is reached and now DSP can start skipping padding delay. Also next write
+data would belong to next track
+
+Sequence flow for gapless would be:
+- Open
+- Get caps / codec caps
+- Set params
+- Set metadata of the first track
+- Fill data of the first track
+- Trigger start
+- User-space finished sending all,
+- Indicaite next track data by sending set_next_track
+- Set metadata of the next track
+- then call partial_drain to flush most of buffer in DSP
+- Fill data of the next track
+- DSP switches to second track
+(note: order for partial_drain and write for next track can be reversed as well)
+
Not supported:
- Support for VoIP/circuit-switched calls is not the target of this
diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
index f2912ab..95e955f 100644
--- a/include/sound/compress_driver.h
+++ b/include/sound/compress_driver.h
@@ -110,6 +110,10 @@ struct snd_compr_ops {
struct snd_compr_params *params);
int (*get_params)(struct snd_compr_stream *stream,
struct snd_codec *params);
+ int (*set_metadata)(struct snd_compr_stream *stream,
+ struct snd_compr_metadata *metadata);
+ int (*get_metadata)(struct snd_compr_stream *stream,
+ struct snd_compr_metadata *metadata);
int (*trigger)(struct snd_compr_stream *stream, int cmd);
int (*pointer)(struct snd_compr_stream *stream,
struct snd_compr_tstamp *tstamp);
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index 1774a5c..88a6770 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -271,7 +271,9 @@ typedef int __bitwise snd_pcm_state_t;
#define SNDRV_PCM_STATE_PAUSED ((__force snd_pcm_state_t) 6) /* stream is paused */
#define SNDRV_PCM_STATE_SUSPENDED ((__force snd_pcm_state_t) 7) /* hardware is suspended */
#define SNDRV_PCM_STATE_DISCONNECTED ((__force snd_pcm_state_t) 8) /* hardware is disconnected */
-#define SNDRV_PCM_STATE_LAST SNDRV_PCM_STATE_DISCONNECTED
+#define SNDRV_PCM_STATE_NEXT_TRACK ((__force snd_pcm_state_t) 9) /* stream will move to next track */
+#define SNDRV_PCM_STATE_PARTIAL_DRAIN ((__force snd_pcm_state_t) 10) /* stream is draining partially */
+#define SNDRV_PCM_STATE_LAST SNDRV_PCM_STATE_PARTIAL_DRAIN
enum {
SNDRV_PCM_MMAP_OFFSET_DATA = 0x00000000,
diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h
index 05341a4..1d63993 100644
--- a/include/uapi/sound/compress_offload.h
+++ b/include/uapi/sound/compress_offload.h
@@ -30,7 +30,7 @@
#include <sound/compress_params.h>
-#define SNDRV_COMPRESS_VERSION SNDRV_PROTOCOL_VERSION(0, 1, 0)
+#define SNDRV_COMPRESS_VERSION SNDRV_PROTOCOL_VERSION(0, 1, 1)
/**
* struct snd_compressed_buffer: compressed buffer
* @fragment_size: size of buffer fragment in bytes
@@ -121,6 +121,32 @@ struct snd_compr_codec_caps {
struct snd_codec_desc descriptor[MAX_NUM_CODEC_DESCRIPTORS];
};
+enum {
+ SNDRV_COMPRESS_ENCODER_PADDING = 1,
+ SNDRV_COMPRESS_ENCODER_DELAY = 2,
+};
+
+/**
+ * struct snd_compr_keyvalue: compressed stream key/value pairs
+ * @key: key id
+ * @value: key value
+ */
+
+struct snd_compr_keyvalue {
+ __u32 key;
+ __u32 value;
+};
+
+/**
+ * struct snd_compr_metadata: compressed stream metadata
+ * @count: number of keys
+ * @keys: pointer to count keys
+ */
+struct snd_compr_metadata {
+ __u32 count;
+ struct snd_compr_keyvalue *keys;
+};
+
/**
* compress path ioctl definitions
* SNDRV_COMPRESS_GET_CAPS: Query capability of DSP
@@ -145,6 +171,10 @@ struct snd_compr_codec_caps {
struct snd_compr_codec_caps)
#define SNDRV_COMPRESS_SET_PARAMS _IOW('C', 0x12, struct snd_compr_params)
#define SNDRV_COMPRESS_GET_PARAMS _IOR('C', 0x13, struct snd_codec)
+#define SNDRV_COMPRESS_SET_METADATA _IOW('C', 0x14,\
+ struct snd_compr_metadata)
+#define SNDRV_COMPRESS_GET_METADATA _IOW('C', 0x15,\
+ struct snd_compr_metadata)
#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)
@@ -152,10 +182,14 @@ struct snd_compr_codec_caps {
#define SNDRV_COMPRESS_START _IO('C', 0x32)
#define SNDRV_COMPRESS_STOP _IO('C', 0x33)
#define SNDRV_COMPRESS_DRAIN _IO('C', 0x34)
+#define SNDRV_COMPRESS_NEXT_TRACK _IO('C', 0x35)
+#define SNDRV_COMPRESS_PARTIAL_DRAIN _IO('C', 0x36)
/*
* TODO
* 1. add mmap support
*
*/
#define SND_COMPR_TRIGGER_DRAIN 7 /*FIXME move this to pcm.h */
+#define SND_COMPR_TRIGGER_NEXT_TRACK 8
+#define SND_COMPR_TRIGGER_PARTIAL_DRAIN 9
#endif
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index ad11dc9..4928209 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -262,9 +262,14 @@ static ssize_t snd_compr_write(struct file *f, const char __user *buf,
stream = &data->stream;
mutex_lock(&stream->device->lock);
- /* write is allowed when stream is running or has been steup */
+ /* write is allowed when stream is running or has been steup
+ * also stream cna be written when next_track info has been setup
+ * or its has partially drained
+ */
if (stream->runtime->state != SNDRV_PCM_STATE_SETUP &&
- stream->runtime->state != SNDRV_PCM_STATE_RUNNING) {
+ stream->runtime->state != SNDRV_PCM_STATE_RUNNING &&
+ stream->runtime->state != SNDRV_PCM_STATE_NEXT_TRACK &&
+ stream->runtime->state != SNDRV_PCM_STATE_PARTIAL_DRAIN) {
mutex_unlock(&stream->device->lock);
return -EBADFD;
}
@@ -288,6 +293,9 @@ static ssize_t snd_compr_write(struct file *f, const char __user *buf,
stream->runtime->state = SNDRV_PCM_STATE_PREPARED;
pr_debug("stream prepared, Houston we are good to go\n");
}
+ if (stream->runtime->state == SNDRV_PCM_STATE_NEXT_TRACK ||
+ stream->runtime->state == SNDRV_PCM_STATE_PARTIAL_DRAIN)
+ stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
mutex_unlock(&stream->device->lock);
return retval;
@@ -514,6 +522,78 @@ out:
return retval;
}
+static int
+snd_compr_copy_metadata(struct snd_compr_metadata **arg, unsigned long user)
+{
+ struct snd_compr_metadata _mdata, *mdata;
+ int len;
+
+ if (copy_from_user(&_mdata, (void __user *)user,
+ sizeof(_mdata)))
+ return -EFAULT;
+
+ len = sizeof(_mdata.count) + _mdata.count * sizeof(*_mdata.keys);
+
+ mdata = kmalloc(len, GFP_KERNEL);
+ if (!mdata)
+ return -ENOMEM;
+
+ if (copy_from_user(mdata, (void __user *)arg, len)) {
+ kfree(mdata);
+ return -EFAULT;
+ }
+ *arg = mdata;
+ return len;
+}
+
+static int
+snd_compr_set_metadata(struct snd_compr_stream *stream, unsigned long arg)
+{
+ struct snd_compr_metadata *mdata;
+ int retval;
+
+ if (!stream->ops->set_metadata)
+ return -ENXIO;
+ /*
+ * we should allow parameter change only when stream has been
+ * opened not in other cases
+ */
+ retval = snd_compr_copy_metadata(&mdata, arg);
+ if (retval <= 0)
+ return retval;
+
+ retval = stream->ops->set_metadata(stream, mdata);
+
+ kfree(mdata);
+ return retval;
+}
+
+static int
+snd_compr_get_metadata(struct snd_compr_stream *stream, unsigned long arg)
+{
+ struct snd_compr_metadata *mdata;
+ int retval, len;
+
+ if (!stream->ops->get_metadata)
+ return -ENXIO;
+
+ len = snd_compr_copy_metadata(&mdata, arg);
+ if (len <= 0)
+ return len;
+ retval = stream->ops->get_metadata(stream, mdata);
+ if (retval != 0)
+ goto out;
+
+ if (copy_to_user((void __user *)arg, mdata, len)) {
+ retval = -EFAULT;
+ goto out;
+ }
+
+out:
+ kfree(mdata);
+ return retval;
+}
+
static inline int
snd_compr_tstamp(struct snd_compr_stream *stream, unsigned long arg)
{
@@ -594,6 +674,38 @@ static int snd_compr_drain(struct snd_compr_stream *stream)
return retval;
}
+static int snd_compr_partial_drain(struct snd_compr_stream *stream)
+{
+ int retval;
+
+ /* we can partially drain streams, only when next track info has been
+ * passed to the dsp
+ */
+ if (stream->runtime->state != SNDRV_PCM_STATE_NEXT_TRACK)
+ return -EPERM;
+
+ retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_PARTIAL_DRAIN);
+ if (retval != 0)
+ return retval;
+ stream->runtime->state = SNDRV_PCM_STATE_PARTIAL_DRAIN;
+ return 0;
+}
+
+static int snd_compr_next_track(struct snd_compr_stream *stream)
+{
+ int retval;
+
+ /* only a running stream can transition to next track */
+ if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING)
+ return -EPERM;
+
+ retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_NEXT_TRACK);
+ if (retval != 0)
+ return retval;
+ stream->runtime->state = SNDRV_PCM_STATE_NEXT_TRACK;
+ return 0;
+}
+
static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
{
struct snd_compr_file *data = f->private_data;
@@ -623,6 +735,12 @@ 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_METADATA):
+ retval = snd_compr_set_metadata(stream, arg);
+ break;
+ case _IOC_NR(SNDRV_COMPRESS_GET_METADATA):
+ retval = snd_compr_get_metadata(stream, arg);
+ break;
case _IOC_NR(SNDRV_COMPRESS_TSTAMP):
retval = snd_compr_tstamp(stream, arg);
break;
@@ -644,6 +762,13 @@ static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
case _IOC_NR(SNDRV_COMPRESS_DRAIN):
retval = snd_compr_drain(stream);
break;
+ case _IOC_NR(SNDRV_COMPRESS_PARTIAL_DRAIN):
+ retval = snd_compr_partial_drain(stream);
+ break;
+ case _IOC_NR(SNDRV_COMPRESS_NEXT_TRACK):
+ retval = snd_compr_next_track(stream);
+ break;
+
}
mutex_unlock(&stream->device->lock);
return retval;
--
1.7.0.4
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH] compress: add support for gapless playback
2013-02-11 12:22 Vinod Koul
@ 2013-02-11 13:41 ` Takashi Iwai
2013-02-11 13:41 ` Vinod Koul
0 siblings, 1 reply; 25+ messages in thread
From: Takashi Iwai @ 2013-02-11 13:41 UTC (permalink / raw)
To: Vinod Koul; +Cc: Jeeja KP, alsa-devel, broonie, liam.r.girdwood
At Mon, 11 Feb 2013 04:22:45 -0800,
Vinod Koul wrote:
>
(snip)
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -271,7 +271,9 @@ typedef int __bitwise snd_pcm_state_t;
> #define SNDRV_PCM_STATE_PAUSED ((__force snd_pcm_state_t) 6) /* stream is paused */
> #define SNDRV_PCM_STATE_SUSPENDED ((__force snd_pcm_state_t) 7) /* hardware is suspended */
> #define SNDRV_PCM_STATE_DISCONNECTED ((__force snd_pcm_state_t) 8) /* hardware is disconnected */
> -#define SNDRV_PCM_STATE_LAST SNDRV_PCM_STATE_DISCONNECTED
> +#define SNDRV_PCM_STATE_NEXT_TRACK ((__force snd_pcm_state_t) 9) /* stream will move to next track */
Isn't this better to be set as another flag instead of the trigger
command? This changes the runtime->state, and it may make things
complicated. For example, what happens if user triggers NEXT_TRACK,
then PAUSE_PUSH and PAUSE_RELEASE?
> +#define SNDRV_PCM_STATE_PARTIAL_DRAIN ((__force snd_pcm_state_t) 10) /* stream is draining partially */
> +#define SNDRV_PCM_STATE_LAST SNDRV_PCM_STATE_PARTIAL_DRAIN
Well, I'd hesitate to add this for PCM, since there is no counterpart
for PCM (yet). Until then, let's avoid touching the PCM API
definition but only compress API.
> enum {
> SNDRV_PCM_MMAP_OFFSET_DATA = 0x00000000,
> diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h
> index 05341a4..1d63993 100644
> --- a/include/uapi/sound/compress_offload.h
> +++ b/include/uapi/sound/compress_offload.h
> @@ -30,7 +30,7 @@
> #include <sound/compress_params.h>
>
>
> -#define SNDRV_COMPRESS_VERSION SNDRV_PROTOCOL_VERSION(0, 1, 0)
> +#define SNDRV_COMPRESS_VERSION SNDRV_PROTOCOL_VERSION(0, 1, 1)
> /**
> * struct snd_compressed_buffer: compressed buffer
> * @fragment_size: size of buffer fragment in bytes
> @@ -121,6 +121,32 @@ struct snd_compr_codec_caps {
> struct snd_codec_desc descriptor[MAX_NUM_CODEC_DESCRIPTORS];
> };
>
> +enum {
> + SNDRV_COMPRESS_ENCODER_PADDING = 1,
> + SNDRV_COMPRESS_ENCODER_DELAY = 2,
> +};
> +
> +/**
> + * struct snd_compr_keyvalue: compressed stream key/value pairs
> + * @key: key id
> + * @value: key value
> + */
> +
> +struct snd_compr_keyvalue {
> + __u32 key;
> + __u32 value;
> +};
> +
> +/**
> + * struct snd_compr_metadata: compressed stream metadata
> + * @count: number of keys
> + * @keys: pointer to count keys
> + */
> +struct snd_compr_metadata {
> + __u32 count;
> + struct snd_compr_keyvalue *keys;
> +};
Do you really want that? Then you'll have to provide 32/64bit
ioctl conversion functions as well. I wouldn't take that mess but let
user repeat single key/value ioctls.
Instead of relying completely on the driver side implementation, you
can keep a metadata array in snd_compr_stream or snd_compr
internally. Then the ioctl handler would be just like:
static int compr_stream_set_metadata(struct snd_compr_stream *stream, void __user * arg)
{
struct snd_compr_keyvalue kv;
if (!stream->ops->set_metadata)
return -ENXIO;
if (copy_from_user(&kv, arg, sizeof(kv)))
return -EFAULT;
if (kv > SNDRV_COMPRESS_METADATA_LAST)
return -EINVAL;
stream->metadata[kv.key] = kv.value;
return stream->ops->set_metadata(stream);
}
As a bonus, we can reduce even get_metadata ops implementation in each
driver:
static int compr_stream_get_metadata(struct snd_compr_stream *stream, void __user * arg)
{
if (!stream->ops->set_metadata)
return -ENXIO;
if (copy_from_user(&kv, arg, sizeof(kv)))
return -EFAULT;
if (kv > SNDRV_COMPRESS_METADATA_LAST)
return -EINVAL;
kv.value = stream->metadata[kv.key];
if (copy_to_user(arg, &kv, sizeof(kv))
return -EFAULT;
return 0;
}
Of course, this assumes that stream->metadata[] is initialized at each
open properly.
Takashi
> +
> /**
> * compress path ioctl definitions
> * SNDRV_COMPRESS_GET_CAPS: Query capability of DSP
> @@ -145,6 +171,10 @@ struct snd_compr_codec_caps {
> struct snd_compr_codec_caps)
> #define SNDRV_COMPRESS_SET_PARAMS _IOW('C', 0x12, struct snd_compr_params)
> #define SNDRV_COMPRESS_GET_PARAMS _IOR('C', 0x13, struct snd_codec)
> +#define SNDRV_COMPRESS_SET_METADATA _IOW('C', 0x14,\
> + struct snd_compr_metadata)
> +#define SNDRV_COMPRESS_GET_METADATA _IOW('C', 0x15,\
> + struct snd_compr_metadata)
> #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)
> @@ -152,10 +182,14 @@ struct snd_compr_codec_caps {
> #define SNDRV_COMPRESS_START _IO('C', 0x32)
> #define SNDRV_COMPRESS_STOP _IO('C', 0x33)
> #define SNDRV_COMPRESS_DRAIN _IO('C', 0x34)
> +#define SNDRV_COMPRESS_NEXT_TRACK _IO('C', 0x35)
> +#define SNDRV_COMPRESS_PARTIAL_DRAIN _IO('C', 0x36)
> /*
> * TODO
> * 1. add mmap support
> *
> */
> #define SND_COMPR_TRIGGER_DRAIN 7 /*FIXME move this to pcm.h */
> +#define SND_COMPR_TRIGGER_NEXT_TRACK 8
> +#define SND_COMPR_TRIGGER_PARTIAL_DRAIN 9
> #endif
> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> index ad11dc9..4928209 100644
> --- a/sound/core/compress_offload.c
> +++ b/sound/core/compress_offload.c
> @@ -262,9 +262,14 @@ static ssize_t snd_compr_write(struct file *f, const char __user *buf,
>
> stream = &data->stream;
> mutex_lock(&stream->device->lock);
> - /* write is allowed when stream is running or has been steup */
> + /* write is allowed when stream is running or has been steup
> + * also stream cna be written when next_track info has been setup
> + * or its has partially drained
> + */
> if (stream->runtime->state != SNDRV_PCM_STATE_SETUP &&
> - stream->runtime->state != SNDRV_PCM_STATE_RUNNING) {
> + stream->runtime->state != SNDRV_PCM_STATE_RUNNING &&
> + stream->runtime->state != SNDRV_PCM_STATE_NEXT_TRACK &&
> + stream->runtime->state != SNDRV_PCM_STATE_PARTIAL_DRAIN) {
> mutex_unlock(&stream->device->lock);
> return -EBADFD;
> }
> @@ -288,6 +293,9 @@ static ssize_t snd_compr_write(struct file *f, const char __user *buf,
> stream->runtime->state = SNDRV_PCM_STATE_PREPARED;
> pr_debug("stream prepared, Houston we are good to go\n");
> }
> + if (stream->runtime->state == SNDRV_PCM_STATE_NEXT_TRACK ||
> + stream->runtime->state == SNDRV_PCM_STATE_PARTIAL_DRAIN)
> + stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
>
> mutex_unlock(&stream->device->lock);
> return retval;
> @@ -514,6 +522,78 @@ out:
> return retval;
> }
>
> +static int
> +snd_compr_copy_metadata(struct snd_compr_metadata **arg, unsigned long user)
> +{
> + struct snd_compr_metadata _mdata, *mdata;
> + int len;
> +
> + if (copy_from_user(&_mdata, (void __user *)user,
> + sizeof(_mdata)))
> + return -EFAULT;
> +
> + len = sizeof(_mdata.count) + _mdata.count * sizeof(*_mdata.keys);
> +
> + mdata = kmalloc(len, GFP_KERNEL);
> + if (!mdata)
> + return -ENOMEM;
> +
> + if (copy_from_user(mdata, (void __user *)arg, len)) {
> + kfree(mdata);
> + return -EFAULT;
> + }
> + *arg = mdata;
> + return len;
> +}
> +
> +static int
> +snd_compr_set_metadata(struct snd_compr_stream *stream, unsigned long arg)
> +{
> + struct snd_compr_metadata *mdata;
> + int retval;
> +
> + if (!stream->ops->set_metadata)
> + return -ENXIO;
> + /*
> + * we should allow parameter change only when stream has been
> + * opened not in other cases
> + */
> + retval = snd_compr_copy_metadata(&mdata, arg);
> + if (retval <= 0)
> + return retval;
> +
> + retval = stream->ops->set_metadata(stream, mdata);
> +
> + kfree(mdata);
> + return retval;
> +}
> +
> +static int
> +snd_compr_get_metadata(struct snd_compr_stream *stream, unsigned long arg)
> +{
> + struct snd_compr_metadata *mdata;
> + int retval, len;
> +
> + if (!stream->ops->get_metadata)
> + return -ENXIO;
> +
> + len = snd_compr_copy_metadata(&mdata, arg);
> + if (len <= 0)
> + return len;
> + retval = stream->ops->get_metadata(stream, mdata);
> + if (retval != 0)
> + goto out;
> +
> + if (copy_to_user((void __user *)arg, mdata, len)) {
> + retval = -EFAULT;
> + goto out;
> + }
> +
> +out:
> + kfree(mdata);
> + return retval;
> +}
> +
> static inline int
> snd_compr_tstamp(struct snd_compr_stream *stream, unsigned long arg)
> {
> @@ -594,6 +674,38 @@ static int snd_compr_drain(struct snd_compr_stream *stream)
> return retval;
> }
>
> +static int snd_compr_partial_drain(struct snd_compr_stream *stream)
> +{
> + int retval;
> +
> + /* we can partially drain streams, only when next track info has been
> + * passed to the dsp
> + */
> + if (stream->runtime->state != SNDRV_PCM_STATE_NEXT_TRACK)
> + return -EPERM;
> +
> + retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_PARTIAL_DRAIN);
> + if (retval != 0)
> + return retval;
> + stream->runtime->state = SNDRV_PCM_STATE_PARTIAL_DRAIN;
> + return 0;
> +}
> +
> +static int snd_compr_next_track(struct snd_compr_stream *stream)
> +{
> + int retval;
> +
> + /* only a running stream can transition to next track */
> + if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING)
> + return -EPERM;
> +
> + retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_NEXT_TRACK);
> + if (retval != 0)
> + return retval;
> + stream->runtime->state = SNDRV_PCM_STATE_NEXT_TRACK;
> + return 0;
> +}
> +
> static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
> {
> struct snd_compr_file *data = f->private_data;
> @@ -623,6 +735,12 @@ 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_METADATA):
> + retval = snd_compr_set_metadata(stream, arg);
> + break;
> + case _IOC_NR(SNDRV_COMPRESS_GET_METADATA):
> + retval = snd_compr_get_metadata(stream, arg);
> + break;
> case _IOC_NR(SNDRV_COMPRESS_TSTAMP):
> retval = snd_compr_tstamp(stream, arg);
> break;
> @@ -644,6 +762,13 @@ static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
> case _IOC_NR(SNDRV_COMPRESS_DRAIN):
> retval = snd_compr_drain(stream);
> break;
> + case _IOC_NR(SNDRV_COMPRESS_PARTIAL_DRAIN):
> + retval = snd_compr_partial_drain(stream);
> + break;
> + case _IOC_NR(SNDRV_COMPRESS_NEXT_TRACK):
> + retval = snd_compr_next_track(stream);
> + break;
> +
> }
> mutex_unlock(&stream->device->lock);
> return retval;
> --
> 1.7.0.4
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] compress: add support for gapless playback
2013-02-11 13:41 ` Takashi Iwai
@ 2013-02-11 13:41 ` Vinod Koul
2013-02-11 14:38 ` Takashi Iwai
0 siblings, 1 reply; 25+ messages in thread
From: Vinod Koul @ 2013-02-11 13:41 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Jeeja KP, alsa-devel, broonie, liam.r.girdwood
On Mon, Feb 11, 2013 at 02:41:18PM +0100, Takashi Iwai wrote:
> At Mon, 11 Feb 2013 04:22:45 -0800,
> Vinod Koul wrote:
> >
> (snip)
> > --- a/include/uapi/sound/asound.h
> > +++ b/include/uapi/sound/asound.h
> > @@ -271,7 +271,9 @@ typedef int __bitwise snd_pcm_state_t;
> > #define SNDRV_PCM_STATE_PAUSED ((__force snd_pcm_state_t) 6) /* stream is paused */
> > #define SNDRV_PCM_STATE_SUSPENDED ((__force snd_pcm_state_t) 7) /* hardware is suspended */
> > #define SNDRV_PCM_STATE_DISCONNECTED ((__force snd_pcm_state_t) 8) /* hardware is disconnected */
> > -#define SNDRV_PCM_STATE_LAST SNDRV_PCM_STATE_DISCONNECTED
> > +#define SNDRV_PCM_STATE_NEXT_TRACK ((__force snd_pcm_state_t) 9) /* stream will move to next track */
>
> Isn't this better to be set as another flag instead of the trigger
> command? This changes the runtime->state, and it may make things
> complicated. For example, what happens if user triggers NEXT_TRACK,
> then PAUSE_PUSH and PAUSE_RELEASE?
hmmm, that makes sense. Also it would make things much easier on all the
transistions
>
> > +#define SNDRV_PCM_STATE_PARTIAL_DRAIN ((__force snd_pcm_state_t) 10) /* stream is draining partially */
> > +#define SNDRV_PCM_STATE_LAST SNDRV_PCM_STATE_PARTIAL_DRAIN
>
> Well, I'd hesitate to add this for PCM, since there is no counterpart
> for PCM (yet). Until then, let's avoid touching the PCM API
> definition but only compress API.
If we do above then we may not need this :)
> > +/**
> > + * struct snd_compr_metadata: compressed stream metadata
> > + * @count: number of keys
> > + * @keys: pointer to count keys
> > + */
> > +struct snd_compr_metadata {
> > + __u32 count;
> > + struct snd_compr_keyvalue *keys;
> > +};
>
> Do you really want that? Then you'll have to provide 32/64bit
> ioctl conversion functions as well. I wouldn't take that mess but let
> user repeat single key/value ioctls.
Ah, i forgot the damn pointer :-)
>
> Instead of relying completely on the driver side implementation, you
> can keep a metadata array in snd_compr_stream or snd_compr
> internally. Then the ioctl handler would be just like:
how do we decide the size of array and what if we have more params in future
> static int compr_stream_set_metadata(struct snd_compr_stream *stream, void __user * arg)
> {
> struct snd_compr_keyvalue kv;
>
> if (!stream->ops->set_metadata)
> return -ENXIO;
> if (copy_from_user(&kv, arg, sizeof(kv)))
> return -EFAULT;
> if (kv > SNDRV_COMPRESS_METADATA_LAST)
> return -EINVAL;
> stream->metadata[kv.key] = kv.value;
> return stream->ops->set_metadata(stream);
> }
And when do we clear this? metadata for gapless, though a same stream session
gets updated for every track.
Right now I am inlcined to put a single value, and be done with it or update
older structs with bunch of padding for future use :)
>
> Of course, this assumes that stream->metadata[] is initialized at each
> open properly.
Yes and it cant be cleared later for next track, perhpas we clear it when we get
NEXT_TRACK signalled from user space?
--
~Vinod
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] compress: add support for gapless playback
2013-02-11 13:41 ` Vinod Koul
@ 2013-02-11 14:38 ` Takashi Iwai
2013-02-11 14:55 ` Vinod Koul
0 siblings, 1 reply; 25+ messages in thread
From: Takashi Iwai @ 2013-02-11 14:38 UTC (permalink / raw)
To: Vinod Koul; +Cc: Jeeja KP, alsa-devel, broonie, liam.r.girdwood
At Mon, 11 Feb 2013 05:41:47 -0800,
Vinod Koul wrote:
>
> On Mon, Feb 11, 2013 at 02:41:18PM +0100, Takashi Iwai wrote:
> > At Mon, 11 Feb 2013 04:22:45 -0800,
> > Vinod Koul wrote:
> > >
> > (snip)
> > > --- a/include/uapi/sound/asound.h
> > > +++ b/include/uapi/sound/asound.h
> > > @@ -271,7 +271,9 @@ typedef int __bitwise snd_pcm_state_t;
> > > #define SNDRV_PCM_STATE_PAUSED ((__force snd_pcm_state_t) 6) /* stream is paused */
> > > #define SNDRV_PCM_STATE_SUSPENDED ((__force snd_pcm_state_t) 7) /* hardware is suspended */
> > > #define SNDRV_PCM_STATE_DISCONNECTED ((__force snd_pcm_state_t) 8) /* hardware is disconnected */
> > > -#define SNDRV_PCM_STATE_LAST SNDRV_PCM_STATE_DISCONNECTED
> > > +#define SNDRV_PCM_STATE_NEXT_TRACK ((__force snd_pcm_state_t) 9) /* stream will move to next track */
> >
> > Isn't this better to be set as another flag instead of the trigger
> > command? This changes the runtime->state, and it may make things
> > complicated. For example, what happens if user triggers NEXT_TRACK,
> > then PAUSE_PUSH and PAUSE_RELEASE?
> hmmm, that makes sense. Also it would make things much easier on all the
> transistions
>
> >
> > > +#define SNDRV_PCM_STATE_PARTIAL_DRAIN ((__force snd_pcm_state_t) 10) /* stream is draining partially */
> > > +#define SNDRV_PCM_STATE_LAST SNDRV_PCM_STATE_PARTIAL_DRAIN
> >
> > Well, I'd hesitate to add this for PCM, since there is no counterpart
> > for PCM (yet). Until then, let's avoid touching the PCM API
> > definition but only compress API.
> If we do above then we may not need this :)
>
> > > +/**
> > > + * struct snd_compr_metadata: compressed stream metadata
> > > + * @count: number of keys
> > > + * @keys: pointer to count keys
> > > + */
> > > +struct snd_compr_metadata {
> > > + __u32 count;
> > > + struct snd_compr_keyvalue *keys;
> > > +};
> >
> > Do you really want that? Then you'll have to provide 32/64bit
> > ioctl conversion functions as well. I wouldn't take that mess but let
> > user repeat single key/value ioctls.
> Ah, i forgot the damn pointer :-)
> >
> > Instead of relying completely on the driver side implementation, you
> > can keep a metadata array in snd_compr_stream or snd_compr
> > internally. Then the ioctl handler would be just like:
> how do we decide the size of array and what if we have more params in future
The point is that it's internal, so we can extend at any time if we
add more parameter definitions.
> > static int compr_stream_set_metadata(struct snd_compr_stream *stream, void __user * arg)
> > {
> > struct snd_compr_keyvalue kv;
> >
> > if (!stream->ops->set_metadata)
> > return -ENXIO;
> > if (copy_from_user(&kv, arg, sizeof(kv)))
> > return -EFAULT;
> > if (kv > SNDRV_COMPRESS_METADATA_LAST)
> > return -EINVAL;
> > stream->metadata[kv.key] = kv.value;
> > return stream->ops->set_metadata(stream);
> > }
> And when do we clear this? metadata for gapless, though a same stream session
> gets updated for every track.
>
> Right now I am inlcined to put a single value, and be done with it or update
> older structs with bunch of padding for future use :)
> >
> > Of course, this assumes that stream->metadata[] is initialized at each
> > open properly.
> Yes and it cant be cleared later for next track, perhpas we clear it when we get
> NEXT_TRACK signalled from user space?
Well, the question is how this value change is supposed. I guess it's
not defined precisely yet.
It'd make sense to clear padding or such automatically at each new
stream switch, indeed. OTOH, what if a metadata is supposed to be
kept for all tracks (e.g. something like the playback speed control)?
So, this won't be only a question about bookkeeping in the compress
core, but rather an API question. If all metadata are supposed to be
cleared at stream change, it's fine -- the refresh of all metadata by
user-space is mandatory. OTOH, if the metadata are never changed
unless the stream is (re-)prepared, again, user-space has to manage
all metadata by itself. There is no big difference between them.
The difference is what happens if user-space doesn't update.
If the behavior is defined per each parameter, maybe it's good for
user-space, but then the driver (or comr core) becomes slightly more
complex since it needs to clear some data selectively.
Takashi
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] compress: add support for gapless playback
2013-02-11 14:38 ` Takashi Iwai
@ 2013-02-11 14:55 ` Vinod Koul
2013-02-11 15:30 ` Takashi Iwai
0 siblings, 1 reply; 25+ messages in thread
From: Vinod Koul @ 2013-02-11 14:55 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Jeeja KP, alsa-devel, broonie, liam.r.girdwood
On Mon, Feb 11, 2013 at 03:38:32PM +0100, Takashi Iwai wrote:
> Well, the question is how this value change is supposed. I guess it's
> not defined precisely yet.
>
> It'd make sense to clear padding or such automatically at each new
> stream switch, indeed. OTOH, what if a metadata is supposed to be
> kept for all tracks (e.g. something like the playback speed control)?
>
> So, this won't be only a question about bookkeeping in the compress
> core, but rather an API question. If all metadata are supposed to be
> cleared at stream change, it's fine -- the refresh of all metadata by
> user-space is mandatory. OTOH, if the metadata are never changed
> unless the stream is (re-)prepared, again, user-space has to manage
> all metadata by itself. There is no big difference between them.
> The difference is what happens if user-space doesn't update.
>
> If the behavior is defined per each parameter, maybe it's good for
> user-space, but then the driver (or comr core) becomes slightly more
> complex since it needs to clear some data selectively.
Current ones would be changed for every track, but then some wouldnt.
So its best left to userspace. So right now we have two things:
- add single key/value only
- or pad up struct.
I am okay with either :-)
--
~Vinod
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] compress: add support for gapless playback
2013-02-11 14:55 ` Vinod Koul
@ 2013-02-11 15:30 ` Takashi Iwai
0 siblings, 0 replies; 25+ messages in thread
From: Takashi Iwai @ 2013-02-11 15:30 UTC (permalink / raw)
To: Vinod Koul; +Cc: Jeeja KP, alsa-devel, broonie, liam.r.girdwood
At Mon, 11 Feb 2013 06:55:58 -0800,
Vinod Koul wrote:
>
> On Mon, Feb 11, 2013 at 03:38:32PM +0100, Takashi Iwai wrote:
> > Well, the question is how this value change is supposed. I guess it's
> > not defined precisely yet.
> >
> > It'd make sense to clear padding or such automatically at each new
> > stream switch, indeed. OTOH, what if a metadata is supposed to be
> > kept for all tracks (e.g. something like the playback speed control)?
> >
> > So, this won't be only a question about bookkeeping in the compress
> > core, but rather an API question. If all metadata are supposed to be
> > cleared at stream change, it's fine -- the refresh of all metadata by
> > user-space is mandatory. OTOH, if the metadata are never changed
> > unless the stream is (re-)prepared, again, user-space has to manage
> > all metadata by itself. There is no big difference between them.
> > The difference is what happens if user-space doesn't update.
> >
> > If the behavior is defined per each parameter, maybe it's good for
> > user-space, but then the driver (or comr core) becomes slightly more
> > complex since it needs to clear some data selectively.
> Current ones would be changed for every track, but then some wouldnt.
> So its best left to userspace. So right now we have two things:
> - add single key/value only
> - or pad up struct.
The implementation of ioctl is basically orthogonal with the question
whether the driver implicitly clears the parameters or not...
Takashi
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2013-02-14 13:38 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-02-12 18:31 [PATCH] compress: add support for gapless playback Vinod Koul
2013-02-13 6:23 ` Takashi Iwai
2013-02-13 6:35 ` Vinod Koul
2013-02-13 7:37 ` Takashi Iwai
2013-02-13 11:34 ` Mark Brown
2013-02-13 13:39 ` Vinod Koul
-- strict thread matches above, loose matches on Subject: below --
2013-02-14 11:22 Vinod Koul
2013-02-14 11:31 ` Takashi Iwai
2013-02-14 13:37 ` Vinod Koul
2013-02-14 8:42 Vinod Koul
2013-02-14 9:13 ` Takashi Iwai
2013-02-14 9:32 ` Vinod Koul
2013-02-14 9:45 ` Takashi Iwai
2013-02-14 11:06 ` Vinod Koul
2013-02-13 16:00 Vinod Koul
2013-02-13 16:40 ` Takashi Iwai
2013-02-13 17:03 ` Vinod Koul
2013-02-13 17:15 ` Takashi Iwai
2013-02-13 17:21 ` Vinod Koul
2013-02-11 12:22 Vinod Koul
2013-02-11 13:41 ` Takashi Iwai
2013-02-11 13:41 ` Vinod Koul
2013-02-11 14:38 ` Takashi Iwai
2013-02-11 14:55 ` Vinod Koul
2013-02-11 15:30 ` Takashi Iwai
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).