Created attachment 306989 [details] [review]
local-metadata: Correctly track asynchronous resolve operations

Currently, only a single asynchronous operation is (potentially) used to
resolve local metadata. However, to fix the problem where the
thumbnail may be set to a non-existent URI, a second asynchronous
operation needs to happen in parallel. The current GrlSourceResolveSpec
closure cannot track this, so a new wrapper closure has to be added
which tracks the number of pending sub-operations and only calls the
GrlSourceResolveSpec’s callback once all are complete, or when a single
one errors.

Signed-off-by: Philip Withnall <philip.withnall@collabora.co.uk>

Created attachment 306990 [details] [review]
local-metadata: Query for media art existence

Don’t set the media art on a GrlMedia if it doesn’t exist — libmediaart
unconditionally returns a path for where it would expect the media art
for a file to be, without checking that some art actually exists at that
path. That’s our job.

Asynchronously query for the file info to check it exists. Ignore
whether the media art is readable; that’s for the caller to handle when
they try to read it (and is consistent with the other thumbnail URIs
Grilo returns).

Signed-off-by: Philip Withnall <philip.withnall@collabora.co.uk>

Created attachment 306991 [details] [review]
local-metadata: Add thumbnails rather than setting them

This allows multiple thumbnails to be supported for a particular piece
of local media. For example, this allows both a thumbnail and a piece of
media art to be displayed.

Signed-off-by: Philip Withnall <philip.withnall@collabora.co.uk>

Review of attachment 306989 [details] [review]:

You can remove the SOB from the commit message.

The commit subject is also misleading, as there are currently no errors, but those changes are necessary if we want to extend its capabilities.

Looks fine otherwise.

Review of attachment 306990 [details] [review]:

Remove the SOB again.

Looks good.

Review of attachment 306991 [details] [review]:

Hmm, the problem here is that it means that there's no way for the caller to know which thumbnail we should use, unless they specifically call resolution in a specific order.

In which case is that useful?

Committed the first two patches with SOB removed, and with the first one’s commit subject tweaked.

Attachment 306990 [details] pushed as 519d205 - local-metadata: Query for media art existence

(In reply to Bastien Nocera from comment #6)
> Review of attachment 306991 [details] [review] [review]:
> 
> Hmm, the problem here is that it means that there's no way for the caller to
> know which thumbnail we should use, unless they specifically call resolution
> in a specific order.
> 
> In which case is that useful?

I was thinking of it as a way of solving the problem of whether to set the thumbnail or the media art as the GrlMedia’s thumbnail, but you’re right; it just punts the choice to the application instead.

I see two ways of solving this:
 1. Ranking the possible thumbnails so that, for example, we always prefer album art over a thumbnail. This would mean blocking the completion of one of the sub-operations on the other, and would not be fun.
 2. Completely ignoring it, and letting the last sub-operation to complete (successfully) overwrite any previous GrlMedia thumbnail.

I can’t really see one type of thumbnail being any more valuable than another, so I’d be in favour of option #2, but I’m very interested in what you think.

(In reply to Philip Withnall from comment #8)
> (In reply to Bastien Nocera from comment #6)
> > Review of attachment 306991 [details] [review] [review] [review]:
> > 
> > Hmm, the problem here is that it means that there's no way for the caller to
> > know which thumbnail we should use, unless they specifically call resolution
> > in a specific order.
> > 
> > In which case is that useful?
> 
> I was thinking of it as a way of solving the problem of whether to set the
> thumbnail or the media art as the GrlMedia’s thumbnail, but you’re right; it
> just punts the choice to the application instead.

Ha, it's useful when the local-metadata plugin sets both the coverart and the thumbnail. In which case it could be useful, but generally, coverart is only set for music and thumbnails for videos. Do we care about thumbnails for audio? Probably not.

So #2 I guess.

Review of attachment 306990 [details] [review]:

I think this actually has a double-free in it: if we go into the g_file_query_info_async() code path, then we free the GCancellable one time too many.

Because it's freed at the end of the overall operation by the GDestroyNotify, there is no need for resolve_album_art_cb() to free it as well.

::: src/local-metadata/grl-local-metadata.c
@@ +851,3 @@
+  resolve_data = user_data;
+
+  cancellable = grl_operation_get_data (resolve_data->rs->operation_id);

No effect on refcount

@@ +868,3 @@
+  g_clear_object (&info);
+  g_clear_error (&error);
+  g_clear_object (&cancellable);

Consumes one ref

@@ +889,1 @@
+  cancellable = g_cancellable_new ();

Creates one ref

@@ +890,3 @@
+  grl_operation_set_data_full (resolve_data->rs->operation_id,
+                               g_object_ref (cancellable),
+                               (GDestroyNotify) g_object_unref);

Adds one ref now, removes one ref later, therefore no net effect

@@ +908,3 @@
 
+  g_clear_object (&cache_file);
+  g_clear_object (&cancellable);

Consumes one ref

Created attachment 307563 [details] [review]
local-metadata: don't double-free cancellable

resolve_album_art() is "balanced" with respect to the cancellable's
refcount: it exits with 1 reference to the cancellable owned by
the operation_id, having arranged for that reference to be destroyed
by a GDestroyNotify when the operation_id finishes. There
is no need for resolve_album_art_cb() to release the same reference.

---

Fix for double-free in Attachment #306990 [details]

Review of attachment 307563 [details] [review]:

Sure.

Comment on attachment 307563 [details] [review]
local-metadata: don't double-free cancellable

committed as b5a05ec for 0.2.15

(In reply to Bastien Nocera from comment #9)
> (In reply to Philip Withnall from comment #8)
> > (In reply to Bastien Nocera from comment #6)
> > > Review of attachment 306991 [details] [review] [review] [review] [review]:
> > > 
> > > Hmm, the problem here is that it means that there's no way for the caller to
> > > know which thumbnail we should use, unless they specifically call resolution
> > > in a specific order.
> > > 
> > > In which case is that useful?
> > 
> > I was thinking of it as a way of solving the problem of whether to set the
> > thumbnail or the media art as the GrlMedia’s thumbnail, but you’re right; it
> > just punts the choice to the application instead.
> 
> Ha, it's useful when the local-metadata plugin sets both the coverart and
> the thumbnail. In which case it could be useful, but generally, coverart is
> only set for music and thumbnails for videos. Do we care about thumbnails
> for audio? Probably not.
> 
> So #2 I guess.

That means we can just drop the patch, so this bug is fixed. Cheers.