Closed ronchon65 closed 4 years ago
Unfortunately, I couldn't tell you why Aqualung would calculate a different discid. Can you provide an example CD with the discid Aqualung calculates and the discid for the same CD with two other tools?
Le 2020-07-16 17:40, Jeremy Evans a écrit :
Unfortunately, I couldn't tell you why Aqualung would calculate a different discid. Can you provide an example CD with the discid Aqualung calculates and the discid for the same CD with two other tools?
-- You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub [1], or unsubscribe [2].
Let us say Beethoven's 9th Symphony by Claudio Abbado and the Wiener Philharmoniker (DG). It is an old disc (1987). Recent ones use CDtext and don't need to search with CDDB...
abcde calculates discid=2c110804
cd-discid command also calculates discid=2c110804
CDex calculates discid=2c110804
Aqualung calculates discid=2b110604
The result is different for all CDs. Often, result Aqualung = result others - 0x200. It is not strictly the case here: there is also a difference on the second hexadecimal digit.
-- GD.
[1] https://github.com/jeremyevans/aqualung/issues/10#issuecomment-659495186 [2] https://github.com/notifications/unsubscribe-auth/AKLVM73KCL44TC25DB2K6ADR34NNJANCNFSM4O4MPMIA
Thank you for doing the research on this. I'll see if I can reproduce this locally.
Aqualung uses libcddb to calculate the discid, so assuming this is reproducible in other environments, this looks like either a bug in libcddb or a misuse of the libcddb API. It looks like libcddb is a long-dead project (last release in 2009), so I wouldn't hope for a fix in it.
Looks like abcde uses cd-discid, so those are the same implementation. Not sure about CDex, since it isn't open source.
One possible way to fix this would be to switch Aqualung to use cd-discid instead of libcddb to calculate the disc id.
I am able to replicate the issue, at least that Aqualung submits a different discid than the one created by cd-discid. I tried a CD and got:
8f0c970b 11 150 26660 56728 75116 95789 110685 137507 154035 175473 193285 210824 3225 # Aqualung / libcddb
8f0c990b 11 150 26660 56728 75116 95789 110685 137507 154035 175473 193285 210824 3227 # cd-discid
@tomszilagyi What are your thoughts and removing libcddb and switching to calling the cd-discid
command line program?
Actually, it looks like the issue might be that Aqualung's CDDB discid calculation does not include the pregap into the length. Can you try this patch:
--- src/store_cdda.c.orig
+++ src/store_cdda.c
@@ -536,7 +536,7 @@ static int
cddb_init_query_data(GtkTreeIter * iter_record, int * ntracks, int ** frames, int * length) {
int i;
- float len = 0.0f;
+ float len = 2.0f;
float offset = 150.0f; /* leading 2 secs in frames */
GtkTreeIter iter_track;
I'm not sure this will handle all cases, but it does fix the particular CD I tried. It might not work for CDs with a non-standard pregap, though.
Le 2020-07-17 02:56, Jeremy Evans a écrit :
Actually, it looks like the issue might be that Aqualung's CDDB discid calculation does not include the pregap into the length. Can you try this patch:
--- src/store_cdda.c.orig +++ src/store_cdda.c @@ -536,7 +536,7 @@ static int cddb_init_query_data(GtkTreeIter iter_record, int ntracks, int * frames, int length) {
int i;
- float len = 0.0f;
- float len = 2.0f; float offset = 150.0f; / leading 2 secs in frames /
GtkTreeIter iter_track;
I'm not sure this will handle all cases, but it does fix the particular CD I tried. It might not work for CDs with a non-standard pregap, though.
-- You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub [1], or unsubscribe [2].
Thanks for your help !
For the CD I gave as example the result calculated by Aqualung is is now : 2b110804. Compared to the result of cd-discid (2c110804) the 6th digit is now the same, but there is still a difference on the second digit.
-- GD.
[1] https://github.com/jeremyevans/aqualung/issues/10#issuecomment-659762904 [2] https://github.com/notifications/unsubscribe-auth/AKLVM765DPZ7WNELTEBLL3DR36OUDANCNFSM4O4MPMIA
My guess is that it's not using libcddb per se that is the problem, but rather, as pointed out, the exact input data going into it. Unfortunately, there is quite some ambiguity in how one should count the track lengths in the face of pre-gaps (with time counting up or down, filled with silence or audio content) -- the Red Book that defines the CD Audio disc has considerably richer semantics than allowed for by the "sequence of tracks" model. I never actually managed to wrap my head around that, and there are probably gaps (no pun intended) in Aqualung's implementation.
As a debugging measure, it might be worth "spying" on another program that uses libcddb (or a similar library with a well documented hashing algorithm) and produces correct hashes, e.g., at a most basic level, by inserting some print statements in its source code and observing what track lengths it deems appropriate to send to libcddb, and compare that with what Aqualung does.
But of course I do not have anything against replacing libcddb with another alternative, if it turns out to be the culprit (I would be a bit surprised though, IIRC the hash algo is well defined, has not changed much in the last several years, which is probably the reason why the library has not seen many changes recently).
To make it clear, if using cd-discid as an external program (runtime dependency) allows removal of libcddb as a compile-time dependency, plus produces correct results, I would totally go for it. Just make sure the user gets some visible feedback (error message) if the external program turns out to be missing.
Le 2020-07-17 10:31, Tom Szilagyi a écrit :
To make it clear, if using cd-discid as an external program (runtime dependency) allows removal of libcddb as a compile-time dependency, plus produces correct results, I would totally go for it. Just make sure the user gets some visible feedback (error message) if the external program turns out to be missing.
-- You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub [1], or unsubscribe [2].
Hello,
Thanks for your help !
In my opinion, libcddb could not be totally removed. The computation of cdiscid is one aspect, but libcddb is also used for obtaining the data from the data base once the right entry is identified by its cdiscid...
I I had time, I would like to compare the way cd-discid calculate the cdiscid with the libcddb one, since both sources are available (as far as I have sufficient competence to do that :) ).
-- GD.
[1] https://github.com/jeremyevans/aqualung/issues/10#issuecomment-659959718 [2] https://github.com/notifications/unsubscribe-auth/AKLVM76PJKOCMQS5Q24WRZLR4AD67ANCNFSM4O4MPMIA
@tomszilagyi The current algorithm for calculating the discid is based off metadata in the store, instead of taking it directly from the libcdio data. I think at least the cddb_disc_set_length
call is not right. Here's an example from another software that uses libcdio/libcddb, showing a cddb_disc_set_length
call that needs the start of the leadout track to calculate the length: https://github.com/neuros/neuroslink-xbmc/blob/0777ccdfb18db9fc264f704f3b854dd2542ee336/xbmc/lib/libcdio/libcdio/src/cddb.c#L126-L128
One thing that may work is calculating the discid up front when we read the CD, and store the discid in the metadata in the store. When it comes time to submit a CDDB query, just send the already calculated disc id, instead of trying to calculate the discid using the store's metadata. What are your thoughts on that approach?
Another approach would be dropping all CDDB support. It apparently has never worked correctly (always calculated the wrong discid), and this is the first bug report I've seen for it (though I'm not sure if there were Mantis bugs filed for it).
Hmm... would removing cddb support also mean that it is no longer possible to run a query of a CD physically present in the drive? I remember using Aqualung a lot for ripping CDs with that functionality (resulting in reasonably well tagged audio files without having to manually type all the track titles); losing that support would be kind of bad. OTOH I would not mind losing the ability to submit entries to the cddb online database.
I like your suggestion of storing the discid after having obtained it though, I think it is the right thing to do (if you can find the time to do it, that is).
The only place the discid is calculated in Aqualung is the submission, the querying doesn't use the discid (it appears to only use the track offsets), which may be why it wasn't broken. It's definitely possible to only disable the submission part. I think unless we are sure the discid is valid, we should probably disable the submission part as we don't want to be publishing bogus data. The server that Aqualung uses by default (freedb.org) has closed as far as I know, so this was only useful with a custom server anyway.
I agree that we should not send bogus data and I'm happy that queries will still work, so feel free to remove the submission part.
OK. I've got a version that compiles, but I'll want to do some runtime testing before pushing it up here.
I removed the CDDB submission feature in 69c2bcdbc3c77843164c1910e15de3235ed806a8. I also added a commit to change the default CDDB server to gnudb.org and CDDBP port to 8880 (which gnudb.org uses), as freedb.org stopped working last month.
I think it may be good to consider a 1.1 release later this month, considering it has been 5 years since 1.0. What are your thoughts on that, @tomszilagyi ?
Le 2020-07-18 05:56, Jeremy Evans a écrit :
I removed the CDDB submission feature in 69c2bcd [1]. I also added a commit to change the default CDDB server to gnudb.org and CDDBP port to 8880 (which gnudb.org uses), as freedb.org stopped working last month.
I think it may be good to consider a 1.1 release later this month, considering it has been 5 years since 1.0. What are your thoughts on that, @tomszilagyi [2] ?
-- You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub [3], or unsubscribe [4].
Hello,
Your update is great ! Thanks a lot !
May I suggest some minor corrections to the french locale ?
Best regards.
-- GD.
[1] https://github.com/jeremyevans/aqualung/commit/69c2bcdbc3c77843164c1910e15de3235ed806a8 [2] https://github.com/tomszilagyi [3] https://github.com/jeremyevans/aqualung/issues/10#issuecomment-660419619 [4] https://github.com/notifications/unsubscribe-auth/AKLVM7Z5475NQRYJ7DQARDDR4EMOPANCNFSM4O4MPMIA
@ronchon65 Definitely. We would be happy to accept corrections to the french localization. Please submit them as a pull request.
Le 2020-07-18 13:09, Jeremy Evans a écrit :
@ronchon65 [1] Definitely. We would be happy to accept corrections to the french localization. Please submit them as a pull request.
-- You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub [2], or unsubscribe [3].
In order to do that:
I forked Aqualung.
I created a new branch
. I ran autogen.sh and configure with the same options as in my previous git clone.
Just as I did hundred of times.
None of the Makefiles are generated, I don't understand why.
I must be tired
-- GD.
[1] https://github.com/ronchon65 [2] https://github.com/jeremyevans/aqualung/issues/10#issuecomment-660466986 [3] https://github.com/notifications/unsubscribe-auth/AKLVM7ZRCFB6HPMXYTWXEK3R4F7GLANCNFSM4O4MPMIA
Le 2020-07-18 16:35, Guy Durrieu a écrit :
Le 2020-07-18 13:09, Jeremy Evans a écrit :
@ronchon65 [1] Definitely. We would be happy to accept corrections to the french localization. Please submit them as a pull request.
-- You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub [2], or unsubscribe [3].
In order to do that:
I forked Aqualung.
I created a new branch
. I ran autogen.sh and configure with the same options as in my previous git clone.
Just as I did hundred of times.
None of the Makefiles are generated, I don't understand why.
I must be tired
-- GD.
For some reason I don't know config.status was not executed...
-- GD.
[1] https://github.com/ronchon65 [2] https://github.com/jeremyevans/aqualung/issues/10#issuecomment-660466986 [3] https://github.com/notifications/unsubscribe-auth/AKLVM7ZRCFB6HPMXYTWXEK3R4F7GLANCNFSM4O4MPMIA
I removed the CDDB submission feature in 69c2bcd. I also added a commit to change the default CDDB server to gnudb.org and CDDBP port to 8880 (which gnudb.org uses), as freedb.org stopped working last month.
Thanks for doing this!
I think it may be good to consider a 1.1 release later this month, considering it has been 5 years since 1.0. What are your thoughts on that, @tomszilagyi ?
Great idea, I'm all for it! Incredible how time flies, yes it's been 5 years since we marked 1.0...
Hello, As I said previously, I like Aqualung, and use it for all my audio sources, especially for my classical CD, because it is quite the only linux tool allowing me to access my local base without problem (free public data bases are down now anyway, except musicbrainz)... however something puzzle me, but is probably out of your scope: Aqualung calculate a cddb discid different from most other tools. Do you know why ? Well, I can correct the entries of my data base, but... Thanks anyway for your tool and for your help !