SI refactorings - Githubissues

I looked a little bit into how you manage save chips content and I think you should be able to reduce easily the gap with upstream by doing the following :

move the "saved_memory" struct (both definition and implementation) in your libretro.c file
connect the saved_memory members with si.eeprom.data*, si.controllers[k].mempak.data, pi.flashram.data and pi.sram.data accordingly. This is done in main.c in m64p, but you can do anywhere before starting emulation.
I didn't find how you effectively save the saved_memory content to disk. I guess this is handled inside retroarch, and cores just return a pointer + size so that can be done externally. So, just provide a dummy "save" callback and connect it to {eeprom,sram,mempak,flashram}.save.
m64p allow only a single pointer of consecutive memory for eeprom content, whereas you need 2 (eeprom,eeprom2). If you want to adhere to the m64p way without changing your data layout, you can :

option1: use a memory mapping trick in order to map eeprom and eeprom2 consecutively and provide that virtual pointer to eeprom.data. The mmap trick consists in allocating 2 consecutive pages such that the end of the 1st page correspond to eeprom1 and the beginning of the 2nd page correspond to eeprom2.

option2: allocate a contiguous buffer for eeprom, pass that pointer to m64p eeprom.data and do copies between that buffer and your saved_memory.eeprom{,2}. You can do the initial copy (saved_memory -> new_eeprom_buff) before emulation start. And keep saved_memory synchronized (new_eeprom_buff -> saved_memory) using the new "save" callback mecanism.

option3: not adhere to m64p for eeprom, and be free to manage that the way you want.

It's up to you to choose what you want for your fork. I'm just proposing options that I think could help. (in my opinion, option 1 is the way to go if you can manage the extra complexity of mmap. option 2 is meh because of the extra synchronization needed. option 3 is simple and effective, but you have to maintain things on your own).

edit: you should also move format_savedmemory to libretro.c. And eventually use the format* functions provided by the core.

Hi there bsmiles32, thanks for dropping in and throwing some helpful suggestions my way.

I've moved save_memory_data out of the core at least and moved it to my libretro-specific files so far.

As for option 1 - well, I don't like relying on mmap as it is not really something that works out all that well from a portability perspective and Android already has lots of gotchas on that front so it's something I'd like to avoid having any dependencies on entirely.

With regard to this -

"m64p allow only a single pointer of consecutive memory for eeprom content, whereas you need 2 (eeprom,eeprom2). If you want to adhere to the m64p way without changing your data layout, you can :"

Yeah, I think this is a legacy issue. I guess we can bite the bullet once and just drop backwards compatibility with what the ABI of the save states/memory was before. Sure it will get some complainining but overall in the name of progress it should be better to have one single pointer of consecutive memory for eeprom instead of two.

Regarding all these commits from https://github.com/bsmiles32/mupen64plus-core/commit/7a0f0081ccd2a5465cb83b882ae482c3ac100aee and on -

all this dancing around saving/loading of internal state is just really confusing to me and it just makes no sense for the libretro port at all. It just seems to cause lots of bloat and lots of indirection and it would probably cause things to be slower as well (especially if we're going to go for solutions like mmap which would make things less portable [a very big issue for me] and would also require stuff like 64Kb alignment on Windows and all sorts of other issues).

Can't we arrive at a stage where I don't have to bother with this for the libretro port but still be somewhat upstream-friendly? Because I really don't like having to do all this stuff since it makes no sense at all for the libretro port. Libretro port should not care at all about any notion of 'saving to disk/reading from file' at all. I liked it way better how things were handled up until this commit and everything was going well until this commit put us back into a mindset of 'reading from file and writing to disk', which libretro does not care about at all as an API and it tries its best to move people away from a file I/O-oriented paradigm. This just messes up the overall code structure in my opinion.

Maybe you can come up with some POC so I can wrap my head around how this might make sense for the libretro API, but right now I just see a lot of pointless indirection and I just see it as something I'd like to stub out altogether.

OK I'm going to try the approach of just setting saved_memory members to the data pointers in the memory subsystems, and not bothering with the '*_file' files (mempak_file, etc). Going to see how that goes.

EDIT: I keep running into issues. I definitely don't know how to proceed and all of the commits after https://github.com/bsmiles32/mupen64plus-core/commit/7a0f0081ccd2a5465cb83b882ae482c3ac100aee to do with moving file I/O out of certain system component files just made things less clean from my perspective and far more obtuse (since for the libretro port, file I/O has never been an issue and therefore these commits just get in my way). I don't think this is the right way to go at all and I'd rather not deal with it altogether. Sad because the previous commits were all welcome improvements in my book but I really dislike the way we are dealing with internal state here from this commit on. The previous way was far better and it gave me the flexibility to do stuff in an efficient manner rather than going through all these unnecessary layers of abstraction.

I'm still open to any suggestions though, but I definitely dislike these file wrapper refactorings at least and would prefer to work around them altogether.

OK, just went on a marathon backporting stuff over.

I'll give a condensed summation of what I did:

PR 'https://github.com/mupen64plus/mupen64plus-core/pull/65' (SI refactorings) -

I cheated somewhat here and mostly backported over the changes that were actually functionally significant, and left behind all the things that I had issues with. For some reason those function pointers aren't working even if I hook them up.

So, input and rumble pak parts are not properly refactored yet. Maybe some part of it isn't hooked up somewhere, not sure yet. The idea is that we look later at what works for libretro and what doesn't and go from there.

PR 'https://github.com/mupen64plus/mupen64plus-core/pull/68' (Interrupt refactorings) -

Ported all these over. These posed little problem. I don't think I made any really significant modifications here or there.

PR 'https://github.com/mupen64plus/mupen64plus-core/pull/71' (AI refactorings) -

Ported all these over. Once again for some reason the function pointers for that audio plugin module didn't work and posed problems even though I hooked it up in main_init (more on that later). So what I did instead was just add the necessary functions to my own statically baked in audio plugin and just call those with some ifdefs. Messy but it gets the job done for now.

What I'd like to see as a result of this audio refactoring is that we can make this somewhat more efficient. I noticed that right now we need to byteswap every sample and I was wondering if things like this are something that we can avoid. Other things that could be important would be 'trapping' of when resampling needs to be done or not. Right now the libretro audio plugin just resamples regardless which while expensive at least ensures audio is always guaranteed to come out right (and our resamplers are quite good and relatively performant so it doesn't matter much).

PR 'https://github.com/mupen64plus/mupen64plus-core/pull/74' - 'Fix vi->field update when in interlaced mode.'

Backported this over. No issues.

Now, here are some issues I'd like to discuss with you later on that would be of benefit to Mupen64plus mainline in general -

You'll notice that in the Mupen64plus libretro fork (in mupen64plus-core/src/main.c), that things have been made properly re-entrant. That means, instead of a 'main_run' function where we get perpetually stuck in, I have instead split it up to main_init, main_run, and main_exit. Similar refactorings were also done to interrupts and other segments of the code to make sure that we can exactly iterate per frame and are not stuck in some infinite while loop. The reason why this is important for libretro is because libretro prefers that your emulator has the ability to iterate exactly one frame so that we have very granular control over exactly how many audio samples and video frames are pushed so the frontend in turn can guarantee that sync is correct. Being stuck in a while loop necessitates using something like libco to run Mupen64plus on one thread and then coswap back to the 'main thread' which is doing the libretro integration code.

Note that the current 'frame iteration' codepath is not yet complete and has bugs of its own, which is what I'd need your help with. We need to properly improve the Mupen64plus codebase so that it can handle this. Problems that will pop up for doing this will be the new-dynarec code with its messy ASM/C code that is using some variables from mupen64plus-core in a way that prevents us from really pushing this through right now as the 'default' in fear of breaking that.

Regarding new-dynarec -

I still have a bunch of iOS backwards compatibility patches for new-dynarec. We really need to start work on trying to merge that because I can't keep up with Gillou's patches and also (honestly) the code keeps getting more and more messy for new-dynarec in upstream so it's hard to make sense of it.

I know how hard it has been for you to make these changes because of new_dynarec lagging behind and I share your frustrations there. Unfortunately, it's significantly faster than dynarec and it has an ARM backend which the 'normal dynarec' system does not, so it's not an option to just drop it, but it really does look bad that we have two dynarec systems in here. One of them needs to be triaged and what comes out of it should be significantly better than both of them combined right now.

On that subject,

new-dynarec is becoming quite unreadable code. It is really astoundingly bad to read, and this is something that should be probably properly addressed right now. Now that you have done most of these refactorings, I think the last major stronghold in terms of messiness in the codebase is that dynarec stuff. It's also the hardest part but I'm willing to spend some time on that as well to help you out. Again, I'd recommend IRC communication for this because I'm always available in real-time and if timezones are an issue we can talk about it. Important thing is to get this done and writing lots of e-mails like this is not the best way to get stuff done really. I have all the hardware required to regression-test stuff on and so on and I want to dedicate the time available to make sure this gets done, so the offer is there.

Sorry for this ultra-lengthy post. Anyway, there's probably a bunch of others I'd like to suggest but that's the stuff I could think of out of the top of my head.

Thanks for the update !

I looked briefly how you backported all the commits and I think you did a fair job.

General remark to keep in mind when backporting: In the m64p codebase I consider the AI,MEMORY,PI,R4300,RDP,RSP,SI,VI folders to be very emulation centric and I'd like to make them very agnostic to external input/output/dependencies. In these folder you should avoid diverging from upstream as much as you can. In other folders like MAIN, I think you can diverge and tweak things for your fork if what we do upstream is not in line with what you require. That's why I moved the save file handling to main folder and made one level of indirection for eeprom,mempak,flashram,sram. For these, you just setup a pointer and an eventual callback if you want to be notified when these save stuff gets modified. I saw you didn't like the extra indirection, but I think it is not that bad as saving is not something a game does often. But again, your fork, your rules.

I followed the same "isolation strategy" for rumblepak, for which you can register what callback you want. If you want to bypass the rumble_via_input_plugin that's fine because this is only a (temporary, I hope) way to keep things working without modifying existing plugins and frontends. Same for emulate_game_controller_via_input_plugin or emulate_speaker_via_audio_plugin, those are meant to bridge the new callbacks to the existing plugins. But if you want to get rid of plugins (at least audio and input) in your fork, you would just have to register your own callbacks before starting the emulation loop, and remove the plugins altogether. I think input and audio plugins references are almost contained in main, plugin directory (there is a reference in the pif but that could be removed if you remove plugins), so no big deal if you diverge here from upstream. At least that's the idea.

Regarding breaking the emulation loop, I would also like to have better control on the execution, but this is quite a challenge given the dynarec's implementation. You can create an issue on our bug tracker so that we can speak about it publicly. But for now I don't think I will work on that because more urgent stuff are needed...

Yes, I'm talking about the new_dynarec. Honest opinion : I don't like it, I don't use it, it slows my refactoring progress and I would prefer if it did not entered upstream until it got more maintainable. That being said, it IS upstream now and I have to deal with it the best I can. So that's what I do.

Nebuleon recently proposed to drop the old dynarec, in favour of the new_dynarec, but that's not really an option for now. What I proposed is to first isolate the r4300 from the rest of the system (AI,PI,RI,SI,VI,RSP,RDP...) via a small interface, then decouple the "old dynarec" and "new dynarec" completely as they are fundamentally different. This should allow to let them evolve at their own pace, and should pave the way for an eventual rewrite of a new r4300 module, without sacrificing usability for end-users. You can see the discussion in the public mailing list [1]. And this is what will occupy me for the comings weeks.

I'll see if I can drop by on IRC this week-end, and eventually do a PR to help with some of the remaining differences between m64p and libretro.

[1] : https://groups.google.com/forum/#!topic/mupen64plus/4rOO7cX0toY

Just wanted to post as a cheerleader from the sidelines. Never have I been more enthusiastic about these projects as I have the past few weeks with all the intense and amicable discussion. I really think everyone's interests are aligned and am hopeful that the upstream code will become more developer friendly to all downstream projects. Keep up the excellent work you guys :+1:

And, yeah, the new_dynarec absolutely needs to be cleaned and modularized. I won't argue if you actually have to break it for awhile to make the overall system better. mupen64plus-ae can temporarily suspend syncing if it has to. I very much appreciate Retroarch's pathfinding on all these matters.

Wow their have been discussions here! Great! I will try to read the whole asap just for my information.

I'm very happy to see upstream getting closer to libretro (and downstream projects in general), keep the good work!

libretro / parallel-n64

SI refactorings #187