GNOME Bugzilla – Bug 773697
tracker-extract: Unify album-artist handling
Last modified: 2016-11-21 15:39:18 UTC
The different extractors were handling album-artist in different ways. Instead make them all do the same thing: use albumartist tag if available, fallback to track artist or performer if unavailable. Also incorporate it in the urn:musicAlbum and urn:musicAlbumDisc uri's to make those more unique, alleviating problems with similar named albums by different artists not being distinghuised (eg. 'Greatest Hits').
Created attachment 338792 [details] [review] tracker-extract: Unify album-artist handling
Created attachment 339043 [details] [review] tracker-extract: Don't mix artist and album artist in mp3 extractor In ID3v2 spec TP(E)1 is the artist & TP(E)2 the album artist, so stop mixing them.
Created attachment 339044 [details] [review] tracker-extract: Don't mix artist and album artist in mp3 extractor In ID3v2 spec TP(E)1 is the artist & TP(E)2 the album artist, so stop mixing them.
Created attachment 339045 [details] [review] tracker-extract: Don't make up an album artist The 'album artist' is not the same thing as the 'performer' or the 'artist' metadata.
Created attachment 339046 [details] [review] libtracker-extract: Use album artist in resource-helpers Have tracker_extract_new_music_album_disc use album artist to distinguish albums if available.
Created attachment 339047 [details] [review] tracker-extract: Use album artist in mp3 extractor Make use of the album artist if available to create a more unique uri in the mp3 extractor.
Created attachment 339048 [details] [review] tracker-extract: Use album artist in vorbis Use album artist if available to create the album uri in the vorbis extractor, to create a more unique uri.
Created attachment 339293 [details] [review] tracker-extract: Don't mix artist and albumartist The 'album artist' is not the same thing as the 'performer' or the 'artist' metadata. In some extractors these tags however got mixed to provide fallback values for certain tags. In the long run this is not a viable approach: the tracker information should be a true representation of what is in the tags.
Created attachment 339294 [details] [review] tracker-extract: Use albumartist if available Use albumartist in libav/vorbis/flac (resource-helpers) & mp3 extractors. This makes the album(-disc) uri more unique and it makes all the extractors behave the same.
Created attachment 339309 [details] [review] tracker-extract: Use albumartist if available Use albumartist in libav/vorbis/flac (resource-helpers), gstreamer & mp3 extractors if available to be part of the album(-disc) uri. This makes the uri more unique and tracker better at distinguishing separate albums and it makes all the extractors behave the same.
Created attachment 339310 [details] [review] tracker-extract: Use albumartist if available Use albumartist in libav/vorbis/flac (resource-helpers), gstreamer & mp3 extractors if available to be part of the album(-disc) uri. This makes the uri more unique and tracker better at distinguishing separate albums and it makes all the extractors behave the same.
Created attachment 339612 [details] [review] tracker-extract: Use date as album uri identifier Use the album creation date as part of the album uri identifier if available. This should make the separation of similar named albums even better. Libav isn't tested, because I couldn't get it build. I consider this a stop-gap solution until we can think and work on something better.
Comment on attachment 339293 [details] [review] tracker-extract: Don't mix artist and albumartist I agree we should not make up data. LGTM
Review of attachment 339310 [details] [review]: Looks fine generally, although IMHO would be great if we could make mp3/gstreamer use the helper functions, would also seem to make things easier for the next patch. ::: src/libtracker-extract/tracker-resource-helpers.c @@ +286,3 @@ tracker_resource_add_relation (album_disc, "nmm:albumDiscAlbum", album); + g_free (album_uri); Oops, reminds me I haven't had a round with valgrind/massif for a while. ::: src/tracker-extract/tracker-extract-gstreamer.c @@ +693,2 @@ #ifdef HAVE_LIBMEDIAART + extractor->media_art_artist = g_strdup (tracker_coalesce_strip (2, album_artist_name, track_artist_temp)); We now seem to be leaking album_artist_name.
Review of attachment 339612 [details] [review]: IMO this change makes sense, presumably some artists gather a few "Greatest Hits" over the years :). Although the logic duplication in helpers/mp3/gstreamer really shows here... ::: src/libtracker-extract/tracker-resource-helpers.c @@ +260,3 @@ "nmm:artistName"); + if (album_artist != NULL && date != NULL) { I miss an URI builder here :)... Oh well, I'd be glad if we could make this the only implementation though.
Created attachment 339683 [details] [review] tracker-extract: Use date as album uri identifier Use the album creation date as part of the album uri identifier if available. This should make the separation of similar named albums even better. Also port mp3 & gstreamer to use the resource helper functon for exctracting album disc data.
Created attachment 339684 [details] [review] Use nie:title instead of nmm:albumTitle nmm:albumTitle is deprecated
Created attachment 339692 [details] [review] tracker-extract: Use albumartist if available Use albumartist in libav/vorbis/flac (resource-helpers), gstreamer & mp3 extractors if available to be part of the album(-disc) uri. This makes the uri more unique and tracker better at distinguishing separate albums and it makes all the extractors behave the same.
Created attachment 339693 [details] [review] tracker-extract: Use date as album uri identifier Use the album creation date as part of the album uri identifier if available. This should make the separation of similar named albums even better. Also port mp3 & gstreamer to use the resource helper functon for exctracting album disc data.
Created attachment 339695 [details] [review] tracker-extract: Use date as album uri identifier Use the album creation date as part of the album uri identifier if available. This should make the separation of similar named albums even better. Also port mp3 & gstreamer to use the resource helper functon for exctracting album disc data.
Created attachment 339720 [details] [review] tracker-extract: Use date as album uri identifier Use the album creation date as part of the album uri identifier if available. This should make the separation of similar named albums even better. Also port mp3 & gstreamer to use the resource helper functon for exctracting album disc data.
Comment on attachment 339684 [details] [review] Use nie:title instead of nmm:albumTitle About time indeed.
Comment on attachment 339692 [details] [review] tracker-extract: Use albumartist if available LGTM
Review of attachment 339720 [details] [review]: Thanks for doing the refactor :). Found just a few nits, feel free to push after fixing. ::: src/libtracker-extract/tracker-resource-helpers.c @@ +271,3 @@ + g_string_append_printf (album_uri, ":%s", date); + + album = tracker_resource_new (tracker_sparql_escape_uri (album_uri->str)); It seems tracker_sparql_escape_uri() returns newly created memory, so it's being leaked here. I guess we need another intermediate var... @@ +291,3 @@ + g_string_append_printf (disc_uri, ":Disc%d", disc_number); + + album_disc = tracker_resource_new (tracker_sparql_escape_uri (disc_uri->str)); Here again :). ::: src/libtracker-sparql/tracker-uri.c @@ +297,3 @@ + * Calls tracker_sparql_escape_uri_printf(). + * + * Returns: a newly-allocated string holding the result. The returned string Returns: (transfer full): ... ::: src/tracker-extract/tracker-extract-libav.c @@ +42,3 @@ TrackerResource *metadata; gchar *absolute_file_path; + gchar *content_created; Better initialize this to NULL, seems like it could contain garbage on certain code paths. @@ +237,3 @@ + if (content_created) { + g_free (content_created); No need to check for != NULL before g_free()
Created attachment 339831 [details] [review] tracker-extract: Don't mix artist and albumartist The 'album artist' is not the same thing as the 'performer' or the 'artist' metadata. In some extractors these tags however got mixed to provide fallback values for certain tags. In the long run this is not a viable approach: the tracker information should be a true representation of what is in the tags.
Created attachment 339832 [details] [review] tracker-extract: Use albumartist if available Use albumartist in libav/vorbis/flac (resource-helpers), gstreamer & mp3 extractors if available to be part of the album(-disc) uri. This makes the uri more unique and tracker better at distinguishing separate albums and it makes all the extractors behave the same.
Created attachment 339833 [details] [review] tracker-extract: Use date as album uri identifier Use the album creation date as part of the album uri identifier if available. This should make the separation of similar named albums even better. Also port mp3 & gstreamer to use the resource helper functon for exctracting album disc data.
Created attachment 339834 [details] [review] Use nie:title instead of nmm:albumTitle nmm:albumTitle is deprecated
Created attachment 339844 [details] [review] tracker-extract: Don't mix artist and albumartist The 'album artist' is not the same thing as the 'performer' or the 'artist' metadata. In some extractors these tags however got mixed to provide fallback values for certain tags. In the long run this is not a viable approach: the tracker information should be a true representation of what is in the tags.
Attachment 339832 [details] pushed as 99e5534 - tracker-extract: Use albumartist if available Attachment 339833 [details] pushed as fb30a3f - tracker-extract: Use date as album uri identifier Attachment 339834 [details] pushed as f482618 - Use nie:title instead of nmm:albumTitle Attachment 339844 [details] pushed as 4c86ccd - tracker-extract: Don't mix artist and albumartist
*** Bug 734265 has been marked as a duplicate of this bug. ***