diff mbox series

tests/qtest/ac97-test: add up-/downsampling tests

Message ID 20221022160052.1132-1-vr_qemu@t-online.de
State New
Headers show
Series tests/qtest/ac97-test: add up-/downsampling tests | expand

Commit Message

Volker Rümelin Oct. 22, 2022, 4 p.m. UTC
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>
---
 tests/qtest/ac97-test.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

Comments

Marc-André Lureau Oct. 24, 2022, 8:13 a.m. UTC | #1
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?


---
>  tests/qtest/ac97-test.c | 40 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/tests/qtest/ac97-test.c b/tests/qtest/ac97-test.c
> index 74103efdfa..ce25f1d588 100644
> --- a/tests/qtest/ac97-test.c
> +++ b/tests/qtest/ac97-test.c
> @@ -42,16 +42,54 @@ static void *ac97_create(void *pci_bus,
> QGuestAllocator *alloc, void *addr)
>      return &ac97->obj;
>  }
>
> +/*
> + * This is rather a test of the audio subsystem and not an AC97 test.
> Test if
> + * the audio subsystem can handle a 44100/1 upsample ratio. With an audio
> + * mixing buffer size of 1024 audio frames, the audio subsystem should
> either
> + * generate 1024 output frames from 0.0232 input frames or silently give
> up.
> + */
> +static void ac97_playback_upsample(void *obj, void *data, QGuestAllocator
> *alloc)
> +{
> +    QAC97 *ac97 = obj;
> +    QPCIDevice *dev = &ac97->dev;
> +    QPCIBar bar0;
> +
> +    qpci_device_enable(dev);
> +    bar0 = qpci_iomap(dev, 0, NULL);
> +    qpci_io_writew(dev, bar0, 0x2c, 0x1);
> +}
> +
> +/*
> + * This test is similar to the playback upsample test. This time the audio
> + * subsystem should either generate 0.0232 audio frames from 1024 input
> frames
> + * or silently give up.
> + */
> +static void ac97_record_downsample(void *obj, void *data, QGuestAllocator
> *alloc)
> +{
> +    QAC97 *ac97 = obj;
> +    QPCIDevice *dev = &ac97->dev;
> +    QPCIBar bar0;
> +
> +    qpci_device_enable(dev);
> +    bar0 = qpci_iomap(dev, 0, NULL);
> +    qpci_io_writew(dev, bar0, 0x32, 0x1);
> +}
> +
>  static void ac97_register_nodes(void)
>  {
>      QOSGraphEdgeOptions opts = {
> -        .extra_device_opts = "addr=04.0",
> +        .extra_device_opts = "addr=04.0,audiodev=snd0",
> +        .after_cmd_line = "-audiodev none,id=snd0"
> +                          ",out.frequency=44100,in.frequency=44100",
>      };
>      add_qpci_address(&opts, &(QPCIAddress) { .devfn = QPCI_DEVFN(4, 0) });
>
>      qos_node_create_driver("AC97", ac97_create);
>      qos_node_produces("AC97", "pci-device");
>      qos_node_consumes("AC97", "pci-bus", &opts);
> +
> +    qos_add_test("playback_upsample", "AC97", ac97_playback_upsample,
> NULL);
> +    qos_add_test("record_downsample", "AC97", ac97_record_downsample,
> NULL);
>  }
>
>  libqos_init(ac97_register_nodes);
> --
> 2.35.3
>
>
>
Volker Rümelin Oct. 24, 2022, 8:31 p.m. UTC | #2
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.

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.

If you would like to see the two tests fail, revert commit 0cbc8bd469 
("audio: remove abort() in audio_bug()") and rerun qos-test.

With best regards,
Volker

>
>
>     ---
>      tests/qtest/ac97-test.c | 40 +++++++++++++++++++++++++++++++++++++++-
>      1 file changed, 39 insertions(+), 1 deletion(-)
>
>     diff --git a/tests/qtest/ac97-test.c b/tests/qtest/ac97-test.c
>     index 74103efdfa..ce25f1d588 100644
>     --- a/tests/qtest/ac97-test.c
>     +++ b/tests/qtest/ac97-test.c
>     @@ -42,16 +42,54 @@ static void *ac97_create(void *pci_bus,
>     QGuestAllocator *alloc, void *addr)
>          return &ac97->obj;
>      }
>
>     +/*
>     + * This is rather a test of the audio subsystem and not an AC97
>     test. Test if
>     + * the audio subsystem can handle a 44100/1 upsample ratio. With
>     an audio
>     + * mixing buffer size of 1024 audio frames, the audio subsystem
>     should either
>     + * generate 1024 output frames from 0.0232 input frames or
>     silently give up.
>     + */
>     +static void ac97_playback_upsample(void *obj, void *data,
>     QGuestAllocator *alloc)
>     +{
>     +    QAC97 *ac97 = obj;
>     +    QPCIDevice *dev = &ac97->dev;
>     +    QPCIBar bar0;
>     +
>     +    qpci_device_enable(dev);
>     +    bar0 = qpci_iomap(dev, 0, NULL);
>     +    qpci_io_writew(dev, bar0, 0x2c, 0x1);
>     +}
>     +
>     +/*
>     + * This test is similar to the playback upsample test. This time
>     the audio
>     + * subsystem should either generate 0.0232 audio frames from 1024
>     input frames
>     + * or silently give up.
>     + */
>     +static void ac97_record_downsample(void *obj, void *data,
>     QGuestAllocator *alloc)
>     +{
>     +    QAC97 *ac97 = obj;
>     +    QPCIDevice *dev = &ac97->dev;
>     +    QPCIBar bar0;
>     +
>     +    qpci_device_enable(dev);
>     +    bar0 = qpci_iomap(dev, 0, NULL);
>     +    qpci_io_writew(dev, bar0, 0x32, 0x1);
>     +}
>     +
>      static void ac97_register_nodes(void)
>      {
>          QOSGraphEdgeOptions opts = {
>     -        .extra_device_opts = "addr=04.0",
>     +        .extra_device_opts = "addr=04.0,audiodev=snd0",
>     +        .after_cmd_line = "-audiodev none,id=snd0"
>     + ",out.frequency=44100,in.frequency=44100",
>          };
>          add_qpci_address(&opts, &(QPCIAddress) { .devfn =
>     QPCI_DEVFN(4, 0) });
>
>          qos_node_create_driver("AC97", ac97_create);
>          qos_node_produces("AC97", "pci-device");
>          qos_node_consumes("AC97", "pci-bus", &opts);
>     +
>     +    qos_add_test("playback_upsample", "AC97",
>     ac97_playback_upsample, NULL);
>     +    qos_add_test("record_downsample", "AC97",
>     ac97_record_downsample, NULL);
>      }
>
>      libqos_init(ac97_register_nodes);
>     -- 
>     2.35.3
>
>
>
>
> -- 
> Marc-André Lureau
Marc-André Lureau Oct. 25, 2022, 7:44 a.m. UTC | #3
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.

>
>
> 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.
Volker Rümelin Oct. 26, 2022, 7:34 p.m. UTC | #4
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. 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.

>> 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.

With best regards,
Volker
Thomas Huth Nov. 4, 2022, 5:33 p.m. UTC | #5
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
Philippe Mathieu-Daudé Nov. 4, 2022, 5:56 p.m. UTC | #6
On 4/11/22 18:33, Thomas Huth wrote:
> 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 €.

Still useful when using a sound card as signal generator, i.e.:
https://www.allaboutcircuits.com/technical-articles/how-to-use-your-computer-as-an-arbitrary-waveform-generator/
Thomas Huth Nov. 4, 2022, 6:15 p.m. UTC | #7
On 04/11/2022 18.56, Philippe Mathieu-Daudé wrote:
> On 4/11/22 18:33, Thomas Huth wrote:
>> 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 €.
> 
> Still useful when using a sound card as signal generator, i.e.:
> https://www.allaboutcircuits.com/technical-articles/how-to-use-your-computer-as-an-arbitrary-waveform-generator/ 

I dare to say that you can reproduce low frequency wave forms with higher 
sample rates, too. In the example on that page, the author also reproduces a 
signal with the rate of 441 Hz with a sample rate of 22050 Hz, so I do not 
really see your point here why lower sample rates should still be useful here.

  Thomas
Volker Rümelin Nov. 6, 2022, 9:37 a.m. UTC | #8
Am 04.11.22 um 18:33 schrieb 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 €.

Hi,

sample rates below a minimum sample rate are currently not supported. 
The audio device gets disabled. This is why you see the confusing 
audio_bug() message. The minimum sample rate depends on the selected 
audio backend and can be changed indirectly with -audiodev arguments. If 
we change the AC97 minimum sample rate to 8000 Hz, it's much more 
difficult to test this code path.

>
>> 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?

I have written two patches for the audio subsystem to address this 
issue. This was two days before QEMU 7.2 soft freeze. I'll send the 
patches to the mailing list after the release of QEMU 7.2.

>
>>>> 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!

I won't forget it.

With best regards,
Volker

>
>  Thanks,
>   Thomas
>
diff mbox series

Patch

diff --git a/tests/qtest/ac97-test.c b/tests/qtest/ac97-test.c
index 74103efdfa..ce25f1d588 100644
--- a/tests/qtest/ac97-test.c
+++ b/tests/qtest/ac97-test.c
@@ -42,16 +42,54 @@  static void *ac97_create(void *pci_bus, QGuestAllocator *alloc, void *addr)
     return &ac97->obj;
 }
 
+/*
+ * This is rather a test of the audio subsystem and not an AC97 test. Test if
+ * the audio subsystem can handle a 44100/1 upsample ratio. With an audio
+ * mixing buffer size of 1024 audio frames, the audio subsystem should either
+ * generate 1024 output frames from 0.0232 input frames or silently give up.
+ */
+static void ac97_playback_upsample(void *obj, void *data, QGuestAllocator *alloc)
+{
+    QAC97 *ac97 = obj;
+    QPCIDevice *dev = &ac97->dev;
+    QPCIBar bar0;
+
+    qpci_device_enable(dev);
+    bar0 = qpci_iomap(dev, 0, NULL);
+    qpci_io_writew(dev, bar0, 0x2c, 0x1);
+}
+
+/*
+ * This test is similar to the playback upsample test. This time the audio
+ * subsystem should either generate 0.0232 audio frames from 1024 input frames
+ * or silently give up.
+ */
+static void ac97_record_downsample(void *obj, void *data, QGuestAllocator *alloc)
+{
+    QAC97 *ac97 = obj;
+    QPCIDevice *dev = &ac97->dev;
+    QPCIBar bar0;
+
+    qpci_device_enable(dev);
+    bar0 = qpci_iomap(dev, 0, NULL);
+    qpci_io_writew(dev, bar0, 0x32, 0x1);
+}
+
 static void ac97_register_nodes(void)
 {
     QOSGraphEdgeOptions opts = {
-        .extra_device_opts = "addr=04.0",
+        .extra_device_opts = "addr=04.0,audiodev=snd0",
+        .after_cmd_line = "-audiodev none,id=snd0"
+                          ",out.frequency=44100,in.frequency=44100",
     };
     add_qpci_address(&opts, &(QPCIAddress) { .devfn = QPCI_DEVFN(4, 0) });
 
     qos_node_create_driver("AC97", ac97_create);
     qos_node_produces("AC97", "pci-device");
     qos_node_consumes("AC97", "pci-bus", &opts);
+
+    qos_add_test("playback_upsample", "AC97", ac97_playback_upsample, NULL);
+    qos_add_test("record_downsample", "AC97", ac97_record_downsample, NULL);
 }
 
 libqos_init(ac97_register_nodes);