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

Add CHD support for Genesis Plus GX #93

Closed
ajshell1 opened this issue Aug 2, 2017 · 66 comments
Closed

Add CHD support for Genesis Plus GX #93

ajshell1 opened this issue Aug 2, 2017 · 66 comments

Comments

@ajshell1
Copy link

ajshell1 commented Aug 2, 2017

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
Copy link

rtissera commented Aug 6, 2017

@ekeeke is it something which could be upstreamed ?

@ekeeke
Copy link

ekeeke commented Aug 6, 2017

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

@rtissera
Copy link

rtissera commented Aug 6, 2017 via email

@ekeeke
Copy link

ekeeke commented Aug 16, 2017

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
Copy link

rtissera commented Aug 16, 2017 via email

@p1pkin
Copy link

p1pkin commented Aug 16, 2017

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
Copy link

@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
Copy link

ekeeke commented Aug 16, 2017

@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
Copy link

p1pkin commented Aug 16, 2017

@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
Copy link

@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
Copy link

ekeeke commented Aug 20, 2017

@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
Copy link

rtissera commented Aug 20, 2017

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

@ekeeke
Copy link

ekeeke commented Aug 20, 2017

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
Copy link

rtissera commented Aug 20, 2017 via email

@ekeeke
Copy link

ekeeke commented Aug 21, 2017

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
Copy link

p1pkin commented Aug 21, 2017

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

@rtissera
Copy link

@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
Copy link

ekeeke commented Aug 22, 2017

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
Copy link

rtissera commented Aug 23, 2017 via email

@rtissera
Copy link

@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
Copy link

ekeeke commented Aug 27, 2017

This commit ekeeke@05dc8fa 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
Copy link

inactive123 commented Aug 27, 2017

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
Copy link

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

@ekeeke
Copy link

ekeeke commented Aug 27, 2017

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
Copy link

inactive123 commented Aug 27, 2017

@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
Copy link

ekeeke commented Aug 27, 2017

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
Copy link

inactive123 commented Aug 27, 2017

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
Copy link

ekeeke commented Aug 27, 2017

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.

@ajshell1
Copy link
Author

ajshell1 commented Sep 5, 2017

@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
Copy link

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
Copy link

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

@ajshell1 ajshell1 changed the title Add CHD support for Genesis Plus GX Add CHD support for Genesis Plus GX and Picodrive Sep 5, 2017
@ekeeke
Copy link

ekeeke commented Sep 6, 2017

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: #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
Copy link
Author

ajshell1 commented Sep 6, 2017

@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
Copy link

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
Copy link

bkoropoff commented Sep 6, 2017 via email

@inactive123
Copy link

@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
Copy link

anothername99 commented Sep 7, 2017

@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
Copy link

ekeeke commented Sep 7, 2017

@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.

@Awakened0
Copy link

@ekeeke, Megamix 4.0b doesn't have any CDDA audio tracks, it uses regular YM2612 for it's BGM. When you download it it doesn't even come with a cue; I just created a basic one back when I needed .cue files for all my Sega CD games to launch through HyperSpin. I already deleted it since I realized I could just load the .iso, but I think it was just a single data track 01 index 00:00 thing. I tried creating a CHD directly from the ISO and that didn't get past the CD player screen either.

@ekeeke
Copy link

ekeeke commented Sep 7, 2017

It's probably because BIOS expects CD games to have at least one audio track following data track. Fake audio tracks are automatically created in Genesis Plus GX for iso or cue/bin but I didn't bothered doing the same for CHD files,

1 similar comment
@ekeeke
Copy link

ekeeke commented Sep 7, 2017

It's probably because BIOS expects CD games to have at least one audio track following data track. Fake audio tracks are automatically created in Genesis Plus GX for iso or cue/bin but I didn't bothered doing the same for CHD files,

@Awakened0
Copy link

That sounds right. It's not a huge deal to not have that game in CHD format since it's only around 14 MB uncompressed; it'd just be nice to have for consistency's sake.

@ajshell1
Copy link
Author

ajshell1 commented Sep 8, 2017

I'd support giving the bounty money to Twinaphex. I'm sure he can think of a good way to use the money.

@inactive123
Copy link

Can you link me to the bounty URL from Bountysource?

Also, have the backers decided where the money should go to? Should it go to an existing bounty or should a new one be created, etc?

@emarleau
Copy link

emarleau commented Sep 8, 2017

I'm ok to donate the money to libretro.

@anothername99
Copy link

@inactive123
Copy link

OK, so since all backers agree, I am going to repeat the question I asked above -

'have the backers decided where the money should go to? Should it go to an existing bounty or should a new one be created, etc?'

@inactive123
Copy link

All three of you can also state individually where your own money should go towards. That might be a better solution vs. having all three of you having to agree on something.

@ajshell1
Copy link
Author

ajshell1 commented Sep 8, 2017

@twinaphex You can have all of my bounty money. Do whatever the hell you want with it.

@p1pkin
Copy link

p1pkin commented Sep 9, 2017

@ekeeke

while original CD-DA format is little endian

afaik data stored at physical CD in big endian. in bit stream most significant bit comes first, least significant bit comes last.
but yes, in common PC CD image formats CD-DA stored as little endian.

@inactive123
Copy link

I'll close the issue then. People in this thread can still feel free to comment here (if possible) or in another issue, we're closing this just for the bounty proceedings.

@ajshell1 ajshell1 changed the title Add CHD support for Genesis Plus GX and Picodrive Add CHD support for Genesis Plus GX Sep 9, 2017
@inactive123
Copy link

@emarleau @anothername99 Can you guys remind me where your money from this bounty should go towards? I will get that done then right now, sorry for the delay.

@emarleau
Copy link

Can you put my money to this bounty please? https://www.bountysource.com/issues/48048939-dynamic-recompiler-305-bounty

@inactive123
Copy link

@emarleau Done.

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

No branches or pull requests

9 participants