Skip to content

Conversation

@oleg-dubinskiy
Copy link
Contributor

@oleg-dubinskiy oleg-dubinskiy commented Apr 17, 2024

Purpose

Fix playing wave header multiple times in case the caller requests to do it. In Windows, it is supported by WHDR_BEGINLOOP and WHDR_ENDLOOP flags (specified together) and dwLoops member of WAVEHDR structure (ususally set to 0xFFFFFFFF (INFINITE constant)).
This fixes the playback in the apps those request looped wave playback of some audio data (e. g., BRD Demo app, which did play its sound only first 500 ms before, now it plays endlessly until closing it manually).

JIRA issue: CORE-10118

Proposed changes

  • Check whenther WHDR_BEGINLOOP | WHDR_ENDLOOP flags are set by the caller.
  • If they are, get the amount of times to play the header from WAVEHDR.dwLoops.
  • Perform wave header competion only when the loop count is equal to zero. Otherwise, don't do it.
  • When the header is entirely committed, in case completion is not needed, reset committed bytes count to the starting value to allow the header play again.
  • Decrement loop count in case it isn't set to INFINITE, to mark loop as played correctly. When this count becomes zero, then the playback is finished.
  • Get rid from SOUND_OVERLAPPED.PerformCompletion member. Use SOUND_DEVICE_INSTANCE.LoopsRemaining == 0 condition instead.
  • Do this only for WaveOut devices, since MSDN states it works only with output buffers. Hence, there is nothing changed for WaveIn.
  • Update an appropriate statement about unimplemented functionality from mmebuddy notes.

TODO

  • Probably handle the case when multiple headers are requested to be looped (WHDR_BEGINLOOP and WHDR_ENDLOOP are set separatedly: 1st flag - in the 1st header, and 2nd in the last header). Currently, only looping a single wave header is supported (if I'm not wrong here).

Result

As visible in the following video, BRD demo app from the ticket is now playing the sound correctly after my changes. 🙂 Also, Minitube 2.4 Keygen also plays the sound correctly now, as it did not play it before them.

BRD_test.webm

@binarymaster binarymaster added the bugfix For bugfix PRs. label Apr 17, 2024
@HBelusca HBelusca self-requested a review April 17, 2024 20:49
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.

Absolutely outstanding Oleg! I waited almost 10 years to get that bug fixed. The playback is perfect with your patch!

@github-actions github-actions bot added the drivers Kernel mode drivers and frameworks label Apr 17, 2024
@JoachimHenze JoachimHenze self-assigned this Apr 18, 2024
@JoachimHenze
Copy link
Contributor

JoachimHenze commented Apr 18, 2024

https://build.reactos.org/#/builders/9/builds/36279 is running (against the testbots)...
VBox https://reactos.org/testman/compare.php?ids=94776,94778 +1 DSOUND crash (which might be related?) maybe we should rerun, the bots are not so stable in general on master head atm.
KVM didn't get the data to testman as it seems....

@oleg-dubinskiy
Copy link
Contributor Author

Just retested, everything still works same as before, no any regressions after applying the suggestions. So LGTM. ✅

Copy link
Contributor

@HBelusca HBelusca left a comment

Choose a reason for hiding this comment

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

LGTM. One trace was missing newline though.

@oleg-dubinskiy
Copy link
Contributor Author

✅ Still works fine for me. No any regressions.

Copy link
Contributor

@SergeGautherie SergeGautherie left a comment

Choose a reason for hiding this comment

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

With regard to this PR's limited scope, LGTM now to make a step forward.

I'll trust Oleg that existing surrounding code works as expected,
though 'I would feel safer if [extra call removal] was done (separately) before committing the current PR.'

Fix playing wave header multiple times in case the caller requests to do it.
In Windows, it is supported by WHDR_BEGINLOOP and WHDR_ENDLOOP flags (specified together) and dwLoops member of WAVEHDR structure (ususally set to 0xFFFFFFFF (INFINITE constant)).
- Check whenther WHDR_BEGINLOOP | WHDR_ENDLOOP flags are set by the caller.
- If they are, get the amount of times to play the header from WAVEHDR.dwLoops.
- Perform wave header competion only when the loop count is equal to zero. Otherwise, don't do it.
- When the header is entirely committed, in case completion is not needed, reset committed bytes count to the starting value to allow the header play again.
- Decrement loop count in case it isn't set to INFINITE, to mark loop as played correctly. When this count becomes zero, then the playback is finished.
- Get rid from SOUND_OVERLAPPED.PerformCompletion member. Use SOUND_DEVICE_INSTANCE.LoopsRemaining == 0 condition instead.
- Do this only for WaveOut devices, since MSDN states it works only with output buffers. Hence, there is nothing changed for WaveIn.
- Update an appropriate statement about unimplemented functionality from mmebuddy notes.
TODO: probably handle the case when multiple headers are requested to be looped (WHDR_BEGINLOOP and WHDR_ENDLOOP are set separatedly: 1st flag - in the 1st header, and 2nd in the last header). Currently, only looping a single wave header is supported (if I'm not wrong here).
This fixes the playback in the apps those request looped wave playback of some audio data (e. g., BRD Demo app, which did play its sound only first 500 ms before, now it plays endlessly until closing it manually).
CORE-10118
@oleg-dubinskiy oleg-dubinskiy force-pushed the wave_looped_playback_support branch from d9ecbdd to d5f2118 Compare April 20, 2024 16:17
@oleg-dubinskiy oleg-dubinskiy merged commit 24e088d into reactos:master Apr 24, 2024
@oleg-dubinskiy oleg-dubinskiy deleted the wave_looped_playback_support branch April 24, 2024 09:09
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants