Incorrectly fetches track length with certain offsets #14
Comments
|
I'm not going to have time to look at this. Perhaps @enzo1982 @eshattow or someone else does. If cdparanoa works a wild guess is that one of the options that was added more recently broke this (although it fixed whatever else it was addressing). So you could try with one of the older releases or better use git bisect to find the problem. |
|
So, I tried git bisecting for a bit, but then I thought that I should've just started from the earliest "compilable" commit, and I did, this is the earliest I got it to compile: And the same -1 nonsense appears (the sector number is different because I tried out another disk). But yes, this still works on Xiph's: |
|
Ok - this is useful information. Notice that in the earlier libcdio version you are not getting the message "Time/sector offset goes beyond end of specified track." I'm not happy about the fact that the error message doesn't indicate what the time/sector offset is nor where the end of the specified track is, but I guess we'll work with that for now. Rethinking things more carefully, I kind of see how this might happen. cdparanoia is Linux specific in its I/O while libcdio isn't. So probably at the I/O level libcdio in the earlier version we are getting different results. In the later libcdio version it probably correctly says that the sample offset is not valid and then refuses to read it. If you give a different offset that libcdio thinks is valid does this work? Fixing this may be difficult if the problem is drive specific. |
|
Actually, I'm not getting Regarding the offset, yes, giving no offset or some other (specific) offsets does work, the program isn't completely broken in this regard. Just that when giving this particular offset (which happens to be the correct one) it breaks and so does some other ones, which I will need to run more lengthy tests to enumerate. But yea, it does seem to be a bit "driver specific", although on the other thread several different models were affected, but as far as I see, all of them work with a big offset such as this one (+667). |
|
Ok, thanks. If you are up to it, would you change that error messge to include what it expected and what was seen instead? That too might point to clues. |
|
Yep, I changed it. An offset that works: And one that doesn't: These lines added: if (minutes != -1) ret += minutes*CDIO_CD_FRAMES_PER_MIN;
if (hours != -1) ret += hours *60*CDIO_CD_FRAMES_PER_MIN;
+ report("d == %d", d);
+ report("ret == %d", ret);
+ report("cdda_sector_gettrack(d, ret) == %d", cdda_sector_gettrack(d, ret));
+ report("i_track == %d", i_track);
/* We don't want to outside of the track; if it's relative, that's OK... */
if( i_track != CDIO_INVALID_TRACK ){
if (cdda_sector_gettrack(d,ret) != i_track) {
report("Time/sector offset goes beyond end of specified track.");
exit(1);
}
} |
|
@fer22f Hi, what happens if you repeat the test with the following offsets? (587, 588, 589) A frame/sector in a CD-DA is made of 588 pairs of left and right samples. The error may be related to this (arbitrary guess), in that case those would be the expected results:
Cheers, |
|
@JoeLametta Just tried it out, it works exactly as you described. Baseline:
|
|
Ok. I'm lost. What does this tell us - that the two bugs are the same? Does this suggest how to fix the problem? |
Unfortunately I'm not sure either: maybe that the offset of the drive isn't taken into consideration when determining the track layout (sectors it spans). |
|
@rocky Something strange happens with CDs having 99 tracks: whipper-team/whipper#302. Same error message too. |
|
Here's a rough idea of what I think is going on. A CD can hold at most 99 tracks If this jitter thing access outside of that then that can cause an error. So probably we need a test here. I don't think I will get to this anytime soon, but I am hoping someone else will. |
|
Not sure if I ran into the same issue. My Lite-On drive needs a correction of "+6" . That works fine. I then tried "-24" to cope with the 30 samples offset-issue of Accurate Rip. Basically all tracks of a CD ended up at 140 bytes and only the last one was completed. With the standard cdparanoia all is fine. Do you guys think this is related somehow? |
|
Slightly off-topic: Since 30 samples offset-issue of Accurate Rip is a
problem with implementation of software other than libcdio, you may try
Python Audio Tools to run verification with a wider window of computation.
The most important metric is to detect an audible error in extraction and
the first or last samples are almost certainly not perceptible; also as
you might be concerned about is the value of automation to do this
verification compatible with most other implementations. Python Audio Tools
can verify AR1 and AR2 signatures within a window that matches the range of
what most optical drives' offset values could be. Note I packaged Python
Audio Tools for Debian and while the author was active initially I have not
heard from them in some years. If you or someone you may know has interest
in refining Python Audio Tools please keep me informed so I may update the
Debian packaging.
Back to topic: I too had some troubles with libcdio-based extraction
compared to what is widely in use (dbPowerAmp). The algorithm itself (at
least AR1) does ignore these very first and very last samples. For me it
does not matter but perhaps there is a usability bug that the output is not
the same as i.e. dbPowerAmp; however we're not likely to want integration
with software-as-a-service Accurate Rip as part of libcdio so the issue of
submitting IDs to the database is moot. What does it matter if the offset
is slightly off other than compatibility with proprietary software? There
may be a technical discussion here about trying to get over-read and
under-read to work as closely to correct as possible but it is not directly
related to Accurate Rip.
…On Tue, Jun 11, 2019 at 10:01 AM soundcheck ***@***.***> wrote:
Not sure if I ran into the same issue.
My Lite-On drive needs a correction of "+6" . That works fine. I then
tried "-24" to cope with the 30 samples offset-issue of Accurate Rip.
Basically all tracks of a CD ended up at 140 bytes and only the last one
was completed.
With the standard cdparanoia all is fine.
Do you guys think this is related somehow?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#14>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AC5GJDS5B7XP4CCLZANJDQ3PZ7K6RANCNFSM4FISVCHQ>
.
|
|
Not that I am aware of. Hey, this is open source so anyone can play - are you interested in investigating and fixing? |
|
I'm interested in investigating and fixing it (whipper relies heavily on cdparanoia) but I'm quite time starved and not particularly confident both with C programming and libcdio's codebase. To debug this I'd start comparing libcdio's implementation of cdparanoia with xiph's one (which seems unaffected by this bug): do you think this is a good idea? |
|
If you are comparing using a debugger at run-time, or adding debug output (or turning some on), it probably could be done. Comparing the source level though may be hard because there have been a number of changes that folks wanted and cdparanoia's programming style is a bit unlike any other that I've seend except in obfiscated minimized C code contests. As a result, comparing at the source-code level there will be many inconsequential or purely style changes. If you lead the way in debugging and bug detection, I may be able to help. |
|
If someone has a 667 drive, maybe try this patch. It makes (Apply with It's not entirely correct (587 should probably be 588), but I am interested to know if it breaks accuraterip output, or ripping of the last track on drives that support overread. cc @JoeLametta |
|
@MerlijnWajer - Thanks for undertaking this. You also might get help if you also mention this on the libcdio-devel mailing list https://lists.gnu.org/mailman/listinfo/libcdio-devel |
|
@MerlijnWajer , I tried you patch against 55e6604 , I have a +667 drive (LG GSA-H62N). Also tested mtdcr's patch (whipper-team/whipper#234 (comment)) Results: [EDITED 2019/12/30] |
|
Thank you for trying - can you explain what exactly the behaviour is that you're seeing? It looks different to me, but maybe the new effect is the same. Does it rip the tracks? Do they have different checksums? What if you rip all the tracks with the offset? |
|
So in my case, I'm here because of the whipper frontend ( whipper-team/whipper#234 ) . cd-paranoia on its own seems to be able to rip the tracks (although with a few bytes different every time, but that's a seperate issue with my drive/media). But the reported bounds throw off whipper ; as I understand [EDIT : I understood wrong, this is not the case] it subtracts the two timecodes to determine the track length, so the '-1' breaks that and whipper fails to rip. |
|
@fenugrec I am okay with extending the libcdio-paranoia behavior is to something that is more helpful to whipper, if it is the case that important lost information is happening inside libcdio-paranoia (as opposed to whipper interpreting the existing data reported back in a better way). It is kinder to existing applications to extend libcdio-paranoia than change the current behavior, unless the current behavior has a bug that is unfixable in its current state. |
|
Hi,
Whether or not current behaviour is acceptable is debatable; some arguments :
No idea if other existing apps expect different behaviour, and that would be a valid point for your consideration. I'm really not informed enough to say more about this. |
|
@TheMuso Hi, is there any progress about this issue? |
|
On Thu, Sep 10, 2020 at 06:22:33PM AEST, JoeLametta wrote:
@TheMuso Hi, is there any progress about this issue?
Sorry, Other things have gotten in the way since I looked into this. I will try and make some time in the next couple of weekends or so to work on the fix.
Luke
|
Thanks for the status update: didn't intend to put pressure on you 😉 |
|
@TheMuso Kindly asking again for a status update, thanks! |
|
I'm sorry, I mentally made a note to reply to this, and forgot.
Not sure when I'll have the time to work on this. Someone is welcome to work on it if they wish.
|
|
Hi. I have put a small bounty on this bug: https://www.bountysource.com/issues/60536098-incorrectly-fetches-track-length-with-certain-offsets for anyone interested in solving this. |
|
I added $15 to that, so it stands at $115
I don't know when I'll have time to try whipper again but I'd like it to
when when I next have got time
Sam
…On Fri, 14 May 2021 at 12:20, 45054 ***@***.***> wrote:
Hi. I have put a small bounty on this bug:
https://www.bountysource.com/issues/60536098-incorrectly-fetches-track-length-with-certain-offsets
for anyone interested in solving this.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#14 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABL7PTHFKLB3VONNAC4DKLTTNUBOPANCNFSM4FISVCHQ>
.
|
As far as I have understood you've made already good progress. Wouldn't it make sense for others to build upon your work? Can you make your branch available? |
|
I've added another $50 so it stands at $165 |
|
Although the Bountysource currently states 165$ I have to add that my bounty of 100$ has expired and was paid back after some mail exchange (the automatic process does not work apparently). Anyone fixing the bug will still get the 100$, we will find a way. Bountysource however does not appear to be very reliable anymore (see bountysource/core#1539) |
|
Any update on this issue? |
|
It looks like there isn't anyone interested on working on this even for the bounty listed above. But, hey, this is open source code so anyone can dive in and fix. Hey, how about you? |
|
Interestingly just last week I was (trying to) rip some CDs for the first time since I last posted earlier up, ran into the same issue again, and remembered this ticket. I still have that drive and could find time to try patches, but no time for any coding. |
No one is born being familiar with the concept of offset on CD drives. But as Richard Feynman has said: "What one fool can learn, so can another". |
|
I think the challenge here lies into understand:
Perhaps if someone could come up with a C snippet that would demonstrate the incorrect output for a given set of inputs as test case, it would make the journey a little simpler. If the issue is only reproable with a physical reader, it makes the troubleshooting much more difficult... In my professional life, I often had to do such archeological work (what did the original developer intended? Can I make the original code work with a modern compiler and use that as reference?) and incrementally study the drift in time to understand what the later changes did. Of course, it depends how much time someone is willing to invest to fix such issue. If it was easy to fix, it'd be already fixed... |
How about instrumenting a few key API calls and logging arguments and return values ? Probably more useful than an strace log . Maybe 'rr' could be used as well ? |
|
I believe I have a fix for this issue #38. If others with different drive offsets can help test (mine is +667), I'd like to hear your results. |
|
I ran Ripping a CD also worked successfully. 👍 |
|
Looks great. It would be great to get confirmation from as many of the below as possible.
Thanks. |
|
Thanks for helping with another validation @mdosch. Looks like things are working for drives with the following offsets:
If anyone has a drive with an offset less than 1 sector (<+588) or greater than 2 sectors (>+1176), I'd love to hear your results. Also (they seem to be much rarer) if anyone has a drive with a negative offset, that would be interesting to see as well. |
|
I'm not sure what I needed to do to trigger bad behavior previously but it seems to work through all the listed tests on a disc in a drive with offset +702. I did try going through the |
|
Thanks for testing @aereaux. I'm assuming you are using libcdio-paranoia via Whipper here... As you have a drive with >1 sector sample offset, you just need to attempt to rip a disc with your +702 read_offset set in whipper.conf (or specified via the cmdline via I suspect that your To find your drive sample offset, I believe Whipper simply runs through multiple extraction attempts with different offsets and compares the results with the AccurateRip database. If there's a match, it attempts a few more tracks with the current offset and if they are all accurate, it considers your offset discovered.
Can you try via In summary, libcdio-paranoia shouldn't attempt to actually read into the disc lead-in or lead-out unless --force-overread is specified. Right now that is not the case for lead-in when a negative offset is specified. This should be treated as a separate bug and I did not attempt to address this. |
|
Yeah, it was -472 where it hanged and doing In short my tests seemed all good for this. Thanks for doing the work on it! |
|
Awesome, seems to work perfectly - tested on drive
@buddyabaddon very well done. |
|
Ok - thanks everyone for the information! I didn't want to belabor this, but I did want to get some sense of what is up. Again @buddyabaddon - thanks. Let's merge and if there is more you can iterate. |
I've seen a number of people running into the same issue when using whopper (whipper-team/whipper#234), and one user going as far as just removing errors checks in the source code to force cd-paranoia to continue working (whipper-team/whipper#234 (comment)).
My hardware is HL-DT-ST GH22NS50, and in the AccurateRip database, it is expected that its read offset is +667. However, when using that number with cd-paranoia, it stops saying that the time/sector goes beyond end of my specified track:
However, Xiph's cdparanoia works ok:
And following the advice to leave out the time argument to cd-paranoia to guess, I end up with a very weird time signature indeed (but the sector number does match and the track is correctly ripped):
The text was updated successfully, but these errors were encountered: