[alsa-devel] [PATCH] compress: add support for gapless playback

[Takashi Iwai]

At Mon, 11 Feb 2013 05:41:47 -0800,
Vinod Koul wrote:
> 
> On Mon, Feb 11, 2013 at 02:41:18PM +0100, Takashi Iwai wrote:
> > At Mon, 11 Feb 2013 04:22:45 -0800,
> > Vinod Koul wrote:
> > > 
> > (snip)
> > > --- a/include/uapi/sound/asound.h
> > > +++ b/include/uapi/sound/asound.h
> > > @@ -271,7 +271,9 @@ typedef int __bitwise snd_pcm_state_t;
> > >  #define	SNDRV_PCM_STATE_PAUSED		((__force snd_pcm_state_t) 6) /* stream is paused */
> > >  #define	SNDRV_PCM_STATE_SUSPENDED	((__force snd_pcm_state_t) 7) /* hardware is suspended */
> > >  #define	SNDRV_PCM_STATE_DISCONNECTED	((__force snd_pcm_state_t) 8) /* hardware is disconnected */
> > > -#define	SNDRV_PCM_STATE_LAST		SNDRV_PCM_STATE_DISCONNECTED
> > > +#define SNDRV_PCM_STATE_NEXT_TRACK	((__force snd_pcm_state_t) 9) /* stream will move to next track */
> > 
> > Isn't this better to be set as another flag instead of the trigger
> > command?  This changes the runtime->state, and it may make things
> > complicated.  For example, what happens if user triggers NEXT_TRACK,
> > then PAUSE_PUSH and PAUSE_RELEASE?
> hmmm, that makes sense. Also it would make things much easier on all the
> transistions
> 
> > 
> > > +#define SNDRV_PCM_STATE_PARTIAL_DRAIN	((__force snd_pcm_state_t) 10) /* stream is draining partially */
> > > +#define	SNDRV_PCM_STATE_LAST		SNDRV_PCM_STATE_PARTIAL_DRAIN
> > 
> > Well, I'd hesitate to add this for PCM, since there is no counterpart
> > for PCM (yet).  Until then, let's avoid touching the PCM API
> > definition but only compress API.
> If we do above then we may not need this :)
> 
> > > +/**
> > > + * struct snd_compr_metadata: compressed stream metadata
> > > + * @count: number of keys
> > > + * @keys: pointer to count keys
> > > + */
> > > +struct snd_compr_metadata {
> > > +	__u32 count;
> > > +	struct snd_compr_keyvalue *keys;
> > > +};
> > 
> > Do you really want that?  Then you'll have to provide 32/64bit
> > ioctl conversion functions as well.  I wouldn't take that mess but let
> > user repeat single key/value ioctls.
> Ah, i forgot the damn pointer :-)
> > 
> > Instead of relying completely on the driver side implementation, you
> > can keep a metadata array in snd_compr_stream or snd_compr
> > internally.  Then the ioctl handler would be just like:
> how do we decide the size of array and what if we have more params in future

The point is that it's internal, so we can extend at any time if we
add more parameter definitions.


> > static int compr_stream_set_metadata(struct snd_compr_stream *stream, void __user * arg)
> > {
> > 	struct snd_compr_keyvalue kv;
> > 
> > 	if (!stream->ops->set_metadata)
> > 		return -ENXIO;
> > 	if (copy_from_user(&kv, arg, sizeof(kv)))
> > 		return -EFAULT;
> > 	if (kv > SNDRV_COMPRESS_METADATA_LAST)
> > 		return -EINVAL;
> > 	stream->metadata[kv.key] = kv.value;
> > 	return stream->ops->set_metadata(stream);
> > }
> And when do we clear this? metadata for gapless, though a same stream session
> gets updated for every track.
> 
> Right now I am inlcined to put a single value, and be done with it or update
> older structs with bunch of padding for future use :)
> > 
> > Of course, this assumes that stream->metadata[] is initialized at each
> > open properly.
> Yes and it cant be cleared later for next track, perhpas we clear it when we get
> NEXT_TRACK signalled from user space?

Well, the question is how this value change is supposed.  I guess it's
not defined precisely yet.

It'd make sense to clear padding or such automatically at each new
stream switch, indeed.  OTOH, what if a metadata is supposed to be
kept for all tracks (e.g. something like the playback speed control)?

So, this won't be only a question about bookkeeping in the compress
core, but rather an API question.  If all metadata are supposed to be
cleared at stream change, it's fine -- the refresh of all metadata by
user-space is mandatory.  OTOH, if the metadata are never changed
unless the stream is (re-)prepared, again, user-space has to manage
all metadata by itself.  There is no big difference between them.
The difference is what happens if user-space doesn't update.

If the behavior is defined per each parameter, maybe it's good for
user-space, but then the driver (or comr core) becomes slightly more
complex since it needs to clear some data selectively.


Takashi