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

Updating Concentus dependency to speed up Opus decoding #6757

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

Conversation

lostromb
Copy link
Contributor

@lostromb lostromb commented Apr 30, 2024

Updating to the new Concentus package version which has updated features:

  • Span-in / Span-out Opus decoding (so we don't have to make temporary buffer copies for every audio packet)
  • Returning more precise error codes from the decoder back to Horizon
  • Automatically utilizing a native opus library with P/invoke if supported on the current system (we don't actually care about this as decoding is very fast anyways)

This will also have the side effect of copying extra binaries to project output directories and increase the install size by a few megabytes - for files like /runtimes/win-x64/native/opus.dll. I have also not tested the P/invoke code path on all platforms - just x64 Windows/Linux/Mac. If you want to see if it's working properly, pass Console.Out as the last parameter to the OpusCodecFactory methods. And if the native library probe is causing problems, it can be disabled globally via OpusCodecFactory.AttemptToUseNativeLibrary

Update: Not bothering with native libs at all. This will continue to just use the pure C# decoder.

Logan Stromberg added 2 commits April 30, 2024 10:29
…span-out Opus decoding (so we don't have to make temporary buffer copies), returning a more precise error code from the decoder, and automatically linking the native opus library with P/invoke if supported on the current system
@github-actions github-actions bot added horizon Related to Ryujinx.HLE infra Related to the project infrastructure labels Apr 30, 2024
@ryujinx-mako ryujinx-mako bot requested review from AcK77, gdkchan, TSRBerry and a team April 30, 2024 19:52
@lostromb
Copy link
Contributor Author

lostromb commented Apr 30, 2024

I have so far been unable to find a game that actually uses hardware Opus to test, besides MLB The Show '22 which crashes on startup for other reasons (though it does successfully create a decoder)
Other ones I've tried: Wario Ware Get It Together, Origami King, Sea of Stars, Groove Coaster. Apparently these games all use software Opus internally?

@riperiperi
Copy link
Member

I think Pokemon Legends Arceus uses it, allocations in Concentus were topping the profile last time I checked.

@gdkchan
Copy link
Member

gdkchan commented Apr 30, 2024

From what I recall, all Pokémon games uses Opus (except maybe Brilliant Diamond and Shining Pearl). There's also Sea of Stars, Unicorn Overlord, etc.

@TSRBerry
Copy link
Member

TSRBerry commented Apr 30, 2024

@lostromb The NuGet package seems to contain the native libraries multiple times at different paths.
They should be located at runtimes/{rid}/native/ (see this document), but it looks like something went wrong during the packaging process maybe?

@lostromb
Copy link
Contributor Author

lostromb commented Apr 30, 2024

Arceus does hit this code path and the music seems to be working fine there. But I don't get a hit from Scarlet, Shield, Shining Pearl, Sea of Stars, or a dozen other random games. There's no regression, they just don't seem to use the service at all (again, I assume they might have compiled libopus directly into the game engine?)

For the packaging, my (maybe incorrect) understanding was that /content is seen by legacy Nuget clients (such as when pulling into a .Net Framework project that doesn't use <PackageReference>), and /contentFiles is the newer .Net Core-style path that allows for more customization. Having the binaries in both places maintains compatibility for old projects. However, there might still be an issue if we distribute Ryujinx via dotnet publish, I might have to add a CopyToPublishDirectory tag into the targets file, as recommended by stackoverflow

e: also hmmm, if an indirect library uses DllImport and not LibraryImport, would that mess with AOT compilation? Might have to change that too...

@lostromb
Copy link
Contributor Author

lostromb commented May 1, 2024

Updated to 2.1.1 which should have better LibraryImport logic and support content files on publish / AOT

@riperiperi
Copy link
Member

It looks like the macos build isn't happy with the libopus natives right now:
https://github.com/Ryujinx/Ryujinx/actions/runs/8905487558/job/24456307091

llvm-lipo: error: /home/runner/work/Ryujinx/Ryujinx/publish_tmp_headless/publish_arm64/runtimes/osx-x64/native/libopus.dylib and /home/runner/work/Ryujinx/Ryujinx/publish_tmp_headless/publish_x64/runtimes/osx-x64/native/libopus.dylib have the same architecture x86_64 and therefore cannot be in the same universal binary

It seems a bit odd putting all the effort into speeding up the C# port of opus decoding, but then using a native library anyways.

@lostromb
Copy link
Contributor Author

lostromb commented May 4, 2024

It seems a bit odd putting all the effort into speeding up the C# port of opus decoding, but then using a native library anyways.

I mean, I guess my intent 8 years ago was to make a pure C# port that had no dependencies and could run anywhere, but then that plan changed to a hybrid approach where it would just give you a wrapper to a native C implementation if your platform supported it (which turns out to be almost all .Net publish targets), keeping the C# code as a fallback.

I could theoretically move the native codec wrappers directly into Ryujinx and skip the Concentus package altogether. But then I would probably have to fuss with things like SetDllImportResolver, and make sure everything loaded properly on every platform at runtime, which would take more testing and there's less of a safety net because there's no managed decoder to fall back to if something fails. And then there'd be complaints that Pokemon crashes on startup or whatever.

It looks like the macos build isn't happy with the libopus natives right now:

libopus.dylib is being pulled in separately as osx-x64 and osx-arm64 flavors. I don't know if something in the build is expecting those to be universal binaries instead - if so, I don't know how to package those and the Nuget documentation for it is unclear. Maybe we need to add a specific file exclusion somewhere?

@gdkchan
Copy link
Member

gdkchan commented May 4, 2024

Is it not possible to split the managed implementation and the native wrapper into separate packages? Then we can consume only the managed package, without shipping the opus decoder binary. That would probably also have size benefits as we don't use the encoder at all (in the C# implementation I believe it can be trimmed). If you want to keep the hybrid approach, then perhaps it could be a third library that consumes the other two and uses the native one if possible.

@lostromb
Copy link
Contributor Author

lostromb commented May 4, 2024

Is it not possible to split the managed implementation and the native wrapper into separate packages? Then we can consume only the managed package, without shipping the opus decoder binary. That would probably also have size benefits as we don't use the encoder at all (in the C# implementation I believe it can be trimmed). If you want to keep the hybrid approach, then perhaps it could be a third library that consumes the other two and uses the native one if possible.

We can go a few different ways that I'm open to. I'm just not sure what path you would want to prioritize:

  1. Pull a C#-only Opus decoder - closest to what master is doing now: 500Kb publish size, slow audio decoding, no fuss with native libs
  2. Pull a P/Invoke-only Opus decoder: +5Mb publish size, fast performance, potential to break at runtime depending on the game and user's computer
  3. The current PR as-is: +5.5Mb publish size, fast performance in presumably all platforms, much less chance to break if native libs can't be loaded

I mention "fast performance" but from a quick benchmark and assuming a 30fps game we're looking at a difference of:
C# code: 0.14 ms per frame spent decoding audio
Native code: 0.08 ms per frame spent decoding audio

@gdkchan
Copy link
Member

gdkchan commented May 4, 2024

I prefer 1. I'm not sure why the perf gap is so large though, I wouldn't expect the C# version to be that much slower, unless its missing some vectorized code path or something. It might be worth profiling it, and maybe forwarding performance issues to the .NET runtime team once they have been identified.

@lostromb
Copy link
Contributor Author

lostromb commented May 4, 2024

Simple solution:

<PackageReference Include="Concentus" >
  <ExcludeAssets>contentFiles</ExcludeAssets>
</PackageReference>

Copy link
Member

@gdkchan gdkchan left a comment

Choose a reason for hiding this comment

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

You could consider moving the pre-compiled libraries to a separate nuget package as well, I have seen some bindings doing that. Something like Concentus.Native.


public int SampleRate => _decoder.SampleRate;
public int ChannelsCount => _decoder.NumChannels;

public Decoder(int sampleRate, int channelsCount)
{
_decoder = new OpusDecoder(sampleRate, channelsCount);
OpusCodecFactory.AttemptToUseNativeLibrary = false;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe that should be done in a static constructor for the HardwareOpusDecoder class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I would normally like to avoid static constructors, but this one seems simple enough. Changed.

@ryujinx-mako ryujinx-mako bot requested a review from a team May 6, 2024 18:03
@lostromb
Copy link
Contributor Author

lostromb commented May 6, 2024

You could consider moving the pre-compiled libraries to a separate nuget package as well, I have seen some bindings doing that. Something like Concentus.Native.

Yeah, looking into it more I see the benefit of modern Nuget dependencies in NetCore, namely that on publish and in AoT workflows, it's smart enough to only pull the native binary for the platform you are targeting, rather then just batch copying a big pile of random other .dlls that you won't need. The only problem was I still want to support older .Net Framework installs which can't do that, so I ended up making a few native packages and a metapackage which should support everyone.

This PR now just updates the pure C# opus decoder and addresses some minor bugs; not touching native at all.

@AcK77 AcK77 requested a review from gdkchan May 14, 2024 14:02
@lostromb
Copy link
Contributor Author

lostromb commented May 15, 2024

Again using Pokemon Legends as benchmark, memory allocation rate during gameplay decreases by 9.4%, and (with a big statistical grain of salt) total CPU usage seems to decrease by 3%. No other metrics move as far as I can tell.

image

Copy link
Member

@gdkchan gdkchan left a comment

Choose a reason for hiding this comment

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

lgtm, thanks.

@ryujinx-mako ryujinx-mako bot requested a review from a team May 16, 2024 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
horizon Related to Ryujinx.HLE infra Related to the project infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants