Genesis-Plus-GX
Genesis-Plus-GX copied to clipboard
libretro
Soft Patching .IPS files do not work
It only happens when soft patching a rom with Genesis Plus GX. It does not happen to me with other cores, they work fine. I tested the rom and the patch using lunar and they both work fine, so the patch and the rom are not to blame.
I've posted abouit this problem here: http://libretro.com/forums/showthread.php?t=6435&p=43389#post43389
Thank you.
I can confirm it, I just tested a sms rom and softpatch doesn't work with Genesis Plus GX core
I might be wrong but I am quite sure soft patching is something handled by the frontend, not the libretro core. There is nothing in libretro.c related to soft patching so this is unlikely a core-specific issue but rather an issue within Retroarch when handling IPS files with Genesis ROM files.
Softpatching is indeed a frontend job. But the frontend can only do it if the core lets it read and alter the file - and this one means core insists on having the file on disk, which kinda contradicts the point of softpatching.
Not sure if the core actually needs the ROM on disk, or if it's just a missing refactoring.
I see, I did not thought about that and this indeed makes sense. So this means core that have need_fullpath set to TRUE cannot use soft-patching through any libretro frontends?
Sure you could "refactor" the core to have it loading ROM from memory instead of file but I think it kinda defeats the purpose of the libretro API if everytime you are facing an incompatibility with some edge case you are altering the core code instead of enhancing API feature to cover said edge cases. Especially when libretro cores actually have the choice to set need_fullpath to TRUE and that option probably exists because many emulation cores need that for good reasons. Genesis Plus GX for example autodetects the system to emulate from rom file extension, and for Sega CD, it needs access to files on disk as obviously only a few kB is read at a given time . How would retroarch handle a 500 MB ISO file by the way if this option is set to FALSE? Will it try to load the entire file into memory?
Shouldn't it be possible for the frontend to request pointer to ROM once it's loaded by the core just like it's done with SRAM, through same API function using a specific memory identifier? It would make much more sense if the frontend was able to patch the ROM this way instead of forcing cores to use ROM already loaded into memory.
So this means core that have need_fullpath set to TRUE cannot use soft-patching through any libretro frontends?
Correct.
And I guess this also prevents cheats from working with these cores, right?
False. Cheats are handled by the core, after loading the ROM.
"refactor"
Yeah, that's wrong; a refactoring doesn't change behavior, but this one quite clearly does. Twinaphex uses that word way too much, apparently it's spreading. (Another example: 'sandbox'; libretro doesn't restrict which syscalls the core may make, therefore it's not a sandbox.)
think it kinda defeats the purpose of the libretro API if everytime you are facing an incompatibility with some edge case you are altering the core code instead of enhancing API feature to cover said edge cases.
Yes, file loading could definitely be made more flexible. But we haven't been able to come up with a method that's flexible enough but doesn't require another 200 lines of code to implement a minimalist frontend or core; we believe quite strongly in keeping libretro simple unless the complex part is actively exercised.
...maybe we should just keep multiple methods around. It's not like we can remove them, anyways.
Genesis Plus GX for example can autodetect the system to emulate from rom file extension
We still hand out the original filename even if need_fullpath is false. Unless the ROM is loaded from stdin or otherwise doesn't have a filename, but that's rare.
How would retroarch handle a 500 MB BIN file by the way if this option is set to FALSE? Will it try to load the entire file into memory?
Badly. Probably. And then core copies the 500MB into its own memory, so better hope you have either 1GB spare or a really smart page deduplication algorithm.
Shouldn't it be possible for the frontend to request pointer to ROM once it's loaded by the core just like it's done with SRAM, through same API function using a specific memory identifier? It would make much more sense if the frontend was able to patch the ROM this way instead of forcing cores to use ROM already loaded into memory.
That doesn't work unless the core keeps the ROM in its own memory unchanged, and that's not common. imageviewer-libretro doesn't keep PNGs around; our SNES cores don't keep the SMC headers. Additionally, it stops the core from rejecting a patched ROM it can't load if it can load the original, and vice versa.
I'd rather see some kind of VFS layer. Something like
struct retro_vfs_t {
size_t len;
// Contains only filename, no directory component.
// If the file is anonymous, must be ""; NULL is forbidden.
const char * filename;
// Obvious.
bool (*read)(retro_vfs_t* file, size_t offset, void* target, size_t len);
// file->open(file, "mario.sfc") returns mario.sfc in the same directory,
// or NULL on failure.
// The function must be callable, but may be a stub that returns NULL.
retro_vfs_t* (*open)(retro_vfs_t* file, const char * path);
// Deletes the file object. Optional to call; if the core doesn't deallocate it,
// it's done after retro_unload_game() returns, including those produced via open().
// Must be callable, but may be a no-op.
void (*free)(retro_vfs_t* file);
};
// pretend this is an env, I'm too lazy to copypaste the required boilerplate
bool retro_load_game_2(retro_vfs_t* game);
Maybe a simpler alternative for cores that need "full_path" could be to patch inside a temporary file on disk, like it was done (and maybe still is) for zipped files?
That still contradicts the point of softpatching, and writing a 500MB ISO every time it's loaded wouldn't really work well. Especially if they're not cleaned up, I'm not sure how much we do that.
But it would work as a temporary solution until someone implements anything better. ...assuming anyone implements either method...
Why not limiting this method only to small files (<10MB) , so only roms will be patched, and isos will be left untouched?
Or use file extension to patch only roms and not isos?
We could have the benefits of softpatching without fearing problems with big ISOs
EDIT: The point of softpatching from an end-user perspective is to make rom and metadata management much simpler, because we could keep no-intro format and still have hacks and translations. I don't mind if behind the scene retroarch is hardpatching a temporary rom on my HDD each time I launch it, even if its ugly from a developer point of view :p
and writing a 500MB ISO every time it's loaded wouldn't really work well.
You would only do that if soft patching is activated and an IPS file is found for the game (which I doubt is very common for ISO files) so that edge case is unlikely going to happen often.
No emulator would load a 500 MB ISO in memory anyway (patched or not) so that would be a small expected downside for the benefit of being able to soft-patch an ISO file.
Actually, the Mednafen cores (Saturn/PSX/PC Engine/PC Engine CD/PCFX) all have a core option to load the entire CD image into memory in case the host system permits it. It prevents audio hiccups at times and it also allows us to read an entire CD image into memory from say some NAS location and then no longer have to worry about the network continuing to be available for further reading from it.
So, actually, a core option to allow for that would be rather nice in fact.
It would require rewriting a lot of CD emulation code so I'm afraid it's not going to happen anytime soon.
Also, system with > 700 MB RAM surely have enough CPU speed to run Sega CD emulation at full speed even when reading from a "slow" device, so I'm not sure if this would be that much worth the effort.
Well a lot of users use this feature for the Mednafen cores and they complain when it's not there, so it does have its uses. And in Mednafen's case it is needed for some games to prevent audio stuttering/hiccups at times, not sure if your CDROM code is more optimal but there it seems to be necessary.
Any update on this? IIRC there is a VFS now in retroarch for this purpose
I use a system i call 'reversible' hardpatch for cds where i don't keep the original, i keep a patch to revert to the original.
It's actually really annoying that genesis-dx (the core) doesn't support softpatching for roms, because it's the main genesis emulation core. I assume that the only reason it's disabled in retroarch (there is code for it upstream) is the SEGA cd support which... yeah, it's silly.
It seems ekeeke is working on it: https://github.com/ekeeke/Genesis-Plus-GX/commit/81becffb6c9d1ee9c754439eb4242633a5ae9076
See https://github.com/libretro/Genesis-Plus-GX/issues/211 for recent discussions about soft-patching.
For the moment, since the core still sets "need_fullpath" to TRUE in libretro.c, Retroarch never attempts to load ROM file into memory and disable auto-patching so provided info->data is always NULL and the added code to support soft-patched ROMs will actually never be called.
As explained above and in linked issue, setting "need_fullpath" to FALSE would allow soft-patching to work but would also force Mega CD games to ALWAYS be loaded in RAM, thus potentially causing unexpected huge RAM use from Retroarch (> 600 MB) or more simply breaking Mega CD support on many platforms supported by Retroarch that have limited RAM.
Since "need_fullpath" option apparently can not be made configurable "on the fly", frontend modifications (either solution 2 or 3 described in linked issue) are required so this can be fully functional.
I don't think anybody from libretro is looking into it.
I've looked quickly at retroarch sourcecode and figured the function that needs to be modified: https://github.com/libretro/RetroArch/blob/6dc758a080fdbd2499168422642b9bb8bc6de618/tasks/task_content.c#L927
A simple modification to implement proposed solution #3 could be to add the following bool initialization above that linked line
bool has_patch = !content_ctx->patch_is_blocked && ((!string_is_empty(content_ctx->name_ips) && path_is_valid(content_ctx->name_ips)) || (!string_is_empty(content_ctx->name_ups) && path_is_valid(content_ctx->name_ups)) || (!string_is_empty(content_ctx->name_bps) && path_is_valid(content_ctx->name_bps)));
then change the conditional test in above linked line by:
if ((!need_fullpath || have_patch) && !path_empty)
The question is if Libretto team can merge those changes because they are pretty simpel. Maybe @twinaphex @hizzlekizzle can support.
I've backported these ekeeke's changes into my fork and it seems to work fine.
I've also added this check that seems to switch the need_fullpath core info to true only when loading MCD images (i've checked via debug prints).
If you confirm me it is ok i can submit a PR and finally solve this issue.
Unfortunately, this check is not going to work since system_hw needs to be initialized AFTER the game file has been loaded (it is done by load_rom core function which is called by retro_load_game function) while retro_get_system_info is likely only called once when the core is loaded and, in any case, before retro_load_game is called and system_hw updated to detect a CD game is loaded.
As already explained, it cannot work in all cases without modification of libretro API and retroarch. The best solution (most versatile and without impact on existing cores) seems to be an extension of libretro API to set need_fullpath to true for a preselected list of file extensions (although this still leaves an issue with bin files which can be both ROM or CD image files).
An alternate solution (between this one and the one I initially proposed) could also be a libretro API extension to allow cores requesting soft-patching (and allocation of data in memory with patched ROM data) even if need_fullpath = true, but only in case a valid patch file is found for the loaded game file (which would prevent this to occur unless users put a patch file on purpose and expect the loaded game to be patched).
Unfortunately, this check is not going to work since system_hw needs to be initialized AFTER the game file has been loaded (it is done by load_rom core function which is called by retro_load_game function) while retro_get_system_info is likely only called once when the core is loaded and, in any case, before retro_load_game is called and system_hw updated to detect a CD game is loaded.
I am not sure if they changed something in the frontend logic, but i've verified that system_hw is getting initialized correctly in retro_get_system_info when RA is started from the commandline with a chd image or a rom file.
It probably got initialized by a previously loaded game. Or maybe it is called a second time after game has been loaded to get other settings.
What is sure is that need_fullpath setting is definitively required by retroarch before calling retro_game_load because it is required to either initialize the path or the data pointer that holds the loaded ROM and are provided by retro_game_load.
And again, system_hw can only be changed when retro_game_load is called as it requires the provided game filepath to be checked by the core to detect what kind of file it is.
If you open Retroarch and the first game you load is a CD file or if you load a CD file after a ROM file, I am sure you will see it will be loaded in RAM despite the check you added (this can easily be confirmed by looking at Retroarch RAM use).
I did some extra testing after adding some debug prints, indeed retro_get_system_info is called multiple times when new content is loaded. Also checking the RA process memory shows it fully loads the image in RAM despite need_fullpath is set correctly. I'll try to look for another solution for this...
the only solution i see now without fidding with the RA code is to actually keep need_fullpath=true and have a core-internal patch routine, separate from the one RA is using. However, since i could not find one in the GPGX code i've given up on this and got used to statically patch my ROMs manually (which in the end is not a bad idea to prevent loading uncompatible savestates/sram).
I'd rather not see that route taken, since that would force us to block the method for hardcore RetroAchievements users and would severely reduce the chance of the issue being addressed in the RetroArch end of things.
