minus the g_debug! whoops

Created attachment 202828 [details] [review]
Check the user's cache for album art

Removed the g_debug

Hi, and sorry for the delay

I've just checked the code and it looks OK. I spotted a couple of minor things related to the style (braces in the line after the "if" while we try to put them in the same line, indentation...), but that's totally a minor thing.

Tomorrow I'll try to actually build Grilo with this patch in and see that it works fine. If that's the case, I'll correct these things myself and push the patch.

Thanks!

I think I will steal the role of Simon and do the review instead, so that he has more time to review my stuff ;)

Hi,

I haven't tried the code yet, but apart from what Simon said, it indeed looks good, and it's nice to see this feature finally implemented.

Nevertheless, there is one issue that I would like to be fixed before allowing this for inclusion in master: in _may_resolve(), you should check for the artist and album keys. If they are not present you should return FALSE and fill missing_keys (if it's not NULL). This is described in the documentation of grl_metadata_source_may_resolve(), and you can take inspiration from (or even probably copy-paste) the _may_resolve() of the last.fm plugin. You also should move your check outside of the scope of the check for _KEY_URL, since as far as I understand you don't use the URL in the audio part.

This is needed for the core to try to ensure it has these needed keys when it calls your resolve().

Cheers.

Created attachment 203149 [details] [review]
Add local album art - corrections to may_resolve

Thanks for the feedback, I've reworked the _may_resolve implementation

Created attachment 203150 [details] [review]
Add local album art - corrections to may_resolve

Add local album art - corrections to may_resolve

Thanks for the feedback, I've reworked the _may_resolve implementation

(wrong patch attached to previous comment)

Created attachment 203163 [details] [review]
has_compatible_media_url return FALSE is URL is NULL

Spotted a miss-assumption I made with the has_compatible_media_url function. Fixed so that it returns false if the url is null

Review of attachment 203150 [details] [review]:

I think your change of may_resolve() is too invasive: you changed the logic of the existing stuff there, in a way that is not wanted. In particular, the presence of the url key in the media is not checked any more for images and videos.
I would advise to simply add the code for the audio case after the missing_keys treatment and before the "return FALSE", or something like that.
Also, the "else" before the "if (missing_keys)" shouldn't be there.

oops, didn't spot there was another attachment supposed to replace the first one. Will look at it now

Created attachment 203169 [details] [review]
local-metadata: Check the user's cache for album art

OK, the may_resolve() change is trickier than I initially thought.
Sorry I misunderstood what you tried to do with has_compatible_media_url(), now I think I get it: you wanted to have it check the presence of the url field as well. The thing is we need to distinguish these two cases for video and image:
 - there is no url field: return FALSE, putting the url key in missing_keys, meaning "we can't resolve this unless the url field is there"
 - there is a url field, but it is not a local url: return FALSE, no modification of missing_keys, meaning "we can't resolve this, period."
Therefore, we need to check separately the presence of the field and whether it is "file://" or not.

The simplest way that I've found (developed in the patch I attached) is to handle the AUDIO case first, and then we can handle the other cases with the code that was already there. I have also changed the code formatting style to match the latest fashion in grilo.
Another point: you need to use GRLKEYID_TO_POINTER() to add a key to a list (reflected in the new patch), else you get compilation warnings and risk bugs on platforms where pointers and ints don't have the same size.

I haven't tested it yet, but it looks OK to me like that. I'd be happy to have a second opinion though, especially yours (Michael) as it's your name in the patch :).

Created attachment 203242 [details] [review]
local-metadata: Check the user's cache for album art

Oops. I attached the old version of the patch, here's the new one.

Thanks for the input and re-jigging the patch. I tested it with Media explorer and that works fine.

As noted on the mailing list we need to normalise the utf8 string before md5suming it (for characters with accents etc).

@@ -708,7 +708,10 @@ albumart_strip_invalid_entities (const gchar *origin
   res = g_utf8_strdown (str, -1);
   g_free (str);
 
-  return res;
+  str = g_utf8_normalize (res, -1, G_NORMALIZE_NFKD);
+  g_free (res);
+
+  return str;
 }
 
 static void
@@ -717,7 +720,9 @@ resolve_album_art (GrlMetadataSource *source,
                    resolution_flags_t flags)
 {
   const gchar *artist_value, *album_value;
-  gchar *artist, *album, *artist_md5, *album_md5, *file_path;
+  gchar *artist, *album, *artist_tmp, *album_tmp,
+        *artist_md5, *album_md5, *file_path;
+
   GRegex *regex;
 
   artist_value = grl_media_audio_get_artist (GRL_MEDIA_AUDIO (rs->media)
@@ -738,12 +743,20 @@ resolve_album_art (GrlMetadataSource *source,
   if ((g_regex_match (regex, artist_value, 0, NULL)))
     artist = albumart_strip_invalid_entities (artist_value);
   else
-    artist = g_utf8_strdown (artist_value, -1);
+    {
+      artist_tmp = g_utf8_strdown (artist_value, -1);
+      artist = g_utf8_normalize (artist_tmp, -1, G_NORMALIZE_NFKD);
+      g_free (artist_tmp);
+    }
 
   if (g_regex_match (regex, album_value, 0, NULL))
     album = albumart_strip_invalid_entities (album_value);
   else
-    album = g_utf8_strdown (album_value,  -1);
+    {
+      album_tmp = g_utf8_strdown (album_value, -1);
+      album = g_utf8_normalize (album_tmp, -1, G_NORMALIZE_NFKD);
+      g_free (album_tmp);
+    }
 
   g_regex_unref (regex);


I'll fixup this into the latest version of the patch.

Created attachment 203346 [details] [review]
Fixup with normalise string

Created attachment 203604 [details] [review]
fixup! local-metadata: Check the user's cache for album art

Here is a "fixup!" for the patch (to be applied after the patch), that I came up with after some testings. It corrects a few things:
 - formatting issues (for consistency with the rest of the code)
 - always call rs->callback() in resolve_album_art(): this is needed to 
 - modify the initial checks in grl_local_metadata_source_resolve(): we only need a file:// url for videos and images.

But then, even after the changes, local-metadata does not seem to get the album art I have on my machine (I suspect it was generated by banshee, but I'm not sure, might be rhythmbox or something else).
For the Beatle's Sgt. Pepper's Lonely Hearts Club Band, the file local-metadata looks for is called "album-2a9ea35253dbec60e76166ec8420fbda-af4881916c7b6e4d7adb283461b2a938.jpeg", but I have the thumbnail in a file called "album-0a229e2fa8e5554bb959f4b3d5cf3e9e.jpg", which is quite different even in its format (only one checksum instead of two and a .jpg extension instead of .jpeg).
Is the patch wrong in some way, or are there different possible interpretations of the spec? I would tend to say that we definitely want to try to be compatible with the way gnome players do things, even if it's slightly wrong (the best is to be compatible with everything, of course ;) )

It picks up the album art in my ~/.cache/media-art/ via Media explorer. This seems to be the same album art that Nautilus uses as well

michael@dorado:~$ echo -n "alice russell" | md5sum
fbd6e3455f04adbc082b82fb28b986d9  -

michael@dorado:~$ echo -n "my favourite letters" | md5sum
9b665c6d0c987fc1c1208fdf8e0db81a  -

michael@dorado:~$ ls ./.cache/media-art/album-fbd6e3455f04adbc082b82fb28b986d9-9b665c6d0c987fc1c1208fdf8e0db81a.jpeg

I don't know where these album art items came from though but I do have them..

I think it's just a matter of time for other packages to implement the media spec.

OK, finally found time to test against tracker, and it does find the cover art cached by tracker.
A future improvement of interest would be to have it find cover art cached by banshee, but I'd consider this outside of scope and this bug, and I therefore mark this as fixed.

Thanks for your time and efforts Michael!


commit c4a0d8ad9f791b1c0d02bca51c8d7b5bf05d6dea
Author: Michael Wood <michael.g.wood@linux.intel.com>
Date:   Thu Dec 1 17:53:25 2011 +0000

    local-metadata: Check the user's cache for album art
    
    Implements getting album art according to:
    http://live.gnome.org/MediaArtStorageSpec