Re: Bug report: paranoia mode reads incorrect track, -Z reads correct track

--===============8106023987868604048==
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="DKU6Jbt7q3WqK7+M"
Content-Disposition: inline


--DKU6Jbt7q3WqK7+M
Content-Type: multipart/mixed; boundary="Nq2Wo0NMKNjxTN9z"
Content-Disposition: inline


--Nq2Wo0NMKNjxTN9z
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Aug 10, 2016 at 06:43:44PM -0400, [email protected] wrot=
e:
> While working on some software based on cdparanoia-III-10.2[0], I have=20
> discovered what I think is a bug in cdparanoia:
>=20
> Writing a wav[1] with md5sum 3b265ba06de8cd27f200e79447c58035 to a disc=
=20
> and then ripping in default full-paranoia mode will result in a track=20
> with incorrect md5sum 063fa2d2df295f7dbfad8d75ce0aeafa -- however,=20
> ripping with paranoia disabled (-Z) will yield the correct track rip.[2]

A developer who wishes to remain anonymous has contributed a bugfix,
with two alternative implementations:

> Pretty sure I fixed the cdparanoia bug. It actually wasn't in a silence
> block. There were 2 identical 109 samples of data in the track right
> near each other. cdparanoia was matching the second one with the first
> one and saying it was jitter.

> This fix doesn't lose out on any error correcting capablities. In the
> case of multiple matching runs it picks the one with the smallest
> (absolute value) offset (diff 1) or the one with the longest match (diff
> 2). Since multiple matching runs should be extremely rare, the impact of
> this change on other rips should be almost nonexistent. Both diffs fix
> the Julie Roberts track 5 bug.

I have attached the two diffs.

--=20
Samantha Baldwin - logik.li

--Nq2Wo0NMKNjxTN9z
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="cdparanoia-longest-match.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/paranoia/paranoia.c b/paranoia/paranoia.c
index 07b0322..dd17099 100644
--- a/paranoia/paranoia.c
+++ b/paranoia/paranoia.c
@@ -863,6 +863,10 @@ static long i_iterate_stage2(cdrom_paranoia *p,v_fragm=
ent *v,
      */
     sort_setup(i,fv(v),&fb(v),fs(v),fbv,fev);
=20
+    long best_matchbegin =3D -1;
+    long best_matchend =3D -1;
+    long best_offset =3D -1;
+
     for(j=3Dsearchbegin;j<searchend;j+=3D23){
=20
       /* Skip past silence in the root.  If there are just a few silent
@@ -894,27 +898,34 @@ static long i_iterate_stage2(cdrom_paranoia *p,v_frag=
ment *v,
        */
       if(try_sort_sync(p,i,NULL,rc(root),j,
 		       &matchbegin,&matchend,&offset,callback)){
-=09
-	/* If we found a matching run, we return the results of our match.
-	 *
-	 * Note that we flip the sign of (offset) because try_sort_sync()
-	 * returns it in terms of the fragment (i.e. what we add
-	 * to the fragment's position to yield the corresponding position
-	 * in the root), but here we consider the root to be canonical,
-	 * and so our returned "offset" reflects how the fragment is offset
-	 * from the root.
-	 *
-	 * E.g.: If the fragment's sample 10 corresponds to root's 12,
-	 * try_sort_sync() would return 2.  But since root is canonical,
-	 * we say that the fragment is off by -2.
-	 */
-	r->begin=3Dmatchbegin;
-	r->end=3Dmatchend;
-	r->offset=3D-offset;
-	if(offset)if(callback)(*callback)(r->begin,PARANOIA_CB_FIXUP_EDGE);
-	return(1);
+        if(matchend - matchbegin > best_matchend - best_matchbegin) {
+          best_matchbegin =3D matchbegin;
+          best_matchend =3D matchend;
+          best_offset =3D offset;
+        }
       }
     }
+
+    /* If we found a matching run, we return the results of our match.
+     *
+     * Note that we flip the sign of (offset) because try_sort_sync()
+     * returns it in terms of the fragment (i.e. what we add
+     * to the fragment's position to yield the corresponding position
+     * in the root), but here we consider the root to be canonical,
+     * and so our returned "offset" reflects how the fragment is offset
+     * from the root.
+     *
+     * E.g.: If the fragment's sample 10 corresponds to root's 12,
+     * try_sort_sync() would return 2.  But since root is canonical,
+     * we say that the fragment is off by -2.
+     */
+    if(best_matchbegin !=3D -1) {
+      r->begin=3Dbest_matchbegin;
+      r->end=3Dbest_matchend;
+      r->offset=3D-best_offset;
+      if(offset)if(callback)(*callback)(r->begin,PARANOIA_CB_FIXUP_EDGE);
+      return(1);
+    }
   }
  =20
   return(0);


--Nq2Wo0NMKNjxTN9z
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="cdparanoia-smallest-offset.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/paranoia/paranoia.c b/paranoia/paranoia.c
index 07b0322..075b937 100644
--- a/paranoia/paranoia.c
+++ b/paranoia/paranoia.c
@@ -863,6 +863,10 @@ static long i_iterate_stage2(cdrom_paranoia *p,v_fragm=
ent *v,
      */
     sort_setup(i,fv(v),&fb(v),fs(v),fbv,fev);
=20
+    long min_matchbegin =3D -1;
+    long min_matchend =3D -1;
+    long min_offset =3D -1;
+
     for(j=3Dsearchbegin;j<searchend;j+=3D23){
=20
       /* Skip past silence in the root.  If there are just a few silent
@@ -894,27 +898,38 @@ static long i_iterate_stage2(cdrom_paranoia *p,v_frag=
ment *v,
        */
       if(try_sort_sync(p,i,NULL,rc(root),j,
 		       &matchbegin,&matchend,&offset,callback)){
-=09
-	/* If we found a matching run, we return the results of our match.
-	 *
-	 * Note that we flip the sign of (offset) because try_sort_sync()
-	 * returns it in terms of the fragment (i.e. what we add
-	 * to the fragment's position to yield the corresponding position
-	 * in the root), but here we consider the root to be canonical,
-	 * and so our returned "offset" reflects how the fragment is offset
-	 * from the root.
-	 *
-	 * E.g.: If the fragment's sample 10 corresponds to root's 12,
-	 * try_sort_sync() would return 2.  But since root is canonical,
-	 * we say that the fragment is off by -2.
-	 */
-	r->begin=3Dmatchbegin;
-	r->end=3Dmatchend;
-	r->offset=3D-offset;
-	if(offset)if(callback)(*callback)(r->begin,PARANOIA_CB_FIXUP_EDGE);
-	return(1);
+        if(abs(offset) < abs(min_offset) || min_offset =3D=3D -1) {
+          min_matchbegin =3D matchbegin;
+          min_matchend =3D matchend;
+          min_offset =3D offset;
+          if(min_offset >=3D 0) {
+            /* We will never find a smaller offset by continuing */
+            break;
+          }
+        }
       }
     }
+
+    /* If we found a matching run, we return the results of our match.
+     *
+     * Note that we flip the sign of (offset) because try_sort_sync()
+     * returns it in terms of the fragment (i.e. what we add
+     * to the fragment's position to yield the corresponding position
+     * in the root), but here we consider the root to be canonical,
+     * and so our returned "offset" reflects how the fragment is offset
+     * from the root.
+     *
+     * E.g.: If the fragment's sample 10 corresponds to root's 12,
+     * try_sort_sync() would return 2.  But since root is canonical,
+     * we say that the fragment is off by -2.
+     */
+    if(min_offset !=3D -1) {
+      r->begin=3Dmin_matchbegin;
+      r->end=3Dmin_matchend;
+      r->offset=3D-min_offset;
+      if(offset)if(callback)(*callback)(r->begin,PARANOIA_CB_FIXUP_EDGE);
+      return(1);
+    }
   }
  =20
   return(0);


--Nq2Wo0NMKNjxTN9z--

--DKU6Jbt7q3WqK7+M
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJYTzD4AAoJENV/VMgUgNllmqUP/1MWbX3uimZwZrsozjuDADzM
7z04wtit9yO8t2mUJptDCvoN/FYJMwfZhb9JHVE1mgWA/66v/hE+KVnvpgjznGle
uJw8ZYKuuBp12OTuM0m4mqZiDWcNBQ+fYZSd3nKxqk6aNxhD1rYd9Nwu47PCWXES
7/xRpgbANB8Rkvra/4HNAZauEs2r94GSTkHCzS0kukzejL/pWm4wOPaQnoBXw40/
+vkG4PZwdAmw3B749WRv09Bt+Qsqu6I5FnLRosrH7H8IVzh76aPJ9+vpq+ISKr8n
kkJLvWB65g1CwyAkEaTRiIMFORRSIiEsyTueKee8JogmLLL75kTL9CLFeMo2HMG7
EYV5Z6OiKTPbLeOKI7HlrWLZs6Q+qLVGYtre7X5QyEq5B/rRWNY5LGHOKmU2a+hZ
AOSEV4Pi9pPoUjdTh6CZtjVoMjDas0egRzzXRTzBWhTntI9BXLEAQT5ClyiMalGQ
v/+L4wAy+ANnhM0qTErf7BWCr/+Ty9vUwMfJBTXpEkA3Yje0IheCZZiN2a21tJDd
44lx6VG1/WJHIw28d3ss5EF/sQVdUlNURyHaKqud4iIlHKjxQLnBwqG6EZaXWIFZ
49J1YBd3ZODGcf8bY7XokDRFJqpAuj9z0ZzXaz/PGDWqrsBNJNyGkxFlrOn8UYYh
l1dnZkQeQrtabJUQ+k5u
=8yRb
-----END PGP SIGNATURE-----

--DKU6Jbt7q3WqK7+M--

--===============8106023987868604048==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: base64
Content-Disposition: inline

X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KUGFyYW5vaWEt
ZGV2IG1haWxpbmcgbGlzdApQYXJhbm9pYS1kZXZAeGlwaC5vcmcKaHR0cDovL2xpc3RzLnhpcGgu
b3JnL21haWxtYW4vbGlzdGluZm8vcGFyYW5vaWEtZGV2Cg==

--===============8106023987868604048==--