[alsa-devel] softvol and snd_pcm_rewind() is broken
Heya!
With 1.0.17rc3 snd_pcm_rewind() is broken for softvol as it seems:
- Sometimes the sound becomes heavily distorted after such a seek:
http://mailman.alsa-project.org/pipermail/alsa-devel/2008-June/008860.html
- And snd_pcm_rewind() might return a value that is higher than was passed in, which as far as I understood should never happen:
http://mailman.alsa-project.org/pipermail/alsa-devel/2008-April/007308.html
These two issues might be caused by the same error.
Takashi, Jaroslav, how can I bribe you into fixing this? I'd love to release my new PulseAudio version soon which heavily relies on snd_pcm_rewind(), but unfortunately the most important driver (hda with softvol) makes the most problems with it. :-(
Lennart
On Wed, 16.07.08 16:30, Lennart Poettering (mznyfn@0pointer.de) wrote:
Heya!
With 1.0.17rc3 snd_pcm_rewind() is broken for softvol as it seems:
Sometimes the sound becomes heavily distorted after such a seek:
http://mailman.alsa-project.org/pipermail/alsa-devel/2008-June/008860.html
And snd_pcm_rewind() might return a value that is higher than was passed in, which as far as I understood should never happen:
http://mailman.alsa-project.org/pipermail/alsa-devel/2008-April/007308.html
These two issues might be caused by the same error.
Takashi, Jaroslav, how can I bribe you into fixing this? I'd love to release my new PulseAudio version soon which heavily relies on snd_pcm_rewind(), but unfortunately the most important driver (hda with softvol) makes the most problems with it. :-(
Hmm, if I understand snd_pcm_plugin_rewind() in pcm_plugin.c correctly, then I read it like this:
1. we clamp the input frames to hw_avail 2. we convert this to slave frames 3. we issue the rewind 4. we convert back to client frames 5. we return hw_avail.
Step #1 seems wrong to me. The code will make sure that we rewind as least as much as can be written right now. Does that make any sense? I don't think so. Shouldn't this be a clamp that makes sure that we rewind at most as much as the buffer is filled right now? I.e. something along the lines of:
if (frames > buffer_size - n) frames = buffer_size - n;
Also, step #5 seems wrong to me. Shouldn't we return the result of the second conversion? I.e. shouldn't "return n" be replaced by "return (snd_pcm_sframes_t) frames"?
I am puzzled though, not sure if I understood that function correctly at all.
Lennart
At Wed, 16 Jul 2008 16:30:04 +0200, Lennart Poettering wrote:
Heya!
With 1.0.17rc3 snd_pcm_rewind() is broken for softvol as it seems:
Sometimes the sound becomes heavily distorted after such a seek:
http://mailman.alsa-project.org/pipermail/alsa-devel/2008-June/008860.html
And snd_pcm_rewind() might return a value that is higher than was passed in, which as far as I understood should never happen:
http://mailman.alsa-project.org/pipermail/alsa-devel/2008-April/007308.html
These two issues might be caused by the same error.
Takashi, Jaroslav, how can I bribe you into fixing this? I'd love to release my new PulseAudio version soon which heavily relies on snd_pcm_rewind(), but unfortunately the most important driver (hda with softvol) makes the most problems with it. :-(
As mentioned earlier, the softvol itself is a simple plain plugin and it has no code to do forward/rewind in itself. Thus, if a bug is present in softvol, it must be in the generic plugin code -- or there can be a missing piece that the generic code doesn't cover. I'm not sure yet, as I didn't write that code.
The second problem, the bigger return size, looks like a thinko in the code. Try the patch below.
Takashi
--- diff --git a/src/pcm/pcm_plugin.c b/src/pcm/pcm_plugin.c index c73a02b..b5e940b 100644 --- a/src/pcm/pcm_plugin.c +++ b/src/pcm/pcm_plugin.c @@ -203,10 +203,10 @@ static snd_pcm_sframes_t snd_pcm_plugin_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t snd_pcm_sframes_t n = snd_pcm_mmap_hw_avail(pcm); snd_pcm_sframes_t sframes;
- if ((snd_pcm_uframes_t)n > frames) - frames = n; - if (frames == 0) + if (n <= 0 || frames == 0) return 0; + if ((snd_pcm_uframes_t)n < frames) + frames = n; if (plugin->slave_frames) sframes = plugin->slave_frames(pcm, (snd_pcm_sframes_t) frames); @@ -236,10 +236,10 @@ static snd_pcm_sframes_t snd_pcm_plugin_forward(snd_pcm_t *pcm, snd_pcm_uframes_ snd_pcm_sframes_t n = snd_pcm_mmap_avail(pcm); snd_pcm_uframes_t sframes;
- if ((snd_pcm_uframes_t)n > frames) - frames = n; - if (frames == 0) + if (n <= 0 || frames == 0) return 0; + if ((snd_pcm_uframes_t)n < frames) + frames = n; if (plugin->slave_frames) sframes = plugin->slave_frames(pcm, (snd_pcm_sframes_t) frames);
I tried applying it but the two hunks failed - Is there a more recent patch available?
Thanks, Stewart
On Thu, 2008-07-17 at 11:56 +0200, Takashi Iwai wrote:
At Wed, 16 Jul 2008 16:30:04 +0200, Lennart Poettering wrote:
Heya!
With 1.0.17rc3 snd_pcm_rewind() is broken for softvol as it seems:
Sometimes the sound becomes heavily distorted after such a seek:
http://mailman.alsa-project.org/pipermail/alsa-devel/2008-June/008860.html
And snd_pcm_rewind() might return a value that is higher than was passed in, which as far as I understood should never happen:
http://mailman.alsa-project.org/pipermail/alsa-devel/2008-April/007308.html
These two issues might be caused by the same error.
Takashi, Jaroslav, how can I bribe you into fixing this? I'd love to release my new PulseAudio version soon which heavily relies on snd_pcm_rewind(), but unfortunately the most important driver (hda with softvol) makes the most problems with it. :-(
As mentioned earlier, the softvol itself is a simple plain plugin and it has no code to do forward/rewind in itself. Thus, if a bug is present in softvol, it must be in the generic plugin code -- or there can be a missing piece that the generic code doesn't cover. I'm not sure yet, as I didn't write that code.
The second problem, the bigger return size, looks like a thinko in the code. Try the patch below.
Takashi
diff --git a/src/pcm/pcm_plugin.c b/src/pcm/pcm_plugin.c index c73a02b..b5e940b 100644 --- a/src/pcm/pcm_plugin.c +++ b/src/pcm/pcm_plugin.c @@ -203,10 +203,10 @@ static snd_pcm_sframes_t snd_pcm_plugin_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t snd_pcm_sframes_t n = snd_pcm_mmap_hw_avail(pcm); snd_pcm_sframes_t sframes;
- if ((snd_pcm_uframes_t)n > frames)
frames = n;- if (frames == 0)
if (n <= 0 || frames == 0) return 0;
if ((snd_pcm_uframes_t)n < frames)
frames = n;if (plugin->slave_frames) sframes = plugin->slave_frames(pcm, (snd_pcm_sframes_t) frames);
@@ -236,10 +236,10 @@ static snd_pcm_sframes_t snd_pcm_plugin_forward(snd_pcm_t *pcm, snd_pcm_uframes_ snd_pcm_sframes_t n = snd_pcm_mmap_avail(pcm); snd_pcm_uframes_t sframes;
- if ((snd_pcm_uframes_t)n > frames)
frames = n;- if (frames == 0)
if (n <= 0 || frames == 0) return 0;
if ((snd_pcm_uframes_t)n < frames)
frames = n;if (plugin->slave_frames) sframes = plugin->slave_frames(pcm, (snd_pcm_sframes_t) frames);
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
At Fri, 18 Jul 2008 12:47:08 -0400, Stewart Adam wrote:
I tried applying it but the two hunks failed - Is there a more recent patch available?
It is against the latest version of alsa-lib.
Takashi
Thanks, Stewart
On Thu, 2008-07-17 at 11:56 +0200, Takashi Iwai wrote:
At Wed, 16 Jul 2008 16:30:04 +0200, Lennart Poettering wrote:
Heya!
With 1.0.17rc3 snd_pcm_rewind() is broken for softvol as it seems:
Sometimes the sound becomes heavily distorted after such a seek:
http://mailman.alsa-project.org/pipermail/alsa-devel/2008-June/008860.html
And snd_pcm_rewind() might return a value that is higher than was passed in, which as far as I understood should never happen:
http://mailman.alsa-project.org/pipermail/alsa-devel/2008-April/007308.html
These two issues might be caused by the same error.
Takashi, Jaroslav, how can I bribe you into fixing this? I'd love to release my new PulseAudio version soon which heavily relies on snd_pcm_rewind(), but unfortunately the most important driver (hda with softvol) makes the most problems with it. :-(
As mentioned earlier, the softvol itself is a simple plain plugin and it has no code to do forward/rewind in itself. Thus, if a bug is present in softvol, it must be in the generic plugin code -- or there can be a missing piece that the generic code doesn't cover. I'm not sure yet, as I didn't write that code.
The second problem, the bigger return size, looks like a thinko in the code. Try the patch below.
Takashi
diff --git a/src/pcm/pcm_plugin.c b/src/pcm/pcm_plugin.c index c73a02b..b5e940b 100644 --- a/src/pcm/pcm_plugin.c +++ b/src/pcm/pcm_plugin.c @@ -203,10 +203,10 @@ static snd_pcm_sframes_t snd_pcm_plugin_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t snd_pcm_sframes_t n = snd_pcm_mmap_hw_avail(pcm); snd_pcm_sframes_t sframes;
- if ((snd_pcm_uframes_t)n > frames)
frames = n;- if (frames == 0)
if (n <= 0 || frames == 0) return 0;
if ((snd_pcm_uframes_t)n < frames)
frames = n;if (plugin->slave_frames) sframes = plugin->slave_frames(pcm, (snd_pcm_sframes_t) frames);
@@ -236,10 +236,10 @@ static snd_pcm_sframes_t snd_pcm_plugin_forward(snd_pcm_t *pcm, snd_pcm_uframes_ snd_pcm_sframes_t n = snd_pcm_mmap_avail(pcm); snd_pcm_uframes_t sframes;
- if ((snd_pcm_uframes_t)n > frames)
frames = n;- if (frames == 0)
if (n <= 0 || frames == 0) return 0;
if ((snd_pcm_uframes_t)n < frames)
frames = n;if (plugin->slave_frames) sframes = plugin->slave_frames(pcm, (snd_pcm_sframes_t) frames);
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Thu, 17.07.08 11:56, Takashi Iwai (tiwai@suse.de) wrote:
And snd_pcm_rewind() might return a value that is higher than was passed in, which as far as I understood should never happen:
http://mailman.alsa-project.org/pipermail/alsa-devel/2008-April/007308.html
These two issues might be caused by the same error.
Takashi, Jaroslav, how can I bribe you into fixing this? I'd love to release my new PulseAudio version soon which heavily relies on snd_pcm_rewind(), but unfortunately the most important driver (hda with softvol) makes the most problems with it. :-(
As mentioned earlier, the softvol itself is a simple plain plugin and it has no code to do forward/rewind in itself. Thus, if a bug is present in softvol, it must be in the generic plugin code -- or there can be a missing piece that the generic code doesn't cover. I'm not sure yet, as I didn't write that code.
The second problem, the bigger return size, looks like a thinko in the code. Try the patch below.
I just posted three patches that fix those issues for me. Please have a look. They do basically what your patch does as well, plus fixing the return issue.
The patches are trivial, look correct to me and fix the issues.
Please merge,
Lennart
On Fri, 2008-07-18 at 21:30 +0200, Lennart Poettering wrote:
I just posted three patches that fix those issues for me. Please have a look. They do basically what your patch does as well, plus fixing the return issue.
The patches are trivial, look correct to me and fix the issues.
Please merge,
Lennart
Worked perfectly, thanks!
Stewart
At Fri, 18 Jul 2008 21:30:17 +0200, Lennart Poettering wrote:
On Thu, 17.07.08 11:56, Takashi Iwai (tiwai@suse.de) wrote:
And snd_pcm_rewind() might return a value that is higher than was passed in, which as far as I understood should never happen:
http://mailman.alsa-project.org/pipermail/alsa-devel/2008-April/007308.html
These two issues might be caused by the same error.
Takashi, Jaroslav, how can I bribe you into fixing this? I'd love to release my new PulseAudio version soon which heavily relies on snd_pcm_rewind(), but unfortunately the most important driver (hda with softvol) makes the most problems with it. :-(
As mentioned earlier, the softvol itself is a simple plain plugin and it has no code to do forward/rewind in itself. Thus, if a bug is present in softvol, it must be in the generic plugin code -- or there can be a missing piece that the generic code doesn't cover. I'm not sure yet, as I didn't write that code.
The second problem, the bigger return size, looks like a thinko in the code. Try the patch below.
I just posted three patches that fix those issues for me. Please have a look. They do basically what your patch does as well, plus fixing the return issue.
The patches are trivial, look correct to me and fix the issues.
Please merge,
Committed now. Thanks.
Takashi
participants (3)
-
Lennart Poettering -
Stewart Adam -
Takashi Iwai