[PATCH TINYCOMPRESS 1/14] compress: Add function to get timestamp in samples

[PATCH TINYCOMPRESS 1/14] compress: Add function to get timestamp in samples

[Richard Fitzgerald]

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

Re: [PATCH TINYCOMPRESS 1/14] compress: Add function to get timestamp in samples

[Vinod Koul]

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

Re: [PATCH TINYCOMPRESS 1/14] compress: Add function to get timestamp in samples

[Pierre-Louis Bossart]

>> 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

Re: [PATCH TINYCOMPRESS 1/14] compress: Add function to get timestamp in samples

[Richard Fitzgerald]

>>> 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.

Re: [PATCH TINYCOMPRESS 1/14] compress: Add function to get timestamp in samples

[Richard Fitzgerald]

>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.

Re: [PATCH TINYCOMPRESS 1/14] compress: Add function to get timestamp in samples

[Vinod Koul]

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

Re: [PATCH TINYCOMPRESS 1/14 v2] compress: Add function to get timestamp in samples

[Richard Fitzgerald]

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
-- 
1.7.2.5

Re: [PATCH TINYCOMPRESS 1/14 v2] compress: Add function to get timestamp in samples

[Vinod Koul]

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
>