Repository navigation
Conversation
Some drives' firmware hands back the Q sub-channel track, index and MSF fields as raw binary instead of the BCD the spec requires. The CRC is written on the disc and is computed over the BCD encoding, so on such a drive every sector fails validation, and pregap detection could only ever end in "unknown (sub-channel CRC mismatches)" -- the search never got off the ground. Re-encode to BCD and re-check before giving up, which recovers those drives; XLD carries a workaround for the same firmware behaviour. Detection is sticky for the rest of one track's search, so the cost is a single extra CRC on the first sector rather than on every sector, and it is kept local to the search rather than on the context so one odd disc cannot poison the next. An all-zero CRC is now treated as invalid: it is what a drive that returns no sub-channel data at all leaves behind, which is an absence of data rather than a sector that happens to check out. Because verification can rewrite the buffer, callers can no longer recompute the CRC themselves -- doing so would re-encode already-encoded fields -- so the read helper reports validity directly and the three call sites use that. Adapted from the sub-channel hardening in upstream PR cyanreg#153. The rest of that PR is not carried: its macOS path calls cdio_get_device_fd(), which is not in libcdio 2.1.0, so it would break the macOS build against current distributions. Disc images resolve pregaps from the TOC, so no fixture can reach any of this. The new unit test drives it on synthetic sectors instead, with a CRC-16/GSM vector computed independently of this code so it pins the polynomial rather than agreeing with itself. Removing the fixup fails 9 of its checks. Still needs verification on real hardware: no disc image can exercise the MMC sub-channel read path, so a drive exhibiting the binary-encoding quirk remains untested end to end. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011rdLYCnFSHEikBZUGTwApu
|
Is it OSX only for now? |
It should work for all OS. I am not sure on windows. I tested it locally on Debian sid and it works (I confirmed with logs from EAC and the pre-gaps are the same). |
Answers "should we rebuild off rc2 or stay on our own line?" with measurements rather than instinct. VERDICT: stay on our line, merge upstream forward when round 12 opens, never rebase, and keep converging on code while diverging on contract. The rebase argument is the decisive one and it is not about effort. 22 of our own SHAs are published and reachable -- 16 release-ledger rows, the manifest pin c4d1a00, every HANDSHAKE-OUR-PIN, and ddf7ac3 which is what Platterpus is running right now. A rebase orphans every one of them, and git gc then destroys them. Every rip ever made also writes its build SHA into a logfile permanently, so rewriting history turns those into claims about builds that no longer exist. Merge rather than cherry-pick, and that answers the deviation worry directly: cherry-picking leaves the merge-base at 958e1ad forever, so every future delta re-presents the same commits and the divergence measurement rots. Merging advances the base. A trial merge conflicts in 3 files of 14, all already analysed, and only 74 of our 300 commits touch src/ at all. THE AUDIT TOOK THREE ATTEMPTS AND THE FIRST TWO LIED: ancestry -- reported 0 of 42 PRs merged, including cyanreg#158 which is demonstrably in master. GitHub squashes and rebases, so a merged PR's head is never an ancestor. whole-tree -- reported 0 of 42 covered, because a PR branch carries its own stale base. added lines -- what is used: literals the PR's diff ADDS, checked against both trees. Answers the actual question. RESULT: 21 PRs have no merge base (ancient, spot-checked as long merged), 17 are fully covered, 4 outstanding -- and we already implement two of them independently: cyanreg#116 improves sample_peak_rel_amp by adding +sample to the ebur128 filter. Ours has "ebur128=peak=true+sample" at cyanrip_encode.c:469; upstream master still has "peak=true" at 466. That is the same defect recorded here as Task 1 (ebu_sample_peak dead field) and the one real finding the failed seven-lane workflow returned. rc2's new hand-rolled Sample peak: line exists precisely BECAUSE their ebur128 sample peak does not work yet. cyanreg#128 adds a "just makecue" option. We have --cue-only (-J); upstream master contains zero occurrences of the PR's string. cyanreg#153 is active as of 2026-08-20 and restructures sub-channel code we carry a divergent copy of. Still not carried, for the recorded reason: it calls cdio_get_device_fd(), absent from libcdio 2.1.0. WHAT COULD NOT BE AUDITED, stated rather than implied: third-party forks. Anonymous git reads of public repos work, but the fork list is an API-only query and the API refuses unattached repositories with HTTP 403. Attaching upstream with push credentials would grant it and is not appropriate for a read-only audit of someone else's repository. Also records the discipline the "so upstream could take it" goal needs: our changes are not currently separable, and nothing lets you point at the upstreamable subset. Labelling as we go is far cheaper than reconstructing it later. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011rdLYCnFSHEikBZUGTwApu
|
I finished cleaning up the algorithm as it was implemented by UltraFuzzy, or at least my understanding of it. Pregaps do not require a previous track and some hidden tracks can only be accessed by rewinding from track 1. The current algorithm will miss them. Do you think it's worthwhile to rewrite the search to avoid using the previous track TOC entirely? |
Nvm. It was supported by returning 0 (ie the start of the disc). Restored. |
|
I'm alive again and interested in finishing up the work to get cyanrip on par with EAC and XLD. Sorry I dropped off, and thanks @nicosp for picking this up! On the discrepancies with XLD: the "retry harder" logic was based on informal testing where I observed a first successful read after ~180 attempts. I was on the fence about including it and I'm completely fine with it being removed. The situation it tries to solve seems fairly unlikely, and the benefit has to be weighed against the risk of generating a spurious CRC validation from a hash collision, although that's also fairly unlikely so weighing the two is a shrug. When there genuinely is a flaky sector that must be read to decide the bounds of a pregap, it might be best to just give up and report an error to the user. Something worth stating and addressing directly: do we want a pregap-finding codepath that 100% reproduces XLD's behavior, quirks included? Users from communities where EAC and XLD are the gold standards would value that but we also want code that's supported by our own reasoning and that we're free to tweak. The two should usually agree, but there are edge cases where the goals are in tension. A good example is XLD's "not a pregap" -> "suspicious" -> "pregap" logic that discards apparently spurious lone
I'd prefer not to blindly copy unexplained idiosyncrasies like that, except when the goal is 100% XLD compatibility. And in the other direction, there are sector consistency checks XLD doesn't do that we may want to do. I'll be out of town and busy next week (until August 30th), but after that this will get a great deal of my attention. |
Happy to pass it to you. My approach was to cleanup the code and follow XLD when I wasn't sure. I added unit tests for various cases. In general, I tried a low risk approach so that we can release this soon. In the meantime I will address any comments for this PR |
|
Trying some more advanced error detection/correction at: nicosp#3 |
An empty short flag string leaves the option with only its --long name. The help output drops the "(-x)" part for such options and pads so the columns still line up. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Run the workflow for pull requests against master and allow starting it by hand. The MinGW job only uploads its artifact and release assets when building master or a release.
Adds .github/static-build.sh, which builds all third-party libraries from pinned release tarballs and links them statically into cyanrip: fully static with musl on Linux, with only the system libraries and frameworks dynamic on macOS. Two new jobs run it on release and pre-release events only: linux-static in an Alpine container producing cyanrip-linux-amd64, and macos-dmg producing an ad-hoc signed cyanrip-macos-arm64.dmg. Both attach their file to the release. Compared to the MinGW build's ffmpeg configuration the pcm_f64le encoder is enabled too, as the raw PCM output needs it. On Linux curl gets no compiled-in CA bundle path, only OpenSSL's default lookup, as a missing bundle would be an error rather than a fallback. libvorbis is built with CMake, as its autotools build hardcodes -force_cpusubtype_ALL which Xcode's linker rejects. libcdio 2.4.0 comes from GitHub, as ftp.gnu.org stops at 2.1.0. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
libcdio only reports pregaps for images, so find them on physical discs by reading the Q sub-channel around each track start and searching for the first sector of the track with index 0. Q frames are checked by their ADR and CRC, drives returning binary instead of BCD fields are handled, and the 2 second lead-in is counted towards the track 1 pregap. Rebased onto master: the track length is now printed with cyanrip_frames_to_duration(). Co-authored-by: Nicos Panayides <nicosp@gmail.com>
Move the sub-channel reads into per-platform backends: MMC READ CD on Linux and Windows, DKIOCCDREAD on macOS using cdio_get_device_fd() instead of libcdio's private structs. The search narrows two bounds between the previous track's start and this one's, confirming each bound with a second read so a single spurious read can't place the pregap. Read failures are retried as many times as XLD does, within an overall failure budget, and the pregap is skipped with a warning instead of asserting. The BCD fixup is only used once a frame matched its CRC with it and none did without. Only index 0 sectors count as pregap, the previous track being a single sector means there is none, and the first track's pregap is the start of the disc so hidden track one audio is kept. tests/pregap_test.c runs the search against a mock disc with read faults, jitter, mode 2 frames and BCD quirks. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Drives hand back the Q sub-channel of a sector a few sectors away from the one asked for, which put the pregap start off by as much. The search still works in terms of the sectors it asks for, but once the bounds meet, the result is taken from the absolute time of the frame the right bound rests on, as cdrdao does. This only happens when the frames on both bounds claim to be neighbours, which shows the drive was off by the same amount for both, and when the result still falls between the two track starts. Otherwise the sector asked for is kept, as before. A correction is logged. The test disc now writes the real absolute time into its Q frames. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
MMC lets a drive leave out the CRC of the formatted Q sub-channel. On such a drive every frame failed the CRC check in both encodings, the failure budget ran out and no pregap was ever found. When the CRC matches neither as is nor after the BCD fixup, and no frame has settled the drive's encoding yet, take a mode 1 frame as good if its absolute time lands within 20 sectors of the one asked for, in whichever encoding fits; the closer one when both do. This leaves the encoding undecided, so on a drive that does supply the CRC the first good frame settles it and ends the fallback. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
With the formatted Q sub-channel the drive checks the CRC itself, and for a sector it can't decode some drives hand back the last frame they did decode, CRC and all. Nothing tells that from a real read, and when it lands on the pregap start the pregap comes out one sector short. Read the 96 raw P-W subcode symbols instead, pick the Q bits out and check the CRC ourselves, so a damaged sector shows up as one. A probe on first use reads a few sectors raw and settles on this mode if the drive accepts the read and most frames carry a valid CRC; the formatted Q is kept otherwise, and on macOS, where DKIOCCDREAD only offers it. In raw mode the frames come straight off the disc, so the BCD fixup and the no-CRC position fallback are switched off. The test disc serves raw frames too, and can stand in for a drive without raw P-W, one returning junk, and one substituting a stale frame. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A sector whose Q frame fails the CRC right at the pregap start left the bounds unable to meet, and the search gave up on the pregap. Read raw, such a frame is typically off by a bit or two: on the disc at hand five of 56 boundaries had one, and all were single bit errors. When only unreadable sectors are left between the bounds, read them raw once more. A single bit error is pinned down by the 16 bit CRC over a frame this short and repaired outright. Beyond that, a frame is used if its absolute time is exactly the sector asked for and it reports one of the two tracks. Such a frame directly above the left bound extends it if it says previous track, one directly below the right bound extends that if it says new track. The formatted Q gains nothing here: the drive substitutes a frame rather than returning the damaged one. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
DKIOCCDREAD offers the 96 raw P-W symbols as kCDSectorAreaSubChannel, next to the formatted Q as kCDSectorAreaSubChannelQ. Share the read between the two and drop the stub that declared raw P-W unsupported; the probe falls back to the formatted Q if a drive rejects the area. Not tested on a macOS machine. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The search runs while the context is set up, before the log file is
opened, so what it had to say about damaged frames, Q skew and failed
searches only reached the terminal. Have it fill in a cyanrip_pregap_info
per track and report from that with the gaps, where it belongs:
3 frame pregap in track 22, found using 1 damaged Q frame (1 repaired), merging into track 21
pregap of track 7 unknown: unreadable sectors at the track boundary
The header gains a line saying how the Q sub-channel was read and how
the raw P-W probe went. The search-time warnings stay on the terminal,
where they carry the LSN and error code.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
pregap.c had grown two halves: reading and making sense of Q frames (CRC, BCD, the raw P-W probe, retries, the position fallback, single bit repair), and the search that uses them. Put the first half in subq_read.c next to the backends it sits on, exporting the three calls the search makes, and leave pregap.c to the search. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
cyanrip_read_audio_subq_sector() and cyanrip_read_audio_subpw_sector() were the same read with a different sub-channel selection and block size, in both backends. Replace them with cyanrip_read_audio_subchannel_sector(), which takes the sub-channel as an enum and the block size from the caller. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The track made by -p N=track was left with start_lsn_sig and end_lsn_sig at zero, so its log entry read "End LSN: 0 (with offset: 32)" for a 32 frame track 0. The file itself was right, as the ripper goes by start_lsn/end_lsn. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Skips the raw P-W probe so the search runs the way it does on a drive without raw sub-channel support, for testing that path on drives that would otherwise pick raw P-W. The log's Q sub-channel line says "raw P-W disabled" when it's in effect. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The macOS sub-channel reader calls cdio_get_device_fd(), which first appeared in libcdio 2.3.0. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
tests/cdrdao_compare.py runs cdrdao read-toc and cyanrip -I on a real disc, turns both into per-track start LSNs and pregap lengths, and prints them side by side, exiting non-zero on any difference. A saved toc or cyanrip output can be reused to skip the slow reads, and --cyanrip-arg passes extra arguments to cyanrip, e.g. --cyanrip-arg=--no-raw-subchannel to compare the formatted Q path. Not a meson test as it needs a drive with a disc in it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3 participants
Pregap detection for physical CDs, continuing @UltraFuzzy's work from #115 (their commits are kept as one commit under their name).
--no-raw-subchannelforces formatted Q.Based on #163 (long-only options) and #164 (static builds); I'll rebase once those are merged.
Tested on an Optiarc AD-7740H: 12 discs, every pregap matches
cdrdao read-tocexcept one frame cdrdao discards for a CRC error and this PR repairs. The Windows build gives the same results under Wine.tests/cdrdao_compare.pydoes the comparison.Not tested: real Windows, raw P-W on macOS, other drive models.
🤖 Generated with Claude Code