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

spu2-x: Update XAudio to 2.9 #3241

Closed
wants to merge 5 commits into from
Closed

spu2-x: Update XAudio to 2.9 #3241

wants to merge 5 commits into from

Conversation

@MonJamp
Copy link
Contributor

@MonJamp MonJamp commented Feb 7, 2020

This allows simplification of XAudio code in spu2-x since XAudio 2.9 comes with a redistributable dll which is backwards compatible with Windows 7/8. I have tested the compiled plugin on a Windows 7 VM and can confirm it works. I removed all code which checks for the version of Windows for XAudio since it is no longer needed.

I'm pretty new to contributing to open source so I would like some feedback on some of the changes I made.

The NuGet package manager is needed to download the package which includes the headers and libs. I chose to exclude the packages directory and instead let NuGet automatically download it when building spu2-x. This does mean there's an extra step to building PCSX2. Another route I could've taken is to leave the packages directory with the XAudio 2.9 package. This would mean distributing Microsoft's dll files and headers which contain a license although I believe they do allow distributing those files without modification.

In order to load the library I had to use this bit of code xAudio2DLL = LoadLibrary(L".\\plugins\\xaudio2_9redist.dll");. This hard coded string to a specific directory is inevitable since the NuGet Package Manager downloads binaries to a project's output directory. I was wondering if there was a better way around this. AFAIK NuGet is pretty rigid and this can't be easily changed.

I'm open to any suggestions and making any changes.

#Resolves #3232

Edit: Looks like CI fails since it doesn't download the NuGet package. So I guess the best way would be to include the package in the repo.

@mirh
Copy link

@mirh mirh commented Feb 8, 2020

Maybe I'm wrong, but wouldn't you need to add

<PackageReference Include="Microsoft.XAudio2.Redist" Version="1.2.0" />

somewhere (as hinted on the website) for automatic download to happen?
Or maybe it's just one line to add in the appveyor config.

@tadanokojin
Copy link
Member

@tadanokojin tadanokojin commented Feb 8, 2020

I don't want all this faffing for an EOL OS.

@MonJamp MonJamp force-pushed the MonJamp:xaudio2_9 branch 2 times, most recently from 4f4ad94 to 9918735 Feb 9, 2020
@MonJamp MonJamp force-pushed the MonJamp:xaudio2_9 branch from 9918735 to 77497e8 Feb 9, 2020
@MonJamp
Copy link
Contributor Author

@MonJamp MonJamp commented Feb 9, 2020

Or maybe it's just one line to add in the appveyor config.

Nuget restore works on appveyor now.

I don't want all this faffing for an EOL OS.

Using XAudio 2.9 allows backwards compatibility with both Windows 7 and 8 while simplifying code for spu2-x. It is annoying dealing with nuget but the process of getting the packages for compiling happens in the background.

It would be nice to have the xuadio2_9redis.dll next to the executable file instead of in the plugins folder but I'm having difficult figuring out how to do that since it seems we don't get control over where a nuget package installs the binaries.

@tadanokojin
Copy link
Member

@tadanokojin tadanokojin commented Feb 9, 2020

It is annoying dealing with nuget but the process of getting the packages for compiling happens in the background.

Yes it's annoying. Which is why I don't want to deal with it now or in the future.
You can just update it to 2.8 and we can move on from win7. We'll end up killing it one way or another anyway.

@MonJamp
Copy link
Contributor Author

@MonJamp MonJamp commented Feb 9, 2020

Fair enough, is it safe to assume Windows 7 won't be supported for 1.8 release? If that's the case I can still simplify the code without dealing with nuget. Or I can include the contents of the nuget package into the 3rdparty folder that way we still don't have to deal with nuget and still use the same code for win7/8/10.

@mirh
Copy link

@mirh mirh commented Feb 9, 2020

Sigh, I could get 2 seconds on the first start are a downer... But doesn't xz init take already even more than that?

It would be nice to have the xuadio2_9redis.dll next to the executable file instead of in the plugins folder but I'm having difficult figuring out how to do that since it seems we don't get control over where a nuget package installs the binaries.

[1]? [2]?
Then putting aside you could hardcode the path, you can just have the "package part" of the build script move it.

@tadanokojin
Copy link
Member

@tadanokojin tadanokojin commented Feb 9, 2020

@MonJamp we plan to drop it following 1.6.0 which is the upcoming release. So it'll get dropped in 1.7.0 and 1.8.0 (won't be out for some time) will not support win7.

Sorry you put this effort in just for me to tell you to kill it.

@MonJamp
Copy link
Contributor Author

@MonJamp MonJamp commented Feb 9, 2020

Sorry you put this effort in just for me to tell you to kill it.

lol it's fine, I had a feeling this pr would get killed due to nuget. The changes needed to drop XAudio 2.7 (dropping win7 support) are basically the same anyways.

@MonJamp MonJamp closed this Feb 9, 2020
@tadanokojin
Copy link
Member

@tadanokojin tadanokojin commented Feb 9, 2020

You can still make those changes if you feel inclined to do so btw.
We'll merge it after 1.6.0.

@PCSX2 PCSX2 locked and limited conversation to collaborators Jul 12, 2020
@PCSX2 PCSX2 deleted a comment from Squall-Leonhart Jul 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.