Skip to content

Add pregap detection for physical CDs. - #115

Open
UltraFuzzy wants to merge 12 commits into
cyanreg:masterfrom
UltraFuzzy:cd-pregap-detection
Open

Add pregap detection for physical CDs.#115
UltraFuzzy wants to merge 12 commits into
cyanreg:masterfrom
UltraFuzzy:cd-pregap-detection

Conversation

@UltraFuzzy

@UltraFuzzy UltraFuzzy commented Sep 27, 2025 •
edited
Loading

Copy link
Copy Markdown

This one still needs a little more time in the oven so that I can add error checking with the Q subchannel CRC16s but I wanted to get the ball rolling on the pull request.

I wrapped the cdio internal structs needed on macOS in an ifdef like we discussed.

Also, libcdio says they'll accept a pull request for a function to get the underlying file descriptor from a CdIo_t which would eventually allow the macOS implementation to be hack-free (libcdio/libcdio#35). How do you want to handle that? Can we go ahead for now with the hacky macOS implementation? Or would you prefer to wait for me to submit a pull request with libcdio and get it merged?
@UltraFuzzy

UltraFuzzy commented Sep 30, 2025 •
edited
Loading

Copy link
Copy Markdown
Author

Related issue and pull request I opened with libcdio so that the macOS implementation doesn't have to use libcdio private structs:

libcdio/libcdio#35
libcdio/libcdio#37
@UltraFuzzy

UltraFuzzy commented Oct 1, 2025 •
edited
Loading

Copy link
Copy Markdown
Author

I was struggling with why everything seemed fine except for the track 1 pregap lengths. Apparently it's standard practice to count the 2 second lead-in towards the track 1 pregap length even though the lead-in doesn't usually get included when the track 1 pregap is dumped as an HTOA.

For example, XLD hard-codes in 2 bonus seconds for track 1 pregap here:
https://sourceforge.net/p/xld/code/HEAD/tree/trunk/XLD/XLDCDDAResult.m#l548

I suspect EAC is doing the same but I don't have any way of confirming that.

I've similarly adjusted the log writer to give track 1 a fixed 2 seconds extra pre-gap duration. I've been told that https://github.com/superg/redumper has a more rigorous solution that can read into the lead-in and search for non-zero audio data or account for rare cases of extended lead-in if we ever want to go that route.
cyanreg
cyanreg reviewed Oct 2, 2025
View reviewed changes
Comment thread src/pregap.c
Comment thread src/pregap.c
Comment on lines +10 to +140
#ifdef __APPLE__
#include <IOKit/storage/IOCDTypes.h>
#include <IOKit/storage/IOCDMediaBSDClient.h>
#endif


// TODO The implementation for macOS requires access to private internal cdio
// data, namely p_cdio->env.fd. Right now we just copy-paste some struct
// definitions to work around this. Is there a less brittle way to do do this? :-/

#ifdef __APPLE__

// lib/driver/mmc/mmc_private.h
typedef driver_return_code_t (*mmc_run_cmd_fn_t)
( void *p_user_data,
unsigned int i_timeout_ms,
unsigned int i_cdb,
const mmc_cdb_t *p_cdb,
cdio_mmc_direction_t e_direction,
unsigned int i_buf, void *p_buf);

// lib/driver/cdio_private.h
typedef struct {
driver_return_code_t (*audio_get_volume)
(void *p_env, /*out*/ cdio_audio_volume_t *p_volume);
driver_return_code_t (*audio_pause) (void *p_env);
driver_return_code_t (*audio_play_msf) ( void *p_env,
msf_t *p_start_msf,
msf_t *p_end_msf );
driver_return_code_t (*audio_play_track_index)
( void *p_env, cdio_track_index_t *p_track_index );
driver_return_code_t (*audio_read_subchannel)
( void *p_env, cdio_subchannel_t *subchannel );
driver_return_code_t (*audio_resume) ( void *p_env );
driver_return_code_t (*audio_set_volume)
( void *p_env, cdio_audio_volume_t *p_volume );
driver_return_code_t (*audio_stop) ( void *p_env );
driver_return_code_t (*eject_media) ( void *p_env );
void (*free) (void *p_env);
const char * (*get_arg) (void *p_env, const char key[]);
int (*get_blocksize) ( void *p_env );
cdtext_t * (*get_cdtext) ( void *p_env );
uint8_t * (*get_cdtext_raw) ( void *p_env );
char ** (*get_devices) ( void );
char * (*get_default_device) ( void );
lsn_t (*get_disc_last_lsn) ( void *p_env );
discmode_t (*get_discmode) ( void *p_env );
void (*get_drive_cap) (const void *p_env,
cdio_drive_read_cap_t *p_read_cap,
cdio_drive_write_cap_t *p_write_cap,
cdio_drive_misc_cap_t *p_misc_cap);
track_t (*get_first_track_num) ( void *p_env );
bool (*get_hwinfo)
( const CdIo_t *p_cdio, /* out*/ cdio_hwinfo_t *p_hw_info );
driver_return_code_t (*get_last_session)
( void *p_env, /*out*/ lsn_t *i_last_session );
int (*get_media_changed) ( const void *p_env );
char * (*get_mcn) ( const void *p_env );
track_t (*get_num_tracks) ( void *p_env );
int (*get_track_channels) ( const void *p_env, track_t i_track );
track_flag_t (*get_track_copy_permit) ( void *p_env, track_t i_track );
lba_t (*get_track_lba) ( void *p_env, track_t i_track );
lba_t (*get_track_pregap_lba) ( const void *p_env, track_t i_track );
char * (*get_track_isrc) ( const void *p_env, track_t i_track );
track_format_t (*get_track_format) ( void *p_env, track_t i_track );
bool (*get_track_green) ( void *p_env, track_t i_track );
bool (*get_track_msf) ( void *p_env, track_t i_track, msf_t *p_msf );
track_flag_t (*get_track_preemphasis)
( const void *p_env, track_t i_track );
off_t (*lseek) ( void *p_env, off_t offset, int whence );
ssize_t (*read) ( void *p_env, void *p_buf, size_t i_size );
int (*read_audio_sectors) ( void *p_env, void *p_buf, lsn_t i_lsn,
unsigned int i_blocks );
driver_return_code_t (*read_data_sectors)
( void *p_env, void *p_buf, lsn_t i_lsn, uint16_t i_blocksize,
uint32_t i_blocks );
int (*read_mode2_sector)
( void *p_env, void *p_buf, lsn_t i_lsn, bool b_mode2_form2 );
int (*read_mode2_sectors)
( void *p_env, void *p_buf, lsn_t i_lsn, bool b_mode2_form2,
unsigned int i_blocks );
int (*read_mode1_sector)
( void *p_env, void *p_buf, lsn_t i_lsn, bool mode1_form2 );
int (*read_mode1_sectors)
( void *p_env, void *p_buf, lsn_t i_lsn, bool mode1_form2,
unsigned int i_blocks );
bool (*read_toc) ( void *p_env ) ;
mmc_run_cmd_fn_t run_mmc_cmd;
int (*set_arg) ( void *p_env, const char key[], const char value[] );
driver_return_code_t (*set_blocksize) ( void *p_env,
uint16_t i_blocksize );
int (*set_speed) ( void *p_env, int i_speed );
} cdio_funcs_t;

// lib/driver/cdio_private.h
typedef struct {
uint16_t u_type;
uint16_t u_flags;
} cdio_header_t;

// lib/driver/cdio_private.h
struct _CdIo {
cdio_header_t header;
driver_id_t driver_id;
cdio_funcs_t op;
void* env;
};

// lib/driver/generic.h
typedef struct {
char *source_name;
bool init;
bool toc_init;
bool b_cdtext_error;
int ioctls_debugged;
void *data_source;
int fd;
track_t i_first_track;
track_t i_tracks;
uint8_t u_joliet_level;
iso9660_pvd_t pvd;
iso9660_svd_t svd;
CdIo_t *cdio;
cdtext_t *cdtext;
track_flags_t track_flags[CDIO_CD_MAX_TRACKS+1];
unsigned char scsi_mmc_sense[263];
int scsi_mmc_sense_valid;
char *scsi_tuple;
} generic_img_private_t;

#endif

@cyanreg cyanreg Oct 2, 2025

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Could you move everything under the APPLE ifdef into pregap_internal_osx.c, and everything else into pregap_internal_mmc.c. Then add a pregap_internal.h which provides the signature for a generic get_lba_index(). pregap.c can just retain the cyanrip_get_track_pregap_lba() function.
There's so little code shared. Just make meson switch between which version to compile based on the platform.
Comment thread src/pregap.c Outdated
@UltraFuzzy
UltraFuzzy force-pushed the cd-pregap-detection branch 3 times, most recently from 9bba1a1 to 8ad62fe Compare October 7, 2025 18:55
@UltraFuzzy

UltraFuzzy commented Oct 20, 2025

Copy link
Copy Markdown
Author

Just a quick note that libcdio accepted my pull request that will allow us to avoid the hacky part of the macOS implementation. They say they want to do a new release by the end of the year.

libcdio/libcdio#37

If it ends up that the new libcdio release with that fix comes out before we put out a new release then I'd say we could just ditch the hacky part of the macOS implementation. I don't think there's much need for us to support old versions of libcdio on macOS when we don't even currently have a cyanrip macOS package.
rmccann-hub pushed a commit to rmccann-hub/cyanrip that referenced this pull request Jul 31, 2026
@claude
Carry upstream PR cyanreg#115 pregap-LSN detection, with 3 bugs fixed
aaa0892 
cyanrip only trusted the disc's TOC (cdio_get_track_pregap_lsn()) to say
where a track's pregap starts. Some discs' TOCs don't declare a pregap
that physically exists -- the same gap EAC's own detection pass closes.
Upstream PR cyanreg#115 (UltraFuzzy, still open/unmerged) adds a fallback that
reads Q sub-channel data directly via MMC READ CD and binary-searches for
the actual boundary when the TOC doesn't know.

This fork's INDEX 00/PREGAP cue-sheet *reporting* was already complete
(via separately-merged upstream PRs cyanreg#104/cyanreg#118/cyanreg#122); this only adds the
missing detection half.

Carried the PR's algorithm as designed, but fixed three defects found
during review before integrating, all detailed in
PREGAP-DETECTION-CARRY.md:

- The failure path returned DRIVER_OP_ERROR (-1) as the lsn_t result
  instead of CDIO_INVALID_LSN (-45301) -- every existing caller in this
  codebase only checks against the latter, so a failed detection would
  have been read as a genuine pregap at LSN -1, corrupting cue-sheet
  output in exactly the highest-risk area of this whole feature set.
- A buffer leak on the "confirmed no pregap" early-return path.
- Three assert(!ret) sites treated routine drive-read failures (which the
  PR's own comments say can take up to ~180 retries to succeed) as
  programmer-logic violations, aborting the entire rip in progress on any
  persistent I/O hiccup. Converted to a graceful CDIO_INVALID_LSN
  fallback, matching the "no pregap known" behavior from before this
  feature existed.

Also dropped a dead debug-only include and a no-op #undef left over from
the original PR, and switched malloc/free to this codebase's own
av_malloc/av_free convention.

Verified in this environment: clean build (0 warnings), full test suite
(12/12), and a full before/after log + EAC CRC32 diff across three
fixture types confirming CRCs are byte-identical -- this only changes
pregap metadata, never audio. The actual sub-channel-reading algorithm
cannot be exercised here (no physical drive, and the synthetic test
images never leave libcdio's own TOC-based detection short of an answer
except for track 1's unconditional case). PREGAP-DETECTION-CARRY.md spells
out exactly what remains unverified and how to validate it on real
hardware.
rmccann-hub pushed a commit to rmccann-hub/cyanrip that referenced this pull request Aug 1, 2026
@claude
Don't enumerate zero length pregaps in the gap report
12127d2 
Carrying PR cyanreg#115 made track 1's pregap LSN the disc start instead of
CDIO_INVALID_LSN, so the gap enumeration -- which only skipped the invalid
sentinel -- began reporting "0 frame pregap in track 1" on discs that
signal no pregaps at all, where it previously reported "None signalled".

A zero length pregap is not a gap: there is nothing to drop, merge or
split off, and every branch of the action switch below is a no-op for it.
It also contradicted the per-track block, which reports track 1's lead-in
as a 150 frame pregap on the same disc.

Skip them. Genuine pregaps, including a real non-zero pregap on track 1,
are still enumerated exactly as before -- verified against the pregap and
NRG image fixtures, whose 150 and 75 frame pregaps are unaffected. Cue
sheets are byte-identical across all four fixture types, so this only
changes the human/parsed log text, not the generated cue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants

@UltraFuzzy @cyanreg