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

fix(audio): set the bits-per-sample of Steam Streaming Speakers to 16-bit when the default audio device is 16-bit #3704

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

andygrundman
Copy link
Contributor

@andygrundman andygrundman commented Mar 5, 2025

Description

fix(audio): set the bits-per-sample of Steam Streaming Speakers to 16-bit when the default audio device is 16-bit. This may help with audio glitches when enabling the virtual device.

Screenshot

Issues Fixed or Closed

Resolves https://github.com/orgs/LizardByte/discussions/682

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Dependency update (updates to dependencies)
  • Documentation update (changes to documentation)
  • Repository update (changes to repository files, e.g. .github/...)

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

Copy link

codecov bot commented Mar 5, 2025

Codecov Report

Attention: Patch coverage is 5.88235% with 16 lines in your changes missing coverage. Please review.

Project coverage is 11.60%. Comparing base (fd9f10f) to head (b0db65c).

Files with missing lines Patch % Lines
src/platform/windows/audio.cpp 5.88% 16 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3704      +/-   ##
==========================================
- Coverage   11.61%   11.60%   -0.02%     
==========================================
  Files          92       92              
  Lines       17337    17353      +16     
  Branches     8100     8109       +9     
==========================================
  Hits         2013     2013              
- Misses      14727    14743      +16     
  Partials      597      597              
Flag Coverage Δ
Linux 11.28% <ø> (ø)
Windows 12.99% <5.88%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/platform/windows/audio.cpp 25.27% <5.88%> (-0.77%) ⬇️

@andygrundman
Copy link
Contributor Author

I'm going to rewrite this as a method that returns the default device format and actually matches the SSS to the device. It doesn't need to be specific to 16 bits.

@andygrundman
Copy link
Contributor Author

andygrundman commented Mar 5, 2025

This may have a bad bug :( I somehow caused it to run into some errors. I realized I'm not sure that I'm safely calling default_device(device_enum) and will need to read more of the docs.

Info: Reinitializing audio capture
Error: Couldn't initialize audio client for [Stereo]: [0x8889000A]
Error: Couldn't find supported format for audio
Warning: Couldn't re-initialize audio input

@ReenigneArcher ReenigneArcher marked this pull request as draft March 5, 2025 14:04
@andygrundman andygrundman force-pushed the andyg.16bit-steam-speakers branch from 7859814 to e7df2a3 Compare March 6, 2025 10:59
@andygrundman
Copy link
Contributor Author

andygrundman commented Mar 6, 2025

OK, this is cleaned up and squashed to 1 commit. I discovered that the last patch I made to this file, removing f32 and s32 from stereo, would break this new logic if you had a 32-bit default device. The correct fix for that previous issue is just to put these at the bottom of the list so they aren't preferred over 24 and 16.

This now works well for me and I can't reproduce the issue I had. I have also added extra logging that may prove helpful if this doesn't solve the issue for people. Specifically I log the bps and samplerate of the internal mixer format, with a note about auto-resampling if it's not 48k.

@andygrundman andygrundman marked this pull request as ready for review March 6, 2025 11:07
… format

* Match the virtual device's bits-per-sample to the default audio device, as a mismatch could cause audio glitches.
* Add back f32 and s32 stereo formats which are now needed to support devices set to 32-bit. They are now lower priority than 24 and 16-bit to avoid regressing the fix for spatial audio settings.
* Add more detailed logging around the virtual audio settings. Some users may be having problems caused by the internal mixer format, so that is now logged as well.
@andygrundman andygrundman force-pushed the andyg.16bit-steam-speakers branch from e7df2a3 to 754a825 Compare March 6, 2025 11:15
Comment on lines +120 to +121
// The list of virtual formats are sorted in preference order and the first valid format will be used.
// All bits-per-sample options are listed because we try to match this to the default audio device.
Copy link
Member

@ReenigneArcher ReenigneArcher Mar 19, 2025

Choose a reason for hiding this comment

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

This seems like a good opportunity to document the template better. Could you document this template using a doxygen comment block?

/**
 * @brief ...
 * @tparam ...
 */

};
} else if (channel_count == 6) {
auto channel_mask1 = waveformat_mask_surround51_with_backspeakers;
auto channel_mask2 = waveformat_mask_surround51_with_sidespeakers;
auto channel_mask2 = waveformat_mask_surround51_with_sidespeakers; // XXX will never be used, but is probably the better 5.1 layout
Copy link
Member

Choose a reason for hiding this comment

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

What is XXX indicating here? Just calling extra attention to the note?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure it's actually true either. I thought 5.1 with surrounds as back speakers was pretty universal. @andygrundman did you have more context to add here?

Copy link
Contributor Author

@andygrundman andygrundman Mar 27, 2025

Choose a reason for hiding this comment

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

Back in the DVD era, 5.1 had the 2 rear speakers on the side walls at 90 degrees and also they were a few feet above ear level. In the modern 5.1 layout for Atmos, Dolby says it's anywhere between 110 and 120 degrees, and everything is at ear level now. One thing that was never an actual thing is 5.1 with 180 degree rear wall speakers. Those speakers are what you add to the side speakers to get a 7.1 system (where sides are back to 90 degrees). So the right 5.1 would be putting them at 120, splitting the difference, but Microsoft is stuck with their legacy options, and an Atmos option that we can't really use.

I think in practice, it doesn't matter where the rear speakers are as long as it's sort of behind you a little bit. My comment preferring 5.1-side is partly because I'm old, but mostly referring to the way the ordering works in Sunshine, those other options won't ever be used and the user can't choose them. Really I'm happy if the channels are all lined up correctly on the client side. 7.1 was different from Windows in SDL for years and nobody noticed until we got it patched recently. (That was actually a Mac issue and don't get me started about Apple, with probably a hundred weird speaker layouts in their header files).

win-5 1-back dolby-5 1 dolby-7 1

audio::prop_t prop;
prop_var_t current_device_format;

// clang-format off: easier to read this split over 2 lines
Copy link
Member

Choose a reason for hiding this comment

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

I know it's not ideal to have this on a long line, but I'd prefer not to have clang-format exceptions for this. I'm open to different clang-format rules though to help make things more readable.

@ReenigneArcher ReenigneArcher added this to the stable release milestone Mar 19, 2025
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