Re: [PATCH] tests/qtest/ac97-test: add up-/downsampling tests

[Thomas Huth]

On 26/10/2022 21.34, Volker Rümelin wrote:
> Am 25.10.22 um 09:44 schrieb Marc-André Lureau:
> > Hi
> >
> > On Tue, Oct 25, 2022 at 12:31 AM Volker Rümelin<vr_qemu@t-online.de>  wrote:
> > > Am 24.10.22 um 10:13 schrieb Marc-André Lureau:
> > > > Hi
> > > >
> > > > On Mon, Oct 24, 2022 at 9:28 AM Volker Rümelin<vr_qemu@t-online.de>
> > > > wrote:
> > > >
> > > >      Test if the audio subsystem can handle extreme up- and down-
> > > >      sampling ratios like 44100/1 and 1/44100. For some time these
> > > >      used to trigger QEMU aborts. The test was taken from
> > > >      https://gitlab.com/qemu-project/qemu/-/issues/71  where it was
> > > >      used to demonstrate a very different issue.
> > > >
> > > >      Suggested-by: Marc-André Lureau<marcandre.lureau@redhat.com>
> > > >      Signed-off-by: Volker Rümelin<vr_qemu@t-online.de>
> > > >
> > > >
> > > > Thanks for working on this
> > > >
> > > > It seems to show something different though:
> > > > "
> > > > A bug was just triggered in audio_calloc
> > > > Save all your work and restart without audio
> > > > I am sorry
> > > > "
> > > >
> > > > AUD_open_out() is called with audsettings: {freq = 1, nchannels = 2,
> > > > fmt = AUDIO_FORMAT_S16, endianness = 0}
> > > >
> > > > And that's it. Any idea?
> > > Hi,
> > >
> > > the scary message is expected and doesn't mean this qos-test failed.
> > > This is the currently not so silent 'the audio subsystem should (...)
> > > silently give up' case.
> > Ok, but it's not silent. According to the AC97 spec, "if the value
> > written to the register is supported that value will be echoed back
> > when read, otherwise the closest (higher in case of a tie) sample rate
> > supported is returned". We should probably pick a low sample rate,
> > like 8000 (see Table 32 in spec 2.1) for anything below it.
>
> Hi,
>
> I don't think we should limit the lowest sample rate to 8000 Hz. The sample 
> rates in AC97 revision 2.1 Table 32 are sample rates the codec should 
> support at minimum. We are free to support the whole 1-65535 Hz sample rate 
> range.

FWIW, a minimum sample rate of 1 Hz also does not make much sense. You 
cannot hear that frequency anymore... so it does not really make that much 
sense to support such low frequencies here. Just my 0.02 €.

> This is a convenient way to test edge cases. If you think the 
> audio_bug message is an issue, I'll improve the error handling in AUD_open_* 
> first and then resend this qos test.

I agree with Marc-André - the error message looks confusing when running the 
test. Maybe you could simply fence it with qtest_enable() at least?

> > > The noaudio backend uses a mixing-engine buffer size of 1024 audio
> > > frames and AUD_open_* tries to allocate memory for 1024/44100 = 0.0232
> > > audio frames for the resample buffer in audio_pcm_sw_alloc_resources_*.
> > > This allocation fails and produces the scary message. The error is
> > > handled correctly and AUD_open_* returns NULL. AUD_read and AUD_write
> > > return early if this pointer is NULL and the audio frontend callback
> > > functions will also not be called because the audio_frontend_frames_*
> > > functions return 0 in this case.
> > Thanks, it'd be nice to have such a description in the commit message.
>
> I'll improve the commit message of patch version 2.

A v2 would be appreciated!

 Thanks,
  Thomas