Add data support 0 4 1#34
Conversation
- Added missing IOKit SCSI headers to shim_common.h. - Implemented SCSI Task Device initialization in device_service.c by traversing the IORegistry parent chain. - Corrected the SCSI transfer direction in data_reader.c. - These changes restore the ability to perform direct SCSI commands for data sector reading on macOS. Co-authored-by: danifunker <1643018+danifunker@users.noreply.github.com>
- Replaced `kIOMediaClass` with `kIOCDMediaClass` for compatibility. - Replaced deprecated `kIOMasterPortDefault` with `kIOMainPortDefault`. - Replaced undeclared `S32` with the correct `SInt32` type. - Retained previous fixes: added missing SCSI headers, implemented device session initialization by traversing IORegistry, and corrected SCSI transfer direction for READ CD. - These changes ensure the new data reader works correctly on modern macOS versions. Co-authored-by: danifunker <1643018+danifunker@users.noreply.github.com>
…4448739249 Add data support fix 12974352404448739249
|
@danifunker hey, thanks, I will review in a couple of days!
Do you mean enhanced CDs? The ones which contain some data tracks? I actually was debugging my own CD ripper app (it failed on some CDs), and it turns out that the calculation of the leadout LBA for the last audio track is a bit different if there are data tracks. They actually mention it in musicbrainz docs -- you are supposed to get the start LBA of the data track, and subtract 152 seconds, or 11400 frames. It is described somewhere in the SCSI standard for audio CD extension -- something like 60 seconds gap, 90 seconds buffer between audio and data, and another 2 seconds gap similar to one when we start reading regular data. |
|
Very cool! Please keep me posted on this. I actually extended this initially to support classic CD-ROM games which shipped with CD audio support. Think mid to late 90s games for PC. I know it is a bit of an obscure use case, but for the app I was writing, it was important. I was able to lookup the jewel case by getting the name of the disc through musicbrainz then queries discogs to find the title of the game to get the cd cover art. |
|
Any luck on this? |
|
@danifunker hey, I am really sorry for taking my time! So, I investigated this feature in general, and I think it would be great to add it. But I think it would be better to reuse the existing methods for reading tracks. The difference between your implementation and the existing track reading is just command bytes format (CDB) and the size for cooked data (for raw data it is identical, 2352). Reusing the existing functionality would also give us streaming for free. What do you think? |
|
Oh sweet that's awesome. Do you want me to refactor my PR? I can go ahead and prepare something else in a few days or so. |
|
Yeah, go ahead and make the changes (I'll do my best to prioritize reviewing it). The only thing I'd add is another example for a disc with data tracks. |
…ucture Instead of duplicating the retry loop, chunk reader, parse_sense, and next_chunk_size across a parallel data_reader module, parameterize the existing per-platform read functions with SectorReadMode. This removes ~260 lines of duplication and gives streaming support for data tracks for free. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
PR has been updated. Please have a look when you have a chance! |
…apping) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Formatting errors should be resolved now |
Bloomca
left a comment
Bloomca
left a comment
There was a problem hiding this comment.
Looks good, but I think we need to reuse the existing reading function on macOS as well. I left a few comments.
|
|
||
| pub(crate) fn max_sectors_per_xfer(&self) -> u32 { | ||
| match self.sector_size() { | ||
| 2048 => 32, |
There was a problem hiding this comment.
question: Why do we need to limit max sectors?
There was a problem hiding this comment.
It's the per-command cap for READ CD, and it's the only chunker on the blocking read_track / read_data_sectors paths — those hand a whole track (tens of thousands of sectors) straight to the read loop, whereas streaming already limits via sectors_per_chunk. It's not about OS pass-through limits (modern SG_IO/SPTI handle far larger transfers) but about optical-drive firmware / USB-bridge reliability — large multi-sector READ CD requests are flaky across a number of drives out in the wild. The values keep each transfer ~64 KiB, matching the ~27-sector chunk cdparanoia/libcdio use. I've added a comment documenting this. Happy to drop it and route the blocking reads through the streaming chunker instead if you'd prefer a single shared constant.
The function was dead code left from the data-reader refactor; SCSI ioctls access DRIVE_HANDLE inline. On Linux CI (-D warnings) the dead_code lint became a hard error. macOS builds never exercised this cfg path, so it passed locally while CI kept failing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Let me get all these comments going, they will go right into the LLM again. Thanks for going over everything! Also, I'm going to test this on Windows, Linux and Mac so we don't keep getting the build issues. I'll send an update again once I get to all the comments. |
Unify code paths: - Extract the shared READ CD retry/chunk/backoff loop into src/read_loop.rs as read_sectors_chunked(), taking the platform single-command read as a closure. linux.rs, macos.rs and windows_read_track.rs now keep only their platform-specific read_cd_chunk; the duplicated loop and next_chunk_size copies are removed. macOS data reads (PR review Bloomca#4): - Collapse read_cd_audio + read_cd_data into one read_cd_sectors() that maps the read mode to the CD sector area/type and uses the read-only DKIOCCDREAD ioctl for audio, cooked and raw modes alike. - Delete data_reader.c, which used SCSITaskDeviceInterface and required exclusive access (forcing the volume to unmount). open_dev_session now just records the BSD name and validates it by opening the raw device. Document max_sectors_per_xfer as the sole chunker for the blocking read_track/read_data_sectors paths, kept for drive-firmware/USB-bridge reliability rather than OS pass-through limits. Verified fmt/clippy(-D warnings)/test/examples on macOS and Linux. Windows and on-disc data-read behavior (macOS DKIOCCDREAD sector-type mapping) still need hardware verification. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Windows twin of the earlier get_drive_fd removal: read_toc and read_sectors_with_mode access DRIVE_HANDLE directly, so the helper was dead code and tripped the dead_code lint under CI's -D warnings. Also addresses the maintainer's review comment on windows.rs:82. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Ok, I think I've done everything, I just need to grab a data CD and run it through here on my mac to double-confirm. I'm going to use the Green Day American idiot since there is a data track at the end of the disc with the pressing I have. |
2 participants
I can't recall why I made my code changes, but I think it was to support mixed data/audio CDs and have it working correctly for lookups on musicbrainz.
I'm not sure if you want this, but figured I should submit it.
I recently rebased my changes against your branch without any issues as well. I believe this was mostly AI written just a heads up.
I had to resolve the issues with the MacOS build, but it seems to be working now.