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

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

Am 22.01.23 um 19:13 schrieb Mark Cave-Ayland:
> On 15/01/2023 13:45, Volker Rümelin wrote:
>
>> 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.
>
> Hi Volker,
>
> I know we have discussed this in a separate thread off-list, but this 
> is fantastic!
>
> Just out of interest, if the available bytes wraps the circular buffer 
> will the audio core call the audio callback twice to maximise the 
> ability of the guest to generate samples before the next audio timer? 
> Or does that not make much difference in practice?

Hi Mark,

I guess with circular buffer you refer to the mixing engine buffer. The 
audio system calls the callback once on every audio timer event. If the 
available bytes wrap the mixing engine ringbuffer, the 
audio_pcm_sw_resample_out() function uses two writes to write all 
available bytes. Compared to the unpatched version, nothing has changed 
in this regard. Of course the audio frontend devices are still free to 
write 'avail' bytes with multiple calls to AUD_write().

With best regards,
Volker

>
> I'm not too familiar with the audio subsystem, but a quick skim of the 
> series looks good (and being able to remove the write_audio() loops is 
> a big plus). So I would certainly give this a thumbs up:
>
> Acked-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>
>
> ATB,
>
> Mark.
>
>> 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(-)
>>>
>>
>>
>