[alsa-devel] [PATCH TINYCOMPRESS 1/14] compress: Add function to get timestamp in samples
The compress_get_hpointer() converts the timestamp into actual time. But Android needs it in samples. To avoid the inefficiency of using compress_get_hpointer() and converting into time and then back into samples, this change adds compress_get_tstamp() which returns the raw struct snd_compr_tstamp.
diff --git a/compress.c b/compress.c index faef982..19b9366 100644 --- a/compress.c +++ b/compress.c @@ -287,6 +287,18 @@ int compress_get_hpointer(struct compress *compress, return 0; }
+int compress_get_tstamp(struct compress *compress, + struct snd_compr_tstamp *tstamp ) +{ + if (!is_compress_ready(compress)) + return oops(compress, -ENODEV, "device not ready"); + + if (ioctl(compress->fd, SNDRV_COMPRESS_TSTAMP, tstamp)) + return oops(compress, errno, "cannot get avail"); + + return 0; +} + int compress_write(struct compress *compress, char *buf, unsigned int size) { struct snd_compr_avail avail; diff --git a/include/tinycompress/tinycompress.h b/include/tinycompress/tinycompress.h index 13dea3e..8a98527 100644 --- a/include/tinycompress/tinycompress.h +++ b/include/tinycompress/tinycompress.h @@ -73,6 +73,7 @@ struct compr_config { #define COMPRESS_IN 0x10000000
struct compress; +struct snd_compr_tstamp;
/* * compress_open: open a new compress stream @@ -103,6 +104,18 @@ void compress_close(struct compress *compress); */ int compress_get_hpointer(struct compress *compress, unsigned int *avail, struct timespec *tstamp); + + +/* + * compress_get_tstamp: get the raw hw timestamp + * return 0 on success, negative on error + * + * @compress: compress stream on which query is made + * @tstamp: timestamp information + */ +int compress_get_tstamp(struct compress *compress, + struct snd_compr_tstamp *tstamp ); + /* * compress_write: write data to the compress stream * return bytes written on success, negative on error -- 1.7.2.5
On Sun, Feb 10, 2013 at 12:02:27AM +0000, Richard Fitzgerald wrote:
The compress_get_hpointer() converts the timestamp into actual time. But Android needs it in samples. To avoid the inefficiency of using compress_get_hpointer() and converting into time and then back into samples, this change adds compress_get_tstamp() which returns the raw struct snd_compr_tstamp.
I am fine with adding new API for returning the samples, but then why return the raw struct. Would make sense to just get samples if that is only thing desired.
Let users use whatever field that want, samples or time.
-- ~Vinod
The compress_get_hpointer() converts the timestamp into actual time. But Android needs it in samples. To avoid the inefficiency of using compress_get_hpointer() and converting into time and then back into samples, this change adds compress_get_tstamp() which returns the raw struct snd_compr_tstamp.
I am fine with adding new API for returning the samples, but then why return the raw struct. Would make sense to just get samples if that is only thing desired.
Loss of efficiency? It's not that bad really and the conversion could be handled in tinycompress (user-space) rather than adding a new kernel ioctl that everyone will need to implement. The htimestamp could give you better precision than samples if a wall clock is available in the hardware. -Pierre
The compress_get_hpointer() converts the timestamp into actual time. But Android needs it in samples. To avoid the inefficiency of using compress_get_hpointer() and converting into time and then back into samples, this change adds compress_get_tstamp() which returns the raw struct snd_compr_tstamp.
the conversion could be handled in tinycompress (user-space) rather than adding a new kernel ioctl that everyone will need to implement.
This isn't adding a new ioctl. It's adding a tinycompress function to give the client a way of calling the existing SNDRV_COMPRESS_TSTAMP ioctl, which gets the position in samples.
Loss of efficiency? It's not that bad really
It's silly to get something in the required format, convert it to the wrong format, then convert it back again, when it's trivial to just return it in the correct format. And all unnecessary code wastes a little battery power - if you call it often the waste starts to become measurable.
It's worse if the divide is done in software. Not all ARM processors have hardware divide so the three divides in compress_get_hpointer are not necessarily trivial (especially if you're building a low-power embedded device using a simpler ARM instead of the latest high-end ARM.)
Divide is the most difficult math operation to do in hardware, so any good programmer that cares about efficiency/speed/power consumption on embedded platforms should be suspicious of code that needs a divide.
I am fine with adding new API for returning the samples, but then why return the raw struct. Would make sense to just get samples if that is only thing desired.
Is there a need to hide the underlying ALSA structs from a client of tinyalsa? If so, why?
Might other fields in snd_compr_tstamp be useful to the client? If so it's cleaner to just return them all instead of having to spawn several different functions the client has to call to get a snd_compr_tstamp and then return each field separately.
If snd_compr_tstamp is basically useless to any tinycompress client except for the value of pcm_io_frames (and maybe sampling_rate?) then I'm ok with just returning those instead of the raw snd_compr_tstamp.
On Tue, Feb 19, 2013 at 05:24:29PM +0000, Richard Fitzgerald wrote:
I am fine with adding new API for returning the samples, but then why return the raw struct. Would make sense to just get samples if that is only thing desired.
Is there a need to hide the underlying ALSA structs from a client of tinyalsa? If so, why?
The user shouldnt care what struct ALSA is using. One needs to know the timeline or samples and you provide them without worrying about what is underlying representation.
Might other fields in snd_compr_tstamp be useful to the client? If so it's cleaner to just return them all instead of having to spawn several different functions the client has to call to get a snd_compr_tstamp and then return each field separately.
For now I see use of samples, time and offset value in ring buffer. But yes it depends on how you wrote the clinet and what are you going to do with these values. The kernel represenattion is genric.
If snd_compr_tstamp is basically useless to any tinycompress client except for the value of pcm_io_frames (and maybe sampling_rate?) then I'm ok with just returning those instead of the raw snd_compr_tstamp.
Again, depends on how you wrote the client. You cna do some smart decisions based on how much DSP has fetched, decoded , rendered...
-- ~Vinod
The compress_get_hpointer() converts the timestamp into actual time. But Android needs it in samples. To avoid the inefficiency of using compress_get_hpointer() and converting into time and then back into samples, this change adds compress_get_tstamp() which returns the raw sample count.
--- compress.c | 16 ++++++++++++++++ include/tinycompress/tinycompress.h | 14 ++++++++++++++ 2 files changed, 30 insertions(+), 0 deletions(-)
diff --git a/compress.c b/compress.c index 27c03d9..01fb3f0 100644 --- a/compress.c +++ b/compress.c @@ -296,6 +296,22 @@ int compress_get_hpointer(struct compress *compress, return 0; }
+int compress_get_tstamp(struct compress *compress, + unsigned long *samples, unsigned int *sampling_rate) +{ + struct snd_compr_tstamp ktstamp; + + if (!is_compress_ready(compress)) + return oops(compress, -ENODEV, "device not ready"); + + if (ioctl(compress->fd, SNDRV_COMPRESS_TSTAMP, &ktstamp)) + return oops(compress, errno, "cannot get tstamp"); + + *samples = ktstamp.pcm_io_frames; + *sampling_rate = ktstamp.sampling_rate; + return 0; +} + int compress_write(struct compress *compress, char *buf, unsigned int size) { struct snd_compr_avail avail; diff --git a/include/tinycompress/tinycompress.h b/include/tinycompress/tinycompress.h index 13dea3e..b1fbc65 100644 --- a/include/tinycompress/tinycompress.h +++ b/include/tinycompress/tinycompress.h @@ -73,6 +73,7 @@ struct compr_config { #define COMPRESS_IN 0x10000000
struct compress; +struct snd_compr_tstamp;
/* * compress_open: open a new compress stream @@ -103,6 +104,19 @@ void compress_close(struct compress *compress); */ int compress_get_hpointer(struct compress *compress, unsigned int *avail, struct timespec *tstamp); + + +/* + * compress_get_tstamp: get the raw hw timestamp + * return 0 on success, negative on error + * + * @compress: compress stream on which query is made + * @samples: number of decoded samples played + * @sampling_rate: sampling rate of decoded samples + */ +int compress_get_tstamp(struct compress *compress, + unsigned long *samples, unsigned int *sampling_rate); + /* * compress_write: write data to the compress stream * return bytes written on success, negative on error
On Fri, Feb 22, 2013 at 03:56:18PM +0000, Richard Fitzgerald wrote:
The compress_get_hpointer() converts the timestamp into actual time. But Android needs it in samples. To avoid the inefficiency of using compress_get_hpointer() and converting into time and then back into samples, this change adds compress_get_tstamp() which returns the raw sample count.
Applied all, thanks
compress.c | 16 ++++++++++++++++ include/tinycompress/tinycompress.h | 14 ++++++++++++++ 2 files changed, 30 insertions(+), 0 deletions(-)
diff --git a/compress.c b/compress.c index 27c03d9..01fb3f0 100644 --- a/compress.c +++ b/compress.c @@ -296,6 +296,22 @@ int compress_get_hpointer(struct compress *compress, return 0; }
+int compress_get_tstamp(struct compress *compress,
unsigned long *samples, unsigned int *sampling_rate)+{
- struct snd_compr_tstamp ktstamp;
- if (!is_compress_ready(compress))
return oops(compress, -ENODEV, "device not ready");- if (ioctl(compress->fd, SNDRV_COMPRESS_TSTAMP, &ktstamp))
return oops(compress, errno, "cannot get tstamp");- *samples = ktstamp.pcm_io_frames;
- *sampling_rate = ktstamp.sampling_rate;
- return 0;
+}
int compress_write(struct compress *compress, char *buf, unsigned int size) { struct snd_compr_avail avail; diff --git a/include/tinycompress/tinycompress.h b/include/tinycompress/tinycompress.h index 13dea3e..b1fbc65 100644 --- a/include/tinycompress/tinycompress.h +++ b/include/tinycompress/tinycompress.h @@ -73,6 +73,7 @@ struct compr_config { #define COMPRESS_IN 0x10000000
struct compress; +struct snd_compr_tstamp;
/*
- compress_open: open a new compress stream
@@ -103,6 +104,19 @@ void compress_close(struct compress *compress); */ int compress_get_hpointer(struct compress *compress, unsigned int *avail, struct timespec *tstamp);
+/*
- compress_get_tstamp: get the raw hw timestamp
- return 0 on success, negative on error
- @compress: compress stream on which query is made
- @samples: number of decoded samples played
- @sampling_rate: sampling rate of decoded samples
- */
+int compress_get_tstamp(struct compress *compress,
unsigned long *samples, unsigned int *sampling_rate);/*
- compress_write: write data to the compress stream
- return bytes written on success, negative on error
-- 1.7.2.5
participants (3)
-
Pierre-Louis Bossart -
Richard Fitzgerald -
Vinod Koul