[alsa-devel] [PATCH 0/9] Misc fixes related to rewinds
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(-)
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;
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)
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)
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; }
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; }
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;
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
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
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
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.
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
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
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);
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); }
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);}
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;
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...
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:
- only snd_pcm_avail(), snd_pcm_delay(), snd_pcm_avail_delay() does the real hw sync
- 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.
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:
- only snd_pcm_avail(), snd_pcm_delay(), snd_pcm_avail_delay() does the real hw sync
- 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.
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:
- only snd_pcm_avail(), snd_pcm_delay(), snd_pcm_avail_delay() does the real hw sync
- 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.
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
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.
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.
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:
- Keep a shadow copy of the mmap areas, so that it can look at old
samples.
- 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().
- 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.
Mix the new contents of mmap areas.
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.
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:
- Keep a shadow copy of the mmap areas, so that it can look at old
samples.
- 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().
- 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.
Mix the new contents of mmap areas.
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
participants (4)
-
Alexander E. Patrakov -
Jaroslav Kysela -
Raymond Yau -
Takashi Iwai