Re: [PATCH 00/17] audio: improve callback interface for audio frontends

["Volker Rümelin" <vr_qemu@t-online.de>]

Am 15.01.23 um 14:08 schrieb Volker Rümelin:

Ccing a few more people who might be interested in this patch series.

@Mark:
After this patch series, the code in your out of tree ASC audio device 
(and a few in tree audio devices) could be simplified. write_audio() and 
the loops calling write_audio() could be removed.

With best regards,
Volker

> Based-on: <3b1404eb-a7c5-f64c-3e47-1397c54c45bb@t-online.de>
> ([PATCH 00/11] audio: more improvements)
>
> The callback interface for emulated audio devices is strange. The 
> callback function has an 'avail' parameter that passes the number of 
> bytes that can be written or read. Unfortunately, this value sometimes 
> is only an imprecise estimate and the callback functions must check 
> the actual bytes written or read. For playback devices, this means 
> that they either need a ring buffer or have to write the unwritten 
> bytes again the next time. For recording devices, things are a bit 
> easier. They only need to continue with the actual number of bytes read.
>
> After this patch series, the 'avail' argument for the -audiodev 
> out.mixing-engine=on and in.mixing-engine=on cases is exact. Audio 
> frontends only need a linear frame buffer and there's a guarantee they 
> can write or read 'avail' bytes.
>
> The -audiodev out.mixing-engine=off case is also mostly accurate. Only 
> the D-Bus audio backend is still missing a required function. The 
> -audiodev in.mixing-engine=off case always passes a much too large 
> 'avail' value. I haven't worked on this yet, because there was no 
> reason for it so far.
>
> The following logs show the improvements. Not only the audio frontends 
> can write or read all needed or available bytes. The same is true for 
> the audio backends. For playback, the first six lines in the logs are 
> expected. Here you can see how quickly the guest fills the empty 
> downstream buffers after playback starts.
>
> QEMU was started with -device ich9-intel-hda,addr=0x1b -device 
> hda-duplex,audiodev=audio0 -audiodev 
> pa,out.frequency=96000,in.frequency=96000,id=audio0
>
> playback guest 44100Hz => host 96000Hz
>
> unpatched version:
> hda_audio_output_cb: to write 8188, written 1704
> audio_run_out: free 4458, played 926
> hda_audio_output_cb: to write 6488, written 2384
> audio_run_out: free 3532, played 1297
> hda_audio_output_cb: to write 4104, written 2648
> audio_run_out: free 2235, played 1441
> audio_run_out: free 794, played 793
> audio_run_out: free 897, played 896
> audio_run_out: free 831, played 829
> ...
> hda_audio_output_cb: could not write 4 bytes
> hda_audio_output_cb: to write 1764, written 1760
> audio_run_out: free 960, played 958
> ...
>
> patched version:
> hda_audio_output_cb: to write 8192, written 1620
> audio_run_out: free 4458, played 880
> hda_audio_output_cb: to write 6576, written 2508
> audio_run_out: free 3578, played 1365
> hda_audio_output_cb: to write 4068, written 2500
> audio_run_out: free 2213, played 1360
>
> record host 96000Hz => guest 44100Hz
>
> unpatched version:
> audio_run_in: avail 4458, acquired 4454
> audio_run_in: avail 1574, acquired 1572
> audio_run_in: avail 766, acquired 764
> audio_run_in: avail 1052, acquired 1051
> audio_run_in: avail 761, acquired 760
> audio_run_in: avail 1123, acquired 1121
> ...
> hda_audio_input_cb: could not read 4 bytes
> hda_audio_input_cb: to read 1988, read 1984
> audio_run_in: avail 1082, acquired 1080
> ...
>
> patched version:
> (no output)
>
> QEMU was started with -device ich9-intel-hda,addr=0x1b -device 
> hda-duplex,audiodev=audio0 -audiodev 
> pa,out.frequency=32000,in.frequency=32000,id=audio0
>
> playback guest 44100Hz => host 32000Hz
>
> unpatched version:
> hda_audio_output_cb: to write 8188, written 1620
> audio_run_out: free 1486, played 294
> hda_audio_output_cb: to write 6568, written 2512
> audio_run_out: free 1192, played 455
> hda_audio_output_cb: to write 4060, written 2504
> audio_run_out: free 737, played 455
> audio_run_out: free 282, played 281
> audio_run_out: free 357, played 356
> audio_run_out: free 314, played 313
> ...
> hda_audio_output_cb: could not write 4 bytes
> hda_audio_output_cb: to write 1416, written 1412
> audio_run_out: free 257, played 256
> ...
>
> patched version:
> hda_audio_output_cb: to write 8192, written 1656
> audio_run_out: free 1486, played 300
> hda_audio_output_cb: to write 6536, written 2516
> audio_run_out: free 1186, played 457
> hda_audio_output_cb: to write 4020, written 2540
> audio_run_out: free 729, played 460
>
> record host 32000Hz => guest 44100Hz
>
> unpatched version:
> audio_run_in: avail 1486, acquired 1485
> audio_run_in: avail 272, acquired 271
> audio_run_in: avail 366, acquired 365
> hda_audio_input_cb: could not read 4 bytes
> hda_audio_input_cb: to read 1420, read 1416
> audio_run_in: avail 258, acquired 257
> audio_run_in: avail 375, acquired 374
> hda_audio_input_cb: could not read 4 bytes
> hda_audio_input_cb: to read 2056, read 2052
> audio_run_in: avail 260, acquired 259
> ...
>
> patched version:
> (no output)
>
> This is the debug code for the logs above.
>
> ---snip--
> --- a/audio/audio.c    2022-12-13 19:14:31.793153558 +0100
> +++ b/audio/audio.c    2022-12-11 16:24:48.842649711 +0100
> @@ -1228,6 +1228,10 @@ static void audio_run_out (AudioState *s
>  #ifdef DEBUG_OUT
>          dolog("played=%zu\n", played);
>  #endif
> +        if (hw_free - played) {
> +            fprintf(stderr, "%s: free %zu, played %zu\n",
> +                    __func__, hw_free, played);
> +        }
>
>          if (played) {
>              hw->ts_helper += played;
> @@ -1318,6 +1322,7 @@ static void audio_run_in (AudioState *s)
>              if (sw->active) {
>                  size_t sw_avail = audio_get_avail(sw);
>                  size_t avail;
> +                size_t prev_acquired = sw->total_hw_samples_acquired;
>
>                  avail = st_rate_frames_out(sw->rate, sw_avail);
>                  if (avail > 0) {
> @@ -1325,6 +1330,11 @@ static void audio_run_in (AudioState *s)
>                      sw->callback.fn(sw->callback.opaque,
>                                      avail * sw->info.bytes_per_frame);
>                  }
> +
> +                if (sw_avail + prev_acquired - 
> sw->total_hw_samples_acquired) {
> +                    fprintf(stderr, "%s: avail %zu, acquired %zu\n", 
> __func__,
> +                            sw_avail, sw->total_hw_samples_acquired - 
> prev_acquired);
> +                }
>              }
>          }
>      }
> --- a/hw/audio/hda-codec.c    2023-01-04 14:07:31.954304889 +0100
> +++ b/hw/audio/hda-codec.c    2023-01-04 13:57:47.687320406 +0100
> @@ -265,20 +265,28 @@ static void hda_audio_input_cb(void *opa
>      int64_t rpos = st->rpos;
>
>      int64_t to_transfer = MIN(B_SIZE - (wpos - rpos), avail);
> +    unsigned int total_read = 0;
>
>      while (to_transfer) {
>          uint32_t start = (uint32_t) (wpos & B_MASK);
>          uint32_t chunk = (uint32_t) MIN(B_SIZE - start, to_transfer);
>          uint32_t read = AUD_read(st->voice.in, st->buf + start, chunk);
>          wpos += read;
> +        total_read += read;
>          to_transfer -= read;
>          st->wpos += read;
>          if (chunk != read) {
> +            fprintf(stderr, "%s: could not read %u bytes\n", __func__,
> +                    chunk - read);
>              break;
>          }
>      }
>
>      hda_timer_sync_adjust(st, -((wpos - rpos) - (B_SIZE >> 1)));
> +    if (avail != total_read) {
> +        fprintf(stderr, "%s: to read %d, read %u\n", __func__,
> +                avail, total_read);
> +    }
>  }
>
>  static void hda_audio_output_timer(void *opaque)
> @@ -329,6 +337,7 @@ static void hda_audio_output_cb(void *op
>      int64_t rpos = st->rpos;
>
>      int64_t to_transfer = MIN(wpos - rpos, avail);
> +    unsigned int total_written = 0;
>
>      if (wpos - rpos == B_SIZE) {
>          /* drop buffer, reset timer adjust */
> @@ -343,15 +352,22 @@ static void hda_audio_output_cb(void *op
>          uint32_t start = (uint32_t) (rpos & B_MASK);
>          uint32_t chunk = (uint32_t) MIN(B_SIZE - start, to_transfer);
>          uint32_t written = AUD_write(st->voice.out, st->buf + start, 
> chunk);
> +        total_written += written;
>          rpos += written;
>          to_transfer -= written;
>          st->rpos += written;
>          if (chunk != written) {
> +            fprintf(stderr, "%s: could not write %u bytes\n", __func__,
> +                    chunk - written);
>              break;
>          }
>      }
>
>      hda_timer_sync_adjust(st, (wpos - rpos) - (B_SIZE >> 1));
> +    if (avail != total_written) {
> +        fprintf(stderr, "%s: to write %d, written %u\n", __func__,
> +                avail, total_written);
> +    }
>  }
>
>  static void hda_audio_compat_input_cb(void *opaque, int avail)
> ---snip--
>
> Volker Rümelin (17):
>   audio: change type of mix_buf and conv_buf
>   audio: change type and name of the resample buffer
>   audio: make the resampling code greedy
>   audio: replace the resampling loop in audio_pcm_sw_write()
>   audio: remove sw == NULL check
>   audio: rename variables in audio_pcm_sw_write()
>   audio: don't misuse audio_pcm_sw_write()
>   audio: remove unused noop_conv() function
>   audio/mixeng: calculate number of input frames
>   audio: wire up st_rate_frames_in()
>   audio: replace the resampling loop in audio_pcm_sw_read()
>   audio: rename variables in audio_pcm_sw_read()
>   audio/mixeng: calculate number of output frames
>   audio: wire up st_rate_frames_out()
>   audio: handle leftover audio frame from upsampling
>   audio/audio_template: substitute sw->hw with hw
>   audio: remove sw->ratio
>
>  audio/audio.c          | 366 +++++++++++++++++++++--------------------
>  audio/audio_int.h      |  12 +-
>  audio/audio_template.h |  41 +++--
>  audio/mixeng.c         |  73 ++++++++
>  audio/mixeng.h         |   2 +
>  audio/rate_template.h  |  21 ++-
>  6 files changed, 304 insertions(+), 211 deletions(-)
>