[sound-juicer] Cache MusicBrainz artist details.

[Phillip Wood <pwood src gnome org>]

commit 0ecb20f362d4f14d0d37744781c85c3b79b48f96
Author: Phillip Wood <phillip wood dunelm org uk>
Date:   Tue Feb 4 19:00:38 2014 +0000

    Cache MusicBrainz artist details.
    
    This will avoid performing unnecessary composer alias queries and
    scanning an artist's alias list more than once.
    
    Remove 'joinphrase' from ArtistDetails structures to enable caching
    just one ArtistDetails per artist. Create ArtistCredit structure to
    hold the join phrase and an ArtistDetails* when creating artist
    lists. Remove the artist lists from AlbumDetails and TrackDetails
    structures as they are only used to generate the artist text and not
    having them simplifies things a little.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=723297

 libjuicer/sj-metadata-musicbrainz5.c |  104 ++++++++++++++++++++++------------
 libjuicer/sj-structures.c            |   32 +++++++++--
 libjuicer/sj-structures.h            |   13 +++-
 3 files changed, 103 insertions(+), 46 deletions(-)
---
diff --git a/libjuicer/sj-metadata-musicbrainz5.c b/libjuicer/sj-metadata-musicbrainz5.c
index ce3030b..db4018c 100644
--- a/libjuicer/sj-metadata-musicbrainz5.c
+++ b/libjuicer/sj-metadata-musicbrainz5.c
@@ -53,6 +53,7 @@ typedef struct {
   Mb5Query mb;
   DiscId  *disc;
   char    *cdrom;
+  GHashTable *artist_cache;
   /* Proxy */
   char    *proxy_host;
   char    *proxy_username;
@@ -101,6 +102,30 @@ sj_mb5_album_details_dump (AlbumDetails *details)
 #define sj_mb5_album_details_dump(...)
 #endif
 
+
+static ArtistDetails*
+make_artist_details (SjMetadataMusicbrainz5 *self,
+                     Mb5Artist               artist)
+{
+  char buffer[512]; /* For the GET macro */
+  ArtistDetails *details;
+  SjMetadataMusicbrainz5Private *priv = GET_PRIVATE (self);
+
+  mb5_artist_get_id (artist, buffer, sizeof(buffer));
+  details = g_hash_table_lookup (priv->artist_cache, buffer);
+  if (details == NULL) {
+    details = g_new0 (ArtistDetails, 1);
+    details->id = g_strdup (buffer);
+    GET (details->name, mb5_artist_get_name, artist);
+    GET (details->sortname, mb5_artist_get_sortname, artist);
+    GET (details->disambiguation, mb5_artist_get_disambiguation, artist);
+    GET (details->gender, mb5_artist_get_gender, artist);
+    GET (details->country, mb5_artist_get_country, artist);
+    g_hash_table_insert (priv->artist_cache, details->id, details);
+  }
+  return details;
+}
+
 static void
 print_musicbrainz_query_error (SjMetadataMusicbrainz5 *self,
                                const char             *entity,
@@ -165,25 +190,19 @@ get_artist_list (SjMetadataMusicbrainz5 *self,
   for (i = 0; i < mb5_namecredit_list_size (name_list); i++) {
     Mb5NameCredit name_credit;
     Mb5Artist artist;
-    ArtistDetails *details;
+    ArtistCredit *artist_credit = g_slice_new0 (ArtistCredit);
 
     name_credit = mb5_namecredit_list_item (name_list, i);
-    details = g_new0 (ArtistDetails, 1);
-    GET (details->joinphrase, mb5_namecredit_get_joinphrase, name_credit);
-    artists = g_list_prepend (artists, details);
     artist = mb5_namecredit_get_artist (name_credit);
-    if (!artist) {
+    if (artist != NULL) {
+      artist_credit->details = make_artist_details (self, artist);
+    } else {
       g_warning ("no Mb5Artist associated with Mb5NameCredit, falling back to Mb5NameCredit::name");
-      GET (details->name, mb5_namecredit_get_name, name_credit);
-      continue;
+      artist_credit->details = g_new0 (ArtistDetails, 1);
+      GET (artist_credit->details->name, mb5_namecredit_get_name, name_credit);
     }
-
-    GET (details->id, mb5_artist_get_id, artist);
-    GET (details->name, mb5_artist_get_name, artist);
-    GET (details->sortname, mb5_artist_get_sortname, artist);
-    GET (details->disambiguation, mb5_artist_get_disambiguation, artist);
-    GET (details->gender, mb5_artist_get_gender, artist);
-    GET (details->country, mb5_artist_get_country, artist);
+    GET (artist_credit->joinphrase, mb5_namecredit_get_joinphrase, name_credit);
+    artists = g_list_prepend (artists, artist_credit);
   }
 
   return g_list_reverse(artists);
@@ -199,11 +218,11 @@ get_artist_info (GList *artists, char **name, char **sortname, char **id)
   artist_name = g_string_new (NULL);
   artist_count = 0;
   for (it = artists; it != NULL; it = it->next) {
-    ArtistDetails *details = (ArtistDetails *)it->data;
+    ArtistCredit *artist_credit = (ArtistCredit *)it->data;
     artist_count++;
-    g_string_append (artist_name, details->name);
-    if (details->joinphrase != NULL)
-      g_string_append (artist_name, details->joinphrase);
+    g_string_append (artist_name, artist_credit->details->name);
+    if (artist_credit->joinphrase != NULL)
+      g_string_append (artist_name, artist_credit->joinphrase);
   }
 
   if (artist_count != 1) {
@@ -213,7 +232,7 @@ get_artist_info (GList *artists, char **name, char **sortname, char **id)
       if (id != NULL)
         *id = NULL;
   } else {
-      ArtistDetails *details = (ArtistDetails *)artists->data;
+      ArtistDetails *details = ((ArtistCredit *)artists->data)->details;
       if (sortname != NULL)
         *sortname = g_strdup (details->sortname);
       if (id != NULL)
@@ -386,6 +405,23 @@ fill_recording_relations (SjMetadataMusicbrainz5 *self,
 }
 
 static void
+parse_artist_credit (SjMetadataMusicbrainz5  *self,
+                     Mb5ArtistCredit          credit,
+                     char                   **name,
+                     char                   **sortname,
+                     char                   **id)
+{
+  if (credit) {
+    GList *artists;
+    artists = get_artist_list (self, credit);
+    if (artists) {
+      get_artist_info (artists, name, sortname, id);
+      g_list_free_full (artists, artist_credit_destroy);
+    }
+  }
+}
+
+static void
 fill_tracks_from_medium (SjMetadataMusicbrainz5 *self,
                          Mb5Medium               medium,
                          AlbumDetails           *album)
@@ -439,16 +475,10 @@ fill_tracks_from_medium (SjMetadataMusicbrainz5 *self,
           credit = mb5_recording_get_artistcredit (recording);
     }
 
-    if (credit) {
-      GList *artists;
-      artists = get_artist_list (self, credit);
-      if (artists) {
-        get_artist_info (artists, &track->artist,
-                         &track->artist_sortname,
-                         &track->artist_id);
-      }
-      track->artists = artists;
-    }
+      parse_artist_credit(self, credit,
+                          &track->artist,
+                          &track->artist_sortname,
+                          &track->artist_id);
     if (track->artist == NULL)
       track->artist = g_strdup (album->artist);
     if (track->artist_sortname == NULL)
@@ -532,7 +562,6 @@ make_album_from_release (SjMetadataMusicbrainz5 *self,
   Mb5ArtistCredit credit;
   Mb5RelationListList relationlists;
   Mb5MediumList media;
-  GList *artists;
   char *date = NULL;
   char buffer[512]; /* for the GET macro */
 
@@ -552,13 +581,10 @@ make_album_from_release (SjMetadataMusicbrainz5 *self,
 
   credit = mb5_release_get_artistcredit (release);
 
-  artists = get_artist_list (self, credit);
-  if (artists) {
-    get_artist_info (artists, &album->artist,
-                     &album->artist_sortname,
-                     &album->artist_id);
-  }
-  album->artists = artists;
+  parse_artist_credit (self, credit,
+                       &album->artist,
+                       &album->artist_sortname,
+                       &album->artist_id);
 
   GET (date, mb5_release_get_date, release);
   album->release_date = sj_metadata_helper_scan_date (date);
@@ -744,6 +770,9 @@ sj_metadata_musicbrainz5_init (SjMetadataMusicbrainz5 *self)
 
   priv = GET_PRIVATE (self);
   priv->mb = mb5_query_new (SJ_MUSICBRAINZ_USER_AGENT, NULL, 0);
+
+  priv->artist_cache = g_hash_table_new_full (g_str_hash, g_str_equal,
+                                              NULL, artist_details_destroy);
 }
 
 static void
@@ -837,6 +866,7 @@ sj_metadata_musicbrainz5_finalize (GObject *object)
   g_free (priv->proxy_host);
   g_free (priv->proxy_username);
   g_free (priv->proxy_password);
+  g_hash_table_unref (priv->artist_cache);
 
   G_OBJECT_CLASS (sj_metadata_musicbrainz5_parent_class)->finalize (object);
 }
diff --git a/libjuicer/sj-structures.c b/libjuicer/sj-structures.c
index 29ef92c..09f4009 100644
--- a/libjuicer/sj-structures.c
+++ b/libjuicer/sj-structures.c
@@ -35,8 +35,6 @@ void track_details_free(TrackDetails *track)
   g_free (track->track_id);
   g_free (track->artist_id);
   g_free (track->artist_sortname);
-  g_list_foreach (track->artists, (GFunc)artist_details_free, NULL);
-  g_list_free (track->artists);
   g_free (track);
 }
 
@@ -64,8 +62,6 @@ void album_details_free(AlbumDetails *album)
   g_free (album->country);
   g_free (album->type);
   g_list_foreach (album->labels, (GFunc)label_details_free, NULL);
-  g_list_foreach (album->artists, (GFunc)artist_details_free, NULL);
-  g_list_free (album->artists);
   g_free (album);
 }
 
@@ -80,11 +76,37 @@ void artist_details_free (ArtistDetails *artist)
   g_free (artist->disambiguation);
   g_free (artist->gender);
   g_free (artist->country);
-  g_free (artist->joinphrase);
   g_free (artist);
 }
 
 /*
+ * GDestroyNotify callback to free a ArtistDetails*
+ */
+void artist_details_destroy (gpointer artist)
+{
+  artist_details_free (artist);
+}
+
+/*
+ * Free an ArtistCredit*
+ */
+void artist_credit_free (ArtistCredit *credit, gboolean free_details)
+{
+  if (free_details)
+    artist_details_free (credit->details);
+  g_free (credit->joinphrase);
+  g_slice_free (ArtistCredit, credit);
+}
+
+/*
+ * GDestroyNotify callback to free a ArtistCredit*
+ */
+void artist_credit_destroy (gpointer artist)
+{
+  artist_credit_free (artist, FALSE);
+}
+
+/*
  * Free a LabelDetails
  */
 void label_details_free (LabelDetails *label)
diff --git a/libjuicer/sj-structures.h b/libjuicer/sj-structures.h
index 8af7e29..f5c60a8 100644
--- a/libjuicer/sj-structures.h
+++ b/libjuicer/sj-structures.h
@@ -27,6 +27,7 @@
 typedef enum _MetadataSource MetadataSource;
 
 typedef struct _AlbumDetails AlbumDetails;
+typedef struct _ArtistCredit ArtistCredit;
 typedef struct _ArtistDetails ArtistDetails;
 typedef struct _LabelDetails LabelDetails;
 typedef struct _TrackDetails TrackDetails;
@@ -50,7 +51,6 @@ struct _TrackDetails {
   int duration; /* seconds */
   char* track_id;
   char* artist_id;
-  GList *artists;
 };
 
 struct _AlbumDetails {
@@ -80,7 +80,6 @@ struct _AlbumDetails {
   char *type;
   char *lyrics_url;
   char *country;
-  GList *artists;
 };
 
 struct _ArtistDetails {
@@ -90,9 +89,11 @@ struct _ArtistDetails {
   char *disambiguation;
   char *gender;
   char *country;
+};
 
-  /* doesn't belong in here, prevent sharing the artist structure between
-   * distinct ReleaseGroups - more convenient for now */
+struct _ArtistCredit
+{
+  ArtistDetails *details;
   char *joinphrase;
 };
 
@@ -102,7 +103,11 @@ struct _LabelDetails {
 };
 
 void album_details_free(AlbumDetails *album);
+ArtistDetails* artist_details_copy (const ArtistDetails *artist);
+void artist_credit_free (ArtistCredit *credit, gboolean free_details);
+void artist_credit_destroy (gpointer credit);
 void artist_details_free(ArtistDetails *artist);
+void artist_details_destroy (gpointer artist);
 void label_details_free (LabelDetails *label);
 void track_details_free(TrackDetails *track);
 #endif