Check for lsn errors on span processing to avoid issues with track 0 on CDs without hidden track

[lazypingu]

This PR adds a check for lsn errors (e.g. no initial pregap orinvalid track number) on the span processing of cdparanoia. This happens before the lsns are used to find the respective tracks. This should avoid infinite loops if track 0 is asked for ripping on a CD without a hidden track. This now results in a 402: No initial pregap error.

Some additional tests where added, which should cover various cases for different CD types. The bin files are just taken from cdda.bin. Only the corresponding cue files where adapted according to the CD type.

[rocky]

Looks excellent to me!

@buddyabaddon you had mentioned some additional problems. Was this one? Do you have any comments regarding this PR?

[buddyabaddon]

My concern would be with drives having positive offsets not being able to read the true data in the first sector (as much as possible). If --force-overread is specified, I would expect a negative sector index to be allowed (drive behavior may vary). If a drive has a positive offset > a sector's worth of samples, it should be allowed to attempt reading before sector 0. If --force-overread is NOT specified, I would expect the data in non-existent sectors to be padded with zero's.

This change seems to never allow negative sector indices and instead errors out.

[buddyabaddon]

@lazypingu what's your drive's read offset?

[lazypingu]

@buddyabaddon Hm, according to the Accuraterip table, my drive (ASUS SDRW-08D2S-U BA01) seems to have no constant offset ([purged]).

Your concerns sound resonable. The errors codes here are in the range of [-403; -400]. We could explicitly filter for them, but this will not elimite the possibility of false positives.

Would it make sense to take the offset into account if an negative lsn is recognized?

Otherwise we could also think about other error code ranges (e.g. unreachable lsn values).

[buddyabaddon]

Here's the scenario I have in mind. Let me know what you think.

Say you have one of the drives on the Accuraterip CD Drive Offsets list requiring a negative read sample offset (sorry, I said positive in my earlier reply). With such a drive, you would need to specify --sample-offset=-N to get accurate rip results because whenever this drive is instructed to read sector M, it actually returns sector M + N samples later, hence all read results are biased by -N samples.

As I understand it, negative sector lsn should be allowed, if

• a drive requires a negative read sample offset -and-
• the --force-overread option is specified -and-
• the requested track starts at sector 0

Example: Say you have a drive requiring a -1164 read sample offset.

In order to start reading the first real audio sample of the CD with such a drive, you need to instruct it to read sector lsn -2 (-2*588=-1176 samples) and skip in +12 samples of the returned data (-1176+12=-1164).

• toc_offset = -2
• sample_offset = 12

Now, it sounds like few (if any) drives will actually allow read attempts outside the user data area of the disc; hence the --force-overread option...

If the --force-overread option IS specified, I would expect libcdio-paranoia to attempt to read the disc at a negative sector index. In this case, it's up to the drive as to whether or not this succeeds, causes an error or lockup, etc. - all risks of using the --force-overread option.

If the --force-overread option is NOT specified, I would expect libcdio-paranoia to NOT attempt to read prior to lsn sector index 0 nor after lsn sector index LAST for the disc. In this case the first 1176 samples of audio data that can't actually be indexed with such a drive would simply be padded with 0's in the output (libcdio-paranoia does not appear to do this today at the beginning of tracks but it does appear to do this at the end of tracks).

For this change, it may simply be sufficient to add a check for the --force-overread option and allow for a negative i_first_lsn in such cases. Later, the padding logic I describe can be attempted for such cases where the --force-overread option is omitted but the other conditions are present (a drive with a negative sample read offset and a track starting at lsn 0) - essentially pad the output with N empty samples for a -N sample offset drive reading from lsn 0.

Or maybe I'm overthinking this as most drives seem to have positive offsets... or the --force-overread option was only ever meant to be used for over-reading at the end of a track...

Now I'm curious what the old cdparanoia or EAC does in such a case.

[lazypingu]

Thanks for the explanation!

Hm, I've added the lsn check after the span parsing and before the actual offset adaptations. The intention was to filter out error cases such as -402 no pregap before they are passed to the cdda_sector_gettrack function.

If I got this right, the cdda_track_firstsector and cdda_track_lastsector functions will just return the lsns specified in the TOC of the CD. So these lsns are at this point not affected by any offset shift of the drive, right? In this case, negative offsets may just indicate errors, which should be fine to filter them out at this point.

But if this is not the case and the returned lsns form cdda_track_*sector could be negative, this might also affect the logic of the functions itself, since e.g. the pregap error might not correct if the start sector has an offset. If this is the case I guess more adaptations (including your thoughts) are required to cover those siturations.

[lazypingu]

I came across a snprintf truncation compiler warning at the outfile_name construction and though I could fix this in this PR. I've changed the code to verify the snprintf result and exit on error or truncation. Hope that's OK.

[rocky]

If I got this right, the cdda_track_firstsector and cdda_track_lastsector functions will just return the lsns specified in the TOC of the CD.

I believe this is correct.

[rocky]

I came across a snprintf truncation compiler warning at the outfile_name construction and though I could fix this in this PR. I've changed the code to verify the snprintf result and exit on error or truncation. Hope that's OK.

That is okay.

@buddyabaddon Your thoughts? We good to merge?

[buddyabaddon]

So these lsns are at this point not affected by any offset shift of the drive, right?

Ya, if you're making your boundary checks before the sector offsets are applied, this sounds fine to me.

[rocky]

Thanks @lazypingu. Thanks @buddyabaddon.

[lazypingu]

@rocky Just to mention: I saw that the pipeline failed after merging this PR. I've tested it locally and the tests passed. Not sure, but I might have missed something.

[rocky]

Thanks for mentioning. Yes, I see the CircleCI failure. I will investigate what's up with CircleCI later. (I have a number of projects all in need of attention).

I too have tested this locally without problem.

[rocky]

I looked at this more. The situation is that on these servers they do not have a CDROM drive and image testing here is kind of silly, but the actual message is about not being able to find the image to test on.

I'll figure something out to do here.