[alsa-devel] [RFC] compress: add support for gapless playback
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;
+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?
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
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?
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
- 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.
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
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
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
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
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);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
- 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;@@ -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;-- 1.7.0.4
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
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
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
participants (4)
-
Mark Brown -
Pierre-Louis Bossart -
Takashi Iwai -
Vinod Koul