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

Port Main.DSP to MainSettings #10144

Merged
merged 1 commit into from Oct 16, 2021
Merged

Conversation

malleoz
Copy link
Contributor

@malleoz malleoz commented Oct 2, 2021

While trying to work on adding audiodump support for CLI, I was alerted that it was important to first try moving the DSP configs to the new OnionConfig before continuing, as that makes it substantially easier to write clean code to add such a feature.

This PR aims to allow for Dolphin to only rely on OnionConfig for DSP-related settings.

I am not very familiar with the new config, so please let me know if I missed any concepts in this PR. Notably, I was unsure what to do in BootManager.cpp BootCore() in the case of netplay... Seems like it uses its own SConfig which checks for DSP JIT.

Source/Core/Core/Config/MainSettings.cpp Outdated Show resolved Hide resolved
Source/Core/Core/BootManager.cpp Outdated Show resolved Hide resolved
Source/Core/Core/BootManager.cpp Outdated Show resolved Hide resolved
Source/Core/Core/Config/MainSettings.h Show resolved Hide resolved
Source/Core/DolphinQt/DolphinQt.vcxproj.user Outdated Show resolved Hide resolved
Source/Core/Core/ConfigLoaders/IsSettingSaveable.cpp Outdated Show resolved Hide resolved
Source/Core/Core/ConfigLoaders/NetPlayConfigLoader.cpp Outdated Show resolved Hide resolved
@JosJuice
Copy link
Member

JosJuice commented Oct 2, 2021

Also, the new config system is not officially called OnionConfig. But this is just me being opinionated and not something that necessarily should block the PR :)

@malleoz
Copy link
Contributor Author

malleoz commented Oct 2, 2021

Implemented Jos's review notes. Kept MAIN_DSP_HLE in Core so as to not break previous config files, finished porting over DSP HLE to use the new config, removed references to the new config from ConfigCache, and ported over MAIN_DSP_THREAD too.

@malleoz malleoz force-pushed the dsp-onion branch 2 times, most recently from 6ee0688 to 4860693 Compare October 3, 2021 00:46
Copy link
Member

@leoetlino leoetlino left a comment

Choose a reason for hiding this comment

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

Reviewed 26 of 26 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @malleoz)


Source/Core/AudioCommon/CubebStream.cpp, line 39 at r1 (raw file):

    return false;

  m_stereo = !Config::Get(Config::MAIN_DPL2_DECODER) || Config::Get(Config::MAIN_DSP_HLE);

Can we avoid duplicating this check? We could easily have a Config::ShouldUseDPL2Decoder that does the same thing as the existing SConfig::ShouldUseDPL2Decoder.


Source/Core/Core/Config/MainSettings.cpp, line 142 at r1 (raw file):

const Info<bool> MAIN_AUDIO_MUTED{{System::Main, "DSP", "Muted"}, false};
#ifdef _WIN32
const Info<std::string> MAIN_WASAPI_DEVICE{{System::Main, "DSP", "WASAPI"}, "Default"};

The config key should be WASAPIDevice.


Source/Core/DolphinQt/Settings/AudioPane.cpp, line 285 at r1 (raw file):

  if (selection != backend)
  {
    backend = selection;

This should be a Config::SetBaseOrCurrent call. (Notice that backend was a reference in the previous version of the code.)

@malleoz malleoz force-pushed the dsp-onion branch 2 times, most recently from cefa5bd to 811ae27 Compare October 14, 2021 00:40
@malleoz
Copy link
Contributor Author

malleoz commented Oct 14, 2021

Applied the requested changes from @leoetlino. For the AudioPane.cpp line 285 fix, I wasn't sure if it was better to get rid of backend and just use Config::Get() each time, or leave it how it is such that backend is re-used for if statement on line 302. If one way is preferred over the other, let me know!

@malleoz malleoz changed the title Port Main.DSP to OnionConfig Port Main.DSP to MainSettings Oct 14, 2021
Copy link
Member

@leoetlino leoetlino left a comment

Choose a reason for hiding this comment

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

Reviewed 12 of 12 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @malleoz)


Source/Core/AudioCommon/CubebStream.cpp, line 39 at r2 (raw file):

    return false;

  m_stereo = Config::ShouldUseDPL2Decoder();

Should be !Config::ShouldUseDPL2Decoder()

While trying to work on adding audiodump support for CLI, I was alerted that it was important to first try moving the DSP configs to the new config before continuing, as that makes it substantially easier to write clean code to add such a feature.

This commit aims to allow for Dolphin to only rely on the new config for DSP-related settings.
@malleoz
Copy link
Contributor Author

malleoz commented Oct 16, 2021

@leoetlino thanks for catching that, fixed!

Copy link
Member

@leoetlino leoetlino left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @malleoz)

@leoetlino leoetlino merged commit 8195d0b into dolphin-emu:master Oct 16, 2021
10 checks passed
@malleoz malleoz deleted the dsp-onion branch October 16, 2021 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants