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

Adds --force-overread #11

Merged
merged 1 commit into from
Aug 22, 2017
Merged

Adds --force-overread #11

merged 1 commit into from
Aug 22, 2017

Conversation

rocky
Copy link
Collaborator

@rocky rocky commented Apr 26, 2017

@enzo1982 @eshattow Please review and let me know if you have any thoughts or comments on this?
I am trying to get clarification of the origin of this patch from AnwarShah.

Force overreading into the lead-out portion of the disc. This option is
only applicable when using the -O option with a positive sample offset
value. Many drives are not capable of reading into this portion of the
disc and attempting to do so on those drives will produce read errors
and possibly hard lockups.

See:

https://gist.github.com/AnwarShah/560d77f7d7da52553f918f3ecb7401e3

Force overreading into the lead-out portion of the disc. This option is
only applicable when using the-O option with a positive sample offset
value. Many drives are not capable of reading into this portion of the
disc and attempting to do so on those drives will produce read errors
and possibly hard lockups.

See:

https://gist.github.com/AnwarShah/560d77f7d7da52553f918f3ecb7401e3
@rocky rocky requested review from enzo1982 and eshattow April 26, 2017 00:20
@rocky
Copy link
Collaborator Author

rocky commented Apr 26, 2017

@enzo1982
Copy link
Collaborator

enzo1982 commented May 3, 2017

I had a quick look at the patch and have no objections so far.

In general, it's a good idea to not try to overread into the lead-out by default, as most drives do not support it. Such drives might return silence or garbage when trying to read those sectors or they might even lock up completely. Also, in 99.9% of cases those sectors will contain silence anyway and in the remaining 0.1% it won't matter much, 'cause we're talking only about a tiny fraction of a second at the end of the last track of a disc.

I'll take a closer look at it in the next few days, when I have more time.

@rocky
Copy link
Collaborator Author

rocky commented May 3, 2017

In general, it's a good idea to not try to overread into the lead-out by default,

Is that's what's going on here? My vague understanding is that this was done only when option --force-overread is given right?

I'll take a closer look at it in the next few days, when I have more time.

Many thanks.

@enzo1982
Copy link
Collaborator

enzo1982 commented May 4, 2017

Is that's what's going on here? My vague understanding is that this was done only when option --force-overread is given right?

Without this patch, cdparanoia always tries to read into the leadout when a positive sample offset is given.

The patch changes the default behaviour to simply provide null samples for leadout sectors instead of actually reading them. Only when --force-overread is given, it actually tries to read these sectors.

@rocky
Copy link
Collaborator Author

rocky commented May 4, 2017

Thanks for the clarification. My understanding from whipper-team/whipper#146 (comment) is that with no option we give a warning.

Given what you report here a day ago:

In general, it's a good idea to not try to overread into the lead-out by default, as most drives do not support it.

Should we change the warning be an error instead;? And in conjuction, add an option to allow override reading into the these sectors? This would be in addition to the proposed --force-overread?

@JoeLametta
Copy link

Hi, is there any status update regarding this PR?

Thanks,
Joe

P.S. @rocky In case it's going to be merged, could you tag a new release (which includes this change)?

@rocky
Copy link
Collaborator Author

rocky commented Aug 15, 2017

@enzo1982 thoughts ? @JoeLametta If there is no action in a week, I'll merge as is and make a new release.

@rocky
Copy link
Collaborator Author

rocky commented Aug 22, 2017

Haven't gotten any feedback. So I'll take no news as good news. If things break, we'll fix them down the line. But we err on the side of moving forward.

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.

3 participants