[RFC] compress: add support for gapless playback

[RFC] compress: add support for gapless playback

[Vinod Koul]

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>
---
 Documentation/sound/alsa/compress_offload.txt |   22 +++++++++++
 include/sound/compress_driver.h               |    2 +
 include/uapi/sound/compress_offload.h         |   18 ++++++++-
 sound/core/compress_offload.c                 |   51 +++++++++++++++++++++++++
 4 files changed, 92 insertions(+), 1 deletions(-)

diff --git a/Documentation/sound/alsa/compress_offload.txt b/Documentation/sound/alsa/compress_offload.txt
index 90e9b3a..071c4b6 100644
--- a/Documentation/sound/alsa/compress_offload.txt
+++ b/Documentation/sound/alsa/compress_offload.txt
@@ -145,6 +145,28 @@ 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
+
+The decoder needs to know the encoder delay and encoder padding. So we need to
+pass this to 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
+
+- 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
+
 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..42d6d7c 100644
--- a/include/sound/compress_driver.h
+++ b/include/sound/compress_driver.h
@@ -110,6 +110,8 @@ 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 (*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..65ca2f1 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,18 @@ 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;
+};
+
+/**
  * compress path ioctl definitions
  * SNDRV_COMPRESS_GET_CAPS: Query capability of DSP
  * SNDRV_COMPRESS_GET_CODEC_CAPS: Query capability of a codec
@@ -145,6 +157,8 @@ 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_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 +166,12 @@ 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_PARTIAL_DRAIN	_IO('C', 0x35)
 /*
  * TODO
  * 1. add mmap support
  *
  */
 #define SND_COMPR_TRIGGER_DRAIN 7 /*FIXME move this to pcm.h */
+#define SND_COMPR_TRIGGER_PARTIAL_DRAIN 8
 #endif
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index ad11dc9..2928971 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -514,6 +514,37 @@ out:
 	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);
+
+out:
+	kfree(metadata);
+	return retval;
+}
+
 static inline int
 snd_compr_tstamp(struct snd_compr_stream *stream, unsigned long arg)
 {
@@ -594,6 +625,20 @@ 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;
+	if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
+			stream->runtime->state == SNDRV_PCM_STATE_SETUP)
+		return -EPERM;
+	retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_PARTIAL_DRAIN);
+	if (retval)
+		pr_err("Partial drain returned failure\n");
+	else
+		stream->runtime->state = SNDRV_PCM_STATE_SETUP;
+	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 +668,9 @@ static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
 	case _IOC_NR(SNDRV_COMPRESS_GET_PARAMS):
 		retval = snd_compr_get_params(stream, arg);
 		break;
+	case _IOC_NR(SNDRV_COMPRESS_SET_METADATA):
+		retval = snd_compr_set_metadata(stream, arg);
+		break;
 	case _IOC_NR(SNDRV_COMPRESS_TSTAMP):
 		retval = snd_compr_tstamp(stream, arg);
 		break;
@@ -644,6 +692,9 @@ 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;
 	}
 	mutex_unlock(&stream->device->lock);
 	return retval;
-- 
1.7.0.4

Re: [RFC] compress: add support for gapless playback

[Pierre-Louis Bossart]

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

You want to clarify that 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.
Also you want to mention that 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.

> +- set_metadata
> +This routine sets the encoder delay and encoder padding. This can be used by
> +decoder to strip the silence

You did not specify that this needs to be set before the first write of 
the next 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

DSP skips padding delay after decoding all samples in the track, not as 
soon as EOF is reached.
A sequence as seen from userspace might be useful here to avoid random 
interpretations of the API semantics.

>   /**
> + * 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;
> +};

You need to pad this structure with reserved bytes for future 
evolutions. Things like ReplayGain, etc, might be of interest for some 
implementations. Let's not be cheap here


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

Is this really a fatal error? Or do we want to mandate that gapless be 
supported by all implementations?

Re: [RFC] compress: add support for gapless playback

[Vinod Koul]

On Tue, Feb 05, 2013 at 08:44:42PM -0600, Pierre-Louis Bossart wrote:

 
> >  /**
> >+ * 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;
> >+};
> 
> You need to pad this structure with reserved bytes for future
> evolutions. Things like ReplayGain, etc, might be of interest for
> some implementations. Let's not be cheap here
Ok, makes sense, will update this and documentation.

 
> >+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;
> 
> Is this really a fatal error? Or do we want to mandate that gapless
> be supported by all implementations?
Fatal err? if DSP doesnt support metadata callback then we are reporting error
ENXIO to userpsace. No we shouldnt mandate, its upto DSP folks to see what they
should and can support.

--
~Vinod

Re: [RFC] compress: add support for gapless playback

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 997 bytes --]

On Tue, Feb 05, 2013 at 11:54:19PM -0800, Vinod Koul wrote:
> On Tue, Feb 05, 2013 at 08:44:42PM -0600, Pierre-Louis Bossart wrote:

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

> > Is this really a fatal error? Or do we want to mandate that gapless
> > be supported by all implementations?

> Fatal err? if DSP doesnt support metadata callback then we are reporting error
> ENXIO to userpsace. No we shouldnt mandate, its upto DSP folks to see what they
> should and can support.

Well, up to userspace to see how it handles the error anyway.  For
example an application may want to fall back to PCM playback with
gapless done on the CPU if the driver doesn't do it.  But then given
that there's more than one piece of metadata in the struct perhaps we
need some way for the application to figure out what's supported?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC] compress: add support for gapless playback

[Vinod Koul]

On Wed, Feb 06, 2013 at 01:32:53PM +0000, Mark Brown wrote:
> On Tue, Feb 05, 2013 at 11:54:19PM -0800, Vinod Koul wrote:
> > On Tue, Feb 05, 2013 at 08:44:42PM -0600, Pierre-Louis Bossart wrote:
> 
> > > >+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;
> 
> > > Is this really a fatal error? Or do we want to mandate that gapless
> > > be supported by all implementations?
> 
> > Fatal err? if DSP doesnt support metadata callback then we are reporting error
> > ENXIO to userpsace. No we shouldnt mandate, its upto DSP folks to see what they
> > should and can support.
> 
> Well, up to userspace to see how it handles the error anyway.  For
> example an application may want to fall back to PCM playback with
> gapless done on the CPU if the driver doesn't do it.  But then given
> that there's more than one piece of metadata in the struct perhaps we
> need some way for the application to figure out what's supported?
This return would mean that API is not supported. If there is some other error
wrt metadata set then driver needs to send appropriate error which can help
userland figure out what caused this error

--
~Vinod

Re: [RFC] compress: add support for gapless playback

[Pierre-Louis Bossart]

>>>> +	if (!stream->ops->set_metadata)
>>>> +		return -ENXIO;
>
>>> Is this really a fatal error? Or do we want to mandate that gapless
>>> be supported by all implementations?
>
>> Fatal err? if DSP doesnt support metadata callback then we are reporting error
>> ENXIO to userpsace. No we shouldnt mandate, its upto DSP folks to see what they
>> should and can support.
>
> Well, up to userspace to see how it handles the error anyway.  For
> example an application may want to fall back to PCM playback with
> gapless done on the CPU if the driver doesn't do it.  But then given
> that there's more than one piece of metadata in the struct perhaps we
> need some way for the application to figure out what's supported?

Maybe we should expose what's supported as part of the decoder query 
process, i.e. expose support for metadata as part of the decoder 
capabilities rather than as part of the set_metadata part? Error checks 
after inits are painful. we could have a "metadata_support" bit-field 
that the application could check.

Re: [RFC] compress: add support for gapless playback

[Vinod Koul]

On Wed, Feb 06, 2013 at 07:59:56AM -0600, Pierre-Louis Bossart wrote:
> 
> >>>>+	if (!stream->ops->set_metadata)
> >>>>+		return -ENXIO;
> >
> >>>Is this really a fatal error? Or do we want to mandate that gapless
> >>>be supported by all implementations?
> >
> >>Fatal err? if DSP doesnt support metadata callback then we are reporting error
> >>ENXIO to userpsace. No we shouldnt mandate, its upto DSP folks to see what they
> >>should and can support.
> >
> >Well, up to userspace to see how it handles the error anyway.  For
> >example an application may want to fall back to PCM playback with
> >gapless done on the CPU if the driver doesn't do it.  But then given
> >that there's more than one piece of metadata in the struct perhaps we
> >need some way for the application to figure out what's supported?
> 
> Maybe we should expose what's supported as part of the decoder query
> process, i.e. expose support for metadata as part of the decoder
> capabilities rather than as part of the set_metadata part? Error
> checks after inits are painful. we could have a "metadata_support"
> bit-field that the application could check.
Well I agree that would be a smarter way to handle. Make one of the reserved
fields in decoder caps for metadata support.

--
~Vinod

Re: [RFC] compress: add support for gapless playback

[Vinod Koul]

On Wed, Feb 06, 2013 at 03:14:22PM +0100, Takashi Iwai wrote:
> At Tue, 05 Feb 2013 20:44:42 -0600,
> Pierre-Louis Bossart wrote:
> > 
> > >   /**
> > > + * 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;
> > > +};
> > 
> > You need to pad this structure with reserved bytes for future 
> > evolutions. Things like ReplayGain, etc, might be of interest for some 
> > implementations. Let's not be cheap here
> 
> Or, make it a single key/value.  User-space can call the ioctl
> multiple times with different key/value pairs.
Yes that would be smarter from ABI POV. But do we really have many params. And
also in this case setting too many varible one by one can be time consuming....

> 
> (BTW, don't you need get_metadata?  IMO, it was a mistake that we
>  didn't provide get_* variants for PCM parameters.)
For above case no, as metadata is from file parser and send to decoder. But yes
when we are encoding this will help. I think I will add that as well

--
~Vinod

Re: [RFC] compress: add support for gapless playback

[Vinod Koul]

On Wed, Feb 06, 2013 at 03:11:02PM +0100, Takashi Iwai wrote:
> At Tue,  5 Feb 2013 06:21:25 -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
> 
> I can understand that the metadata is somehow handled in DSP for
> seamless switching, 
yes for stripping the encoder delay and encoder padding.
> but the operation with partial drain is a bit
> unclear.
> How is the operation flow?  Does it drain until which point?
Right, we want to move from one track to another. And while at this DSP needs to
exhaust the buffers from first track and then we should start writing second
track. When userspace has finsihed wirting first track it sends partial_drain
and DSP decods finsihed off and returns when it is ready to accept second track
data. The userland sends the metadata for this followed by data write.

> In your patch, the runtime state is changed to SETUP after the partial
> drain, so the app is requested to give START again?
Ah, thats a mistake, it should be in running state, no START again :)

--
~Vinod

Re: [RFC] compress: add support for gapless playback

[Takashi Iwai]

At Tue,  5 Feb 2013 06:21:25 -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

I can understand that the metadata is somehow handled in DSP for
seamless switching, but the operation with partial drain is a bit
unclear.  How is the operation flow?  Does it drain until which point?
In your patch, the runtime state is changed to SETUP after the partial
drain, so the app is requested to give START again?


thanks,

Takashi


> 
> Also bump the compress API version
> 
> Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> ---
>  Documentation/sound/alsa/compress_offload.txt |   22 +++++++++++
>  include/sound/compress_driver.h               |    2 +
>  include/uapi/sound/compress_offload.h         |   18 ++++++++-
>  sound/core/compress_offload.c                 |   51 +++++++++++++++++++++++++
>  4 files changed, 92 insertions(+), 1 deletions(-)
> 
> diff --git a/Documentation/sound/alsa/compress_offload.txt b/Documentation/sound/alsa/compress_offload.txt
> index 90e9b3a..071c4b6 100644
> --- a/Documentation/sound/alsa/compress_offload.txt
> +++ b/Documentation/sound/alsa/compress_offload.txt
> @@ -145,6 +145,28 @@ 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
> +
> +The decoder needs to know the encoder delay and encoder padding. So we need to
> +pass this to 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
> +
> +- 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
> +
>  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..42d6d7c 100644
> --- a/include/sound/compress_driver.h
> +++ b/include/sound/compress_driver.h
> @@ -110,6 +110,8 @@ 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 (*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..65ca2f1 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,18 @@ 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;
> +};
> +
> +/**
>   * compress path ioctl definitions
>   * SNDRV_COMPRESS_GET_CAPS: Query capability of DSP
>   * SNDRV_COMPRESS_GET_CODEC_CAPS: Query capability of a codec
> @@ -145,6 +157,8 @@ 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_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 +166,12 @@ 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_PARTIAL_DRAIN	_IO('C', 0x35)
>  /*
>   * TODO
>   * 1. add mmap support
>   *
>   */
>  #define SND_COMPR_TRIGGER_DRAIN 7 /*FIXME move this to pcm.h */
> +#define SND_COMPR_TRIGGER_PARTIAL_DRAIN 8
>  #endif
> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> index ad11dc9..2928971 100644
> --- a/sound/core/compress_offload.c
> +++ b/sound/core/compress_offload.c
> @@ -514,6 +514,37 @@ out:
>  	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);
> +
> +out:
> +	kfree(metadata);
> +	return retval;
> +}
> +
>  static inline int
>  snd_compr_tstamp(struct snd_compr_stream *stream, unsigned long arg)
>  {
> @@ -594,6 +625,20 @@ 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;
> +	if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
> +			stream->runtime->state == SNDRV_PCM_STATE_SETUP)
> +		return -EPERM;
> +	retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_PARTIAL_DRAIN);
> +	if (retval)
> +		pr_err("Partial drain returned failure\n");
> +	else
> +		stream->runtime->state = SNDRV_PCM_STATE_SETUP;
> +	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 +668,9 @@ static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
>  	case _IOC_NR(SNDRV_COMPRESS_GET_PARAMS):
>  		retval = snd_compr_get_params(stream, arg);
>  		break;
> +	case _IOC_NR(SNDRV_COMPRESS_SET_METADATA):
> +		retval = snd_compr_set_metadata(stream, arg);
> +		break;
>  	case _IOC_NR(SNDRV_COMPRESS_TSTAMP):
>  		retval = snd_compr_tstamp(stream, arg);
>  		break;
> @@ -644,6 +692,9 @@ 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;
>  	}
>  	mutex_unlock(&stream->device->lock);
>  	return retval;
> -- 
> 1.7.0.4
> 

Re: [RFC] compress: add support for gapless playback

[Takashi Iwai]

At Tue, 05 Feb 2013 20:44:42 -0600,
Pierre-Louis Bossart wrote:
> 
> >   /**
> > + * 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;
> > +};
> 
> You need to pad this structure with reserved bytes for future 
> evolutions. Things like ReplayGain, etc, might be of interest for some 
> implementations. Let's not be cheap here

Or, make it a single key/value.  User-space can call the ioctl
multiple times with different key/value pairs.

(BTW, don't you need get_metadata?  IMO, it was a mistake that we
 didn't provide get_* variants for PCM parameters.)


thanks,

Takashi

Re: [RFC] compress: add support for gapless playback

[Vinod Koul]

On Wed, Feb 06, 2013 at 03:48:26PM +0100, Takashi Iwai wrote:
> > > How is the operation flow?  Does it drain until which point?
> > Right, we want to move from one track to another. And while at this DSP needs to
> > exhaust the buffers from first track and then we should start writing second
> > track. When userspace has finsihed wirting first track it sends partial_drain
> > and DSP decods finsihed off and returns when it is ready to accept second track
> > data. The userland sends the metadata for this followed by data write.
> 
> Hm, could you depict the flow?  Will be like below?
> 
> - 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, then trigger PARTIAL_DRAIN
PARTAIL_DRAIN is returned when DSP has finished most of data from first track
and is ready to accept second track data
> - Set metadata of the next track
> - Fill data of the next track
DSP switches to second track
Keep writing till we finish track, then trigger PARTIAL_DRAIN
Continue till you reach end of album
Then trigger DRAIN
Close

I will add this to documentation patch as well...

> OK, then PARTIAL_DRAIN doesn't sound intuitive.
> Something like CONTINUE_TO_NEXT?
Sure, anyone else with a better name :-)
> 
> (I'm not particularly good at naming, so someone must have a better
>  idea...)
Welcome to club, I wont mind naming after you :D

--
~Vinod

Re: [RFC] compress: add support for gapless playback

[Takashi Iwai]

At Wed, 6 Feb 2013 06:02:29 -0800,
Vinod Koul wrote:
> 
> On Wed, Feb 06, 2013 at 03:14:22PM +0100, Takashi Iwai wrote:
> > At Tue, 05 Feb 2013 20:44:42 -0600,
> > Pierre-Louis Bossart wrote:
> > > 
> > > >   /**
> > > > + * 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;
> > > > +};
> > > 
> > > You need to pad this structure with reserved bytes for future 
> > > evolutions. Things like ReplayGain, etc, might be of interest for some 
> > > implementations. Let's not be cheap here
> > 
> > Or, make it a single key/value.  User-space can call the ioctl
> > multiple times with different key/value pairs.
> Yes that would be smarter from ABI POV. But do we really have many params. And
> also in this case setting too many varible one by one can be time consuming....

Would it really matter?  The ioctl itself is cheap and fast.
And, with the fixed metadata key/value pair, you don't need a kmalloc.
It's known to be small to fit on stack.

Of course, if DSP's response to each parameter change is slow, it's a
problem.  Or if DSP requires the updates of multiple parameters at the
same time, the key/value model doesn't work, too.


Takashi

Re: [RFC] compress: add support for gapless playback

[Takashi Iwai]

At Wed, 6 Feb 2013 06:09:32 -0800,
Vinod Koul wrote:
> 
> On Wed, Feb 06, 2013 at 03:11:02PM +0100, Takashi Iwai wrote:
> > At Tue,  5 Feb 2013 06:21:25 -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
> > 
> > I can understand that the metadata is somehow handled in DSP for
> > seamless switching, 
> yes for stripping the encoder delay and encoder padding.
> > but the operation with partial drain is a bit
> > unclear.
> > How is the operation flow?  Does it drain until which point?
> Right, we want to move from one track to another. And while at this DSP needs to
> exhaust the buffers from first track and then we should start writing second
> track. When userspace has finsihed wirting first track it sends partial_drain
> and DSP decods finsihed off and returns when it is ready to accept second track
> data. The userland sends the metadata for this followed by data write.

Hm, could you depict the flow?  Will be like below?

- 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, then trigger PARTIAL_DRAIN
- Set metadata of the next track
- Fill data of the next track
....


> > In your patch, the runtime state is changed to SETUP after the partial
> > drain, so the app is requested to give START again?
> Ah, thats a mistake, it should be in running state, no START again :)

OK, then PARTIAL_DRAIN doesn't sound intuitive.
Something like CONTINUE_TO_NEXT?

(I'm not particularly good at naming, so someone must have a better
 idea...)


Takashi

Re: [RFC] compress: add support for gapless playback

[Vinod Koul]

On Thu, Feb 07, 2013 at 01:15:54AM +0000, Richard Fitzgerald wrote:
> > +- 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
> 
> We're really doing two different tasks inside the "partial drain". The name,
> and the reason we are doing two tasks in one, comes from a particular higher-
> layer scenario, but there's no reason the driver API need use the same
> terminology as one particular application of the functionality.
> The two tasks are:
> 
> 1) Tell the DSP that we have sent all data for the current track and following
> data will be for the next track. This lets the DSP lay down a marker for where
> it should strip padding at the end of a track, and know it should be expecting
> more data for another track to follow gaplessly where it must strip the
> encoder delay from the start.
> 
> 2) Ask for notification when DSP has reached the changeover point between the
> playback of the two tracks
> 
> I think it would be more logical and less confusing not to combine the two into
> a single ioctl. Instead, add only one new ioctl specifically to provide the
> track changeover hint, something like SNDRV_COMPRESS_NEXT_TRACK (meaning DSP
> should expect data for next track to follow). Don't add a new drain, just use
> the existing SNDRV_COMPRESS_DRAIN - the driver/DSP can make a decision whether
> it needs to do something special with the drain if we have told it that we
> will be sending it some more data for a following track.
> 
> So the SNDRV_COMPRESS_PARTIAL_DRAIN in this patch would become
> 
> 1) Send SNDRV_COMPRESS_NEXT_TRACK
> 2) Send SNDRV_COMPRESS_DRAIN
The problem would be in that case the defination of SNDRV_COMPRESS_DRAIN which
expects the decoder to completely drain its buffers and come to complete halt.
This would also mean the framework will treat a drained stream as stopped and
needs a new start. Certainly we dont want that in this case. So we can't use
SNDRV_COMPRESS_DRAIN to indicate. Yes we can put conditional check but IMO that
would overtly complicate this. If we are not doing proper drian lets not
call it that.

But I think I like the idea of splitting the two up to do a cleaner interface.
Let me check this...

> When we reach the final track we just do SNDRV_COMPRESS_DRAIN to wait for it
> to finish.
> 
> If we setup a next track by doing SNDRV_COMPRESS_NEXT_TRACK and then change
> our mind and decide that this is going to be the last track, we do a
> SNDRV_COMPRESS_DRAIN without sending any next track data, then we do
> SNDRV_COMPRESS_STOP
Nope that is where we would have issue. You dont call SNDRV_COMPRESS_STOP for
compressed streams. You have to wait till DSP has decoded and rendered data for
the last track which can be done by using SNDRV_COMPRESS_DRAIN only.
SNDRV_COMPRESS_STOP should not be called in this case.

--
~Vinod

Re: [RFC] compress: add support for gapless playback

[Takashi Iwai]

At Wed, 6 Feb 2013 18:18:07 -0800,
Vinod Koul wrote:
> 
> On Thu, Feb 07, 2013 at 01:15:54AM +0000, Richard Fitzgerald wrote:
> > > +- 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
> > 
> > We're really doing two different tasks inside the "partial drain". The name,
> > and the reason we are doing two tasks in one, comes from a particular higher-
> > layer scenario, but there's no reason the driver API need use the same
> > terminology as one particular application of the functionality.
> > The two tasks are:
> > 
> > 1) Tell the DSP that we have sent all data for the current track and following
> > data will be for the next track. This lets the DSP lay down a marker for where
> > it should strip padding at the end of a track, and know it should be expecting
> > more data for another track to follow gaplessly where it must strip the
> > encoder delay from the start.
> > 
> > 2) Ask for notification when DSP has reached the changeover point between the
> > playback of the two tracks
> > 
> > I think it would be more logical and less confusing not to combine the two into
> > a single ioctl. Instead, add only one new ioctl specifically to provide the
> > track changeover hint, something like SNDRV_COMPRESS_NEXT_TRACK (meaning DSP
> > should expect data for next track to follow). Don't add a new drain, just use
> > the existing SNDRV_COMPRESS_DRAIN - the driver/DSP can make a decision whether
> > it needs to do something special with the drain if we have told it that we
> > will be sending it some more data for a following track.
> > 
> > So the SNDRV_COMPRESS_PARTIAL_DRAIN in this patch would become
> > 
> > 1) Send SNDRV_COMPRESS_NEXT_TRACK
> > 2) Send SNDRV_COMPRESS_DRAIN
> The problem would be in that case the defination of SNDRV_COMPRESS_DRAIN which
> expects the decoder to completely drain its buffers and come to complete halt.
> This would also mean the framework will treat a drained stream as stopped and
> needs a new start. Certainly we dont want that in this case. So we can't use
> SNDRV_COMPRESS_DRAIN to indicate. Yes we can put conditional check but IMO that
> would overtly complicate this. If we are not doing proper drian lets not
> call it that.
> 
> But I think I like the idea of splitting the two up to do a cleaner interface.
> Let me check this...

In the way above, NEXT_TRACK is no longer a trigger op, but it's
rather an operational option.  This would set a flag something like
	compr->drain_mode = SND_COMPR_DRAIN_RESTART;
while its default is SND_COMPR_DRAIN_STOP.
(Or, set compr->next_track_after_drain = true, or whatever.)


Though, I'm also not sure whether the scenario above improves the
situation better...


Takashi

Re: [RFC] compress: add support for gapless playback

[Takashi Iwai]

At Thu, 7 Feb 2013 11:37:38 +0000,
Richard Fitzgerald wrote:
> 
> > > 1) Send SNDRV_COMPRESS_NEXT_TRACK
> > > 2) Send SNDRV_COMPRESS_DRAIN
> > The problem would be in that case the defination of SNDRV_COMPRESS_DRAIN which
> > expects the decoder to completely drain its buffers and come to complete halt.
> > This would also mean the framework will treat a drained stream as stopped and
> > needs a new start. Certainly we dont want that in this case. So we can't use
> > SNDRV_COMPRESS_DRAIN to indicate. Yes we can put conditional check but IMO that
> > would overtly complicate this. If we are not doing proper drian lets not
> > call it that.
> 
> Ok, so let's keep the partial drain but split out the NEXT_TRACK hint so we keep
> the two operations separate. It's clearer to understand. NEXT_TRACK tells DSP to
> prepare for data for new track, PARTIAL_DRAIN asks DSP when it has played all
> data of current track. I don't think that application _must_ drain when doing
> gapless, only if it cares to know when DSP reaches the join between tracks.
> Possible problem - application calls NEXT_TRACK and PARTIAL_DRAIN when DSP has
> already reached end of current track, so what does PARTIAL_DRAIN mean in this
> case? Which track is it draining? Maybe define that it always refers to the
> track _before_ the most recent NEXT_TRACK call?
> 
> Also can we make metadata a key-pair list as Takashi suggested, with unlimited
> length. Something like
> 
> struct snd_compr_metadata_pair {
> 	enum snd_compr_metadata_key key;
> 	u32 value;
> };
> 
> struct snd_compr_metadata {
> 	int count; /* number of actual entries in following array */
> 	struct snd_compr_metadata_pair *pairs;
> };
> 
> Should we union the u32 value with a u8* buf to allow for any case where the
> value needs to be a buffer of binary data?

Oh no, don't do it.  Never.

In general,
 - Never put types that depending on the architecture in struct passed
   to ioctls (e.g. long, pointer)
 - Never use bit fields for ABI
 - If 64bit value is used, think of alignment; safer to add packed
   attribute

So, in general, use only u32, s16, or whatever, the types with
explicit size.


Takashi

Re: [RFC] compress: add support for gapless playback

[Vinod Koul]

On Thu, Feb 07, 2013 at 09:49:37AM +0100, Takashi Iwai wrote:
> At Wed, 6 Feb 2013 18:18:07 -0800,
> Vinod Koul wrote:
> > 
> > On Thu, Feb 07, 2013 at 01:15:54AM +0000, Richard Fitzgerald wrote:
> > > > +- 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
> > > 
> > > We're really doing two different tasks inside the "partial drain". The name,
> > > and the reason we are doing two tasks in one, comes from a particular higher-
> > > layer scenario, but there's no reason the driver API need use the same
> > > terminology as one particular application of the functionality.
> > > The two tasks are:
> > > 
> > > 1) Tell the DSP that we have sent all data for the current track and following
> > > data will be for the next track. This lets the DSP lay down a marker for where
> > > it should strip padding at the end of a track, and know it should be expecting
> > > more data for another track to follow gaplessly where it must strip the
> > > encoder delay from the start.
> > > 
> > > 2) Ask for notification when DSP has reached the changeover point between the
> > > playback of the two tracks
> > > 
> > > I think it would be more logical and less confusing not to combine the two into
> > > a single ioctl. Instead, add only one new ioctl specifically to provide the
> > > track changeover hint, something like SNDRV_COMPRESS_NEXT_TRACK (meaning DSP
> > > should expect data for next track to follow). Don't add a new drain, just use
> > > the existing SNDRV_COMPRESS_DRAIN - the driver/DSP can make a decision whether
> > > it needs to do something special with the drain if we have told it that we
> > > will be sending it some more data for a following track.
> > > 
> > > So the SNDRV_COMPRESS_PARTIAL_DRAIN in this patch would become
> > > 
> > > 1) Send SNDRV_COMPRESS_NEXT_TRACK
> > > 2) Send SNDRV_COMPRESS_DRAIN
> > The problem would be in that case the defination of SNDRV_COMPRESS_DRAIN which
> > expects the decoder to completely drain its buffers and come to complete halt.
> > This would also mean the framework will treat a drained stream as stopped and
> > needs a new start. Certainly we dont want that in this case. So we can't use
> > SNDRV_COMPRESS_DRAIN to indicate. Yes we can put conditional check but IMO that
> > would overtly complicate this. If we are not doing proper drian lets not
> > call it that.
> > 
> > But I think I like the idea of splitting the two up to do a cleaner interface.
> > Let me check this...
> 
> In the way above, NEXT_TRACK is no longer a trigger op, but it's
> rather an operational option.  This would set a flag something like
> 	compr->drain_mode = SND_COMPR_DRAIN_RESTART;
> while its default is SND_COMPR_DRAIN_STOP.
> (Or, set compr->next_track_after_drain = true, or whatever.)
So the basic difference would be that stream is never stopped in gapless case.
The stream should retain running state maybe trasition to intermediate one for
book keeping but then needs to go back to running.

But, should framework really care about that as DSP is doing handling!

--
~Vinod

Re: [RFC] compress: add support for gapless playback

[Vinod Koul]

On Thu, Feb 07, 2013 at 11:37:38AM +0000, Richard Fitzgerald wrote:
> > > 1) Send SNDRV_COMPRESS_NEXT_TRACK
> > > 2) Send SNDRV_COMPRESS_DRAIN
> > The problem would be in that case the defination of SNDRV_COMPRESS_DRAIN which
> > expects the decoder to completely drain its buffers and come to complete halt.
> > This would also mean the framework will treat a drained stream as stopped and
> > needs a new start. Certainly we dont want that in this case. So we can't use
> > SNDRV_COMPRESS_DRAIN to indicate. Yes we can put conditional check but IMO that
> > would overtly complicate this. If we are not doing proper drian lets not
> > call it that.
> 
> Ok, so let's keep the partial drain but split out the NEXT_TRACK hint so we keep
> the two operations separate. It's clearer to understand. NEXT_TRACK tells DSP to
> prepare for data for new track, PARTIAL_DRAIN asks DSP when it has played all
> data of current track. I don't think that application _must_ drain when doing
> gapless, only if it cares to know when DSP reaches the join between tracks.
If we split this then I dont see why next write/meta_data call should wait. Then
the PARTIAL_DRAIN should be used for knowing in userpsace that switch happened,
right?

> Possible problem - application calls NEXT_TRACK and PARTIAL_DRAIN when DSP has
> already reached end of current track, so what does PARTIAL_DRAIN mean in this
> case? 
It should just return immediately, and if your output is starved you are in
error state, that can happen when you dont give data as well.

> Also can we make metadata a key-pair list as Takashi suggested, with unlimited
> length. Something like
> 
> struct snd_compr_metadata_pair {
> 	enum snd_compr_metadata_key key;
> 	u32 value;
> };
No enums... Thats not how kernel ABI is designed.
I am leaning towards key/value pair, but is this fair assumption that value is
always 32bit, what if we have larger single value, then should we split?

> 
> struct snd_compr_metadata {
> 	int count; /* number of actual entries in following array */
> 	struct snd_compr_metadata_pair *pairs;
> };
> 
> Should we union the u32 value with a u8* buf to allow for any case where the
> value needs to be a buffer of binary data? Is there any known case like this?
Again nope, that aint good from ABI POV.

What I think it should be is:

enum {
	SNDRV_COMPRESS_ENCODER_PADDING = 1,
	SNDRV_COMPRESS_ENCODER_DELAY = 2,
};

struct snd_compr_metadata {
	u32 key;
	u32 value;
};

--
~Vinod