search

libcdio / libcdio-paranoia

CD paranoia on top of libcdio
GNU General Public License v3.0
50 stars 38 forks source link

libparanoia reads incorrect track with full paranoia, correct track with -Z and -Y #5

Closed RecursiveForest closed 7 years ago

RecursiveForest commented 7 years ago

This is a duplicate of a bug report I made an savannah. I don't know where the preferable place to report bugs for this project is, and wanted to get as wide coverage as possible.

https://savannah.gnu.org/bugs/index.php?49831

rocky commented 7 years ago

Thanks for the report. Robert Kausch is looking at it and he's the best person to be doing so.

zacklb commented 7 years ago

I ran a bisect on the Xiph.org cdparanoia repository, ripping track 5 of the Roberts album, and found that this particular bug was introduced with the following revision:

$ git bisect good
6a89afab34f3319f926bd217dfd8d68737ac80df is the first bad commit
commit 6a89afab34f3319f926bd217dfd8d68737ac80df
Author: xiphmont <xiphmont@0101bb08-14d6-0310-b084-bc0e0c8e3800>
Date:   Thu Sep 11 20:09:40 2008 +0000

    New cache modelling in place and functioning.

    git-svn-id: https://svn.xiph.org/trunk/cdparanoia@15298 0101bb08-14d6-0310-b084-bc0e0c8e3800

:100644 100644 959b960859991419aaf622f745bbaa2e935fc150 ba03d995ddc2f0a0797d11543e4407b509d4fa31 M  cachetest.c
:100644 100644 97092c5e1bf6bfe877aedb0097fdb71bfd746efe cb625cc5bc540bb17851fa2d53e862f1201273b2 M  cdparanoia.1
:040000 040000 ed42a53c170c99a6728c8bc8f060b11361e702f3 49be449011a46647640db959e66584a61fc83f8a M  interface
:100644 100644 40941e7dc095af144a580d2d1ddccf3241d3977b f4ac812cdc7b2df3cff562c9d455abf9b4a88ea1 M  main.c
:040000 040000 366d21c0493ccd562792fa3a4e0798a301b8ee17 9b2ff5bff68774394d67940f3cc05bc5cf7f88a7 M  paranoia
rocky commented 7 years ago

Only just now have I had the time to look at this. In a branch I've applied both patches as branches pending Robert's decision as to which to go with.

It would be good to get in place a test for this. This is in neither patch, but I do see some references to it in the cd-paranoia bug report.

Do you think you could work up a test for this? Thanks.

rocky commented 7 years ago

The smallest offset patch is is failing: https://circleci.com/gh/rocky/libcdio-paranoia/15

The longest match patch however passes CI.

Note that both patches could not be applied without modification. They were probably made of the cdparanoia source, not the libcdio-paranoia source.

ghost commented 7 years ago

Hi, original author of the patches here. The failing test was because of a small error when applying the patch. I fixed this in #7.

As far as I know, there is no test yet that exposes this bug. I'll try and work something up.

rocky commented 7 years ago

@a10footsquirrel many thanks for the change and thanks in advance for working up a test.

enzo1982 commented 7 years ago

Hi all!

I apologize for not taking a closer look at this earlier. I did not have time to set up a test environment to reproduce this, but took the time today after your mails.

I prefer the smallest offset patch. However, I think there are some minor issues with it:

  1. I think a match might be found at offset -1. In this case, -1 should not be used to signal no match found; it would then go undetected. A better signal value might be LONG_MAX.
  2. Before calling the callback with PARANOIA_CB_FIXUP_EDGE, it should check for min_offset instead of offset (the result will be the same, but it would be easier to read).

BR, Robert

Robert Kausch robert.kausch@freac.org

Am 05.02.2017 um 04:19 schrieb R. Bernstein:

Just now have I had a time to look at this. In a branch I've applied both patches as branches pending Robert's decision as to which to go with.

It would be good to get in place a test for this. This is in neither patch, but I do see some references to it in the cd-paranoia bug report.

Do you think you could work up a test for this? Thanks.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/rocky/libcdio-paranoia/issues/5#issuecomment-277494648, or mute the thread https://github.com/notifications/unsubscribe-auth/AHb36TGGswvL0bvqY9Gg0E1H72MvRUsUks5rZT-sgaJpZM4LLK1e.

ghost commented 7 years ago

Created another pull request for the changes suggested by @enzo1982 (#8).

Still working on the test case, it's a bit harder than expected because the bug really depends on the matching runs starting and ending around specific byte locations. (And I can't work on it that much because of personal life stuff having a higher priority.)

ghost commented 7 years ago

Hi, sorry that it took a while, but I created a pull request with a test case that fails on master and succeeds on both branches that fix the bug.

ArchangeGabriel commented 7 years ago

So, now that #7, #8, #9 and #10 are merged, this should be closed right?

Congrats to @a10footsquirrel for fixing this many years old bug! :)

rocky commented 7 years ago

I think so, but I usually let the reporter of an issue verify and close. But yes, thanks are due to @a10footsquirrel for fixing this many-years-old bug.

(A pity it isn't yet cd paranoia from which this derived, but maybe now this is here that might aid adding it there.)

ghost commented 7 years ago

Thanks, it was my pleasure. Any chance for an official release for this bug fix? I think the whipper (and other projects) will appreciate an updated version in the official repositories of most distributions as soon as possible.

rocky commented 7 years ago

Don't know when I'll have time, but I'll try to make a new release when I'm able.

rocky commented 7 years ago

Pushed libcdio-paranoia-10.2+0.94+1.tar.gz to ftp.gnu.org and tagged release-10.2+0.94+1

I am not sure this change will get reflected in whipper since it uses the stock cdparanoia

ArchangeGabriel commented 7 years ago

@rocky Thanks for the release, we will update whipper to libcdio-paranoia soon https://github.com/JoeLametta/whipper/issues/87.