* [RFC] compress: add support for gapless playback
@ 2013-02-05 14:21 Vinod Koul
2013-02-06 2:44 ` Pierre-Louis Bossart
` (2 more replies)
0 siblings, 3 replies; 19+ messages in thread
From: Vinod Koul @ 2013-02-05 14:21 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>
---
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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [RFC] compress: add support for gapless playback
2013-02-05 14:21 [RFC] compress: add support for gapless playback Vinod Koul
@ 2013-02-06 2:44 ` Pierre-Louis Bossart
2013-02-06 7:54 ` Vinod Koul
2013-02-06 14:14 ` Takashi Iwai
2013-02-06 14:11 ` Takashi Iwai
[not found] ` <37A133201056E44A80888420ECA881BF1093DC5F68@EXCMB2.wolfsonmicro.main>
2 siblings, 2 replies; 19+ messages in thread
From: Pierre-Louis Bossart @ 2013-02-06 2:44 UTC (permalink / raw)
To: Vinod Koul; +Cc: tiwai, Jeeja KP, alsa-devel, broonie, liam.r.girdwood
> +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?
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC] compress: add support for gapless playback
2013-02-06 2:44 ` Pierre-Louis Bossart
@ 2013-02-06 7:54 ` Vinod Koul
2013-02-06 13:32 ` Mark Brown
2013-02-06 14:14 ` Takashi Iwai
1 sibling, 1 reply; 19+ messages in thread
From: Vinod Koul @ 2013-02-06 7:54 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: tiwai, Jeeja KP, alsa-devel, broonie, liam.r.girdwood
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC] compress: add support for gapless playback
2013-02-06 7:54 ` Vinod Koul
@ 2013-02-06 13:32 ` Mark Brown
2013-02-06 13:56 ` Vinod Koul
2013-02-06 13:59 ` Pierre-Louis Bossart
0 siblings, 2 replies; 19+ messages in thread
From: Mark Brown @ 2013-02-06 13:32 UTC (permalink / raw)
To: Vinod Koul
Cc: tiwai, Jeeja KP, alsa-devel, Pierre-Louis Bossart, liam.r.girdwood
[-- 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 --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC] compress: add support for gapless playback
2013-02-06 13:32 ` Mark Brown
@ 2013-02-06 13:56 ` Vinod Koul
2013-02-06 13:59 ` Pierre-Louis Bossart
1 sibling, 0 replies; 19+ messages in thread
From: Vinod Koul @ 2013-02-06 13:56 UTC (permalink / raw)
To: Mark Brown
Cc: tiwai, Jeeja KP, alsa-devel, Pierre-Louis Bossart, liam.r.girdwood
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC] compress: add support for gapless playback
2013-02-06 13:32 ` Mark Brown
2013-02-06 13:56 ` Vinod Koul
@ 2013-02-06 13:59 ` Pierre-Louis Bossart
2013-02-06 14:00 ` Vinod Koul
1 sibling, 1 reply; 19+ messages in thread
From: Pierre-Louis Bossart @ 2013-02-06 13:59 UTC (permalink / raw)
To: Mark Brown
Cc: alsa-devel, Vinod Koul, Pierre-Louis Bossart, tiwai,
liam.r.girdwood, Jeeja KP
>>>> + 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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC] compress: add support for gapless playback
2013-02-06 13:59 ` Pierre-Louis Bossart
@ 2013-02-06 14:00 ` Vinod Koul
0 siblings, 0 replies; 19+ messages in thread
From: Vinod Koul @ 2013-02-06 14:00 UTC (permalink / raw)
Cc: alsa-devel, tiwai, Mark Brown, Pierre-Louis Bossart,
liam.r.girdwood, Jeeja KP
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC] compress: add support for gapless playback
2013-02-06 2:44 ` Pierre-Louis Bossart
2013-02-06 7:54 ` Vinod Koul
@ 2013-02-06 14:14 ` Takashi Iwai
2013-02-06 14:02 ` Vinod Koul
1 sibling, 1 reply; 19+ messages in thread
From: Takashi Iwai @ 2013-02-06 14:14 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: Vinod Koul, Jeeja KP, alsa-devel, broonie, liam.r.girdwood
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC] compress: add support for gapless playback
2013-02-06 14:14 ` Takashi Iwai
@ 2013-02-06 14:02 ` Vinod Koul
2013-02-06 14:33 ` Takashi Iwai
0 siblings, 1 reply; 19+ messages in thread
From: Vinod Koul @ 2013-02-06 14:02 UTC (permalink / raw)
To: Takashi Iwai
Cc: Jeeja KP, alsa-devel, broonie, Pierre-Louis Bossart, liam.r.girdwood
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC] compress: add support for gapless playback
2013-02-06 14:02 ` Vinod Koul
@ 2013-02-06 14:33 ` Takashi Iwai
0 siblings, 0 replies; 19+ messages in thread
From: Takashi Iwai @ 2013-02-06 14:33 UTC (permalink / raw)
To: Vinod Koul
Cc: Jeeja KP, alsa-devel, broonie, Pierre-Louis Bossart, liam.r.girdwood
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC] compress: add support for gapless playback
2013-02-05 14:21 [RFC] compress: add support for gapless playback Vinod Koul
2013-02-06 2:44 ` Pierre-Louis Bossart
@ 2013-02-06 14:11 ` Takashi Iwai
2013-02-06 14:09 ` Vinod Koul
[not found] ` <37A133201056E44A80888420ECA881BF1093DC5F68@EXCMB2.wolfsonmicro.main>
2 siblings, 1 reply; 19+ messages in thread
From: Takashi Iwai @ 2013-02-06 14:11 UTC (permalink / raw)
To: Vinod Koul; +Cc: Jeeja KP, alsa-devel, broonie, liam.r.girdwood
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
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC] compress: add support for gapless playback
2013-02-06 14:11 ` Takashi Iwai
@ 2013-02-06 14:09 ` Vinod Koul
2013-02-06 14:48 ` Takashi Iwai
0 siblings, 1 reply; 19+ messages in thread
From: Vinod Koul @ 2013-02-06 14:09 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Jeeja KP, alsa-devel, broonie, liam.r.girdwood
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC] compress: add support for gapless playback
2013-02-06 14:09 ` Vinod Koul
@ 2013-02-06 14:48 ` Takashi Iwai
2013-02-06 14:31 ` Vinod Koul
0 siblings, 1 reply; 19+ messages in thread
From: Takashi Iwai @ 2013-02-06 14:48 UTC (permalink / raw)
To: Vinod Koul; +Cc: Jeeja KP, alsa-devel, broonie, liam.r.girdwood
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC] compress: add support for gapless playback
2013-02-06 14:48 ` Takashi Iwai
@ 2013-02-06 14:31 ` Vinod Koul
0 siblings, 0 replies; 19+ messages in thread
From: Vinod Koul @ 2013-02-06 14:31 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Jeeja KP, alsa-devel, broonie, liam.r.girdwood
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
^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <37A133201056E44A80888420ECA881BF1093DC5F68@EXCMB2.wolfsonmicro.main>]
end of thread, other threads:[~2013-02-07 17:16 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-02-05 14:21 [RFC] compress: add support for gapless playback Vinod Koul
2013-02-06 2:44 ` Pierre-Louis Bossart
2013-02-06 7:54 ` Vinod Koul
2013-02-06 13:32 ` Mark Brown
2013-02-06 13:56 ` Vinod Koul
2013-02-06 13:59 ` Pierre-Louis Bossart
2013-02-06 14:00 ` Vinod Koul
2013-02-06 14:14 ` Takashi Iwai
2013-02-06 14:02 ` Vinod Koul
2013-02-06 14:33 ` Takashi Iwai
2013-02-06 14:11 ` Takashi Iwai
2013-02-06 14:09 ` Vinod Koul
2013-02-06 14:48 ` Takashi Iwai
2013-02-06 14:31 ` Vinod Koul
[not found] ` <37A133201056E44A80888420ECA881BF1093DC5F68@EXCMB2.wolfsonmicro.main>
[not found] ` <20130207011518.GA30348@opensource.wolfsonmicro.com>
2013-02-07 2:18 ` Vinod Koul
2013-02-07 8:49 ` Takashi Iwai
[not found] ` <20130207113738.GA15824@opensource.wolfsonmicro.com>
2013-02-07 11:49 ` Takashi Iwai
2013-02-07 16:51 ` Vinod Koul
2013-02-07 16:34 ` Vinod Koul
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.