After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 757247 - Use MCN and barcode to automatically resolve multiple album matches
Use MCN and barcode to automatically resolve multiple album matches
Status: RESOLVED FIXED
Product: sound-juicer
Classification: Applications
Component: metadata
git master
Other Linux
: Normal normal
: ---
Assigned To: Sound Juicer Maintainers
Sound Juicer Maintainers
Depends on: 757156
Blocks:
 
 
Reported: 2015-10-28 11:29 UTC by Phillip Wood
Modified: 2016-02-17 09:54 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
use MCN to resolve multiple album matches (3.53 KB, patch)
2015-11-03 22:04 UTC, Andreas H
none Details | Review
I've modified the patch slightly to bail out early if there is a (2.96 KB, patch)
2016-02-05 11:14 UTC, Phillip Wood
none Details | Review
Use MCN to resolve multiple album matches (2.95 KB, patch)
2016-02-15 10:34 UTC, Phillip Wood
committed Details | Review

Description Phillip Wood 2015-10-28 11:29:08 UTC
Many CDs contain the media catalog number (MCN) which is the same as the barcode (give or take a leading zero in some cases?). Bug 757156 adds support for retrieving the barcode from MusicBrainz and libdiscid has support for reading the MCN so we should compare the two to try and find which MusicBrainz release to use for discs with more than one match.
Comment 1 Andreas H 2015-11-03 22:04:26 UTC
Created attachment 314771 [details] [review]
use MCN to resolve multiple album matches

Added read-out of the MCN (Media catalog number) from the CD using
libdiscid. Added functions to check MCN against barcode provided by
musicbrainz and filtering the list of album matches if a barcode
matching the MCN is found.
Comment 2 Phillip Wood 2015-11-05 11:43:27 UTC
Review of attachment 314771 [details] [review]:

Thanks for working on this it will be really good to minimize the number of times we have to show the multiple album dialog. I tried it on https://musicbrainz.org/release-group/32688eb1-f196-3f68-b26b-7e478d5add83 and it selects the right disc.

The patch looks good but I've a few comments:

Formatting wise the indent size should be 2 spaces, there should be a space before '(' when it's preceded by a keyword of function name but not otherwise. There should be spaces around all operators.

I think once we find a match we should just return that disc as there shouldn't be any other matches. There's no point getting more details from MusicBrainz if we're only going to free them in remove_album_if_mcn_barcode_mismatch()

Thanks again for you contributions.

::: libjuicer/sj-metadata-musicbrainz5.c
@@ +934,3 @@
+  int len = strlen(barcode);
+  if(len == 12) /* UPC barcode - skip leading '0' */
+      return strncmp ((mcn+1), barcode, SJ_DISCID_MCN_STRING_LENGTH-1) == 0;

As SJ_DISCID_MCN_STRING_LENGTH is defined it might be better to use that rather than hard coded lengths in these conditionals.

I think to be on the safe side we should check mcn[0] == '0' if the barcode is shorter than SJ_DISCID_MCN_STRING_LENGTH

I'd use strcmp for the comparison.

The brackets around mcn + 1 are unnecessary.

@@ +1088,3 @@
             album->metadata_source = SOURCE_MUSICBRAINZ;
             albums = g_list_append (albums, album);
+            found_mcn = found_mcn || album_barcode_matches_discid_mcn (album->barcode, disc);

I think maybe we should just bail out of the loops once we find a mcn that matches. Getting the results from MusicBrainz is fairly slow and if we can avoid unnecessary queries it will speed things up a bit. This assumes that there will only be one match on MusicBrainz for our mcn but I think barcodes are meant to be unique so it shouldn't be a problem (shout if you think I'm wrong on this!).

You'll need to clear albums and then set it with

g_list_free_full (albums, (GDestroyNotify) album_details_free);
albums = g_list_prepend (NULL, album);

and make sure all the libmusicbrainz resources get freed properly in the outer loops.

@@ +1101,3 @@
+
+  if (found_mcn == TRUE)
+      remove_album_if_mcn_barcode_mismatch(&albums, disc);

If we bail out of the loops we don't need this or remove_album_if_mcn_barcode_mismatch()
Comment 3 Andreas H 2015-11-11 21:38:32 UTC
Hi Phillip, sorry for the late reply. I'm a little busy lately.
Thanks for the comments and for applying the other patch.

This patch considered multiple releases with same barcode on purpose. I never encountered it, but I just wasn't sure if it really never happens.
But I guess if the barcode/mcn is not a unique identifier, what is?
So yes, I'll modify the patch to bail out of the loops as soon as we find a matching barcode.
Comment 4 Phillip Wood 2016-02-05 11:14:23 UTC
Created attachment 320493 [details] [review]
I've modified the patch slightly to bail out early if there is a

match. I'll merge it in time for the next (3.20) release

use MCN to resolve multiple album matches

Added read-out of the MCN (Media catalog number) from the CD using
libdiscid. Added functions to check MCN against barcode provided by
musicbrainz and filtering the list of album matches if a barcode
matching the MCN is found.
Comment 5 Phillip Wood 2016-02-15 10:34:26 UTC
Created attachment 321234 [details] [review]
Use MCN to resolve multiple album matches

Updated commit message.

Added read-out of the MCN (Media catalog number) from the CD using
libdiscid. If the MCN matches the barcode from MusicBrainz then return
only the matching release which avoids showing the multiple album
dialog.
Comment 6 Phillip Wood 2016-02-15 10:35:52 UTC
g# Bug 757247 - Use MCN and barcode to automatically resolve multiple album matches - NEW

Attachment 321234 [details] pushed as 8a17c08 - Use MCN to resolve multiple album matches
Comment 7 Andreas H 2016-02-15 20:38:52 UTC
Thanks for committing the patch. Fixing the patch was still on my todo list, but I didn't get around to it... Thanks for fixing it!
Comment 8 Phillip Wood 2016-02-17 09:54:25 UTC
(In reply to Andreas H from comment #7)
> Thanks for committing the patch. Fixing the patch was still on my todo list,
> but I didn't get around to it... Thanks for fixing it!

Thanks I hope you don't mind but it didn't need much work and I wanted to get it into the next release. Thanks for the MCN patches they're a really useful addition to sound-juicer.