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

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

Closed
RecursiveForest opened this Issue Dec 12, 2016 · 15 comments

Comments

Projects
None yet
5 participants
@RecursiveForest
Copy link

RecursiveForest commented Dec 12, 2016

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

This comment has been minimized.

Copy link
Owner

rocky commented Dec 13, 2016

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

@zacklb

This comment has been minimized.

Copy link

zacklb commented Dec 15, 2016

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 added a commit that referenced this issue Feb 5, 2017

Minor changes
Also, see issue #5
@rocky

This comment has been minimized.

Copy link
Owner

rocky commented Feb 5, 2017

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

This comment has been minimized.

Copy link
Owner

rocky commented Feb 5, 2017

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

This comment has been minimized.

Copy link

ghost commented Feb 5, 2017

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 added a commit that referenced this issue Feb 5, 2017

Minor changes
* Move declarations in patch to beginning of the block to support to
  support C language dialects that don't support mixed declaration
* Bump copyright date

Also, see issue #5 (#5)
and https://savannah.gnu.org/bugs/index.php?49831
@rocky

This comment has been minimized.

Copy link
Owner

rocky commented Feb 5, 2017

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

@enzo1982

This comment has been minimized.

Copy link
Collaborator

enzo1982 commented Feb 6, 2017

@ghost

This comment has been minimized.

Copy link

ghost commented Feb 7, 2017

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

This comment has been minimized.

Copy link

ghost commented Feb 22, 2017

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

This comment has been minimized.

Copy link

ArchangeGabriel commented Mar 20, 2017

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

This comment has been minimized.

Copy link
Owner

rocky commented Mar 20, 2017

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

This comment has been minimized.

Copy link

ghost commented Mar 20, 2017

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

This comment has been minimized.

Copy link
Owner

rocky commented Mar 20, 2017

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

@rocky

This comment has been minimized.

Copy link
Owner

rocky commented Mar 26, 2017

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

@rocky rocky closed this Mar 26, 2017

@ArchangeGabriel

This comment has been minimized.

Copy link

ArchangeGabriel commented Mar 26, 2017

@rocky Thanks for the release, we will update whipper to libcdio-paranoia soon whipper-team/whipper#87.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.