libretro / Genesis-Plus-GX

An enhanced port of Genesis Plus - accurate & portable Sega 8/16 bit emulator
Other
74 stars 71 forks source link

Add CHD support for Genesis Plus GX #93

Closed ajshell1 closed 6 years ago

ajshell1 commented 6 years ago

This is a follow-up to a previous bounty.

All I ask is that someone adds CHD support to the Genesis Plus GX core.

I won't be posting a bounty to add CHD support to the PicoDrive. Someone else can do that if they really want to play all six Sega CD games that require the 32X expansion (Corpse Killer, Night Trap, Fahrenheit, Slam City, Supreme Warrior, and Surgical Strike*).

This will have a bounty of $15.

*Surgical Strike for the 32X was only released in Brazil and no dump of it can be found on the internet.

EDIT: There is hope for Surgical Strike! Some Brazilian guys are re-releasing it. They say that they will release an ISO of it on August 15 of 2017.

EDIT2: Because ekeeke doesn't want the money, I am changing this to a Geneis Plus GX and Picodrive bounty. Thus, whoever ports it to Picodrive will get all the money.

rtissera commented 6 years ago

@ekeeke is it something which could be upstreamed ?

ekeeke commented 6 years ago

Sure, depending how this is implemented and interfaces with current core. Ideally, this would be made optional like ogg file support.

rtissera commented 6 years ago

OK good to know. The biggest burden is TOC rebuilding. I will try to have a look after adding clean support to various mednafen cores.

Le 6 août 2017 1:35 PM, "ekeeke" notifications@github.com a écrit :

Sure, depending how this is implemented and interfaces with current core. Ideally, this would be made optional like ogg file support.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/libretro/Genesis-Plus-GX/issues/93#issuecomment-320501399, or mute the thread https://github.com/notifications/unsubscribe-auth/AC3LBSnIp3cO21e0X05WTJ1IKjzGGDZJks5sVaTjgaJpZM4OrzS0 .

ekeeke commented 6 years ago

The biggest burden is TOC rebuilding. I will try to have a look after adding clean support to various mednafen cores.

I actually started implementing it as a coding exercise. The hardest part was indeed to figure how to build the TOC since CHD documentation is quite scarce but I eventually managed to get how it works by reading MAME code (chdman.cpp, cdrom.cpp and chdcd.c) and integrated it in Genesis Plus GX code without much work. Still need to compile the whole thing and test/debug it but this does not look like it will be much difficult.

rtissera commented 6 years ago

@ekeeke Great!

Let me know if you have any issue and feel free to have a look at CDAccess_CHD class in beetle pcfx or pce fast libretro cores as example of doing that.

Le 16 août 2017 10:35 AM, "ekeeke" notifications@github.com a écrit :

The biggest burden is TOC rebuilding. I will try to have a look after adding clean support to various mednafen cores.

I actually started implementing it as a coding exercise. The hardest part was indeed to figure how to build the TOC since CHD documentation is quite scarce but I eventually managed to get how it works by reading MAME code (chdman.cpp, cdrom.cpp and chdcd.c) and integrated it in Genesis Plus GX code without much work. Still need to compile the whole thing and test/debug it but this does not look like it will be much difficult.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/libretro/Genesis-Plus-GX/issues/93#issuecomment-322703783, or mute the thread https://github.com/notifications/unsubscribe-auth/AC3LBT-yqLnOzinA34bUy1HI66KMyrM4ks5sYqm2gaJpZM4OrzS0 .

p1pkin commented 6 years ago

its actually not hard at all to use CHD in your project using original MAME cdrom.h/cdrom.cpp: cdrom_open, cdrom_read_data, cdrom_read_subcode, cdrom_get_toc - that's all functions you will need to know about. moreover it will handle not only CHD but all other image formats supported by MAME core (cue/bin, nrg, toc/bin etc)

on other hand, it is pain in the arse if you will use libchdr port instead ...

rtissera commented 6 years ago

@p1pkin I was actually thinking adding helper functions from cdrom.cpp to libchdr in order to make it easier to use.

But it's always nice to see support and kind words ;)

ekeeke commented 6 years ago

@p1pkin

MAME code is actually quite heavy and tied to its own structure definitions for CDROM, tracks, etc

It was actually much easier to use stripped-out C version of chd.c and cdrom.c from @rtissera and make a few modifications to my own code in cdd.c than reusing MAME abstraction and adapting my code for it.

p1pkin commented 6 years ago

@ekeeke I see you point, indeed C version is more lightweight, if it matters for you target platforms.

I just trying to warn - it is not good idea to parse TOC metadata tag in each emulator, but it have to be implemented in libchdr. mainly because CHD format is not static, there couple of things planned to be implemented in future, like multisession CD support and few others. this is likely lead to change from CUE-like metadata TOC, to binary/RAW TOC, basically in the same form as it "stored" in CD lead-in area subcodes. guess what will happen in this case ? - all emulators will have to change metadata TOC parsing routines to support newer chd images. doesn't look good eh ? so, as was said above, in my honest opinion, this must be handled in chd library, instead of each emulator on its own.

rtissera commented 6 years ago

@p1pkin Thanks for the valuable insight on the metadata parsing, I totally agree with you this should be added to libchdr.

The fact is that libchdr was initially written for solely use in redream and then I added it in libretro stuff. I will add the metadata parsing and high level cdrom TOC logic to the library and push updates to current implementations (so far only redream and beetle-*-libretro cores).

ekeeke commented 6 years ago

@rtissera: there seems to be a lot of memory allocation (malloc) that never get freed when calling chd_close. Similarly, lzma_codec_free is defined but never called and there is a lot of "TODO" in many of the codec free functions. This seems it could cause some memory leak when loading consecutive. chd files.

Another thing I noticed is that chd->cache and chd->compare buffers are allocated with the size of a chunk but never get actually used by anything. It seems safe to remove these to save some RAM.

rtissera commented 6 years ago

@ekeeke Yes definitely I will try to clean this up ASAP. Thanks for pointing out the chd->cache and chd->compare.

ekeeke commented 6 years ago

If this can be some help, here is my modified version which should have all potential memory leaks fixed hopefully, as well as some unused stuff removed. Do whatever you want with it anddo not hesitate to tell me if you think I missed something.

chd.zip

I also had to modify a few header files to deal with multiple similar typedefs.

rtissera commented 6 years ago

Oh thanks a lot, I will merge this in a couple o days.

Le 20 août 2017 à 21:53, ekeeke notifications@github.com a écrit :

If this can be some help, here is my modified version which should have all potential memory leaks fixed hopefully, as well as some unused stuff removed. Do whatever you want with it anddo not hesitate to tell me if you think I missed something.

chd.zip https://github.com/libretro/Genesis-Plus-GX/files/1237239/chd.zip I also had to modify a few header files to deal with multiple similar typedefs.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/libretro/Genesis-Plus-GX/issues/93#issuecomment-323607774, or mute the thread https://github.com/notifications/unsubscribe-auth/AC3LBYg1vdGZ88o5t_OKk-LSXQsA24xeks5saI6sgaJpZM4OrzS0.

ekeeke commented 6 years ago

Beware it was not fully tested yet. I had to modify a change I made that was incorrect. To fix errors with subcode decompressing in CD FLAC codec interface, I had to replace

zlib_codec_init(cdfl,... zlib_codec_free(cdfl); by zlib_codec_init(&cdfl->inflater,... zlib_codec_free(&cdfl->inflater); in cdfl_codec_init() cdfl_codec_free()

So far, CHD support works in Genesis Plus GX except for audio tracks which only produce noise. I'm using an old version of chdman I had from 2012 with data track compressed with LZMA and audio tracks compressed with FLAC. Any idea where this could come from? Unfortunately, I cannot find a more recent version of chdman.exe anywhere (that old version also seems to handle cuesheet INDEX00 commands incorrectly as it treats them exactly like non-included PREGAP).

p1pkin commented 6 years ago

chdman included in MAME distributive http://www.mamedev.org/release.html

rtissera commented 6 years ago

@ekeeke I do remember some CHDMAN binaries floating around produce bad metadata (inducing TOC issues) but I do not this this is your issue. Have you tried byte-swapping FLAC decoded audio ? I do remember seeing some code into MAME related to endianness on CDDA compressed tracks.

ekeeke commented 6 years ago

Yes, that was actually the issue. I actually fixed it some minutes ago just before I got the opportunity to read your message ;-)

16-bit audio samples are indeed big-endian when uncompressed (while original CD-DA format is little endian). I figured it by comparing emulator PCM output when using CUE/BIN vs CHD.

Btw, all crypto sources can safely be removed from your library: sha1.h and md5.h are included in chd.c but they are only used to define compmd5 and compsha1 fields in CHD structure although those fields are never used anywhere so they could as well be deleted (I think it's only needed in original MAME code to verify CHD files).

Here are the modified source files (with all potential memory leaks normally fixed and unused stuff removed or made optional): libchdr_mod.zip

rtissera commented 6 years ago

OK great! I will merge your fixes and try to improve libchdr adding high level CD-ROM code for TOC handling.

Le 22 août 2017 11:29 PM, "ekeeke" notifications@github.com a écrit :

Yes, that was actually the issue. I actually just fixed it before I got the opportunity to read your message ;-)

16-bit audio samples are indeed big-endian when uncompressed (while original CD-DA format is little endian). I figured it by comparing emulator PCM outputs when using CUE/BIN and CHD.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/libretro/Genesis-Plus-GX/issues/93#issuecomment-324157105, or mute the thread https://github.com/notifications/unsubscribe-auth/AC3LBZ_23c3UXw5N1LTi6hsLxjvfboyBks5sa0gmgaJpZM4OrzS0 .

rtissera commented 6 years ago

@ekeeke Merged most of your changes into https://github.com/rtissera/libchdr/tree/ekeeke I will test this against redream and beetle-*-libretro implementations and update the branch accordingly.

ekeeke commented 6 years ago

This commit https://github.com/ekeeke/Genesis-Plus-GX/commit/05dc8faa0419869438fc1a3c2b1741e87fea0178 added CHD support in Genesis Plus GX repository.

This was tested fine with official Wii port and Retroarch Win32 port using a cue/bin ("Super Strike Trilogy") converted to .chd with latest chdman version.

Please confirm it works for you (this will need to be backported and compiled in libretro repository first) and close if everything is fine.

@rtissera : to get FLAC decoding working on Wii, I needed to add the following defines in Makefile -DCPU_IS_BIG_ENDIAN=1 -DWORDS_BIGENDIAN=1 You might need to add these flags as well in Makefile.libretro for the various cores where you already implemented CHD support when they build for big endian platforms (such as Wii, WiiU, PS3,etc).

inactive123 commented 6 years ago

Hi there @ekeeke ,

awesome to see.

https://www.bountysource.com/issues/47874325-add-chd-support-for-genesis-plus-gx

Try to register on this site if you haven't already and submit a solution. That way, once we have closed the issue, the bounty pledgers can accept the solution.

rtissera commented 6 years ago

Great work @ekeeke. Indeed I missed the big endian platforms :)

ekeeke commented 6 years ago

To be honest with you, I don't really feel comfortable with that bounty thing. I did not asked any 'bounty' for implementing sega cd support in the first place or for the continued bugfixes since then, which were honestly much more workload than adding this simple feature. If people who use my work want to show their appreciation, there has always been a donation link on my repository especially for that but I surely don't want getting 'paid' by you (or anyone) to fulfill end-users requests.

inactive123 commented 6 years ago

@ekeeke I am not paying at all, it is the users whom would pay you. You can look at the backers and see that our name does not pop up there. https://www.bountysource.com/issues/47874325-add-chd-support-for-genesis-plus-gx/backers

Anyway, I was just trying to be nice. It is up to you. The bounty exists and it would be an easy $45 to fetch, so well, up to you.

BTW - I looked at the Donate button - just to warn you but Paypal is really not safe for emulation donations. They can and will shut that down. They have already done it in the past for Yabause and us as well. And when they shut your account down, you can't appeal it. I'd seek a different way to gather donations as quickly as possible.

ekeeke commented 6 years ago

I meant you or anyone (I edited previous post) , it's just I don't want people 'paying' me to get features or me using that bountysource thing. I personally think in long term this discourages other developers from releasing any new stuff 'for free' and give end-users the impression they can only get new stuff by offering enough money for it. Once again, I have a donate button on Genesis Plus GX homepage if people want to throw a few bucks to show support for already done and upcoming work, if I wanted to get paid for my hobby, I would have a patreon or a bountysource account.

inactive123 commented 6 years ago

Fair enough, if that's your stance on it then so be it, the bounty backers can probably close it then. Still though, I would really recommend you find some other way to obtain donations other than Paypal. Obtaining donations for emulators is against their Terms of Service, and it's only a matter of time until it will be shut down, and then if your bank is tied to your Paypal account, you will never be able to use it again with Paypal.

https://yabause.org/2016/05/26/donation-changes/

I also don't see how having a Donation page is any different from Bountysource or Patreon, but I respect your stance nonetheless. I am just warning you against Paypal since it is pretty much a 100% given that it will be shut down.

ekeeke commented 6 years ago

I appreciate your concern but I really doubt they give a shit now considering it has been running for ten years and I received less than 100 euros in total :-)

And on that regard, I consider this indeed to be very different from a patreon where you get paid every month and have stretch goals to motivate your patrons or bountysource where you setup a price for your contributions to the project.

anothername99 commented 6 years ago

@ekeeke It's up to you. If you want the money, we'll pay you, if not, we won't. :)

anothername99 commented 6 years ago

@twinaphex @rtissera There is more money on this bounty than the beetle-psx CHD bounty and I'm not sure if refunds are a straight-forward thing. So if ekeeke really doesn't want the money, then perhaps we can use this money on that instead.

rtissera could then "start a solution" on this bounty if/when he has implemented CHD support for beetle-psx (but let's wait for confirmation from @ekeeke and the others first).

rtissera commented 6 years ago

Well on the PSX the current limitation is no support for subchannels (in CHDMAN) so libcrypt games won't work except if we hook SBI support to CHD code.

Another approach is to add a proper CloneCD parser to MAME CHDMAN and get it upstreamed.

I am also considering improving the libchdr by adding some more high level code in order to ease CHD support into various emulators as suggested by @p1kpin

p1pkin commented 6 years ago

it is not entirely correct, chdman supports CDRDAO TOC/BIN format, including subchanel data.

anothername99 commented 6 years ago

I would suggest adding .sbi file support to chdman at least. Even if the user only has a cue/bin image, he can get a .sbi file for his game from redump.org. Of course a full CloneCD parser would be even better though.

p1pkin commented 6 years ago

.sbi ?

rtissera commented 6 years ago

@p1pkin SBI files are basically patches containing PSX libcrypt protection information (stored in sub channel data).

http://psxdatacenter.com/sbifiles.html

p1pkin commented 6 years ago

thanks for explanation, sounds like a piece of hack instead of proper subchanel data.

rtissera commented 6 years ago

It is.

ajshell1 commented 6 years ago

Any progress? It seems as though standalone GPGX now has CHD support. Implementing that into the RA core is an easy $45.

Besides, I already converted all my Sega CD games to CHD.

inactive123 commented 6 years ago

The problem is that ekeeke doesn't want the money.

I'd say the best thing we could do is that the backers agree how that money should get reallocated instead, I then send a solution on Bountysource, you agree to the solution, and then I make sure we transfer the money to whatever else needs it. I don't think Bountysource does refunds or anything, and you'd lose 10% as part of transfer fees.

anothername99 commented 6 years ago

@twinaphex Yeah, I agree that we should avoid going the refund route. If the money can be transferred somehow I'm fine with that. Otherwise maybe ajshell1 could just edit the title and first post on this issue and add a note that the genplusgx CHD issue is already resolved and doesn't apply.

@ajshell1 CHD already works in the genplusgx libretro core as well, it just needs to be merged and added to the buildbot. Is there anything else you want for RetroArch/libretro that needs funding? If it's something I also want, then I'm in.

ajshell1 commented 6 years ago

@twinaphex @anothername99 I agree with anothername99's idea of editing the title and first post.

Unless anothername99 can come up with a better idea, I'm going to change this to "Add CHD support for Genesis Plus GX and Picodrive".

anothername99 commented 6 years ago

Personally I would like to see a solution to the multi-disc problem. m3u playlists and the like are a bit of a pain, so I'd prefer if there was support for putting every disc in an uncompressed zip file or something like that. With predictable filenames (disc1.chd, disc2.chd, etc) it should work pretty well.

But if you would rather go with Picodrive CHD support, I'm fine with that too. I wonder what the other backer @emarleau thinks though.

Awakened0 commented 6 years ago

I'm getting a crash loading a CHD with the latest Win64 buildbot build (60ca366941ce92d12c51500b32ea9793e743e960). The bin/cue loads fine. Here's a backtrace: https://hastebin.com/aloqesical.tex

ekeeke commented 6 years ago

Crash is caused by incompatibility between libretro FILE virtualization and libchdr file access. It didn't occured to me because libretro port in my repository is missing all the recent FILE virtualization changes added for UWP support (my repository is actually more than 80 commits behind this repository so please someone submit a pull request to sync everything back properly on both sides, not just libretro side).

This pull request should normally fix it: https://github.com/libretro/Genesis-Plus-GX/pull/96

Btw, I'd suggest you give that bounty to @bkoropoff if my refusal is causing you so much issues. Afterall, he is the one who got CHD file support working in retroarch in the end and that's the only thing that matters, right?

ajshell1 commented 6 years ago

@ekeeke Alright. Good to hear. As for the bounty, if @bkoropoff shows up and decides that he wants the money, I will not object to giving it to him.

Awakened0 commented 6 years ago

Ok, with the latest build there's no more crashing and all the official games I tried work perfectly. Sonic Megamix 4.0b won't go past the CD player screen when converted to CHD though. The cue/iso still works fine.

bkoropoff commented 6 years ago

I just submitted a trivial fix since I happened to be trying out the feature and dug into the crash out of curiosity. The bounty should go to whoever did the actual implementation work.

On Wed, Sep 6, 2017, 7:01 AM ajshell1 notifications@github.com wrote:

@ekeeke https://github.com/ekeeke Alright. Good to hear. As for the bounty, if @bkoropoff https://github.com/bkoropoff shows up and decides that he wants the money, I will not object to giving it to him.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/libretro/Genesis-Plus-GX/issues/93#issuecomment-327492559, or mute the thread https://github.com/notifications/unsubscribe-auth/ACAQN9nnbyGMmvm69U4XwkQuCh8ysAsHks5sfqWvgaJpZM4OrzS0 .

inactive123 commented 6 years ago

@bkoropoff The problem with that is that he doesn't feel comfortable taking it, so since Bountysource doesn't do refunds, it still has to go towards somebody otherwise the bounty backers just lose their money altogether.

We are reaching a kind of dead road here. If bkoropoff doesn't want it either, I'd suggest doing what I suggested before where the bounty backers let me claim a solution, then we redistribute the money to another issue of the bounty backer's choosing.

anothername99 commented 6 years ago

@twinaphex Now I get what you mean. I wouldn't mind. If you make a claim, I'll accept. You can then put my money in another bounty, or put it back in the libretro organization, whichever you prefer.

ekeeke commented 6 years ago

@Awakened0: does it show correct number of tracks in CD player screen and does it let you play audio tracks correctly? Please copy the cue file content so I can see if there are any particularities I might have missed.