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

XDelta patch support (Take 2) #15915

Merged
merged 31 commits into from
Nov 24, 2023
Merged

Conversation

JesseTG
Copy link
Contributor

@JesseTG JesseTG commented Nov 16, 2023

Description

This PR adds support for XDelta. It restores the code that was merged (then reverted shortly thereafter) by this PR from last year. The code itself hasn't changed much, but now there are more GitHub pipelines in place to detect build problems before they surface on GitLab.

Reviewers

@LibretroAdmin @hizzlekizzle

- Otherwise the static_assert calls can fail
- The patching itself isn't fully implemented yet
- Now checks max values instead of relying on autotools
- Move them outside the XD3_USE_LARGEFILE64 block
- Add more SIZE declarations
- Make SIZEOF_UNSIGNED_LONG_LONG contingent on the presence of ULLONG_MAX
- HAVE_XDELTA is on by default
- HAVE_PATCH is still required for HAVE_XDELTA to be meaningful
- Support is mostly contingent on the availability of LZMA
- Anything modern should be okay
- Legacy platforms (e.g. DOS) may need to have Xdelta support disabled
- At least until some other solution can be found
- These come from looking at the failed builds on GitHub
- These are guesses, and may turn out to be wrong
- Whoops, looks like I need to call two cleanup functions
- xd3_close_stream exists separately from xd3_free_stream
- GCC was complaining about #ifdefs within macro arguments being non-portable
- It's mostly using RetroArch's INLINE macro instead of the inline keyword
.gitignore Outdated Show resolved Hide resolved
- To be in line with other recent refactoring
- Whoops, this part was from before I figured out how to get the size of a patched file
- Exclude the encoder, since we're not making patches
- Move some #defines to after inclusion of <stdint.h>, to fix undefined behavior
- Remove _WIN32_WINNT overrides, since they were for code that we're not using
@JesseTG
Copy link
Contributor Author

JesseTG commented Nov 17, 2023

@LibretroAdmin @hunterk I consider this PR complete and await your review.

@i30817
Copy link
Contributor

i30817 commented Nov 17, 2023

Not any of my business but I'd like to ask to see if I will be able to use this or not. Id build to check this but my only computing device is a android tablet right now.

  1. Can the patched file be a track in a cue\bin or cue\iso (same thing I guess)? This is the most important part of making this useful I suspect, since redump dumps are (almost) exclusively cue\bin, most xdelta patches target cd tracks, and mednafen doesn't even support bare iso, just cue\iso. Analysing the loaded cue, patching the tracks that need patching and creating a fake cue to replace the original pointing to all softpatched tracks with a absolute path, would be enough if it uses the tmp file method.
  2. Does it patch to memory, temporary file or a memory size threshold mix?
  3. It won't get confused if the xdelta file is named originalnameexceptextension + '.revert.xdelta' right? I use that scheme in a personal program to revert a hardpatched rom for matching, and now I'm worried, although nothing should happen because the name would have to match originalgamenameexceptextension + .'revert' for it to get confused and it's kind of impossible if the same strategy\call is used to find the extension and the basename (last or first dot to find extension), and the obvious and correct is last dot.
  4. Finally, does this support the multiple patches format that ips,bps and ups do, .ips, .ips1...,.ips# etc?

@JesseTG
Copy link
Contributor Author

JesseTG commented Nov 17, 2023

  1. Can the patched file be a track in a cue\bin or cue\iso (same thing I guess)? This is the most important part of making this useful I suspect, since redump dumps are (almost) exclusively cue\bin, most xdelta patches target cd tracks, and mednafen doesn't even support bare iso, just cue\iso. Analysing the loaded cue, patching the tracks that need patching and creating a fake cue to replace the original pointing to all softpatched tracks with a absolute path, would be enough if it uses the tmp file method.

Not sure. I don't see any fundamental reason it shouldn't work, but I don't know how softpatching disk images works (if at all).

  1. Does it patch to memory, temporary file or a memory size threshold mix?

Memory only. It makes two passes over the file; one to compute the final file size (since it's stored per block, not globally), and one to actually patch the data with a buffer that's exactly as big as needed.

  1. It won't get confused if the xdelta file is named originalnameexceptextension + '.revert.xdelta' right? I use that scheme in a personal program to revert a hardpatched rom for matching, and now I'm worried, although nothing should happen because the name would have to match originalgamenameexceptextension + .'revert' for it to get confused and it's kind of impossible if the same strategy\call is used to find the extension and the basename (last or first dot to find extension), and the obvious and correct is last dot.

You'll be fine, I think. Xdelta file detection literally works the same as .ips/.ups/.bps detection, .xdelta just has slightly more characters.

  1. Finally, does this support the multiple patches format that ips,bps and ups do, .ips, .ips1...,.ips# etc?

Yep, fully. Works exactly the same.

@i30817
Copy link
Contributor

i30817 commented Nov 17, 2023

I kind of suspect that it won't work for cues ... or iso, precisely because all RetroArch softpatching is disabled with cd images by a core property (it sometimes even has collateral damage in cores that also use roms).

Id personally appreciate a lifting of this limitation for those people that have a lot of memory and operating systems that can use virtual filesystems files as files.

Part of that work I suppose, will be to scan, and alter if needed any cue file before it's passed to the core to change tracks for patched versions. And it needs to point to 'files' because that's what the cores expect for cues tracks, even if it's a virtual, filesystem file.

Would be easier with tmp files, but I'd actually prefer in memory only solution because modern devices have truly decadent amounts of RAM.

It's less pressing with chd support though, assuming the emulator\library implemented parent-child... which is not actually certain, since some platforms can't even open 2 files at once, and... you kind of have to scan the filesystem to find the parent or hardcode a scheme.

So, this is cool and a necessary step, but I suspect still niche because it isn't 'softpatch cds' that a user would expect if he saw 'xdelta softpatch support' on release notes. But ok, many games on the NDS, 3DS and even some snes games patches are released as xdelta, still useful.

Particularly if the n64, nds and 3ds cores limitations against softpatch are lifted.

@JesseTG
Copy link
Contributor Author

JesseTG commented Nov 18, 2023

I kind of suspect that it won't work for cues ... or iso, precisely because all RetroArch softpatching is disabled with cd images by a core property (it sometimes even has collateral damage in cores that also use roms).

Id personally appreciate a lifting of this limitation for those people that have a lot of memory and operating systems that can use virtual filesystems files as files.

Which property are you referring to?

Part of that work I suppose, will be to scan, and alter if needed any cue file before it's passed to the core to change tracks for patched versions. And it needs to point to 'files' because that's what the cores expect for cues tracks, even if it's a virtual, filesystem file.

Here's an idea; what if we use the VFS abstraction here? Here's what I'm thinking:

At the moment, the RETRO_ENVIRONMENT_GET_VFS_INTERFACE that RetroArch implements is identical to the one in libretro-common. What if instead, we had a list of layers:

struct vfs_layer {
   bool (*handles_path)(const char* path);
   retro_vfs_get_path_t get_path;
   retro_vfs_open_t open;
   retro_vfs_read_t read;
   retro_vfs_write_t write;
   // ..and the other VFS functions
};

static struct vfs_layer layers[] = {
   archive_layer, // open specific files in archives ("/games/mario.zip#supermario.bin")
   core_file_layer, // open another core file via URI syntax ("core://mgba/saves/Pokemon Ruby/pokemon.sav")
   softpatch_layer, // open a softpatched version of a file if one exists
   default_layer, // all other paths
};

I'd say this would introduce a whole new way for frontends to expose information to cores, but the existing VFS interface has always allowed this. It just hasn't been done yet. If the VFS implementation were generalized like this, the core wouldn't have to know about the patch; the frontend would just return the softpatched version via the softpatch_layer.

So, this is cool and a necessary step, but I suspect still niche because it isn't 'softpatch cds' that a user would expect if he saw 'xdelta softpatch support' on release notes. But ok, many games on the NDS, 3DS and even some snes games patches are released as xdelta, still useful.

Particularly if the n64, nds and 3ds cores limitations against softpatch are lifted.

Boy, do I have good news to tell you! The new melonDS core I'm working on does exactly this. I should find a place to mention it in the docs.

@hizzlekizzle
Copy link
Contributor

yes, needs_fullpath would preclude any cdrom softpatching anyway, currently. And yes, being able to softpatch anything was one of the original driving concepts behind the VFS stuff, it just never actually came to pass.

@JesseTG
Copy link
Contributor Author

JesseTG commented Nov 18, 2023

yes, needs_fullpath would preclude any cdrom softpatching anyway, currently. And yes, being able to softpatch anything was one of the original driving concepts behind the VFS stuff, it just never actually came to pass.

Can I take that to mean you'd merge a PR that brings it to pass? At the very least the abstraction layer necessary for it to happen? Because I can already think of another use case for it.

I once had a spirited debate over a proposed environment call that lets a core ask the frontend for info about other cores. What if instead of an environment call, it was a custom URI that the VFS recognized? Something like retro://cores/mGBA/saves/Pokemon Red (U).sav? The actual disk structure becomes irrelevant, this would just let us access the save data.

And the reason I wanted to do that was to integrate support for transferring data to and from a GBA cartridge within melonDS DS.

@hizzlekizzle
Copy link
Contributor

Can I take that to mean you'd merge a PR that brings it to pass?

I don't have unilateral authority to approve such a thing, but I would certainly vouch for it. Let's talk to some other folks in discord and see who's on board with it :)

@LibretroAdmin LibretroAdmin merged commit cbf49a0 into libretro:master Nov 24, 2023
22 checks passed
Sunderland93 pushed a commit to Sunderland93/RetroArch that referenced this pull request Dec 26, 2024
* Add xdelta in deps

* Include <assert.h> in xdelta3.h

- Otherwise the static_assert calls can fail

* Build xdelta3 in Makefile.common

* Add xdelta support to the softpatching infrastructure

- The patching itself isn't fully implemented yet

* Adjust how xdelta3.h checks the sizes of some types

- Now checks max values instead of relying on autotools

* Add some enums that were excluded by the cherry-pick

* Remove stray whitespace

* Adjust SIZE macros in xdelta3.h

- Move them outside the XD3_USE_LARGEFILE64 block
- Add more SIZE declarations
- Make SIZEOF_UNSIGNED_LONG_LONG contingent on the presence of ULLONG_MAX

* Reintegrate xdelta support

* Enable support for xdelta's secondary compressors

- Necessary for some patches

* Fix some format specifiers

* Remove unnecessary files from xdelta

* Include xdelta3.h with a relative path

* Add xdelta3 headers to HEADERS variable

* Gate Xdelta support behind HAVE_XDELTA

- HAVE_XDELTA is on by default
- HAVE_PATCH is still required for HAVE_XDELTA to be meaningful
- Support is mostly contingent on the availability of LZMA
- Anything modern should be okay
- Legacy platforms (e.g. DOS) may need to have Xdelta support disabled
- At least until some other solution can be found

* Disable HAVE_XDELTA on platforms where the build recently failed

- These come from looking at the failed builds on GitHub
- These are guesses, and may turn out to be wrong

* Fix a potential memory leak

- Whoops, looks like I need to call two cleanup functions
- xd3_close_stream exists separately from xd3_free_stream

* Split the --help printout for --xdelta into its own strlcat call

- GCC was complaining about #ifdefs within macro arguments being non-portable

* Fix some incorrect printf format specifiers

* Modify Xdelta to adhere to C89

- It's mostly using RetroArch's INLINE macro instead of the inline keyword

* Slight cleanups

* Remove a stray comma that was hindering C89 builds

* Add XDelta support to CHANGES.md

* Change how the xdelta patch's name is computed

- To be in line with other recent refactoring

* Fix an incorrect merge

- Whoops, this part was from before I figured out how to get the size of a patched file

* Explain the song-and-dance behind computing a patched file's size

* Define some XDelta3-related constants to 0 on 32-bit platforms

* Adjust some Xdelta-related macro definitions

- Exclude the encoder, since we're not making patches
- Move some #defines to after inclusion of <stdint.h>, to fix undefined behavior
- Remove _WIN32_WINNT overrides, since they were for code that we're not using

* Fix Xdelta support

* Wrap an encoder-only function in `#if XD3_ENCODER`
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