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

[Vinod Koul <vinod.koul@intel.com>]

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