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

Cosmos Audio Infrastructure (CAI) #2318

Merged
merged 25 commits into from Jul 8, 2022

Conversation

ascpixi
Copy link
Contributor

@ascpixi ascpixi commented Jul 2, 2022

This pull request implements an audio infrastructure that helps with audio-related operations and interfacing with audio devices at a low-level. An AC97 driver is included.

The following features can be implemented in future pull requests:

  • Surround channel support for the AC97
  • 20-bit AC97 audio support by down-sampling 24-bit audio
  • Sound Blaster 16/Ensoniq AudioPCI compatibility for VMWare

The AC97 driver is not supported by VMWare, as it does not feature emulation for any fully-compatible AC97 devices. It's recommended to use CAI with emulators/virtualizers such as QEMU or VirtualBox.

TODO:

  • Resolve all requested changes
  • Create documentation
  • Create tests

You can download a demo kernel solution by clicking here.

These files will be replaced with the new Cosmos Audio Infrastructure (CAI).
The Cosmos Audio Infrastructure (CAI) helps with audio-related operations and with interfacing to audio devices at a low-level.
The AC97 is an audio codec standard developed by Intel Architecture Labs. The standard is used in motherboards, modems, and sound cards, and allows for sound cards to share a common architecture.

This commit also modifies the PIC to allow for IRQ 11.
@Kiirx
Copy link
Contributor

Kiirx commented Jul 2, 2022

This is a really interesting pr, but the problem is that in virtualbox the VFS doesnt work, and so we cant really use virtual box when we use the vfs, maybe ill try to give a preformatted vmdk file for virtualbox when I get home.

@ascpixi
Copy link
Contributor Author

ascpixi commented Jul 2, 2022

This is a really interesting pr, but the problem is that in virtualbox the VFS doesnt work, and so we cant really use virtual box when we use the vfs, maybe ill try to give a preformatted vmdk file for virtualbox when I get home.

There may be a way to get AC97 support in VMWare using the sound.virtualDev VMX property - I'll look into it

@Kiirx
Copy link
Contributor

Kiirx commented Jul 2, 2022

This is a really interesting pr, but the problem is that in virtualbox the VFS doesnt work, and so we cant really use virtual box when we use the vfs, maybe ill try to give a preformatted vmdk file for virtualbox when I get home.

There may be a way to get AC97 support in VMWare using the sound.virtualDev VMX property - I'll look into it

This could maybe help you https://kb.vmware.com/s/article/2010400

@ascpixi
Copy link
Contributor Author

ascpixi commented Jul 2, 2022

It looks like that while the ES1371 is a AC97-compatible sound card, it still defines its own method to set it up.... This explains why the AC97 driver does detect the sound card, but it doesn't want to perform a PCM Out reset. For example, the AC97 spec defines a buffer descriptor list, while the ES1371 uses a single buffer. I feel like this will require another driver class :/

For now, the driver works fine on virtualizers/emulators that define a fully-compatible AC97 sound card (like QEMU and VBox)

Copy link
Member

@quajak quajak left a comment

Choose a reason for hiding this comment

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

Thanks for the great PR! Overall the code looks very good with just a few tiny changes required. I already added a few questions/comments but will need to look through it again.

Could you please add some documentation for both users to use it and other developers to better understand the design approach? I think it would also be good to add a few tests to ensure that the code does not break.

source/Cosmos.Core/PIC.cs Show resolved Hide resolved
source/Cosmos.HAL2/Audio/AudioBuffer.cs Outdated Show resolved Hide resolved
source/Cosmos.HAL2/Audio/AudioBuffer.cs Show resolved Hide resolved
source/Cosmos.HAL2/Audio/AudioBuffer.cs Show resolved Hide resolved
source/Cosmos.HAL2/Audio/SampleFormat.cs Outdated Show resolved Hide resolved
source/Cosmos.System2/Audio/AudioMixer.cs Outdated Show resolved Hide resolved
source/Cosmos.System2/Audio/AudioMixer.cs Outdated Show resolved Hide resolved
source/Cosmos.System2/Audio/AudioMixer.cs Show resolved Hide resolved
source/Cosmos.System2/Audio/AudioMixer.cs Show resolved Hide resolved
Enables unsafe code in all configurations when compiling the Cosmos.HAL2 project.
This commit allows the kernel to change the buffer size of an AC97-compatible sound card while it is running.
Slight AudioMixer clean-up: this commit makes the syntax cleaner, and removes an obsolete constructor created for an older version of the class when it was still in development.

The removed constructor is not needed, as AudioMixer automatically creates its buffers to compensate for any buffers that will be written to.
@ascpixi ascpixi requested a review from quajak July 3, 2022 18:52
This commit implements a poll limit for the AC97 driver when a reset request is performed. AC97-compatible sound cards that have a different set-up routine, i.e. Ensoniq AudioPCI, will not respond to such requests.
if you have any questions, feel free to contact me at Discord: ascpixi#8013
i'll be happy to help with any questions :)
i forgor 💀 to proof-read it
source/Cosmos.HAL2/Audio/AudioBuffer.cs Outdated Show resolved Hide resolved
source/Cosmos.HAL2/Cosmos.HAL2.csproj Outdated Show resolved Hide resolved
@MEMESCOEP
Copy link
Contributor

How do I use the example? I got 13 errors when I first opened the project :(

@ascpixi
Copy link
Contributor Author

ascpixi commented Jul 5, 2022

How do I use the example? I got 13 errors when I first opened the project :(

Right now, the CAI isn't merged into Cosmos - if you're using the devkit built using this branch and you're experiencing problems, please post the errors you get here

@ascpixi ascpixi requested a review from quajak July 5, 2022 16:26
@Arawn-Davies
Copy link
Member

SB16 in Cosmos, next up, DOOM!
👍🏽 looks exciting!

Copy link
Member

@quajak quajak left a comment

Choose a reason for hiding this comment

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

It looks good except for the style differences. Could you please fix capitalization and {} around single line ifs?

@ascpixi
Copy link
Contributor Author

ascpixi commented Jul 6, 2022

It looks good except for the style differences. Could you please fix capitalization and {} around single line ifs?

So the only thing to change is adding brackets to all ifs and all public fields should be PascalCase, right?

This member will probably never be used, as an audio mixer shouldn't really have a "position" property.
Changes:
- public field names changed to PascalCase
- added braces to all "if" statements
@ascpixi ascpixi requested a review from quajak July 6, 2022 19:33
quajak
quajak previously approved these changes Jul 7, 2022
Copy link
Member

@quajak quajak left a comment

Choose a reason for hiding this comment

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

Thanks for changing it. Looks good to me now!

@valentinbreiz valentinbreiz self-requested a review July 7, 2022 21:48
valentinbreiz
valentinbreiz previously approved these changes Jul 7, 2022
Copy link
Member

@valentinbreiz valentinbreiz left a comment

Choose a reason for hiding this comment

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

Really great work thanks for your contribution, it looks fine to me. Plus since the AC97 card supports MSI it will be really helpful to test interrupts for PCI devices using APIC.

Edit: @quajak is it okay for you to have unsafe code under Cosmos.HAL2?

@ascpixi ascpixi dismissed stale reviews from valentinbreiz and quajak via 941309e July 7, 2022 22:04
Sometimes, .WAV files can contain extra information by encoders. This is de facto obsolete, and no modern encoder uses any extra fields in the header, however, older encoders can introduce such headers. This commit will skip such extra information.

An example of a file with a non-standard field attached at the end: https://cdn.discordapp.com/attachments/833978908189392916/994710945337188472/out_of_gum_x.wav
@ascpixi
Copy link
Contributor Author

ascpixi commented Jul 7, 2022

Some minor adjustments, I feel like it's ready to be merged now!

@quajak
Copy link
Member

quajak commented Jul 8, 2022

@valentinbreiz Good point, I think its fine with unsafe code in HAL. IMO The code definitely belongs in HAL. Not using pointers is slower and I think speed is quite important for CAI. Rewriting it all to use Spans would maybe work 🤷 but simply using pointers if fine for me.

@quajak quajak self-requested a review July 8, 2022 15:58
@valentinbreiz valentinbreiz merged commit ea6350b into CosmosOS:master Jul 8, 2022
@ascpixi ascpixi deleted the cosmos-audio-infrastructure branch July 8, 2022 16:59
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.

None yet

7 participants