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

chdman: Add support for exporting Dreamcast cues and outputting one bin per track #12191

Merged
merged 10 commits into from
Mar 30, 2024

Conversation

987123879113
Copy link
Contributor

@987123879113 987123879113 commented Mar 29, 2024

The output split bins code took inspiration from from @landfillbaby's PR (#11727). I included some of their changes and did some additional refactoring, and also implemented the start of a formatting system for output filenames after some general design feedback from cuavas.

An example of the formatting system:
chdman extractcd -i test.chd -o test.cue -ob "test %03t.bin"
will result in the output bin filenames in the format of "test 001.bin", "test 002.bin", "test 003.bin", etc.
And %t will give the track number without any additional formatting.

The output split bins is a prerequisite for implementing exporting of Dreamcast cues so the two changes have been included together. The commits are fully independent changes so I recommend looking at the specific commits separately instead of the full PR diff for reviewing.

Throwing this out as a draft PR for now to just get some feedback and additional testing.

@landfillbaby
Copy link

landfillbaby commented Mar 29, 2024

if you can figure out what i broke about split .toc in 70e8418 too this would completely obsolete my semi-abandoned PR :)

... or wait did you already do that? have you checked it works?

@987123879113
Copy link
Contributor Author

@landfillbaby Is this an updated version of the code that wasn't in the PR? I didn't realize you worked on a version with TOC support.

I gave a quick comparison between the output this PR and the linked commit:

  • GDI: same output (both .gdi and bins), although filenames are forced to a different format than is conventional for gdi
  • Split TOC: same output (both .toc and bins)
  • Non-split TOC: the first track has the proper filename in the DATAFILE command but every other track becomes DATAFILE "". Output bins are the same.
  • Split cue/bin: the INDEX times aren't starting at 00:00:00 for tracks starting in a new file. The indexes are relative to the loaded file so this would be a big bug.
  • Non-split cue/bin: same output (both .cue and bins)

The issue with non-split TOCs seems to be because you're passing trackbin_name to output_track_metadata but trackbin_name is only set when a new file is loaded, but it's inside the for loop so always starts as a blank string. Would be an easy fix to just move the std::string trackbin_name; out of the loop. The INDEX issue could be fixed by clearing discoffs every time a file is loaded but only if it's not GDI (GDI keeps the running total between each track but TOC and CUE work relative to the loaded file).

@landfillbaby
Copy link

landfillbaby commented Mar 29, 2024

oh you're right! oops!
anyway i think this PR is better than mine :)

src/tools/chdman.cpp Outdated Show resolved Hide resolved
src/tools/chdman.cpp Show resolved Hide resolved
src/tools/chdman.cpp Outdated Show resolved Hide resolved
@landfillbaby
Copy link

i think i'll close my PR in favour of this one now :)

Copy link
Member

@cuavas cuavas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m OK with this PR in principle. Of course it would be nice to have a few people try it over the next few weeks before release when it gets merged.

src/tools/chdman.cpp Outdated Show resolved Hide resolved
src/tools/chdman.cpp Outdated Show resolved Hide resolved
src/tools/chdman.cpp Outdated Show resolved Hide resolved
@cuavas
Copy link
Member

cuavas commented Mar 29, 2024

The last thing this is missing is to add the new option to the “Additional options” list for extractcd in this file: https://github.com/mamedev/mame/blob/master/docs/source/tools/chdman.rst

Yes, the majority of the options in that file aren’t explained. You don’t have to explain the new option – just add it after --outputbin.

@cuavas
Copy link
Member

cuavas commented Mar 30, 2024

This will address #5867.

@987123879113
Copy link
Contributor Author

987123879113 commented Mar 30, 2024

Went through and tested all of the GD-ROM "types" when extracting as cue using data from both TOSEC and Redump sources. Found some bugs addressed in f5d6bb8. Wrote a previous comment here for some info about the breakdown of the "types".

Also tested various cue/bin (split tracks), cue/bin (single bin), toc/bin (single bin) sources and extracting back out to cue/bin (split tracks), cue/bin (single bin), toc/bin (single bin), and toc/bin (split tracks) and didn't find any particular issues with those.

I've tested everything I can think of so I've opened the PR for proper review.

@987123879113 987123879113 marked this pull request as ready for review March 30, 2024 17:15
@cuavas
Copy link
Member

cuavas commented Mar 30, 2024

Let’s go! It isn’t even April yet, so we should have plenty of time to fix it if an issue surfaces.

@cuavas cuavas merged commit 79c1ae3 into mamedev:master Mar 30, 2024
6 checks passed
@987123879113 987123879113 deleted the gdrom_cue branch April 12, 2024 11:40
@johnsanc314
Copy link

johnsanc314 commented May 6, 2024

@987123879113 - Was this tested for Dreamcast? I tried this with a sample of every DC disc type with chdman from mame 0.265 and every single one failed to match the source tracks.

Test                Result   Notes
----------------------------------------------------------------------------------
Type 1 Redump CUE   Fail     Track 3 correct, track 1 and 2 incorrect
Type 1 Redump GDI   Fail     Tracks 1 and 3 correct, track 2 incorrect
Type 1 TOSEC GDI    Fail     Tracks 1 and 3 correct, track 2 incorrect

Type 2 Redump CUE   Fail     All tracks incorrect
Type 2 Redump GDI   Fail     Tracks 1 and 3 correct, tracks 2, 4, and 5 incorrect
Type 2 TOSEC GDI    Fail     Tracks 1 and 3 correct, tracks 2, 4, and 5 incorrect

Type 3 Redump CUE   Fail     All tracks incorrect
Type 3 Redump GDI   Fail     Tracks 1, 3, and 5 correct, tracks 2 and 4 incorrect
Type 3 TOSEC GDI    Fail     Tracks 1, 3, and 5 correct, tracks 2 and 4 incorrect


Sources used:
----------------------------------------------------------------------------------
Type 1 Redump: Virtua Athlete 2000 (USA) (En,Fr,De,Es)
Type 1 TOSEC:  Virtua Athlete 2000 v1.002 (2000)(Agetec)(US)(M4)[!]

Type 2 Redump: Memories Off 2nd - Making Disc (Japan)
Type 2 TOSEC:  Memories Off 2nd Making Disc v1.002 (2001)(KID)(JP)[!][Memories Off 2nd, T-19708M]

Type 3 Redump: Namco Museum (USA)
Type 3 TOSEC:  Namco Museum v1.010 (2000)(Namco)(US)[!][2S][compilation]

EDIT: Nevermind. I didn't realize that the output for the split tracks is completely different depending on if you use .cue, .gdi, or no extension in the output.

I did notice the conversion between types does not work, but perhaps that's expected. For example a CHD created based on a redump cue can only be extracted back to .cue format and not .gdi, and vice versa.

@987123879113
Copy link
Contributor Author

@johnsanc314 I gave it another test just now with latest master and everything is working as expected. What's expected is that you should be able to round trip the data (convert a Redump cue to CHD and extract to cue again, or convert a TOSEC GDI to CHD and extract to GDI again) and the output data should be the same. You'll never get data that matches one of the original dumps if you try converting between Redump and TOSEC formats because they're just fundamentally different, so that's expected too.

You'd probably be interested in this comment for more details. All of the comments about invalid cues were fixed with this PR. #12087 (comment)

@johnsanc314
Copy link

@987123879113 - I could be wrong, but some discs may not be stored "correctly" in CHD format even though the roundtrip works ok. For example "MoHo (Europe) (En,Fr,De)" seems to have some data encoded as audio when it shouldn't be. Try comparing an uncompressed CHD with track 5 of that disc.

@angelosa
Copy link
Member

Gotta love people still using filenames as a way to identify discs instead of using checksums like already mentioned in the chain of events that lead me to open the dc.xml incomplete SW list: #12154

@johnsanc314
Copy link

More specifically, its Redump ID 49774

@987123879113
Copy link
Contributor Author

@johnsanc314 That's just an unfortunate side effect of having to convert to TOSEC layout internally. If it's not done then either the data is irrecoverably discarded, which is bad, or it breaks compatibility with pretty much everything that uses Dreamcast CHDs, which is also bad.

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.

6 participants