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

Remove duplicate CD modes #12012

Merged
merged 2 commits into from
Feb 5, 2024
Merged

Remove duplicate CD modes #12012

merged 2 commits into from
Feb 5, 2024

Conversation

stonedDiscord
Copy link
Contributor

Resolves #9084

Comment on lines -660 to -664
else if (!strcmp(typestring, "MODE2/2336"))
{
*trktype = CD_TRACK_MODE2_FORM_MIX;
*datasize = 2336;
}
Copy link
Member

Choose a reason for hiding this comment

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

I get that this code is unreachable so this won’t change behaviour, but what’s it supposed to be doing? The place where it actually catches this is at line 630:

	else if (!strcmp(typestring, "MODE2/2336"))
	{
		*trktype = CD_TRACK_MODE2;
		*datasize = 2336;
	}

It gives CD_TRACK_MODE2_FORM_MIX. Why is it trying to use CD_TRACK_MODE2_FORM_MIX for the same track type here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I have a strong feeling that this was a slip of the mind type of mistake. Looking at the original commit that introduced the two MODE2/2336s, it looks like they just added a corresponding MODEx/y for all of the existing .toc type strings.

.toc defines both MODE2 and MODE2_FORM_MIX, and both also are defined as 2336 bytes per sector.

"The block length of the input data depends on the : AUDIO: 2352 bytes (588 samples), MODE1: 2048 bytes, MODE1_RAW: 2352 bytes, MODE2: 2336 bytes, MODE2_FORM1: 2048 bytes, MODE2_FORM2: 2324 bytes, MODE2_FORM_MIX: 2336 bytes including the sub-header, MODE2_RAW: 2352 bytes."
source: https://manpages.ubuntu.com/manpages/focal/en/man1/cdrdao.1.html

CD_TRACK_MODE2_FORM_MIX is like a meta type. It means the track is either MODE2_FORM1 or MODE2_FORM2, padded to the same block size and has the sub-header included in the 2336 bytes. MODE2_FORM1's user data size is 2048 bytes and MODE2_FORM2's user data size is 2328 bytes. The subheader for both form 1 and form 2 is 8 bytes.

CD_TRACK_MODE2 means it's 2336 bytes of just user data (no headers).

Copy link
Member

Choose a reason for hiding this comment

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

Right, CD_TRACK_MODE2 is the CD-ROM Mode 2 data track format, with 2,336 bytes of data per sector, and no additional error detection/correction data.

But do CD_TRACK_MODE2_FORM1 and CD_TRACK_MODE2_FORM2 really make sense? The whole point of CD-ROM XA tracks is that they can contain a mixture of Form 1 and Form 2 sectors (with and without additional error correction data, respectively). Or are these “artificial” formats for authoring, where the software interpreting the data is expected to generate a track containing only a single form?

And why would there be padding in a CD_TRACK_MODE2_FORM_MIX track? The sector payload size is the same for both forms. Form 1 is 8-byte subheader, 2,048-byte data, 4-byte checksum, 276-byte ECC = 2,336 bytes total. Form 2 is 8-byte subheader, 2,324-byte data, 4-byte checksum = 2,336 bytes total. Or is there some weirdess where the checksum and ECC bytes are treated as “padding” and replaced with the appropriate calculated content?

Comment on lines 670 to 674
else if (!strcmp(typestring, "MODE2/2352"))
{
*trktype = CD_TRACK_MODE2_RAW;
*datasize = 2352;
}
Copy link
Member

Choose a reason for hiding this comment

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

There’s no other place that catches MODE2/2352 so it isn’t a duplicate. Why did you remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh sorry i confused it with MODE1/2352

@stonedDiscord
Copy link
Contributor Author

I've read the ISO9660 spec and I can't find a mention of a mixed form Mode2 track, can we get someone who is familiar with CDI and Dreamcast discs in here?

@cuavas
Copy link
Member

cuavas commented Feb 5, 2024

I've read the ISO9660 spec and I can't find a mention of a mixed form Mode2 track, can we get someone who is familiar with CDI and Dreamcast discs in here?

It has nothing to do with Dreamcast – it’s CD-ROM XA. The Philips spec is easy enough to find.

@cuavas cuavas merged commit a62fa6e into mamedev:master Feb 5, 2024
5 checks passed
@stonedDiscord stonedDiscord deleted the patch-2 branch February 5, 2024 16:52
Mokona pushed a commit to Mokona/mame that referenced this pull request Feb 28, 2024
stonedDiscord added a commit to stonedDiscord/mame that referenced this pull request Apr 8, 2024
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.

cdrom.cpp: MODE2/2336 defined twice
3 participants