softvol and snd_pcm_rewind() is broken

softvol and snd_pcm_rewind() is broken

[Lennart Poettering]

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

-- 
Lennart Poettering                        Red Hat, Inc.
lennart [at] poettering [dot] net         ICQ# 11060553
http://0pointer.net/lennart/           GnuPG 0x1A015CC4

Re: softvol and snd_pcm_rewind() is broken

[Lennart Poettering]

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

-- 
Lennart Poettering                        Red Hat, Inc.
lennart [at] poettering [dot] net         ICQ# 11060553
http://0pointer.net/lennart/           GnuPG 0x1A015CC4

Re: softvol and snd_pcm_rewind() is broken

[Takashi Iwai]

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

Re: softvol and snd_pcm_rewind() is broken

[Stewart Adam]

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

Re: softvol and snd_pcm_rewind() is broken

[Takashi Iwai]

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
> 

Re: softvol and snd_pcm_rewind() is broken

[Lennart Poettering]

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

-- 
Lennart Poettering                        Red Hat, Inc.
lennart [at] poettering [dot] net         ICQ# 11060553
http://0pointer.net/lennart/           GnuPG 0x1A015CC4

Re: softvol and snd_pcm_rewind() is broken

[Stewart Adam]

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

Re: softvol and snd_pcm_rewind() is broken

[Takashi Iwai]

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