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

[AUDIO] Implement support for WAVE_FORMAT_EXTENSIBLE audio format #6686

Merged
merged 3 commits into from
Apr 11, 2024

Conversation

oleg-dubinskiy
Copy link
Contributor

@oleg-dubinskiy oleg-dubinskiy commented Mar 29, 2024

Purpose

Fix opening audio device error when trying to open it with WAVE_FORMAT_EXTENSIBLE format tag set in wFormatTag member of WAVEFORMATEX by the proper passing additional audio data size and removing a WAVE_FORMAT_PCM limitation check, both for Legacy and MMixer APIs.
This fixes audio playback in several apps those are using extensible audio data (e. g. AIMP 5.30 (needs to manually set BPS to 16 in settings still!), QMMP 0.12.17, all Chrome/Chromium based browsers, GameDevTycoon Demo game etc.).
Supersedes #3981.
Can be tested in pair with #6682 for better effect (is merged now).

JIRA issue: CORE-10907, CORE-14783, other audio issues might also be fixed, they need to be retested too and added here in case they are fixed!

Proposed changes

[MMIXER]

  • Pass additional data size from WAVEFORMATEX.cbSize to pin data format.
  • Append it to the whole size of pin data format (KSDATAFORMAT.FormatSize).
  • Set additional fields in WAVEFORMATEXTENSIBLE structure (data format, BPS and channel mask) when WAVE_FORMAT_EXTENSIBLE is used. They are used by our inbuilt Intel AC97 miniport driver at least. It simply fails when these members are not set.
  • Fix pin connect allocation size by appending an additional data size from WAVEFORMATEX.cbSize to KSPIN_CONNECT + KSDATAFORMAT + WAVEFORMATEX. This allows to properly initialize additional extensible data and avoids kernel memory leakage when using extensible audio format.
  • Remove format tag check which allowed WAVE_FORMAT_PCM to be opened correctly. So now all possible audio formats can be opened properly at least (although it does not mean they may work correctly).

[KS]

  • Allow passing additional extensible audio data when extensible audio format is used, by appending additional data size from WAVEFORMATEX.cbSize to pin connect size passed to KsCreatePin. If the tag is WAVE_FORMAT_PCM, then this member should always be zero. So in that case, no any additional data is passed to creation request, and the passed data size is correct for PCM too (KSDATAFORMAT + WAVEFORMATEX).

[WDMAUD.DRV]

  • Pass the correct additional data size to I/O control request, by storing a correct size of additional data in WAVEFORMATEX.cbSize when performing open of audio device, when WAVE_FORMAT_EXTENSIBLE audio format is used. It allows to properly open audio device with Legacy APIs enabled too.

Result

As visible in the following video, all mentioned apps are now playing the sound correctly (can be played only in Chrome). 🙂

ros_extensible_format.mp4

GameDevTycoon Demo was not tested in the video, but @julenuri confirmed this app is now playing the sound properly too, as it did not do before my changes.

@github-actions github-actions bot added the drivers Kernel mode drivers and frameworks label Mar 29, 2024
@binarymaster binarymaster added enhancement For PRs with an enhancement/new feature. bugfix For bugfix PRs. labels Mar 29, 2024
@binarymaster binarymaster added this to New PRs in ReactOS PRs via automation Mar 29, 2024
@JoachimHenze
Copy link
Contributor

JoachimHenze commented Mar 29, 2024

Reminder to myself: I will have to retest the following stuff with this PR:
AIMP CORE-10907
and the former Heap-corruptions
https://jira.reactos.org/browse/CORE-16340?focusedCommentId=130039&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-130039
https://jira.reactos.org/browse/CORE-17744?focusedCommentId=130035&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-130035
https://jira.reactos.org/browse/CORE-15324?focusedCommentId=130037&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-130037
That I experienced when testing an older version of this patch on an older state of 0.4.15-dev' (where the MM still sucked much more).
Please allow me some time to redo my testing before merging. I will not forget, promised!
We should consider rebasing it after the tangaming MM fix got merged before testing the buildbots artifacts in here!

@oleg-dubinskiy
Copy link
Contributor Author

Reminder to myself: I will have to retest the following stuff with this PR: AIMP CORE-10907 and the former Heap-corruptions https://jira.reactos.org/browse/CORE-16340?focusedCommentId=130039&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-130039 https://jira.reactos.org/browse/CORE-17744?focusedCommentId=130035&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-130035 https://jira.reactos.org/browse/CORE-15324?focusedCommentId=130037&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-130037 That I experienced when testing an older version of this patch on an older state of 0.4.15-dev' (where the MM still sucked much more). Please allow me some time to redo my testing before merging. I will not forget, promised!

OK, thank you. Np. 👍
Then those tickets can be added to the PR description too I guess. 🙂

Fix opening audio device error when trying to open it with WAVE_FORMAT_EXTENSIBLE format tag set in wFormatTag member of WAVEFORMATEX:
- Pass additional data size from WAVEFORMATEX.cbSize to pin data format.
- Append it to the whole size of pin data format (KSDATAFORMAT.FormatSize).
- Set additional fields in WAVEFORMATEXTENSIBLE structure (data format, BPS and channel mask) when WAVE_FORMAT_EXTENSIBLE is used. They are used by our inbuilt Intel AC97 miniport driver at least. It simply fails when these members are not set.
- Fix pin connect allocation size by appending an additional data size from WAVEFORMATEX.cbSize to KSPIN_CONNECT + KSDATAFORMAT + WAVEFORMATEX. This allows to proerly initialize additional extensible data and avoids kernel memory leakage when using extensible audio format.
- Remove format tag check which allowed WAVE_FORMAT_PCM to be opened correctly. So now all possible audio formats can be opened properly at least (although it does not mean they may work correctly).
This fixes the audio playback for all apps those are supporting extensible audio data and use it by default (e. g. AIMP 5.30, QMMP 0.12.17, all Chrome/Chromium-based browsers, GameDev Tycoon Demo game etc.).
CORE-10907, CORE-14783
…udio format is used

- Append additional data size from WAVEFORMATEX.cbSize to pin connect size passed to KsCreatePin. If the tag is WAVE_FORMAT_PCM, then this member should always be zero. So in that case, no any additional data is passed to creation request, and the passed data size is correct for PCM too (KSDATAFORMAT + WAVEFORMATEX).
This fixes audio playback in several apps those are supporting extensibble audio and use it by default (e. g. AIMP 5.30, QMMP 0.12.17, all Chrome/Chromium based browsers, GameDev Tycoon Demo game etc.).
CORE-10907, CORE-14783.
…uest

Store a correct size of additional data in WAVEFORMATEX.cbSize when performing open of audio device, when WAVE_FORMAT_EXTENSIBLE audio format is used.
It allows to properly open audio device with Legacy APIs enabled too.
This fixes audio playback in several apps those are using extensible audio data (e. g. AIMP 5.30, QMMP 0.12.17, all Chrome/Chromium based browsers, GameDevTycoon Demo game etc.).
CORE-10907, CORE-14783
Copy link
Contributor

@JoachimHenze JoachimHenze left a comment

Choose a reason for hiding this comment

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

My testing result of this PR's state
using the reactos-gcc-i386-Debug-0x502-b83bc3e1193965c4ef950b4004aee3fba0709402 GCC8.4.0dbg artifact (which is after the rebase and therefore implicitly contained also the PR6682:

CORE-10907 'AIMP DSOUND 16bit-cfg' is properly fixed.
see https://jira.reactos.org/browse/CORE-10907?focusedId=141749&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-141749 for details.

CORE-14783 'QMMP DSOUND vs. waveout' is properly fixed.
see https://jira.reactos.org/browse/CORE-14783?focusedId=141748&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-141748 for details.

The PR has no negative impact on the already earlier resolved https://jira.reactos.org/browse/CORE-16340 . Still plays sound and music fine. Still without any signs of heap-corruption.

The PR has absolutely no positive or negative impact on the yet unresolved https://jira.reactos.org/browse/CORE-17744 'FoxAudioPlayer with its WMM audio output'.

The PR has absolutely no positive or negative impact on the yet unresolved https://jira.reactos.org/browse/CORE-10118 'BRD demo app stops playing after 500ms'

The PR has absolutely no positive or negative impact on the already resolved https://jira.reactos.org/browse/CORE-15324

https://jira.reactos.org/browse/CORE-15693 can currently no longer be retested, because 'Hover' does not start anymore in master head 'Unable to allocate memory'. I can prove meanwhile, that this regression is not caused by Olegs PR, as it happens also on master head without his PR. I filed the fresh https://jira.reactos.org/browse/CORE-19505 for that now.

All in All: I could spot only improvements, but nothing breaking by this PR. I do think it is a very valuable contribution.
If nobody will add any negative review-comments to this PR, then I will merge it in t minus 1 week.

ReactOS PRs automation moved this from New PRs to Approved by reviewers Apr 4, 2024
@oleg-dubinskiy
Copy link
Contributor Author

My testing result of this PR's state
using the reactos-gcc-i386-Debug-0x502-b83bc3e1193965c4ef950b4004aee3fba0709402 GCC8.4.0dbg artifact (which is after the rebase and therefore implicitly contained also the PR6682:

CORE-10907 'AIMP DSOUND 16bit-cfg' is properly fixed.
see https://jira.reactos.org/browse/CORE-10907?focusedId=141749&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-141749 for details.

CORE-14783 'QMMP DSOUND vs. waveout' is properly fixed.
see https://jira.reactos.org/browse/CORE-14783?focusedId=141748&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-141748 for details.

The PR has no negative impact on the already earlier resolved https://jira.reactos.org/browse/CORE-16340 . Still plays sound and music fine. Still without any signs of heap-corruption.

The PR has absolutely no positive or negative impact on the yet unresolved https://jira.reactos.org/browse/CORE-17744 'FoxAudioPlayer with its WMM audio output'.

The PR has absolutely no positive or negative impact on the yet unresolved https://jira.reactos.org/browse/CORE-10118 'BRD demo app stops playing after 500ms'

The PR has absolutely no positive or negative impact on the already resolved https://jira.reactos.org/browse/CORE-15324

https://jira.reactos.org/browse/CORE-15693 can currently no longer be retested, because 'Hover' does not start anymore in master head 'Unable to allocate memory' but I am pretty sure, that this is not related to Olegs PR. We should nevertheless create a fresh ticket for that new bug, if it will turn out to happen also on unmodified master head. I might retest that tomorrow on unmodified master head to file a fresh regression-ticket.

All in All: I could spot only improvements, but nothing breaking by this PR. I do think it is a very valuable contribution.
If nobody will add any negative review-comments to this PR, then I will merge it in t minus 1 week.

Thank you very much for your testing Joachim. 😊

@JoachimHenze JoachimHenze self-assigned this Apr 4, 2024
@JoachimHenze JoachimHenze merged commit 12b3272 into reactos:master Apr 11, 2024
38 checks passed
ReactOS PRs automation moved this from Approved by reviewers to Done Apr 11, 2024
@oleg-dubinskiy oleg-dubinskiy deleted the extensible_audio_format branch April 11, 2024 10:01
@JoachimHenze
Copy link
Contributor

JoachimHenze commented Apr 13, 2024

PRs testbot result 0.4.15-dev-7884-gd72d61f vs. 0.4.15-dev-7885-g12b3272:
VBox: https://reactos.org/testman/compare.php?ids=94590,94599 +2 crashes winmm:capture and winmm:wave
KVM https://reactos.org/testman/compare.php?ids=94591,94600 (doesn't show anything additionally failing, but KVM bots do not perform any sound testing, that is only the case for the VBox-testbot afaik)

{code}
capture.c:147: Test failed: waveInOpen(0): format=96000x16x2 flags=50004(CALLBACK_EVENT|WAVE_MAPPED) rc=MMSYSERR_ERROR(Undefined external error.)
Unhandled exception
ExceptionCode: c0000005
Faulting Address: 0CCD1000
CS:EIP 1b:70c7f574
DS 23 ES 23 FS 3b GS 0
EAX: 0ccd0ff0 EBX: 70c86020 ECX: 00000001
EDX: 0023cea8 EBP: 0022f928 ESI: 002365b8 ESP: 0022f8f8
EDI: 00000001 EFLAGS: 00010206
Address:
wdmaud.drv:f574 (C:\ReactOS\System32\wdmaud.drv@70c70000)
Frames:
wdmaud.drv:f9dc (C:\ReactOS\System32\wdmaud.drv@70c70000)
wdmaud.drv:2ca9 (C:\ReactOS\System32\wdmaud.drv@70c70000)
wdmaud.drv:725e (C:\ReactOS\System32\wdmaud.drv@70c70000)
wdmaud.drv:6dde (C:\ReactOS\System32\wdmaud.drv@70c70000)
wdmaud.drv:60f6 (C:\ReactOS\System32\wdmaud.drv@70c70000)
winmm.dll:3c5b (C:\ReactOS\System32\winmm.dll@7bab0000)
winmm.dll:4187 (C:\ReactOS\System32\winmm.dll@7bab0000)
winmm.dll:11359 (C:\ReactOS\System32\winmm.dll@7bab0000)
winmm.dll:1678c (C:\ReactOS\System32\winmm.dll@7bab0000)
<winmm_winetest.exe:17ca> (C:\ReactOS\bin\winmm_winetest.exe@400000)
<winmm_winetest.exe:2e8b> (C:\ReactOS\bin\winmm_winetest.exe@400000)
<winmm_winetest.exe:3b90> (C:\ReactOS\bin\winmm_winetest.exe@400000)
<winmm_winetest.exe:22434> (C:\ReactOS\bin\winmm_winetest.exe@400000)
<winmm_winetest.exe:2a362> (C:\ReactOS\bin\winmm_winetest.exe@400000)
<winmm_winetest.exe:2a3d9> (C:\ReactOS\bin\winmm_winetest.exe@400000)
kernel32.dll:11cde (C:\ReactOS\system32\kernel32.dll@7c600000)
fixme:(dll/win32/winmm/lolvldrv.c:620) Closing while ll-driver open
{code}

We were not skipping tests before the code changes, at least that's what the testbot says.
So I guess that it makes sense to investigate those crashes next in regards to "improving sound", even if they would turn out to occur only in stuff that couldn't be accessed at all before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix For bugfix PRs. drivers Kernel mode drivers and frameworks enhancement For PRs with an enhancement/new feature.
Projects
ReactOS PRs
  
Done
4 participants