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 773697 - tracker-extract: Unify album-artist handling
tracker-extract: Unify album-artist handling
Status: RESOLVED FIXED
Product: tracker
Classification: Core
Component: Extractor
unspecified
Other All
: Normal normal
: ---
Assigned To: tracker-extractor
tracker-extractor
: 734265 (view as bug list)
Depends on: 773607
Blocks: 766579 774260
 
 
Reported: 2016-10-30 14:08 UTC by Marinus Schraal
Modified: 2016-11-21 15:39 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
tracker-extract: Unify album-artist handling (6.23 KB, patch)
2016-10-30 14:08 UTC, Marinus Schraal
none Details | Review
tracker-extract: Don't mix artist and album artist in mp3 extractor (1.99 KB, patch)
2016-11-03 15:38 UTC, Marinus Schraal
none Details | Review
tracker-extract: Don't mix artist and album artist in mp3 extractor (1.99 KB, patch)
2016-11-03 15:38 UTC, Marinus Schraal
none Details | Review
tracker-extract: Don't make up an album artist (1.80 KB, patch)
2016-11-03 15:39 UTC, Marinus Schraal
none Details | Review
libtracker-extract: Use album artist in resource-helpers (2.91 KB, patch)
2016-11-03 15:39 UTC, Marinus Schraal
none Details | Review
tracker-extract: Use album artist in mp3 extractor (2.84 KB, patch)
2016-11-03 15:39 UTC, Marinus Schraal
none Details | Review
tracker-extract: Use album artist in vorbis (997 bytes, patch)
2016-11-03 15:39 UTC, Marinus Schraal
none Details | Review
tracker-extract: Don't mix artist and albumartist (4.16 KB, patch)
2016-11-08 00:33 UTC, Marinus Schraal
none Details | Review
tracker-extract: Use albumartist if available (5.32 KB, patch)
2016-11-08 00:33 UTC, Marinus Schraal
none Details | Review
tracker-extract: Use albumartist if available (7.33 KB, patch)
2016-11-08 12:03 UTC, Marinus Schraal
none Details | Review
tracker-extract: Use albumartist if available (9.27 KB, patch)
2016-11-08 12:07 UTC, Marinus Schraal
none Details | Review
tracker-extract: Use date as album uri identifier (16.81 KB, patch)
2016-11-11 11:28 UTC, Marinus Schraal
none Details | Review
tracker-extract: Use date as album uri identifier (16.44 KB, patch)
2016-11-12 12:54 UTC, Marinus Schraal
none Details | Review
Use nie:title instead of nmm:albumTitle (1.96 KB, patch)
2016-11-12 12:54 UTC, Marinus Schraal
none Details | Review
tracker-extract: Use albumartist if available (9.29 KB, patch)
2016-11-12 14:55 UTC, Marinus Schraal
none Details | Review
tracker-extract: Use date as album uri identifier (16.43 KB, patch)
2016-11-12 14:55 UTC, Marinus Schraal
none Details | Review
tracker-extract: Use date as album uri identifier (16.93 KB, patch)
2016-11-12 15:03 UTC, Marinus Schraal
none Details | Review
tracker-extract: Use date as album uri identifier (17.94 KB, patch)
2016-11-12 20:20 UTC, Marinus Schraal
none Details | Review
tracker-extract: Don't mix artist and albumartist (5.57 KB, patch)
2016-11-14 21:11 UTC, Marinus Schraal
none Details | Review
tracker-extract: Use albumartist if available (9.29 KB, patch)
2016-11-14 21:12 UTC, Marinus Schraal
committed Details | Review
tracker-extract: Use date as album uri identifier (19.81 KB, patch)
2016-11-14 21:12 UTC, Marinus Schraal
committed Details | Review
Use nie:title instead of nmm:albumTitle (5.31 KB, patch)
2016-11-14 21:12 UTC, Marinus Schraal
committed Details | Review
tracker-extract: Don't mix artist and albumartist (5.96 KB, patch)
2016-11-14 23:09 UTC, Marinus Schraal
committed Details | Review

Description Marinus Schraal 2016-10-30 14:08:43 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').
Comment 1 Marinus Schraal 2016-10-30 14:08:48 UTC
Created attachment 338792 [details] [review]
tracker-extract: Unify album-artist handling
Comment 2 Marinus Schraal 2016-11-03 15:38:01 UTC
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.
Comment 3 Marinus Schraal 2016-11-03 15:38:53 UTC
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.
Comment 4 Marinus Schraal 2016-11-03 15:39:06 UTC
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.
Comment 5 Marinus Schraal 2016-11-03 15:39:19 UTC
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.
Comment 6 Marinus Schraal 2016-11-03 15:39:35 UTC
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.
Comment 7 Marinus Schraal 2016-11-03 15:39:52 UTC
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.
Comment 8 Marinus Schraal 2016-11-08 00:33:52 UTC
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.
Comment 9 Marinus Schraal 2016-11-08 00:33:56 UTC
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.
Comment 10 Marinus Schraal 2016-11-08 12:03:32 UTC
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.
Comment 11 Marinus Schraal 2016-11-08 12:07:23 UTC
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.
Comment 12 Marinus Schraal 2016-11-11 11:28:04 UTC
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 13 Carlos Garnacho 2016-11-11 13:17:03 UTC
Comment on attachment 339293 [details] [review]
tracker-extract: Don't mix artist and albumartist

I agree we should not make up data. LGTM
Comment 14 Carlos Garnacho 2016-11-11 13:33:42 UTC
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.
Comment 15 Carlos Garnacho 2016-11-11 13:41:16 UTC
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.
Comment 16 Marinus Schraal 2016-11-12 12:54:06 UTC
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.
Comment 17 Marinus Schraal 2016-11-12 12:54:13 UTC
Created attachment 339684 [details] [review]
Use nie:title instead of nmm:albumTitle

nmm:albumTitle is deprecated
Comment 18 Marinus Schraal 2016-11-12 14:55:21 UTC
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.
Comment 19 Marinus Schraal 2016-11-12 14:55:26 UTC
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.
Comment 20 Marinus Schraal 2016-11-12 15:03:09 UTC
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.
Comment 21 Marinus Schraal 2016-11-12 20:20:34 UTC
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 22 Carlos Garnacho 2016-11-13 16:12:12 UTC
Comment on attachment 339684 [details] [review]
Use nie:title instead of nmm:albumTitle

About time indeed.
Comment 23 Carlos Garnacho 2016-11-13 16:12:34 UTC
Comment on attachment 339692 [details] [review]
tracker-extract: Use albumartist if available

LGTM
Comment 24 Carlos Garnacho 2016-11-13 16:12:50 UTC
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()
Comment 25 Marinus Schraal 2016-11-14 21:11:57 UTC
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.
Comment 26 Marinus Schraal 2016-11-14 21:12:03 UTC
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.
Comment 27 Marinus Schraal 2016-11-14 21:12:09 UTC
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.
Comment 28 Marinus Schraal 2016-11-14 21:12:15 UTC
Created attachment 339834 [details] [review]
Use nie:title instead of nmm:albumTitle

nmm:albumTitle is deprecated
Comment 29 Marinus Schraal 2016-11-14 23:09:01 UTC
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.
Comment 30 Marinus Schraal 2016-11-14 23:16:17 UTC
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
Comment 31 Marinus Schraal 2016-11-21 15:39:18 UTC
*** Bug 734265 has been marked as a duplicate of this bug. ***