[PATCH] audio: fix audio recording

[PATCH] audio: fix audio recording

[Volker Rümelin]

With current code audio recording with all audio backends
except PulseAudio and DirectSound is broken. The generic audio
recording buffer management forgot to update the current read
position after a read.

Fixes: ff095e5231 "audio: api for mixeng code free backends"

Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 audio/audio.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/audio/audio.c b/audio/audio.c
index 7fc3aa9d16..56fae55047 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1390,6 +1390,7 @@ void *audio_generic_get_buffer_in(HWVoiceIn *hw, size_t *size)
         size_t read = hw->pcm_ops->read(hw, hw->buf_emul + hw->pos_emul,
                                         read_len);
         hw->pending_emul += read;
+        hw->pos_emul = (hw->pos_emul + read) % hw->size_emul;
         if (read < read_len) {
             break;
         }
-- 
2.16.4

Re: [PATCH] audio: fix audio recording

[Philippe Mathieu-Daudé]

Cc'ing Zoltán.

On 11/19/19 7:58 AM, Volker Rümelin wrote:
> With current code audio recording with all audio backends
> except PulseAudio and DirectSound is broken. The generic audio
> recording buffer management forgot to update the current read
> position after a read.
> 
> Fixes: ff095e5231 "audio: api for mixeng code free backends"
> 
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
> ---
>   audio/audio.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/audio/audio.c b/audio/audio.c
> index 7fc3aa9d16..56fae55047 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -1390,6 +1390,7 @@ void *audio_generic_get_buffer_in(HWVoiceIn *hw, size_t *size)
>           size_t read = hw->pcm_ops->read(hw, hw->buf_emul + hw->pos_emul,
>                                           read_len);
>           hw->pending_emul += read;
> +        hw->pos_emul = (hw->pos_emul + read) % hw->size_emul;

Anyway since read() can return a negative value, both previous 
assignments should go after this if/break check...

>           if (read < read_len) {
>               break;
>           }
> 

Re: [PATCH] audio: fix audio recording

[Richard Henderson]

On 11/19/19 9:01 AM, Philippe Mathieu-Daudé wrote:
> Cc'ing Zoltán.
> 
> On 11/19/19 7:58 AM, Volker Rümelin wrote:
>> With current code audio recording with all audio backends
>> except PulseAudio and DirectSound is broken. The generic audio
>> recording buffer management forgot to update the current read
>> position after a read.
>>
>> Fixes: ff095e5231 "audio: api for mixeng code free backends"
>>
>> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
>> ---
>>   audio/audio.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/audio/audio.c b/audio/audio.c
>> index 7fc3aa9d16..56fae55047 100644
>> --- a/audio/audio.c
>> +++ b/audio/audio.c
>> @@ -1390,6 +1390,7 @@ void *audio_generic_get_buffer_in(HWVoiceIn *hw, size_t
>> *size)
>>           size_t read = hw->pcm_ops->read(hw, hw->buf_emul + hw->pos_emul,
>>                                           read_len);
>>           hw->pending_emul += read;
>> +        hw->pos_emul = (hw->pos_emul + read) % hw->size_emul;
> 
> Anyway since read() can return a negative value, both previous assignments
> should go after this if/break check...

This isn't read(2).

    size_t (*read)    (HWVoiceIn *hw, void *buf, size_t size);

Since this isn't ssize_t, no negative return value possible.


r~

Re: [PATCH] audio: fix audio recording

[Zoltán Kővágó]

On 2019-11-19 20:43, Richard Henderson wrote:
> On 11/19/19 9:01 AM, Philippe Mathieu-Daudé wrote:
>> Cc'ing Zoltán.
>>
>> On 11/19/19 7:58 AM, Volker Rümelin wrote:
>>> With current code audio recording with all audio backends
>>> except PulseAudio and DirectSound is broken. The generic audio
>>> recording buffer management forgot to update the current read
>>> position after a read.
>>>
>>> Fixes: ff095e5231 "audio: api for mixeng code free backends"
>>>
>>> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
>>> ---
>>>    audio/audio.c | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/audio/audio.c b/audio/audio.c
>>> index 7fc3aa9d16..56fae55047 100644
>>> --- a/audio/audio.c
>>> +++ b/audio/audio.c
>>> @@ -1390,6 +1390,7 @@ void *audio_generic_get_buffer_in(HWVoiceIn *hw, size_t
>>> *size)
>>>            size_t read = hw->pcm_ops->read(hw, hw->buf_emul + hw->pos_emul,
>>>                                            read_len);
>>>            hw->pending_emul += read;
>>> +        hw->pos_emul = (hw->pos_emul + read) % hw->size_emul;
>>
>> Anyway since read() can return a negative value, both previous assignments
>> should go after this if/break check...
> 
> This isn't read(2).
> 
>      size_t (*read)    (HWVoiceIn *hw, void *buf, size_t size);
> 
> Since this isn't ssize_t, no negative return value possible.

Yes, read failures are handled inside the backends.  If the backend 
really can't read anything, it'll return zero, which is harmless here.

Zoltan

Re: [PATCH] audio: fix audio recording

[Zoltán Kővágó]

On 2019-11-19 07:58, Volker Rümelin wrote:
> With current code audio recording with all audio backends
> except PulseAudio and DirectSound is broken. The generic audio
> recording buffer management forgot to update the current read
> position after a read.

Indeed, pos_emul is updated in audio_generic_put_buffer_out_nowrite but 
it's never updated on the capture end.  I tested it with alsa and 
hda-micro and it fixes the problem (that is, if I add in.try-poll=off, 
but I need that for output too).

Thanks.

> 
> Fixes: ff095e5231 "audio: api for mixeng code free backends"
> 
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
Reviewed-by: Zoltán Kővágó <DirtY.iCE.hu@gmail.com>

> ---
>   audio/audio.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/audio/audio.c b/audio/audio.c
> index 7fc3aa9d16..56fae55047 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -1390,6 +1390,7 @@ void *audio_generic_get_buffer_in(HWVoiceIn *hw, size_t *size)
>           size_t read = hw->pcm_ops->read(hw, hw->buf_emul + hw->pos_emul,
>                                           read_len);
>           hw->pending_emul += read;
> +        hw->pos_emul = (hw->pos_emul + read) % hw->size_emul;
>           if (read < read_len) {
>               break;
>           }
> 

Re: [PATCH] audio: fix audio recording

[Gerd Hoffmann]

On Tue, Nov 19, 2019 at 07:58:49AM +0100, Volker Rümelin wrote:
> With current code audio recording with all audio backends
> except PulseAudio and DirectSound is broken. The generic audio
> recording buffer management forgot to update the current read
> position after a read.
> 
> Fixes: ff095e5231 "audio: api for mixeng code free backends"
> 
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>

Queued for 4.2

thanks,
  Gerd