Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement VirtIO sound device playback #53

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Cuda-Chen
Copy link
Contributor

@Cuda-Chen Cuda-Chen commented Aug 31, 2024

Implement VirtIO sound device supporting these operations (the item with checked box checked means it is implemented right now):

  • For setting up the device:
    • VIRTIO_SND_R_JACK_INFO
    • VIRTIO_SND_R_PCM_INFO
    • VIRTIO_SND_R_CHMAP_INFO
  • For playing the sound (PCM):
    • VIRTIO_SND_R_PCM_SET_PARAMS
    • VIRTIO_SND_R_PCM_PREPARE
    • VIRTIO_SND_R_PCM_RELEASE
    • VIRTIO_SND_R_PCM_START
    • VIRTIO_SND_R_PCM_STOP

Limitations

  1. The emulator will hang if PulseAudio is enabled on host.
    It causes to CNFA and CNFA cannot initialize if PulseAudio
    is disabled, What's worse, it needs re-working as CNFA uses
    different threading models between pure ALSA and PulseAudio environment.
    Therefore, users need mitigations (i.e., restart the emulator after
    playing sound) or just wait for the future release.

  2. The playback may plays with repeating artifact (for example,
    A "front center" ALSA example sound will sound like "front front
    center"). The root cause is the Linux Kernel and it hasn't got
    fixed even in mainline version.

Test Cases

boot up test

test procedures

  1. Execute make check to run semu.
  2. Check kernel message (dmesg).

expected results

  1. The following message should appear while booting up:
[    4.011962] ALSA device list:
[    4.015962]   #0: Loopback 1
[    4.015962]   #1: VirtIO SoundCard at platform/f4400000.virtio/virtio2

check driver configuration

test procedures

  1. Execute aplay -l in emulator.
  2. Check the output messages.

expected results

  1. The following message should appear after Step 1:
$ aplay -l          
**** List of PLAYBACK Hardware Devices ****

<other sound device here>

card 1: SoundCard [VirtIO SoundCard], device 0: virtio-snd [VirtIO PCM 0]
  Subdevices: 1/1
  Subdevice #0: subdevice #0

play sound

test procedures

  1. Execute speaker-test in emulator.
  2. Check the host speaker.

expected results

  1. A white noise will be played by host speaker while speaker-test is executing.

play sequence sound

test procedures

  1. Execute aplay /usr/share/sounds/alsa/Front_Center.wav in emulator.
  2. Check the host speaker.

expected results

  1. A sound with "Front Center" speaking should be played.

@shengwen-tw

This comment was marked as outdated.

CNFA_sf.h Outdated Show resolved Hide resolved
virtio-snd.c Outdated Show resolved Hide resolved
configs/linux.config Outdated Show resolved Hide resolved
configs/linux.config Outdated Show resolved Hide resolved
os_generic.h Outdated Show resolved Hide resolved
@Cuda-Chen Cuda-Chen force-pushed the add-virtio-snd branch 3 times, most recently from 2b1009d to be7ad90 Compare September 1, 2024 13:24
Makefile Outdated Show resolved Hide resolved
configs/linux.config Outdated Show resolved Hide resolved
virtio-snd.c Outdated Show resolved Hide resolved
virtio-snd.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@jserv jserv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Run clang-format before submitting.

.gitmodules Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Copy link
Collaborator

@jserv jserv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consult https://github.com/cntools/cnfa/blob/master/.github/workflows/build-cnfa.yml and mention the build dependency in top-level README.md.

configs/linux.config Outdated Show resolved Hide resolved
feature.h Outdated Show resolved Hide resolved
@Cuda-Chen Cuda-Chen force-pushed the add-virtio-snd branch 2 times, most recently from a67216b to 1fc471c Compare September 3, 2024 09:02
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
configs/linux.config Outdated Show resolved Hide resolved
@Cuda-Chen Cuda-Chen force-pushed the add-virtio-snd branch 2 times, most recently from 862cb51 to da29ce5 Compare September 3, 2024 12:05
@Cuda-Chen Cuda-Chen force-pushed the add-virtio-snd branch 3 times, most recently from 712d08a to 5bbfde2 Compare January 9, 2025 06:03
@Cuda-Chen Cuda-Chen force-pushed the add-virtio-snd branch 2 times, most recently from c6fb743 to 62a58cf Compare January 14, 2025 08:05
@Cuda-Chen
Copy link
Contributor Author

Cuda-Chen commented Jan 19, 2025

Hi @jserv ,
After playing the sound for a whole month, I consider that there might be a pitfall of my implementation of playing sound or the Linux Kernel version this project is using.

The virtio-snd plays the sound with repeating artifact (taking "front center" as example, it sounds like "front front center").

Kindly leave some thoughts for some references or some folks of dealing with repeating artifact if I had a chance, and it will be a great thanks for your help!

If you would like to get the file, I will convert the file into base64 encoded text in order to upload to here.

@Cuda-Chen Cuda-Chen force-pushed the add-virtio-snd branch 6 times, most recently from 561d9d5 to c3a13e4 Compare January 22, 2025 05:02
@Cuda-Chen Cuda-Chen requested a review from jserv January 22, 2025 05:07
@Cuda-Chen Cuda-Chen changed the title Implement VirtIO sound device Implement VirtIO sound device playback Jan 22, 2025
@Cuda-Chen Cuda-Chen marked this pull request as ready for review January 22, 2025 05:08
@jserv
Copy link
Collaborator

jserv commented Jan 22, 2025

If you would like to get the file, I will convert the file into base64 encoded text in order to upload to here.

Simply share the prebuilt Linux kernel image file, rootfs, and test files via Google Drive, Dropbox, or similar services as they are intended to be temporary.

@Cuda-Chen
Copy link
Contributor Author

Hi @jserv ,

This Dropbox share link contains the files that reviewers will need: https://www.dropbox.com/scl/fo/vv3luiccfco5tjwt8tqxm/AANAyYgL9qvVPhuT4dtkkIA?rlkey=dv84qtw9y4e0ea8go03q5qwf8&st=73j9agts&dl=0

The usage of files are described as follows:

  1. Image: guest Linux OS image.
  2. rootfs.cpio: disk image.
  3. out_stream.bin: a binary dump of output stream (i.e., the Front_Center.wav of ALSA sound example). You can use aplay to play the file like this: cat out_stream.bin | aplay --format=S16_LE --rate=48000 -.

@jserv
Copy link
Collaborator

jserv commented Jan 23, 2025

Tested on macOS + Apple M1:

# aplay -l
**** List of PLAYBACK Hardware Devices ****
card 0: SoundCard [VirtIO SoundCard], device 0: virtio-snd [VirtIO PCM 0]
  Subdevices: 1/1
  Subdevice #0: subdevice #0
# aplay /usr/share/sounds/alsa/Front_Center.wav
Playing WAVE '/usr/share/sounds/alsa/Front_Center.wav' : Signed 16 bit Little Endian, Rate 48000 Hz, Mono
CoreAudio: from: [O/I] (null)/(null) (semu-virtio-snd) / (48000,0)x(1,0) (4800)

Then, it got stuck without progress.

@Cuda-Chen
Copy link
Contributor Author

Cuda-Chen commented Jan 23, 2025

Then, it got stuck without progress.

Thanks for helping the tests on macOS. I guess it is some kind of locking issue.
While trying to fix, I would like to ask whether a "front center" sound comes out from host or not while executing aplay /usr/share/sounds/alsa/Front_Center.wav?

For comparison on Linux + X86 it runs like this:

# aplay /usr/share/sounds/alsa/Front_Center.wav 
Playing WAVE '/usr/share/sounds/alsa/Front_Center.wav' : Signed 16 bit Little Endian, Rate 48000 Hz, Mono
CNFA Alsa Init (nil) (nil)  (48000 0) 1 0
0x55f41abed1d0 (nil)
PERIOD: 4800  BUFFER: 14400
CNFA Alsa Init Out -> 0x55f41abed1d0 (nil)  (48000 0) 1 0
(... play "front front center" then stuck in aplay program)

@jserv
Copy link
Collaborator

jserv commented Jan 23, 2025

Consider the change below to suppress compilation warnings raised by CNFA:

--- a/Makefile
+++ b/Makefile
@@ -85,6 +85,7 @@ CNFA_LIB := cnfa/CNFA_sf.h
 $(CNFA_LIB): cnfa/Makefile cnfa/os_generic
        $(MAKE) -C $(dir $<) CNFA_sf.h
 main.o: $(CNFA_LIB)
+virtio-snd.o: CFLAGS += -Wno-unused-parameter
 endif
 
 # .DEFAULT_GOAL should be set to all since the very first target is not all

@Cuda-Chen
Copy link
Contributor Author

Hi @jserv ,

Then, it got stuck without progress.

After checking the CNFA part, the coreaudio just like the PulseAudio/PipeWire part: no need to use a dedicated thread for pushing PCM frames.

I would like to ask which one you prefer for resolving this issue:

  1. Exclude macOS support.
    • No need to change the threading model of pushing PCM.
  2. Use PulseAudio/PipeWire solution.
    • Threading module of pushing PCM frames need to be re-designed.

@Cuda-Chen Cuda-Chen force-pushed the add-virtio-snd branch 2 times, most recently from 8d1a115 to 244b758 Compare January 23, 2025 13:06
virtio-snd.c Outdated Show resolved Hide resolved
The limitations are listed as follows:

1. The emulator will hang if PulseAudio is enabled on host.
The reason is that the host OS cannot close CNFA driver
because CNFA driver reference count is not zero (PulseAudio
holds one reference count). What's worse, CNFA cannot initialize if PulseAudio
is disabled, As it needs a dedicated threading model (CNFA uses
different threading models between pure ALSA and PulseAudio environment),
we suggest users need mitigations (for instance, restart the emulator after
playing sound) or just wait for the future release.

2. The playback may play with repeating artifact (for example,
A "front center" ALSA example sound will sound like "front front
center"). The root cause is the Linux Kernel and it hasn't got
fixed even in mainline version. See
https://lore.kernel.org/all/[email protected]/T/ for more
information.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants