[PATCH 0/9] Misc fixes related to rewinds

[PATCH 0/9] Misc fixes related to rewinds

[Alexander E. Patrakov]

The idea of the series is to fix the two issues that I found [1] for the
hw plugin. snd_pcm_rewindable() sometimes returned negative values that
are actually negative amounts of samples and not error codes. Also, it
bases its calculations on stale hardware position pointer, which is not
what PulseAudio wants (alternatively, we can document the need to call
snd_pcm_avail() before snd_pcm_rewindable(), but I don't like it).

Also, similar issues in other plugins are fixed, except for "share" and
"shm" plugins that I could not really test due to unrelated crashes. I also
fixed miscelanneous cosmetic issues and bugs that I found along the way.

Note: this series touches pcm_dmix.c, but does not make it rewindable. In
other words, a variant of the test in [2] now produces a tone instead of
failing due to snd_pcm_rewind() returning 0. But it should ideally produce
silence. Obviously, there is some bug left that I have not pinpointed yet.

Same for dshare: the test produces a tone, and I don't yet know why.

[1] http://permalink.gmane.org/gmane.linux.alsa.devel/122843 and
    http://permalink.gmane.org/gmane.linux.alsa.devel/122848 (modify the
    test program to set the stop threshold larger than the buffer size)
[2] http://permalink.gmane.org/gmane.linux.alsa.devel/122179

Alexander E. Patrakov (9):
  dmix: actually rewind when running or being drained
  pcm: express the rewind size limitation logic better
  pcm: handle negative values from snd_pcm_mmap_hw_avail
  pcm, rate: use the snd_pcm_mmap_hw_avail function
  pcm, null: use the snd_pcm_mmap_avail function
  rate: handle negative values from snd_pcm_mmap_playback_hw_avail
  dsnoop: rewindable and forwardable logic was swapped
  pcm: rewindable, forwardable: don't return stale data
  pcm, file: don't recurse in the rewindable and forwardable callbacks

 src/pcm/pcm_dmix.c   | 20 ++++++++++++++------
 src/pcm/pcm_dshare.c | 16 +++++++++-------
 src/pcm/pcm_dsnoop.c | 18 ++++++++++--------
 src/pcm/pcm_file.c   |  4 ++--
 src/pcm/pcm_hw.c     |  8 +++++++-
 src/pcm/pcm_ioplug.c |  4 +++-
 src/pcm/pcm_local.h  | 18 ++++++++++++++++++
 src/pcm/pcm_null.c   |  5 +----
 src/pcm/pcm_plugin.c | 12 +++++++++---
 src/pcm/pcm_rate.c   |  9 +++------
 10 files changed, 76 insertions(+), 38 deletions(-)

-- 
2.1.0

[PATCH 1/9] dmix: actually rewind when running or being drained

[Alexander E. Patrakov]

Signed-off-by: Alexander E. Patrakov <patrakov@gmail.com>
---
 src/pcm/pcm_dmix.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/pcm/pcm_dmix.c b/src/pcm/pcm_dmix.c
index 7c53509..73cbe3f 100644
--- a/src/pcm/pcm_dmix.c
+++ b/src/pcm/pcm_dmix.c
@@ -669,11 +669,15 @@ static snd_pcm_sframes_t snd_pcm_dmix_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t f
 	snd_pcm_direct_t *dmix = pcm->private_data;
 	snd_pcm_uframes_t slave_appl_ptr, slave_size;
 	snd_pcm_uframes_t appl_ptr, size, transfer, result;
+	int err;
 	const snd_pcm_channel_area_t *src_areas, *dst_areas;
 
 	if (dmix->state == SND_PCM_STATE_RUNNING ||
-	    dmix->state == SND_PCM_STATE_DRAINING)
-	    	return snd_pcm_dmix_hwsync(pcm);
+	    dmix->state == SND_PCM_STATE_DRAINING) {
+		err = snd_pcm_dmix_hwsync(pcm);
+		if (err < 0)
+			return err;
+	}
 
 	if (dmix->last_appl_ptr < dmix->appl_ptr)
 		size = dmix->appl_ptr - dmix->last_appl_ptr;
-- 
2.1.0

[PATCH 2/9] pcm: express the rewind size limitation logic better

[Alexander E. Patrakov]

There are a few places where the argument of the .rewind or .forward
callback is checked against the same value as returned by .rewindable or
.forwardable. Express this "don't rewind more than rewindable" logic
explicitly, so that the future fixes to the rewindable size can go to
one function instead of two.

While at it, take advantage of the fact that snd_pcm_mmap_avail() cannot
return negative values (except due to integer overflow, which is AFAICS
impossible given the current boundary choice).

Signed-off-by: Alexander E. Patrakov <patrakov@gmail.com>
---
 src/pcm/pcm_dmix.c   | 4 +---
 src/pcm/pcm_dshare.c | 6 ++----
 src/pcm/pcm_dsnoop.c | 6 ++----
 src/pcm/pcm_plugin.c | 4 ++--
 4 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/src/pcm/pcm_dmix.c b/src/pcm/pcm_dmix.c
index 73cbe3f..ffde12a 100644
--- a/src/pcm/pcm_dmix.c
+++ b/src/pcm/pcm_dmix.c
@@ -751,9 +751,7 @@ static snd_pcm_sframes_t snd_pcm_dmix_forward(snd_pcm_t *pcm, snd_pcm_uframes_t
 {
 	snd_pcm_sframes_t avail;
 
-	avail = snd_pcm_mmap_playback_avail(pcm);
-	if (avail < 0)
-		return 0;
+	avail = snd_pcm_dmix_forwardable(pcm);
 	if (frames > (snd_pcm_uframes_t)avail)
 		frames = avail;
 	snd_pcm_mmap_appl_forward(pcm, frames);
diff --git a/src/pcm/pcm_dshare.c b/src/pcm/pcm_dshare.c
index b985172..f1a1a1d 100644
--- a/src/pcm/pcm_dshare.c
+++ b/src/pcm/pcm_dshare.c
@@ -419,7 +419,7 @@ static snd_pcm_sframes_t snd_pcm_dshare_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t
 {
 	snd_pcm_sframes_t avail;
 
-	avail = snd_pcm_mmap_playback_hw_avail(pcm);
+	avail = snd_pcm_dshare_rewindable(pcm);
 	if (avail < 0)
 		return 0;
 	if (frames > (snd_pcm_uframes_t)avail)
@@ -437,9 +437,7 @@ static snd_pcm_sframes_t snd_pcm_dshare_forward(snd_pcm_t *pcm, snd_pcm_uframes_
 {
 	snd_pcm_sframes_t avail;
 
-	avail = snd_pcm_mmap_playback_avail(pcm);
-	if (avail < 0)
-		return 0;
+	avail = snd_pcm_dshare_forwardable(pcm);
 	if (frames > (snd_pcm_uframes_t)avail)
 		frames = avail;
 	snd_pcm_mmap_appl_forward(pcm, frames);
diff --git a/src/pcm/pcm_dsnoop.c b/src/pcm/pcm_dsnoop.c
index 0f9c9df..e56e402 100644
--- a/src/pcm/pcm_dsnoop.c
+++ b/src/pcm/pcm_dsnoop.c
@@ -342,9 +342,7 @@ static snd_pcm_sframes_t snd_pcm_dsnoop_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t
 {
 	snd_pcm_sframes_t avail;
 
-	avail = snd_pcm_mmap_capture_avail(pcm);
-	if (avail < 0)
-		return 0;
+	avail = snd_pcm_dsnoop_rewindable(pcm);
 	if (frames > (snd_pcm_uframes_t)avail)
 		frames = avail;
 	snd_pcm_mmap_appl_backward(pcm, frames);
@@ -360,7 +358,7 @@ static snd_pcm_sframes_t snd_pcm_dsnoop_forward(snd_pcm_t *pcm, snd_pcm_uframes_
 {
 	snd_pcm_sframes_t avail;
 
-	avail = snd_pcm_mmap_capture_hw_avail(pcm);
+	avail = snd_pcm_dsnoop_forwardable(pcm);
 	if (avail < 0)
 		return 0;
 	if (frames > (snd_pcm_uframes_t)avail)
diff --git a/src/pcm/pcm_plugin.c b/src/pcm/pcm_plugin.c
index 4ddf10c..a607ccf 100644
--- a/src/pcm/pcm_plugin.c
+++ b/src/pcm/pcm_plugin.c
@@ -204,7 +204,7 @@ static snd_pcm_sframes_t snd_pcm_plugin_rewindable(snd_pcm_t *pcm)
 snd_pcm_sframes_t snd_pcm_plugin_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t frames)
 {
 	snd_pcm_plugin_t *plugin = pcm->private_data;
-	snd_pcm_sframes_t n = snd_pcm_mmap_hw_avail(pcm);
+	snd_pcm_sframes_t n = snd_pcm_plugin_rewindable(pcm);
 	snd_pcm_sframes_t sframes;
 
 	if ((snd_pcm_uframes_t)n < frames)
@@ -232,7 +232,7 @@ static snd_pcm_sframes_t snd_pcm_plugin_forwardable(snd_pcm_t *pcm)
 snd_pcm_sframes_t snd_pcm_plugin_forward(snd_pcm_t *pcm, snd_pcm_uframes_t frames)
 {
 	snd_pcm_plugin_t *plugin = pcm->private_data;
-	snd_pcm_sframes_t n = snd_pcm_mmap_avail(pcm);
+	snd_pcm_sframes_t n = snd_pcm_plugin_forwardable(pcm);
 	snd_pcm_sframes_t sframes;
 
 	if ((snd_pcm_uframes_t)n < frames)
-- 
2.1.0

[PATCH 3/9] pcm: handle negative values from snd_pcm_mmap_hw_avail

[Alexander E. Patrakov]

Such negative values can happen when an underrun happens and xrun
detection is disabled. Another situation is if the device updated the
pointer before alsa-lib has a chance to detect the xrun.

The problem is that these negative values could propagate to the
snd_pcm_rewindable return value, where it is specified that negative
returns must be interpreted as error codes and not as negative amount of
samples.

Signed-off-by: Alexander E. Patrakov <patrakov@gmail.com>
---
 src/pcm/pcm_dmix.c   |  2 +-
 src/pcm/pcm_dshare.c |  4 +---
 src/pcm/pcm_hw.c     |  2 +-
 src/pcm/pcm_ioplug.c |  2 +-
 src/pcm/pcm_local.h  | 18 ++++++++++++++++++
 src/pcm/pcm_plugin.c |  2 +-
 6 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/src/pcm/pcm_dmix.c b/src/pcm/pcm_dmix.c
index ffde12a..babde6a 100644
--- a/src/pcm/pcm_dmix.c
+++ b/src/pcm/pcm_dmix.c
@@ -661,7 +661,7 @@ static int snd_pcm_dmix_pause(snd_pcm_t *pcm ATTRIBUTE_UNUSED, int enable ATTRIB
 
 static snd_pcm_sframes_t snd_pcm_dmix_rewindable(snd_pcm_t *pcm)
 {
-	return snd_pcm_mmap_hw_avail(pcm);
+	return snd_pcm_mmap_playback_hw_rewindable(pcm);
 }
 
 static snd_pcm_sframes_t snd_pcm_dmix_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t frames)
diff --git a/src/pcm/pcm_dshare.c b/src/pcm/pcm_dshare.c
index f1a1a1d..020e6f7 100644
--- a/src/pcm/pcm_dshare.c
+++ b/src/pcm/pcm_dshare.c
@@ -412,7 +412,7 @@ static int snd_pcm_dshare_pause(snd_pcm_t *pcm ATTRIBUTE_UNUSED, int enable ATTR
 
 static snd_pcm_sframes_t snd_pcm_dshare_rewindable(snd_pcm_t *pcm)
 {
-	return snd_pcm_mmap_playback_hw_avail(pcm);
+	return snd_pcm_mmap_playback_hw_rewindable(pcm);
 }
 
 static snd_pcm_sframes_t snd_pcm_dshare_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t frames)
@@ -420,8 +420,6 @@ static snd_pcm_sframes_t snd_pcm_dshare_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t
 	snd_pcm_sframes_t avail;
 
 	avail = snd_pcm_dshare_rewindable(pcm);
-	if (avail < 0)
-		return 0;
 	if (frames > (snd_pcm_uframes_t)avail)
 		frames = avail;
 	snd_pcm_mmap_appl_backward(pcm, frames);
diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c
index 74cff67..c34b766 100644
--- a/src/pcm/pcm_hw.c
+++ b/src/pcm/pcm_hw.c
@@ -659,7 +659,7 @@ static int snd_pcm_hw_pause(snd_pcm_t *pcm, int enable)
 
 static snd_pcm_sframes_t snd_pcm_hw_rewindable(snd_pcm_t *pcm)
 {
-	return snd_pcm_mmap_hw_avail(pcm);
+	return snd_pcm_mmap_hw_rewindable(pcm);
 }
 
 static snd_pcm_sframes_t snd_pcm_hw_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t frames)
diff --git a/src/pcm/pcm_ioplug.c b/src/pcm/pcm_ioplug.c
index 85a8891..fe9347c 100644
--- a/src/pcm/pcm_ioplug.c
+++ b/src/pcm/pcm_ioplug.c
@@ -503,7 +503,7 @@ static int snd_pcm_ioplug_pause(snd_pcm_t *pcm, int enable)
 
 static snd_pcm_sframes_t snd_pcm_ioplug_rewindable(snd_pcm_t *pcm)
 {
-	return snd_pcm_mmap_hw_avail(pcm);
+	return snd_pcm_mmap_hw_rewindable(pcm);
 }
 
 static snd_pcm_sframes_t snd_pcm_ioplug_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t frames)
diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h
index 74ebd60..394505f 100644
--- a/src/pcm/pcm_local.h
+++ b/src/pcm/pcm_local.h
@@ -464,6 +464,24 @@ static inline snd_pcm_sframes_t snd_pcm_mmap_hw_avail(snd_pcm_t *pcm)
 	return pcm->buffer_size - snd_pcm_mmap_avail(pcm);
 }
 
+static inline snd_pcm_sframes_t snd_pcm_mmap_playback_hw_rewindable(snd_pcm_t *pcm)
+{
+	snd_pcm_sframes_t ret = snd_pcm_mmap_playback_hw_avail(pcm);
+	return (ret >= 0) ? ret : 0;
+}
+
+static inline snd_pcm_sframes_t snd_pcm_mmap_capture_hw_rewindable(snd_pcm_t *pcm)
+{
+	snd_pcm_sframes_t ret = snd_pcm_mmap_capture_hw_avail(pcm);
+	return (ret >= 0) ? ret : 0;
+}
+
+static inline snd_pcm_uframes_t snd_pcm_mmap_hw_rewindable(snd_pcm_t *pcm)
+{
+	snd_pcm_sframes_t ret = snd_pcm_mmap_hw_avail(pcm);
+	return (ret >= 0) ? ret : 0;
+}
+
 static inline const snd_pcm_channel_area_t *snd_pcm_mmap_areas(snd_pcm_t *pcm)
 {
 	if (pcm->stopped_areas &&
diff --git a/src/pcm/pcm_plugin.c b/src/pcm/pcm_plugin.c
index a607ccf..c19e2f1 100644
--- a/src/pcm/pcm_plugin.c
+++ b/src/pcm/pcm_plugin.c
@@ -198,7 +198,7 @@ static int snd_pcm_plugin_reset(snd_pcm_t *pcm)
 
 static snd_pcm_sframes_t snd_pcm_plugin_rewindable(snd_pcm_t *pcm)
 {
-	return snd_pcm_mmap_hw_avail(pcm);
+	return snd_pcm_mmap_hw_rewindable(pcm);
 }
 
 snd_pcm_sframes_t snd_pcm_plugin_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t frames)
-- 
2.1.0

[PATCH 4/9] pcm, rate: use the snd_pcm_mmap_hw_avail function

[Alexander E. Patrakov]

instead of the open-coded equivalent

Signed-off-by: Alexander E. Patrakov <patrakov@gmail.com>
---
 src/pcm/pcm_rate.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c
index 5e811bb..b436a8e 100644
--- a/src/pcm/pcm_rate.c
+++ b/src/pcm/pcm_rate.c
@@ -593,10 +593,7 @@ static int snd_pcm_rate_hwsync(snd_pcm_t *pcm)
 static int snd_pcm_rate_delay(snd_pcm_t *pcm, snd_pcm_sframes_t *delayp)
 {
 	snd_pcm_rate_hwsync(pcm);
-	if (pcm->stream == SND_PCM_STREAM_PLAYBACK)
-		*delayp = snd_pcm_mmap_playback_hw_avail(pcm);
-	else
-		*delayp = snd_pcm_mmap_capture_hw_avail(pcm);
+	*delayp = snd_pcm_mmap_hw_avail(pcm);
 	return 0;
 }
 
-- 
2.1.0

[PATCH 5/9] pcm, null: use the snd_pcm_mmap_avail function

[Alexander E. Patrakov]

instead of the open-coded equivalent

Signed-off-by: Alexander E. Patrakov <patrakov@gmail.com>
---
 src/pcm/pcm_null.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/src/pcm/pcm_null.c b/src/pcm/pcm_null.c
index f11a102..0529820 100644
--- a/src/pcm/pcm_null.c
+++ b/src/pcm/pcm_null.c
@@ -86,10 +86,7 @@ static snd_pcm_sframes_t snd_pcm_null_avail_update(snd_pcm_t *pcm)
         if (null->state == SND_PCM_STATE_PREPARED) {
                 /* it is required to return the correct avail count for */
                 /* the prepared stream, otherwise the start is not called */
-                if (pcm->stream == SND_PCM_STREAM_PLAYBACK)
-                        return snd_pcm_mmap_playback_avail(pcm);
-                else
-                        return snd_pcm_mmap_capture_avail(pcm);
+                return snd_pcm_mmap_avail(pcm);
         }
 	return pcm->buffer_size;
 }
-- 
2.1.0

[PATCH 6/9] rate: handle negative values from snd_pcm_mmap_playback_hw_avail

[Alexander E. Patrakov]

Such negative returns are possible during an underrun if xrun detection
is disabled.

So, don't store the result in an unsigned variable (where it will
overflow), and postpone the trigger in such case, too.

Signed-off-by: Alexander E. Patrakov <patrakov@gmail.com>
---

The patch is only compile-tested and the second hunk may well be wrong.

There are also similar issues in pcm_share.c, but, as I don't completely
understand the code there and cannot test that plugin at all due to
unrelated crashes, there will be no patch from me.

 src/pcm/pcm_rate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c
index b436a8e..736d558 100644
--- a/src/pcm/pcm_rate.c
+++ b/src/pcm/pcm_rate.c
@@ -1058,7 +1058,7 @@ static snd_pcm_state_t snd_pcm_rate_state(snd_pcm_t *pcm)
 static int snd_pcm_rate_start(snd_pcm_t *pcm)
 {
 	snd_pcm_rate_t *rate = pcm->private_data;
-	snd_pcm_uframes_t avail;
+	snd_pcm_sframes_t avail;
 		
 	if (pcm->stream == SND_PCM_STREAM_CAPTURE)
 		return snd_pcm_start(rate->gen.slave);
@@ -1069,7 +1069,7 @@ static int snd_pcm_rate_start(snd_pcm_t *pcm)
 	gettimestamp(&rate->trigger_tstamp, pcm->tstamp_type);
 
 	avail = snd_pcm_mmap_playback_hw_avail(rate->gen.slave);
-	if (avail == 0) {
+	if (avail <= 0) {
 		/* postpone the trigger since we have no data committed yet */
 		rate->start_pending = 1;
 		return 0;
-- 
2.1.0

Re: [PATCH 6/9] rate: handle negative values from snd_pcm_mmap_playback_hw_avail

[Takashi Iwai]

At Sun, 14 Sep 2014 00:30:18 +0600,
Alexander E. Patrakov wrote:
> 
> Such negative returns are possible during an underrun if xrun detection
> is disabled.
> 
> So, don't store the result in an unsigned variable (where it will
> overflow), and postpone the trigger in such case, too.
> 
> Signed-off-by: Alexander E. Patrakov <patrakov@gmail.com>
> ---
> 
> The patch is only compile-tested and the second hunk may well be wrong.
> 
> There are also similar issues in pcm_share.c, but, as I don't completely
> understand the code there and cannot test that plugin at all due to
> unrelated crashes, there will be no patch from me.

In general, hw_avail must not be negative before starting the stream.
If it is, then it means most likely the driver problem, so we should
return error immediately instead.


Takashi

> 
>  src/pcm/pcm_rate.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c
> index b436a8e..736d558 100644
> --- a/src/pcm/pcm_rate.c
> +++ b/src/pcm/pcm_rate.c
> @@ -1058,7 +1058,7 @@ static snd_pcm_state_t snd_pcm_rate_state(snd_pcm_t *pcm)
>  static int snd_pcm_rate_start(snd_pcm_t *pcm)
>  {
>  	snd_pcm_rate_t *rate = pcm->private_data;
> -	snd_pcm_uframes_t avail;
> +	snd_pcm_sframes_t avail;
>  		
>  	if (pcm->stream == SND_PCM_STREAM_CAPTURE)
>  		return snd_pcm_start(rate->gen.slave);
> @@ -1069,7 +1069,7 @@ static int snd_pcm_rate_start(snd_pcm_t *pcm)
>  	gettimestamp(&rate->trigger_tstamp, pcm->tstamp_type);
>  
>  	avail = snd_pcm_mmap_playback_hw_avail(rate->gen.slave);
> -	if (avail == 0) {
> +	if (avail <= 0) {
>  		/* postpone the trigger since we have no data committed yet */
>  		rate->start_pending = 1;
>  		return 0;
> -- 
> 2.1.0
> 

Re: [PATCH 6/9] rate: handle negative values from snd_pcm_mmap_playback_hw_avail

[Alexander E. Patrakov]

15.09.2014 14:49, Takashi Iwai пишет:
> At Sun, 14 Sep 2014 00:30:18 +0600,
> Alexander E. Patrakov wrote:
>>
>> Such negative returns are possible during an underrun if xrun detection
>> is disabled.
>>
>> So, don't store the result in an unsigned variable (where it will
>> overflow), and postpone the trigger in such case, too.
>>
>> Signed-off-by: Alexander E. Patrakov <patrakov@gmail.com>
>> ---
>>
>> The patch is only compile-tested and the second hunk may well be wrong.
>>
>> There are also similar issues in pcm_share.c, but, as I don't completely
>> understand the code there and cannot test that plugin at all due to
>> unrelated crashes, there will be no patch from me.
>
> In general, hw_avail must not be negative before starting the stream.
> If it is, then it means most likely the driver problem, so we should
> return error immediately instead.

Thanks for the review. Would -EBADFD be a correct error code, or do you 
want something different? or maybe even an assert?

>
>
> Takashi
>
>>
>>   src/pcm/pcm_rate.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c
>> index b436a8e..736d558 100644
>> --- a/src/pcm/pcm_rate.c
>> +++ b/src/pcm/pcm_rate.c
>> @@ -1058,7 +1058,7 @@ static snd_pcm_state_t snd_pcm_rate_state(snd_pcm_t *pcm)
>>   static int snd_pcm_rate_start(snd_pcm_t *pcm)
>>   {
>>   	snd_pcm_rate_t *rate = pcm->private_data;
>> -	snd_pcm_uframes_t avail;
>> +	snd_pcm_sframes_t avail;
>>   		
>>   	if (pcm->stream == SND_PCM_STREAM_CAPTURE)
>>   		return snd_pcm_start(rate->gen.slave);
>> @@ -1069,7 +1069,7 @@ static int snd_pcm_rate_start(snd_pcm_t *pcm)
>>   	gettimestamp(&rate->trigger_tstamp, pcm->tstamp_type);
>>
>>   	avail = snd_pcm_mmap_playback_hw_avail(rate->gen.slave);
>> -	if (avail == 0) {
>> +	if (avail <= 0) {
>>   		/* postpone the trigger since we have no data committed yet */
>>   		rate->start_pending = 1;
>>   		return 0;
>> --
>> 2.1.0
>>


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

Re: [PATCH 6/9] rate: handle negative values from snd_pcm_mmap_playback_hw_avail

[Takashi Iwai]

At Mon, 15 Sep 2014 16:03:57 +0600,
Alexander E. Patrakov wrote:
> 
> 15.09.2014 14:49, Takashi Iwai пишет:
> > At Sun, 14 Sep 2014 00:30:18 +0600,
> > Alexander E. Patrakov wrote:
> >>
> >> Such negative returns are possible during an underrun if xrun detection
> >> is disabled.
> >>
> >> So, don't store the result in an unsigned variable (where it will
> >> overflow), and postpone the trigger in such case, too.
> >>
> >> Signed-off-by: Alexander E. Patrakov <patrakov@gmail.com>
> >> ---
> >>
> >> The patch is only compile-tested and the second hunk may well be wrong.
> >>
> >> There are also similar issues in pcm_share.c, but, as I don't completely
> >> understand the code there and cannot test that plugin at all due to
> >> unrelated crashes, there will be no patch from me.
> >
> > In general, hw_avail must not be negative before starting the stream.
> > If it is, then it means most likely the driver problem, so we should
> > return error immediately instead.
> 
> Thanks for the review. Would -EBADFD be a correct error code, or do you 
> want something different? or maybe even an assert?

I'd take either EPIPE or EBADFD.
An assert would be an overkill, IMO.


Takashi

> 
> >
> >
> > Takashi
> >
> >>
> >>   src/pcm/pcm_rate.c | 4 ++--
> >>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c
> >> index b436a8e..736d558 100644
> >> --- a/src/pcm/pcm_rate.c
> >> +++ b/src/pcm/pcm_rate.c
> >> @@ -1058,7 +1058,7 @@ static snd_pcm_state_t snd_pcm_rate_state(snd_pcm_t *pcm)
> >>   static int snd_pcm_rate_start(snd_pcm_t *pcm)
> >>   {
> >>   	snd_pcm_rate_t *rate = pcm->private_data;
> >> -	snd_pcm_uframes_t avail;
> >> +	snd_pcm_sframes_t avail;
> >>   		
> >>   	if (pcm->stream == SND_PCM_STREAM_CAPTURE)
> >>   		return snd_pcm_start(rate->gen.slave);
> >> @@ -1069,7 +1069,7 @@ static int snd_pcm_rate_start(snd_pcm_t *pcm)
> >>   	gettimestamp(&rate->trigger_tstamp, pcm->tstamp_type);
> >>
> >>   	avail = snd_pcm_mmap_playback_hw_avail(rate->gen.slave);
> >> -	if (avail == 0) {
> >> +	if (avail <= 0) {
> >>   		/* postpone the trigger since we have no data committed yet */
> >>   		rate->start_pending = 1;
> >>   		return 0;
> >> --
> >> 2.1.0
> >>
> 
> 
> -- 
> Alexander E. Patrakov
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 6/9] rate: handle negative values from snd_pcm_mmap_playback_hw_avail

[Alexander E. Patrakov]

[-- Attachment #1: Type: text/plain, Size: 1397 bytes --]

15.09.2014 16:14, Takashi Iwai wrote:
> At Mon, 15 Sep 2014 16:03:57 +0600,
> Alexander E. Patrakov wrote:
>>
>> 15.09.2014 14:49, Takashi Iwai пишет:
>>> At Sun, 14 Sep 2014 00:30:18 +0600,
>>> Alexander E. Patrakov wrote:
>>>>
>>>> Such negative returns are possible during an underrun if xrun detection
>>>> is disabled.
>>>>
>>>> So, don't store the result in an unsigned variable (where it will
>>>> overflow), and postpone the trigger in such case, too.
>>>>
>>>> Signed-off-by: Alexander E. Patrakov <patrakov@gmail.com>
>>>> ---
>>>>
>>>> The patch is only compile-tested and the second hunk may well be wrong.
>>>>
>>>> There are also similar issues in pcm_share.c, but, as I don't completely
>>>> understand the code there and cannot test that plugin at all due to
>>>> unrelated crashes, there will be no patch from me.
>>>
>>> In general, hw_avail must not be negative before starting the stream.
>>> If it is, then it means most likely the driver problem, so we should
>>> return error immediately instead.
>>
>> Thanks for the review. Would -EBADFD be a correct error code, or do you
>> want something different? or maybe even an assert?
>
> I'd take either EPIPE or EBADFD.
> An assert would be an overkill, IMO.

I have sent the fix to the list, but nobody reacted. Resending as an 
attachment to this message.

-- 
Alexander E. Patrakov

[-- Attachment #2: 0001-pcm-rate-hw_avail-must-not-be-negative-before-starti.patch --]
[-- Type: text/x-patch, Size: 1079 bytes --]

>From 81d3b1b107e0f55f041eb9df8f6c3edd3e6450f4 Mon Sep 17 00:00:00 2001
From: "Alexander E. Patrakov" <patrakov@gmail.com>
Date: Mon, 15 Sep 2014 20:17:47 +0600
Subject: [PATCH] pcm, rate: hw_avail must not be negative before starting the
 stream

If it is, then it means most likely the driver problem, so we should
return error immediately instead.

Signed-off-by: Alexander E. Patrakov <patrakov@gmail.com>
---

As suggested by Takashi Iwai.

 src/pcm/pcm_rate.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c
index 736d558..c76db25 100644
--- a/src/pcm/pcm_rate.c
+++ b/src/pcm/pcm_rate.c
@@ -1069,7 +1069,10 @@ static int snd_pcm_rate_start(snd_pcm_t *pcm)
 	gettimestamp(&rate->trigger_tstamp, pcm->tstamp_type);
 
 	avail = snd_pcm_mmap_playback_hw_avail(rate->gen.slave);
-	if (avail <= 0) {
+	if (avail < 0) /* can't happen on healthy drivers */
+		return -EBADFD;
+
+	if (avail == 0) {
 		/* postpone the trigger since we have no data committed yet */
 		rate->start_pending = 1;
 		return 0;
-- 
2.1.0


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 6/9] rate: handle negative values from snd_pcm_mmap_playback_hw_avail

[Jaroslav Kysela]

Date 16.9.2014 17:52, Alexander E. Patrakov wrote:
> 15.09.2014 16:14, Takashi Iwai wrote:
>> At Mon, 15 Sep 2014 16:03:57 +0600,
>> Alexander E. Patrakov wrote:
>>>
>>> 15.09.2014 14:49, Takashi Iwai пишет:
>>>> At Sun, 14 Sep 2014 00:30:18 +0600,
>>>> Alexander E. Patrakov wrote:
>>>>>
>>>>> Such negative returns are possible during an underrun if xrun detection
>>>>> is disabled.
>>>>>
>>>>> So, don't store the result in an unsigned variable (where it will
>>>>> overflow), and postpone the trigger in such case, too.
>>>>>
>>>>> Signed-off-by: Alexander E. Patrakov <patrakov@gmail.com>
>>>>> ---
>>>>>
>>>>> The patch is only compile-tested and the second hunk may well be wrong.
>>>>>
>>>>> There are also similar issues in pcm_share.c, but, as I don't completely
>>>>> understand the code there and cannot test that plugin at all due to
>>>>> unrelated crashes, there will be no patch from me.
>>>>
>>>> In general, hw_avail must not be negative before starting the stream.
>>>> If it is, then it means most likely the driver problem, so we should
>>>> return error immediately instead.
>>>
>>> Thanks for the review. Would -EBADFD be a correct error code, or do you
>>> want something different? or maybe even an assert?
>>
>> I'd take either EPIPE or EBADFD.
>> An assert would be an overkill, IMO.
> 
> I have sent the fix to the list, but nobody reacted. Resending as an 
> attachment to this message.

Applied now. Thanks.

                         Jaroslav

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


-- 
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project; Red Hat, Inc.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 6/9] rate: handle negative values from snd_pcm_mmap_playback_hw_avail

[Takashi Iwai]

At Tue, 16 Sep 2014 21:52:51 +0600,
Alexander E. Patrakov wrote:
> 
> 15.09.2014 16:14, Takashi Iwai wrote:
> > At Mon, 15 Sep 2014 16:03:57 +0600,
> > Alexander E. Patrakov wrote:
> >>
> >> 15.09.2014 14:49, Takashi Iwai пишет:
> >>> At Sun, 14 Sep 2014 00:30:18 +0600,
> >>> Alexander E. Patrakov wrote:
> >>>>
> >>>> Such negative returns are possible during an underrun if xrun detection
> >>>> is disabled.
> >>>>
> >>>> So, don't store the result in an unsigned variable (where it will
> >>>> overflow), and postpone the trigger in such case, too.
> >>>>
> >>>> Signed-off-by: Alexander E. Patrakov <patrakov@gmail.com>
> >>>> ---
> >>>>
> >>>> The patch is only compile-tested and the second hunk may well be wrong.
> >>>>
> >>>> There are also similar issues in pcm_share.c, but, as I don't completely
> >>>> understand the code there and cannot test that plugin at all due to
> >>>> unrelated crashes, there will be no patch from me.
> >>>
> >>> In general, hw_avail must not be negative before starting the stream.
> >>> If it is, then it means most likely the driver problem, so we should
> >>> return error immediately instead.
> >>
> >> Thanks for the review. Would -EBADFD be a correct error code, or do you
> >> want something different? or maybe even an assert?
> >
> > I'd take either EPIPE or EBADFD.
> > An assert would be an overkill, IMO.
> 
> I have sent the fix to the list, but nobody reacted. Resending as an 
> attachment to this message.

I'm traveling in this week, so please don't expect reactions in light
speed.


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

[PATCH 7/9] dsnoop: rewindable and forwardable logic was swapped

[Alexander E. Patrakov]

Signed-off-by: Alexander E. Patrakov <patrakov@gmail.com>
---
 src/pcm/pcm_dsnoop.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/pcm/pcm_dsnoop.c b/src/pcm/pcm_dsnoop.c
index e56e402..8333eef 100644
--- a/src/pcm/pcm_dsnoop.c
+++ b/src/pcm/pcm_dsnoop.c
@@ -335,7 +335,7 @@ static int snd_pcm_dsnoop_pause(snd_pcm_t *pcm ATTRIBUTE_UNUSED, int enable ATTR
 
 static snd_pcm_sframes_t snd_pcm_dsnoop_rewindable(snd_pcm_t *pcm)
 {
-	return snd_pcm_mmap_capture_avail(pcm);
+	return snd_pcm_mmap_capture_hw_avail(pcm);
 }
 
 static snd_pcm_sframes_t snd_pcm_dsnoop_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t frames)
@@ -351,7 +351,7 @@ static snd_pcm_sframes_t snd_pcm_dsnoop_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t
 
 static snd_pcm_sframes_t snd_pcm_dsnoop_forwardable(snd_pcm_t *pcm)
 {
-	return snd_pcm_mmap_capture_hw_avail(pcm);
+	return snd_pcm_mmap_capture_avail(pcm);
 }
 
 static snd_pcm_sframes_t snd_pcm_dsnoop_forward(snd_pcm_t *pcm, snd_pcm_uframes_t frames)
@@ -359,8 +359,6 @@ static snd_pcm_sframes_t snd_pcm_dsnoop_forward(snd_pcm_t *pcm, snd_pcm_uframes_
 	snd_pcm_sframes_t avail;
 
 	avail = snd_pcm_dsnoop_forwardable(pcm);
-	if (avail < 0)
-		return 0;
 	if (frames > (snd_pcm_uframes_t)avail)
 		frames = avail;
 	snd_pcm_mmap_appl_forward(pcm, frames);
-- 
2.1.0

[PATCH 8/9] pcm: rewindable, forwardable: don't return stale data

[Alexander E. Patrakov]

The current behavior of snd_pcm_rewindable and snd_pcm_forwardable means
that the returned value is only accurate to one period. Or maybe even
meaningless if period interrupts are off. Fetch the up-to-date position
of the hardware pointer, as that's what is wanted by callers.

Signed-off-by: Alexander E. Patrakov <patrakov@gmail.com>
---
 src/pcm/pcm_dmix.c   | 6 ++++++
 src/pcm/pcm_dshare.c | 6 ++++++
 src/pcm/pcm_dsnoop.c | 6 ++++++
 src/pcm/pcm_hw.c     | 6 ++++++
 src/pcm/pcm_ioplug.c | 2 ++
 src/pcm/pcm_plugin.c | 6 ++++++
 6 files changed, 32 insertions(+)

diff --git a/src/pcm/pcm_dmix.c b/src/pcm/pcm_dmix.c
index babde6a..d9565f9 100644
--- a/src/pcm/pcm_dmix.c
+++ b/src/pcm/pcm_dmix.c
@@ -661,6 +661,9 @@ static int snd_pcm_dmix_pause(snd_pcm_t *pcm ATTRIBUTE_UNUSED, int enable ATTRIB
 
 static snd_pcm_sframes_t snd_pcm_dmix_rewindable(snd_pcm_t *pcm)
 {
+	int err = snd_pcm_dmix_hwsync(pcm);
+	if (err < 0)
+		return err;
 	return snd_pcm_mmap_playback_hw_rewindable(pcm);
 }
 
@@ -744,6 +747,9 @@ static snd_pcm_sframes_t snd_pcm_dmix_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t f
 
 static snd_pcm_sframes_t snd_pcm_dmix_forwardable(snd_pcm_t *pcm)
 {
+	int err = snd_pcm_dmix_hwsync(pcm);
+	if (err < 0)
+		return err;
 	return snd_pcm_mmap_avail(pcm);
 }
 
diff --git a/src/pcm/pcm_dshare.c b/src/pcm/pcm_dshare.c
index 020e6f7..de0b242 100644
--- a/src/pcm/pcm_dshare.c
+++ b/src/pcm/pcm_dshare.c
@@ -412,6 +412,9 @@ static int snd_pcm_dshare_pause(snd_pcm_t *pcm ATTRIBUTE_UNUSED, int enable ATTR
 
 static snd_pcm_sframes_t snd_pcm_dshare_rewindable(snd_pcm_t *pcm)
 {
+	int err = snd_pcm_dshare_hwsync(pcm);
+	if (err < 0)
+		return err;
 	return snd_pcm_mmap_playback_hw_rewindable(pcm);
 }
 
@@ -428,6 +431,9 @@ static snd_pcm_sframes_t snd_pcm_dshare_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t
 
 static snd_pcm_sframes_t snd_pcm_dshare_forwardable(snd_pcm_t *pcm)
 {
+	int err = snd_pcm_dshare_hwsync(pcm);
+	if (err < 0)
+		return err;
 	return snd_pcm_mmap_playback_avail(pcm);
 }
 
diff --git a/src/pcm/pcm_dsnoop.c b/src/pcm/pcm_dsnoop.c
index 8333eef..00cd461 100644
--- a/src/pcm/pcm_dsnoop.c
+++ b/src/pcm/pcm_dsnoop.c
@@ -335,6 +335,9 @@ static int snd_pcm_dsnoop_pause(snd_pcm_t *pcm ATTRIBUTE_UNUSED, int enable ATTR
 
 static snd_pcm_sframes_t snd_pcm_dsnoop_rewindable(snd_pcm_t *pcm)
 {
+	int err = snd_pcm_dsnoop_hwsync(pcm);
+	if (err < 0)
+		return err;
 	return snd_pcm_mmap_capture_hw_avail(pcm);
 }
 
@@ -351,6 +354,9 @@ static snd_pcm_sframes_t snd_pcm_dsnoop_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t
 
 static snd_pcm_sframes_t snd_pcm_dsnoop_forwardable(snd_pcm_t *pcm)
 {
+	int err = snd_pcm_dsnoop_hwsync(pcm);
+	if (err < 0)
+		return err;
 	return snd_pcm_mmap_capture_avail(pcm);
 }
 
diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c
index c34b766..4a52703 100644
--- a/src/pcm/pcm_hw.c
+++ b/src/pcm/pcm_hw.c
@@ -659,6 +659,9 @@ static int snd_pcm_hw_pause(snd_pcm_t *pcm, int enable)
 
 static snd_pcm_sframes_t snd_pcm_hw_rewindable(snd_pcm_t *pcm)
 {
+	int err = snd_pcm_hw_hwsync(pcm);
+	if (err < 0)
+		return err;
 	return snd_pcm_mmap_hw_rewindable(pcm);
 }
 
@@ -679,6 +682,9 @@ static snd_pcm_sframes_t snd_pcm_hw_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t fra
 
 static snd_pcm_sframes_t snd_pcm_hw_forwardable(snd_pcm_t *pcm)
 {
+	int err = snd_pcm_hw_hwsync(pcm);
+	if (err < 0)
+		return err;
 	return snd_pcm_mmap_avail(pcm);
 }
 
diff --git a/src/pcm/pcm_ioplug.c b/src/pcm/pcm_ioplug.c
index fe9347c..3861bc2 100644
--- a/src/pcm/pcm_ioplug.c
+++ b/src/pcm/pcm_ioplug.c
@@ -503,6 +503,7 @@ static int snd_pcm_ioplug_pause(snd_pcm_t *pcm, int enable)
 
 static snd_pcm_sframes_t snd_pcm_ioplug_rewindable(snd_pcm_t *pcm)
 {
+	snd_pcm_ioplug_hw_ptr_update(pcm);
 	return snd_pcm_mmap_hw_rewindable(pcm);
 }
 
@@ -514,6 +515,7 @@ static snd_pcm_sframes_t snd_pcm_ioplug_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t
 
 static snd_pcm_sframes_t snd_pcm_ioplug_forwardable(snd_pcm_t *pcm)
 {
+	snd_pcm_ioplug_hw_ptr_update(pcm);
 	return snd_pcm_mmap_avail(pcm);
 }
 
diff --git a/src/pcm/pcm_plugin.c b/src/pcm/pcm_plugin.c
index c19e2f1..57a1953 100644
--- a/src/pcm/pcm_plugin.c
+++ b/src/pcm/pcm_plugin.c
@@ -198,6 +198,9 @@ static int snd_pcm_plugin_reset(snd_pcm_t *pcm)
 
 static snd_pcm_sframes_t snd_pcm_plugin_rewindable(snd_pcm_t *pcm)
 {
+	int err = snd_pcm_generic_hwsync(pcm);
+	if (err < 0)
+		return err;
 	return snd_pcm_mmap_hw_rewindable(pcm);
 }
 
@@ -226,6 +229,9 @@ snd_pcm_sframes_t snd_pcm_plugin_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t frames
 
 static snd_pcm_sframes_t snd_pcm_plugin_forwardable(snd_pcm_t *pcm)
 {
+	int err = snd_pcm_generic_hwsync(pcm);
+	if (err < 0)
+		return err;
 	return snd_pcm_mmap_avail(pcm);
 }
 
-- 
2.1.0

Re: [PATCH 8/9] pcm: rewindable, forwardable: don't return stale data

[Raymond Yau]

>
> The current behavior of snd_pcm_rewindable and snd_pcm_forwardable means
> that the returned value is only accurate to one period. Or maybe even
> meaningless if period interrupts are off. Fetch the up-to-date position
> of the hardware pointer, as that's what is wanted by callers.

Why do dmix need to support rewind/forward since it also set the slave in
free run mode ?

>
> Signed-off-by: Alexander E. Patrakov <patrakov@gmail.com>
> ---
>  src/pcm/pcm_dmix.c   | 6 ++++++
>  src/pcm/pcm_dshare.c | 6 ++++++
>  src/pcm/pcm_dsnoop.c | 6 ++++++
>  src/pcm/pcm_hw.c     | 6 ++++++
>  src/pcm/pcm_ioplug.c | 2 ++
>  src/pcm/pcm_plugin.c | 6 ++++++
>  6 files changed, 32 insertions(+)
>
> diff --git a/src/pcm/pcm_dmix.c b/src/pcm/pcm_dmix.c
> index babde6a..d9565f9 100644
> --- a/src/pcm/pcm_dmix.c
> +++ b/src/pcm/pcm_dmix.c
> @@ -661,6 +661,9 @@ static int snd_pcm_dmix_pause(snd_pcm_t *pcm
ATTRIBUTE_UNUSED, int enable ATTRIB
>
>  static snd_pcm_sframes_t snd_pcm_dmix_rewindable(snd_pcm_t *pcm)
>  {
> +       int err = snd_pcm_dmix_hwsync(pcm);
> +       if (err < 0)
> +               return err;
>         return snd_pcm_mmap_playback_hw_rewindable(pcm);
>  }
>
> @@ -744,6 +747,9 @@ static snd_pcm_sframes_t
snd_pcm_dmix_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t f
>
>  static snd_pcm_sframes_t snd_pcm_dmix_forwardable(snd_pcm_t *pcm)
>  {
> +       int err = snd_pcm_dmix_hwsync(pcm);
> +       if (err < 0)
> +               return err;
>         return snd_pcm_mmap_avail(pcm);
>  }
>

[PATCH 9/9] pcm, file: don't recurse in the rewindable and forwardable callbacks

[Alexander E. Patrakov]

Signed-off-by: Alexander E. Patrakov <patrakov@gmail.com>
---
 src/pcm/pcm_file.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/pcm/pcm_file.c b/src/pcm/pcm_file.c
index a0b8bf4..5541a93 100644
--- a/src/pcm/pcm_file.c
+++ b/src/pcm/pcm_file.c
@@ -454,7 +454,7 @@ static int snd_pcm_file_drain(snd_pcm_t *pcm)
 static snd_pcm_sframes_t snd_pcm_file_rewindable(snd_pcm_t *pcm)
 {
 	snd_pcm_file_t *file = pcm->private_data;
-	snd_pcm_sframes_t res = snd_pcm_rewindable(pcm);
+	snd_pcm_sframes_t res = snd_pcm_rewindable(file->gen.slave);
 	snd_pcm_sframes_t n = snd_pcm_bytes_to_frames(pcm, file->wbuf_used_bytes);
 	if (res > n)
 		res = n;
@@ -482,7 +482,7 @@ static snd_pcm_sframes_t snd_pcm_file_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t f
 static snd_pcm_sframes_t snd_pcm_file_forwardable(snd_pcm_t *pcm)
 {
 	snd_pcm_file_t *file = pcm->private_data;
-	snd_pcm_sframes_t res = snd_pcm_forwardable(pcm);
+	snd_pcm_sframes_t res = snd_pcm_forwardable(file->gen.slave);
 	snd_pcm_sframes_t n = snd_pcm_bytes_to_frames(pcm, file->wbuf_size_bytes - file->wbuf_used_bytes);
 	if (res > n)
 		res = n;
-- 
2.1.0

Re: [PATCH 0/9] Misc fixes related to rewinds

[Jaroslav Kysela]

Date 13.9.2014 20:30, Alexander E. Patrakov wrote:
> The idea of the series is to fix the two issues that I found [1] for the

I applied all your patches to alsa-lib's repo, but...

> hw plugin. snd_pcm_rewindable() sometimes returned negative values that
> are actually negative amounts of samples and not error codes. Also, it
> bases its calculations on stale hardware position pointer, which is not
> what PulseAudio wants (alternatively, we can document the need to call
> snd_pcm_avail() before snd_pcm_rewindable(), but I don't like it).

The hw sync is expensive and the application might do this sync multiple
times when woken up. I think that it must be clear that:

1) only snd_pcm_avail(), snd_pcm_delay(), snd_pcm_avail_delay()
   does the real hw sync
2) snd_pcm_avail(), snd_pcm_delay(), snd_pcm_avail_delay(),
   snd_pcm_rewindable() and snd_pcm_forwardable() does
   hw sync (and change all plugins to respect this)

I don't like the situation "be somewhere between because it's good for
one purpose"...

			Thanks for your work,
                                           Jaroslav

BTW: I'm starting to think about the 1.0.29 release...

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project; Red Hat, Inc.

Re: [PATCH 0/9] Misc fixes related to rewinds

[Alexander E. Patrakov]

14.09.2014 01:14, Jaroslav Kysela wrote:
> Date 13.9.2014 20:30, Alexander E. Patrakov wrote:
>> The idea of the series is to fix the two issues that I found [1] for the
>
> I applied all your patches to alsa-lib's repo, but...
>
>> hw plugin. snd_pcm_rewindable() sometimes returned negative values that
>> are actually negative amounts of samples and not error codes. Also, it
>> bases its calculations on stale hardware position pointer, which is not
>> what PulseAudio wants (alternatively, we can document the need to call
>> snd_pcm_avail() before snd_pcm_rewindable(), but I don't like it).
>
> The hw sync is expensive and the application might do this sync multiple
> times when woken up. I think that it must be clear that:
>
> 1) only snd_pcm_avail(), snd_pcm_delay(), snd_pcm_avail_delay()
>     does the real hw sync
> 2) snd_pcm_avail(), snd_pcm_delay(), snd_pcm_avail_delay(),
>     snd_pcm_rewindable() and snd_pcm_forwardable() does
>     hw sync (and change all plugins to respect this)
>
> I don't like the situation "be somewhere between because it's good for
> one purpose"...

I understand the concern. I have specifically not added the call to 
hwsync directly to snd_pcm_rewindable implementation (although it would 
have resulted in a smaller patch), because that would indeed cause 
double-hwsync and the resulting inefficiency. I made sure that all 
plugins either make the hwsync thing themselves or rely on the slave to 
do that for them, but not both. If you find an error and/or spot a case 
of a double-hwsync in a plugin chain, please complain.

One known case of double-hwsync is the following pattern: an application 
calls snd_pcm_rewindable(), thinks about it, and then calls 
snd_pcm_rewind(). Which, due to PATCH 2/9, calls the rewindable callback 
again, resulting in the second hwsync. I don't know which way out is 
best: either ignore, or revert the intention of PATCH 2/9, or revert the 
whole PATCH 8/9 and replace it with a documentation change.

OTOH, I made a mistake of not adding David Henningsson to the CC list 
during the initial submission. If PulseAudio would need to synchronize 
hardware pointers even after conversion to snd_pcm_rewindable() for some 
other reason, then the need for PATCH 8/9 is not that obvious, and maybe 
it should be reverted and replaced with a documentation fix.

-- 
Alexander E. Patrakov

Re: [PATCH 0/9] Misc fixes related to rewinds

[Alexander E. Patrakov]

14.09.2014 01:31, Alexander E. Patrakov wrote:
> 14.09.2014 01:14, Jaroslav Kysela wrote:
>> Date 13.9.2014 20:30, Alexander E. Patrakov wrote:
>>> The idea of the series is to fix the two issues that I found [1] for the
>>
>> I applied all your patches to alsa-lib's repo, but...
>>
>>> hw plugin. snd_pcm_rewindable() sometimes returned negative values that
>>> are actually negative amounts of samples and not error codes. Also, it
>>> bases its calculations on stale hardware position pointer, which is not
>>> what PulseAudio wants (alternatively, we can document the need to call
>>> snd_pcm_avail() before snd_pcm_rewindable(), but I don't like it).
>>
>> The hw sync is expensive and the application might do this sync multiple
>> times when woken up. I think that it must be clear that:
>>
>> 1) only snd_pcm_avail(), snd_pcm_delay(), snd_pcm_avail_delay()
>>     does the real hw sync
>> 2) snd_pcm_avail(), snd_pcm_delay(), snd_pcm_avail_delay(),
>>     snd_pcm_rewindable() and snd_pcm_forwardable() does
>>     hw sync (and change all plugins to respect this)
>>
>> I don't like the situation "be somewhere between because it's good for
>> one purpose"...
>
> I understand the concern. I have specifically not added the call to
> hwsync directly to snd_pcm_rewindable implementation (although it would
> have resulted in a smaller patch), because that would indeed cause
> double-hwsync and the resulting inefficiency. I made sure that all
> plugins either make the hwsync thing themselves or rely on the slave to
> do that for them, but not both. If you find an error and/or spot a case
> of a double-hwsync in a plugin chain, please complain.
>
> One known case of double-hwsync is the following pattern: an application
> calls snd_pcm_rewindable(), thinks about it, and then calls
> snd_pcm_rewind(). Which, due to PATCH 2/9, calls the rewindable callback
> again, resulting in the second hwsync. I don't know which way out is
> best: either ignore, or revert the intention of PATCH 2/9, or revert the
> whole PATCH 8/9 and replace it with a documentation change.

Well, after looking again, I see that the multi plugin became especially 
problematic. Previously, it did not forward hwsync requests to slaves 
other than master_slave. Now it does.

Please revert PATCH 8/9. It needs more discussion.

>
> OTOH, I made a mistake of not adding David Henningsson to the CC list
> during the initial submission. If PulseAudio would need to synchronize
> hardware pointers even after conversion to snd_pcm_rewindable() for some
> other reason, then the need for PATCH 8/9 is not that obvious, and maybe
> it should be reverted and replaced with a documentation fix.
>

-- 
Alexander E. Patrakov

Re: [PATCH 0/9] Misc fixes related to rewinds

[Jaroslav Kysela]

Date 13.9.2014 21:50, Alexander E. Patrakov wrote:
> 14.09.2014 01:31, Alexander E. Patrakov wrote:
>> 14.09.2014 01:14, Jaroslav Kysela wrote:
>>> Date 13.9.2014 20:30, Alexander E. Patrakov wrote:
>>>> The idea of the series is to fix the two issues that I found [1] for the
>>>
>>> I applied all your patches to alsa-lib's repo, but...
>>>
>>>> hw plugin. snd_pcm_rewindable() sometimes returned negative values that
>>>> are actually negative amounts of samples and not error codes. Also, it
>>>> bases its calculations on stale hardware position pointer, which is not
>>>> what PulseAudio wants (alternatively, we can document the need to call
>>>> snd_pcm_avail() before snd_pcm_rewindable(), but I don't like it).
>>>
>>> The hw sync is expensive and the application might do this sync multiple
>>> times when woken up. I think that it must be clear that:
>>>
>>> 1) only snd_pcm_avail(), snd_pcm_delay(), snd_pcm_avail_delay()
>>>     does the real hw sync
>>> 2) snd_pcm_avail(), snd_pcm_delay(), snd_pcm_avail_delay(),
>>>     snd_pcm_rewindable() and snd_pcm_forwardable() does
>>>     hw sync (and change all plugins to respect this)
>>>
>>> I don't like the situation "be somewhere between because it's good for
>>> one purpose"...
>>
>> I understand the concern. I have specifically not added the call to
>> hwsync directly to snd_pcm_rewindable implementation (although it would
>> have resulted in a smaller patch), because that would indeed cause
>> double-hwsync and the resulting inefficiency. I made sure that all
>> plugins either make the hwsync thing themselves or rely on the slave to
>> do that for them, but not both. If you find an error and/or spot a case
>> of a double-hwsync in a plugin chain, please complain.
>>
>> One known case of double-hwsync is the following pattern: an application
>> calls snd_pcm_rewindable(), thinks about it, and then calls
>> snd_pcm_rewind(). Which, due to PATCH 2/9, calls the rewindable callback
>> again, resulting in the second hwsync. I don't know which way out is
>> best: either ignore, or revert the intention of PATCH 2/9, or revert the
>> whole PATCH 8/9 and replace it with a documentation change.
> 
> Well, after looking again, I see that the multi plugin became especially 
> problematic. Previously, it did not forward hwsync requests to slaves 
> other than master_slave. Now it does.
> 
> Please revert PATCH 8/9. It needs more discussion.

Reverted.

> 
>>
>> OTOH, I made a mistake of not adding David Henningsson to the CC list
>> during the initial submission. If PulseAudio would need to synchronize
>> hardware pointers even after conversion to snd_pcm_rewindable() for some
>> other reason, then the need for PATCH 8/9 is not that obvious, and maybe
>> it should be reverted and replaced with a documentation fix.
>>
> 


-- 
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project; Red Hat, Inc.

Re: [PATCH 0/9] Misc fixes related to rewinds

[Raymond Yau]

>
> Note: this series touches pcm_dmix.c, but does not make it rewindable. In
> other words, a variant of the test in [2] now produces a tone instead of
> failing due to snd_pcm_rewind() returning 0. But it should ideally produce
> silence. Obviously, there is some bug left that I have not pinpointed yet.
>

It is better to let dmix not support rewind and force your rewind program
to fail

You rewind program should not affect the other application playing through
dmix

Re: [PATCH 0/9] Misc fixes related to rewinds

[Alexander E. Patrakov]

14.09.2014 14:53, Raymond Yau wrote:
>
>  >
>  > Note: this series touches pcm_dmix.c, but does not make it rewindable. In
>  > other words, a variant of the test in [2] now produces a tone instead of
>  > failing due to snd_pcm_rewind() returning 0. But it should ideally
> produce
>  > silence. Obviously, there is some bug left that I have not pinpointed
> yet.
>  >
>
> It is better to let dmix not support rewind and force your rewind
> program to fail
>
> You rewind program should not affect the other application playing
> through dmix

Sorry, you are not an author of a single line in dmix (although you do 
understand how it works), so I cannot just blindly follow this request. 
OTOH, I was about to ask the same "should it work" question.

-- 
Alexander E. Patrakov

Re: [PATCH 0/9] Misc fixes related to rewinds

[Alexander E. Patrakov]

14.09.2014 16:11, Alexander E. Patrakov wrote:
> 14.09.2014 14:53, Raymond Yau wrote:
>>
>>  >
>>  > Note: this series touches pcm_dmix.c, but does not make it
>> rewindable. In
>>  > other words, a variant of the test in [2] now produces a tone
>> instead of
>>  > failing due to snd_pcm_rewind() returning 0. But it should ideally
>> produce
>>  > silence. Obviously, there is some bug left that I have not pinpointed
>> yet.
>>  >
>>
>> It is better to let dmix not support rewind and force your rewind
>> program to fail
>>
>> You rewind program should not affect the other application playing
>> through dmix
>
> Sorry, you are not an author of a single line in dmix (although you do
> understand how it works), so I cannot just blindly follow this request.
> OTOH, I was about to ask the same "should it work" question.

Well, after looking a bit more at this, I think that I know what's wrong 
with dmix. I can probably fix it, but I have tasks with higher priority 
right now.

The real problem is that it tries to do some remixing work in its 
.rewind callback. It shouldn't do this - in fact, the whole callback 
should just move the application pointer. Look - on a hardware device, 
if you disable xrun detection, write something, rewind it, and then let 
it underrun, it will still be played in full. On dmix, it is actively 
attempted to be unmixed.

The real rewind-support work should be done in the mmap_commit callback. 
It should:

0. Keep a shadow copy of the mmap areas, so that it can look at old samples.

1. Zero out the part of the sum buffer crossed by the hardware pointer 
since the last call (possibly in another client). Zero out the 
corresponding part of the mmap areas (both real and shadow). This should 
probably be done in snd_pcm_dmix_sync_ptr(), but I am not sure. There is 
already a hack based on ARCH_CMPXCHG that purports to do this in 
mix_areas_16().

2. Unmix the old contents of the mmap areas in mmap_commit callback 
(instead of the rewind callback). Most of the time, it will unmix the 
sequence of zeros.

3. Mix the new contents of mmap areas.

4. Update the shadow copy.

-- 
Alexander E. Patrakov

Re: [PATCH 0/9] Misc fixes related to rewinds

[Alexander E. Patrakov]

14.09.2014 17:09, Alexander E. Patrakov wrote:
> 14.09.2014 16:11, Alexander E. Patrakov wrote:
>> 14.09.2014 14:53, Raymond Yau wrote:
>>>
>>>  >
>>>  > Note: this series touches pcm_dmix.c, but does not make it
>>> rewindable. In
>>>  > other words, a variant of the test in [2] now produces a tone
>>> instead of
>>>  > failing due to snd_pcm_rewind() returning 0. But it should ideally
>>> produce
>>>  > silence. Obviously, there is some bug left that I have not pinpointed
>>> yet.
>>>  >
>>>
>>> It is better to let dmix not support rewind and force your rewind
>>> program to fail
>>>
>>> You rewind program should not affect the other application playing
>>> through dmix
>>
>> Sorry, you are not an author of a single line in dmix (although you do
>> understand how it works), so I cannot just blindly follow this request.
>> OTOH, I was about to ask the same "should it work" question.
>
> Well, after looking a bit more at this, I think that I know what's wrong
> with dmix. I can probably fix it, but I have tasks with higher priority
> right now.
>
> The real problem is that it tries to do some remixing work in its
> .rewind callback. It shouldn't do this - in fact, the whole callback
> should just move the application pointer. Look - on a hardware device,
> if you disable xrun detection, write something, rewind it, and then let
> it underrun, it will still be played in full. On dmix, it is actively
> attempted to be unmixed.
>
> The real rewind-support work should be done in the mmap_commit callback.
> It should:
>
> 0. Keep a shadow copy of the mmap areas, so that it can look at old
> samples.
>
> 1. Zero out the part of the sum buffer crossed by the hardware pointer
> since the last call (possibly in another client). Zero out the
> corresponding part of the mmap areas (both real and shadow). This should
> probably be done in snd_pcm_dmix_sync_ptr(), but I am not sure. There is
> already a hack based on ARCH_CMPXCHG that purports to do this in
> mix_areas_16().
>
> 2. Unmix the old contents of the mmap areas in mmap_commit callback
> (instead of the rewind callback). Most of the time, it will unmix the
> sequence of zeros.
>
> 3. Mix the new contents of mmap areas.
>
> 4. Update the shadow copy.

Well, scratch that. It obviously does not apply to dshare, where the 
problem also exists, although no real work is done in the rewind 
callback (correctly), and I still don't know why.

Sorry for the spam.

-- 
Alexander E. Patrakov

Re: [PATCH 0/9] Misc fixes related to rewinds

[Takashi Iwai]

At Sun, 14 Sep 2014 17:19:22 +0600,
Alexander E. Patrakov wrote:
> 
> 14.09.2014 17:09, Alexander E. Patrakov wrote:
> > 14.09.2014 16:11, Alexander E. Patrakov wrote:
> >> 14.09.2014 14:53, Raymond Yau wrote:
> >>>
> >>>  >
> >>>  > Note: this series touches pcm_dmix.c, but does not make it
> >>> rewindable. In
> >>>  > other words, a variant of the test in [2] now produces a tone
> >>> instead of
> >>>  > failing due to snd_pcm_rewind() returning 0. But it should ideally
> >>> produce
> >>>  > silence. Obviously, there is some bug left that I have not pinpointed
> >>> yet.
> >>>  >
> >>>
> >>> It is better to let dmix not support rewind and force your rewind
> >>> program to fail
> >>>
> >>> You rewind program should not affect the other application playing
> >>> through dmix
> >>
> >> Sorry, you are not an author of a single line in dmix (although you do
> >> understand how it works), so I cannot just blindly follow this request.
> >> OTOH, I was about to ask the same "should it work" question.
> >
> > Well, after looking a bit more at this, I think that I know what's wrong
> > with dmix. I can probably fix it, but I have tasks with higher priority
> > right now.
> >
> > The real problem is that it tries to do some remixing work in its
> > .rewind callback. It shouldn't do this - in fact, the whole callback
> > should just move the application pointer. Look - on a hardware device,
> > if you disable xrun detection, write something, rewind it, and then let
> > it underrun, it will still be played in full. On dmix, it is actively
> > attempted to be unmixed.
> >
> > The real rewind-support work should be done in the mmap_commit callback.
> > It should:
> >
> > 0. Keep a shadow copy of the mmap areas, so that it can look at old
> > samples.
> >
> > 1. Zero out the part of the sum buffer crossed by the hardware pointer
> > since the last call (possibly in another client). Zero out the
> > corresponding part of the mmap areas (both real and shadow). This should
> > probably be done in snd_pcm_dmix_sync_ptr(), but I am not sure. There is
> > already a hack based on ARCH_CMPXCHG that purports to do this in
> > mix_areas_16().
> >
> > 2. Unmix the old contents of the mmap areas in mmap_commit callback
> > (instead of the rewind callback). Most of the time, it will unmix the
> > sequence of zeros.
> >
> > 3. Mix the new contents of mmap areas.
> >
> > 4. Update the shadow copy.
> 
> Well, scratch that. It obviously does not apply to dshare, where the 
> problem also exists, although no real work is done in the rewind 
> callback (correctly), and I still don't know why.

The lack of dshare support is likely just because of the lack of time
and interest.  The forward/rewind support was added by Jaroslav years
ago, and we haven't worked on it much since then.  Many bugs were
there as you've spotted out.

In the case of dshare it's even easier than dmix / dsnoop, the unmix
would be just clearing sample.  So, it must not be due to its
difficulties, I believe.


Takashi