| Message ID | 2fc947cf-7b42-de68-3f11-cbcf1c096be9@t-online.de |
|---|---|
| State | New |
| Headers | show |
| Series | audio: fix audio recording | expand |
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; > } >
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~
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; > } >
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
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
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; }
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(+)