Skip to content

Libretro rewrite #3212

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

Merged

Conversation

CasualPokePlayer
Copy link
Member

@CasualPokePlayer CasualPokePlayer commented Apr 11, 2022

Gets rid of the LibretroBridge stuff and all the wonky comm stuff, and does everything in C#. Bridge is not really needed anymore at this point and debugging is much easier in C# anyhow.

also yes i imported snprintf lol
…arently ... is really just a variable num of args passed on the stack and not some pointer to some struct, and c# doesn't have any way of representing this. fallback log isn't bad to deal with anyways)
@vadosnaprimer vadosnaprimer requested a review from zeromus April 11, 2022 16:57
@zeromus
Copy link
Contributor

zeromus commented Apr 11, 2022

WHY? This was originally done so that libco could be used, according to the commit log. I guess the same thing could be done with threads and synchronization, but I don't know why we didn't do that originally since at the time we'd probably already done the n64 core that way. I think I may have copied the way libsnes was done at the time because we'd had a need to run libsnes through an external process (I can't remember why) and that was the most future-proof interop method we had when I made the libretro parts. I think we may have done libsnes in the way we did because libco breaks if it's run from inside .net threads (in case GC happens from callbacks?) I'm sorry you didn't ASK SOMEONE for which AWKWARD REASON they made such AWKWARD CODE before you wasted your time nuking it all.

@CasualPokePlayer
Copy link
Member Author

Mostly since newer libco versions don't seem to actually break like they used to (given my experience with ares64 ported as a dll, and testing bsnes libretro cores seems to work fine here). There doesn't seem to be any noticable performance gain using libco as some emulation thread (probably due to it still getting locked to frontend communication, which probably has some performance lost given how it's done).

I would like to be proven wrong here though, if there's some core that needs libco with some emulation thread or just breaks under .NET.

@zeromus
Copy link
Contributor

zeromus commented Apr 11, 2022

The problems with libco and windows threads is deep, deep, deep and related to how .net does things and I feel very confident saying there's nothing magical about libco that got fixed. I've just realized, due to this, whether you use libco or threads, you still have to marshal callbacks to the .net thread via IPC somehow to run there. If this gets merged then it will be REVERTED immediately the first moment anyone proves that there are still libco + GC problems. I don't bless merging it anyway unless you can find some discussion of libco + GC problems which contains a mention of the situation being resolved somehow.

@CasualPokePlayer
Copy link
Member Author

CasualPokePlayer commented Apr 12, 2022

I've did some more looking into this, and do see some issues. (for reference, these are with retro_audio_sample_batch and bsnes-libretro core)

  1. I can't really breakpoint into a callback with a libco core. The debugger says it enters break mode but I can't actually step through code. Removing the breakpoint and letting code run seems to work fine still.
  2. I seem to have some sort of weird memory corruption occurring with CoreComm. At some point somehow retro_message_time is > 0 and calling Comm.Notify after retro_run is completed seems to just throw some NRE for Notify.

I've also was intentionally trying to trigger GC with 2, being I make some arrays, remove references, then sleep for 2 seconds to hopefully give enough time for GC to do something. I assume calling Thread.Sleep in the audio callback isn't having any additional consequences here, so I assume this is what the GC problems are.

Still, perhaps rework can still work out. If callbacks are the only issue, we could remove all callbacks to .NET (not like we can actually have tracing or memory callbacks, and can't even have useful input callbacks as retro input callbacks are typically called every frame), .NET would only ever will call retro_ functions, and .NET will not be ran until those functions return. All the callbacks can probably be reworked into C/C++, and when needed .NET can query to get any variables.

@Morilli
Copy link
Collaborator

Morilli commented Apr 12, 2022

Having someone be able to explain precisely what the issue with libco is and/or have an example of what doesn't work correctly would really help, because the code is definitely awkward as is (current, not this PR).

@CasualPokePlayer
Copy link
Member Author

CasualPokePlayer commented Apr 12, 2022

tl;dr libco fucks the stack in some way GC really doesn't like. Although that might be worse in older libco versions/older .NET/something, since I can't easily get it to actually do something particularly bad (the worst has been some sort of memory corruption, but ??? if that is libco/GC interaction considering the GC definitely didn't outright crash, although it's the most probable I guess).

…mply handles all the callbacks retro cores use, so there is never a c++ -> .NET callback, and probably avoids any libco issues

also a lot of cleanup in various areas, and some bug fixes too (Blit555 now outputs the correct colors)
@CasualPokePlayer
Copy link
Member Author

Still, perhaps rework can still work out. If callbacks are the only issue, we could remove all callbacks to .NET (not like we can actually have tracing or memory callbacks, and can't even have useful input callbacks as retro input callbacks are typically called every frame), .NET would only ever will call retro_ functions, and .NET will not be ran until those functions return. All the callbacks can probably be reworked into C/C++, and when needed .NET can query to get any variables.

I ended up doing this. Please tell me if I'm still wasting my time.

speex seems to perform much worse often (and outright buggy for some libretro cores like sameboy which reports a sample rate of 384000), and blip works well enough even for "newer" systems
@nattthebear
Copy link
Contributor

It is not permitted to have any Windows SEH code observe a thread with the stack pointer pointing to somewhere other than the stack registered with the thread in the kernel's thread information structures. Anything else causes SEH barfholes. Lots of things use SEH; not just C++ exceptions but also signal delivery (access violation exception, etc) and lots of random kernel32 calls.

When a thread stack that has been transitioned to native code calls back into managed code, the .NET trampoline does some mystical SEH stuff. The stack needs to be right at that moment.

In times passed, there were also situations where .NET could send SEH stuff to a thread that was currently transitioned to native code. I don't think that happens anymore; otherwise, Waterbox would be broken.

So I think what you've done should work, so long as we're willing to accept cores with no callbacks now and no callbacks ever.

@CasualPokePlayer
Copy link
Member Author

So I think what you've done should work, so long as we're willing to accept cores with no callbacks now and no callbacks ever.

This should be OK, as stated libretro API doesn't offer any callbacks for debugging purposes (so memory/trace/cdl callbacks are out of the picture) and even their input callbacks are useless as they have no correspondence to the console polling (they're just "update local input buffer every frame just in case console polls"). All callbacks can be redirected to unmanaged code and managed code can set needed data (like inputs) before running the frame then query to get video/audio/etc data once the frame is done emulating.

@CasualPokePlayer CasualPokePlayer merged commit 25fb816 into TASEmulators:master May 5, 2022
@CasualPokePlayer CasualPokePlayer deleted the libretro_rewrite branch May 5, 2022 08:49
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.

4 participants