[RESEND] pcm_rate: Do not discard slave reported delay in status result.

Message ID 52e39131-9141-b466-e92a-ff19ffab6d3a@IEE.org
State New
Headers show

Commit Message

Alan Young Nov. 17, 2016, 5:12 p.m.
On 17/11/16 15:24, Alan Young wrote:
>

> Similar to recent dshare patch.

>

Update with sign-off
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Comments

Takashi Iwai Nov. 28, 2016, 7:11 p.m. | #1
On Thu, 17 Nov 2016 18:12:57 +0100,
Alan Young wrote:
> 

> On 17/11/16 15:24, Alan Young wrote:

> >

> > Similar to recent dshare patch.

> >

> Update with sign-off


Sorry for the late follow up, I've been drug by other things for too
long.

> >From 6f93ee59846d2c0058ef702c1fa68d723bfb14f6 Mon Sep 17 00:00:00 2001

> From: Alan Young <consult.awy@gmail.com>

> Date: Tue, 14 Jun 2016 10:15:01 +0100

> Subject: [PATCH] pcm_rate: Do not discard slave reported delay in status

>  result.

> 

> snd_pcm_rate_status() gets the underlying status from the slave PCM.

> This may contain a delay value that includes elements such as codec and

> other transfer delays. Use this as the base for the returned delay

> value, adjusted for any frames buffered locally (within the rate

> plugin).

> 

> Also update snd_pcm_rate_delay() similarly.

> 

> Signed-off-by: Alan Young <consult.awy@gmail.com>

> ---

>  src/pcm/pcm_rate.c | 46 ++++++++++++++++++++++++++++++++++++++++------

>  1 file changed, 40 insertions(+), 6 deletions(-)

> 

> diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c

> index 6184def..15383ae 100644

> --- a/src/pcm/pcm_rate.c

> +++ b/src/pcm/pcm_rate.c

> @@ -559,10 +559,9 @@ snd_pcm_rate_read_areas1(snd_pcm_t *pcm,

>  		   pcm->channels, rate);

>  }

>  

> -static inline void snd_pcm_rate_sync_hwptr(snd_pcm_t *pcm)

> +static inline void snd_pcm_rate_sync_hwptr0(snd_pcm_t *pcm, snd_pcm_uframes_t slave_hw_ptr)

>  {

>  	snd_pcm_rate_t *rate = pcm->private_data;

> -	snd_pcm_uframes_t slave_hw_ptr = *rate->gen.slave->hw.ptr;

>  

>  	if (pcm->stream != SND_PCM_STREAM_PLAYBACK)

>  		return;

> @@ -576,6 +575,12 @@ static inline void snd_pcm_rate_sync_hwptr(snd_pcm_t *pcm)

>  	rate->hw_ptr %= pcm->boundary;

>  }

>  

> +static inline void snd_pcm_rate_sync_hwptr(snd_pcm_t *pcm)

> +{

> +	snd_pcm_rate_t *rate = pcm->private_data;

> +	snd_pcm_rate_sync_hwptr0(pcm, *rate->gen.slave->hw.ptr);

> +}

> +

>  static int snd_pcm_rate_hwsync(snd_pcm_t *pcm)

>  {

>  	snd_pcm_rate_t *rate = pcm->private_data;

> @@ -586,10 +591,37 @@ static int snd_pcm_rate_hwsync(snd_pcm_t *pcm)

>  	return 0;

>  }

>  

> +static snd_pcm_uframes_t snd_pcm_rate_playback_internal_delay(snd_pcm_t *pcm)

> +{

> +	snd_pcm_rate_t *rate = pcm->private_data;

> +

> +	if (rate->appl_ptr < rate->last_commit_ptr) {

> +		return rate->appl_ptr - rate->last_commit_ptr + pcm->boundary;

> +	} else {

> +		return rate->appl_ptr - rate->last_commit_ptr;

> +	}

> +}

> +

>  static int snd_pcm_rate_delay(snd_pcm_t *pcm, snd_pcm_sframes_t *delayp)

>  {

> +	snd_pcm_rate_t *rate = pcm->private_data;

> +	snd_pcm_sframes_t slave_delay;

> +	int err;

> +

>  	snd_pcm_rate_hwsync(pcm);

> -	*delayp = snd_pcm_mmap_hw_avail(pcm);

> +

> +	err = snd_pcm_delay(rate->gen.slave, &slave_delay);

> +	if (err < 0) {

> +		return err;

> +	}

> +

> +	if (pcm->stream == SND_PCM_STREAM_PLAYBACK) {

> +		*delayp = rate->ops.input_frames(rate->obj, slave_delay)

> +				+ snd_pcm_rate_playback_internal_delay(pcm);

> +	} else {

> +		*delayp = rate->ops.output_frames(rate->obj, slave_delay)

> +				+ snd_pcm_mmap_capture_hw_avail(pcm);

> +	}


Hmm, I'm not 100% sure through a quick review whether it's the correct
calculation.  Maybe it's worth to give more comments either in the
code or in the changelog.


>  	return 0;

>  }

>  

> @@ -1083,15 +1115,17 @@ static int snd_pcm_rate_status(snd_pcm_t *pcm, snd_pcm_status_t * status)

>  			status->state = SND_PCM_STATE_RUNNING;

>  		status->trigger_tstamp = rate->trigger_tstamp;

>  	}

> -	snd_pcm_rate_sync_hwptr(pcm);

> +	snd_pcm_rate_sync_hwptr0(rate, status->hw_ptr);


This can't work.

I can fix it in my side, but OTOH, this made me wonder how you tested
the patch...

>  	status->appl_ptr = *pcm->appl.ptr;

>  	status->hw_ptr = *pcm->hw.ptr;

>  	if (pcm->stream == SND_PCM_STREAM_PLAYBACK) {

> -		status->delay = snd_pcm_mmap_playback_hw_avail(pcm);

> +		status->delay = rate->ops.input_frames(rate->obj, status->delay)

> +					+ snd_pcm_rate_playback_internal_delay(pcm);

>  		status->avail = snd_pcm_mmap_playback_avail(pcm);

>  		status->avail_max = rate->ops.input_frames(rate->obj, status->avail_max);

>  	} else {

> -		status->delay = snd_pcm_mmap_capture_hw_avail(pcm);

> +		status->delay = rate->ops.output_frames(rate->obj, status->delay)

> +					+ snd_pcm_mmap_capture_hw_avail(pcm);

>  		status->avail = snd_pcm_mmap_capture_avail(pcm);

>  		status->avail_max = rate->ops.output_frames(rate->obj, status->avail_max);


Why only playback needs the special handling while the capture
doesn't?  Again, some comments would be helpful for better
understanding your changes.


thanks,

Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Alan Young Dec. 13, 2016, 11:43 a.m. | #2
Hi,

Sorry for the delay. I too got sidetracked by other work.

On 28/11/16 19:11, Takashi Iwai wrote:
>> >@@ -1083,15 +1115,17 @@ static int snd_pcm_rate_status(snd_pcm_t *pcm, snd_pcm_status_t * status)

>> >  			status->state = SND_PCM_STATE_RUNNING;

>> >  		status->trigger_tstamp = rate->trigger_tstamp;

>> >  	}

>> >-	snd_pcm_rate_sync_hwptr(pcm);

>> >+	snd_pcm_rate_sync_hwptr0(rate, status->hw_ptr);

> This can't work.

>

> I can fix it in my side, but OTOH, this made me wonder how you tested

> the patch...


Why do you think that cannot work? I am pretty sure it is working just 
fine for us but I am sure that it is possible that I have missed something.

>> >  	status->appl_ptr = *pcm->appl.ptr;

>> >  	status->hw_ptr = *pcm->hw.ptr;

>> >  	if (pcm->stream == SND_PCM_STREAM_PLAYBACK) {

>> >-		status->delay = snd_pcm_mmap_playback_hw_avail(pcm);

>> >+		status->delay = rate->ops.input_frames(rate->obj, status->delay)

>> >+					+ snd_pcm_rate_playback_internal_delay(pcm);

>> >  		status->avail = snd_pcm_mmap_playback_avail(pcm);

>> >  		status->avail_max = rate->ops.input_frames(rate->obj, status->avail_max);

>> >  	} else {

>> >-		status->delay = snd_pcm_mmap_capture_hw_avail(pcm);

>> >+		status->delay = rate->ops.output_frames(rate->obj, status->delay)

>> >+					+ snd_pcm_mmap_capture_hw_avail(pcm);

>> >  		status->avail = snd_pcm_mmap_capture_avail(pcm);

>> >  		status->avail_max = rate->ops.output_frames(rate->obj, status->avail_max);

> Why only playback needs the special handling while the capture

> doesn't?  Again, some comments would be helpful for better

> understanding your changes.

>


I expect that it could be useful to do something similar with capture 
but I could not work out what would be needed. OTOH, I think that the 
way that capture is used, and the way the underlying audio drivers tend 
to deal with this issue for capture is such that it may not be of much 
value.

I agree that comments are good. In general I was trying to stick with 
the pretty minimal-comment style of the existing code.

Alan.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Patch

From 6f93ee59846d2c0058ef702c1fa68d723bfb14f6 Mon Sep 17 00:00:00 2001
From: Alan Young <consult.awy@gmail.com>
Date: Tue, 14 Jun 2016 10:15:01 +0100
Subject: [PATCH] pcm_rate: Do not discard slave reported delay in status
 result.

snd_pcm_rate_status() gets the underlying status from the slave PCM.
This may contain a delay value that includes elements such as codec and
other transfer delays. Use this as the base for the returned delay
value, adjusted for any frames buffered locally (within the rate
plugin).

Also update snd_pcm_rate_delay() similarly.

Signed-off-by: Alan Young <consult.awy@gmail.com>
---
 src/pcm/pcm_rate.c | 46 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 40 insertions(+), 6 deletions(-)

diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c
index 6184def..15383ae 100644
--- a/src/pcm/pcm_rate.c
+++ b/src/pcm/pcm_rate.c
@@ -559,10 +559,9 @@  snd_pcm_rate_read_areas1(snd_pcm_t *pcm,
 		   pcm->channels, rate);
 }
 
-static inline void snd_pcm_rate_sync_hwptr(snd_pcm_t *pcm)
+static inline void snd_pcm_rate_sync_hwptr0(snd_pcm_t *pcm, snd_pcm_uframes_t slave_hw_ptr)
 {
 	snd_pcm_rate_t *rate = pcm->private_data;
-	snd_pcm_uframes_t slave_hw_ptr = *rate->gen.slave->hw.ptr;
 
 	if (pcm->stream != SND_PCM_STREAM_PLAYBACK)
 		return;
@@ -576,6 +575,12 @@  static inline void snd_pcm_rate_sync_hwptr(snd_pcm_t *pcm)
 	rate->hw_ptr %= pcm->boundary;
 }
 
+static inline void snd_pcm_rate_sync_hwptr(snd_pcm_t *pcm)
+{
+	snd_pcm_rate_t *rate = pcm->private_data;
+	snd_pcm_rate_sync_hwptr0(pcm, *rate->gen.slave->hw.ptr);
+}
+
 static int snd_pcm_rate_hwsync(snd_pcm_t *pcm)
 {
 	snd_pcm_rate_t *rate = pcm->private_data;
@@ -586,10 +591,37 @@  static int snd_pcm_rate_hwsync(snd_pcm_t *pcm)
 	return 0;
 }
 
+static snd_pcm_uframes_t snd_pcm_rate_playback_internal_delay(snd_pcm_t *pcm)
+{
+	snd_pcm_rate_t *rate = pcm->private_data;
+
+	if (rate->appl_ptr < rate->last_commit_ptr) {
+		return rate->appl_ptr - rate->last_commit_ptr + pcm->boundary;
+	} else {
+		return rate->appl_ptr - rate->last_commit_ptr;
+	}
+}
+
 static int snd_pcm_rate_delay(snd_pcm_t *pcm, snd_pcm_sframes_t *delayp)
 {
+	snd_pcm_rate_t *rate = pcm->private_data;
+	snd_pcm_sframes_t slave_delay;
+	int err;
+
 	snd_pcm_rate_hwsync(pcm);
-	*delayp = snd_pcm_mmap_hw_avail(pcm);
+
+	err = snd_pcm_delay(rate->gen.slave, &slave_delay);
+	if (err < 0) {
+		return err;
+	}
+
+	if (pcm->stream == SND_PCM_STREAM_PLAYBACK) {
+		*delayp = rate->ops.input_frames(rate->obj, slave_delay)
+				+ snd_pcm_rate_playback_internal_delay(pcm);
+	} else {
+		*delayp = rate->ops.output_frames(rate->obj, slave_delay)
+				+ snd_pcm_mmap_capture_hw_avail(pcm);
+	}
 	return 0;
 }
 
@@ -1083,15 +1115,17 @@  static int snd_pcm_rate_status(snd_pcm_t *pcm, snd_pcm_status_t * status)
 			status->state = SND_PCM_STATE_RUNNING;
 		status->trigger_tstamp = rate->trigger_tstamp;
 	}
-	snd_pcm_rate_sync_hwptr(pcm);
+	snd_pcm_rate_sync_hwptr0(rate, status->hw_ptr);
 	status->appl_ptr = *pcm->appl.ptr;
 	status->hw_ptr = *pcm->hw.ptr;
 	if (pcm->stream == SND_PCM_STREAM_PLAYBACK) {
-		status->delay = snd_pcm_mmap_playback_hw_avail(pcm);
+		status->delay = rate->ops.input_frames(rate->obj, status->delay)
+					+ snd_pcm_rate_playback_internal_delay(pcm);
 		status->avail = snd_pcm_mmap_playback_avail(pcm);
 		status->avail_max = rate->ops.input_frames(rate->obj, status->avail_max);
 	} else {
-		status->delay = snd_pcm_mmap_capture_hw_avail(pcm);
+		status->delay = rate->ops.output_frames(rate->obj, status->delay)
+					+ snd_pcm_mmap_capture_hw_avail(pcm);
 		status->avail = snd_pcm_mmap_capture_avail(pcm);
 		status->avail_max = rate->ops.output_frames(rate->obj, status->avail_max);
 	}
-- 
2.5.5