GNOME Bugzilla – Bug 752437
local-metadata: no thumbnail reported for Vorbis with embedded art
Last modified: 2018-09-24 09:34:13 UTC
I'm not 100% sure whether this is a Grilo bug or a libmediaart bug, but it seems to be easier to fix via grilo-plugins, so I'm filing it here for now. Steps to reproduce: * have a Vorbis track with embedded art in Base64 in the METADATA_BLOCK_PICTURE Vorbis comment field, of type 3 (FRONT_COVER) (the example I have appears to be a conversion of https://www.jamendo.com/en/track/132873/mami-dale to Vorbis, I'm not sure what converter was used) * put it in ~/Music * let XFCE's Tumbler (an implementation of <http://specifications.freedesktop.org/thumbnail-spec/thumbnail-spec-latest.html>) find it and generate a thumbnail in ~/.cache/thumbnails; GNOME's thumbnailing code would probably work equally here, but I haven't tried it * let Tracker index it - tracker-extract processes it with libmediaart, and does not find folder.jpg etc. adjacent to it - in this particular situation we do not have any ability to find media art via network searches, so that possibility can be ignored * browse media objects in all Grilo sources * resolve each media object with flags including GRL_METADATA_KEY_ARTIST, GRL_METADATA_KEY_ALBUM, GRL_METADATA_KEY_THUMBNAIL and ask Grilo for metadata, in particular grl_media_get_thumbnail() Expected result: * either option 1: - Grilo returns the URL to the thumbnail from ~/.cache/thumbnails or option 2: - libmediaart extracts the embedded art and stores it in ~/.cache/media-art - Grilo returns the URL to the copy in ~/.cache/media-art * either way, the thumbnail is the embedded art Option 1 seems to be easier to implement, and it results in the track looking precisely the same in a media-oriented application as it does in a generic file manager like Nautilus or Thunar, which seems like a nice property to have. I'm working on a patch for this. Option 2 seems to be harder to achieve in the short term, because the libmediaart API seems to mostly be in terms of (artist, album) pairs, which doesn't play nicely with per-track cover images (e.g. look at The Slip or Ghosts I-IV by Nine Inch Nails). It also breaks down if the process writing to the cache and the process reading it disagree on what the relevant artist and album are.
(In reply to Simon McVittie from comment #0) > Option 2 seems to be harder to achieve in the short term, because the > libmediaart API seems to mostly be in terms of (artist, album) pairs It would also require that the process that extracts media art for libmediaart, in my case tracker-extract, is actually able to extract cover art. tracker-extract does not currently know how to extract METADATA_BLOCK_PICTURE (or the deprecated COVERART) from Vorbis files at all, which is why I'm seeing this bug for a track whose embedded METADATA_BLOCK_PICTURE is an album cover. Commit e3eb3757 added support for calling into libmediaart for Vorbis files, but did not include support for extracting art embedded in the file itself. tracker-extract *does* currently know how to extract APIC "front cover" or "other" images from MP3 files, but it always assumes they are album art; APIC does not have a way to distinguish between per-track images (like in The Slip) and per-album images. GStreamer, as used by Tumbler and probably also GNOME's thumbnailing code, knows how to extract METADATA_BLOCK_PICTURE from Vorbis, which is why I do get the desired thumbnail from Tumbler. I think using the track-specific thumbnail is likely to be the best solution here, even if tracker-extract is subsequently taught about METADATA_BLOCK_PICTURE.
(In reply to Simon McVittie from comment #1) > tracker-extract does not currently know how to extract > METADATA_BLOCK_PICTURE (or the deprecated COVERART) from Vorbis files at > all Bug #752475
Created attachment 307564 [details] [review] local-metadata: only free cancellable when operation finishes resolve_album_art() already used this technique, but resolve_image() was relying on releasing an implicit reference in got_file_info() around the time it finished the operation, which is not going to work well if we want to chain additional sub-operations that share a cancellable.
Created attachment 307565 [details] [review] local-metadata: consolidate code for allocating a cancellable If the operation_id already has a cancellable attached to it, which might be in use by parallel asynchronous operations, we don't want to allocate another one. This allows chains of async operations to call into each other without error.
Created attachment 307566 [details] [review] local-metadata: try per-track XDG thumbnails before per-album media art Some XDG thumbnailers (including at least Nautilus and Tumbler) are able to extract embedded art from media files and cache it in XDG_CACHE_HOME/thumbnails, just like they would for images and videos. In some cases the thumbnailer might support embedded art even when the libmediaart extractor (in my case tracker-extract) does not. If this embedded art exists, it is likely to be a better match for the track than the album art would be: for instance, http://freemusicarchive.org/music/Nine_Inch_Nails/The_Slip/ has a distinct piece of artwork embedded in each track, none of which match the album cover. Accordingly, I'm trying the thumbnail before falling back to libmediaart, not the other way round. Using the thumbnailer's interpretation of the correct per-track thumbnail also has the benefit that the track is guaranteed to look the same in a Grilo application as it does in a general-purpose file manager, whereas with libmediaart it is possible that the thumbnailer and the libmediaart extractor might choose different images in corner cases. In principle, https://wiki.gnome.org/DraftSpecs/MediaArtStorageSpec supports per-track artwork, which libmediaart could process and store; the specification recommends treating embedded art as the highest possible priority. However, in practice libmediaart does not currently do this. --- This is the actual bug fix. The other two patches are infrastructure to make it work without leaks, double-frees, or accidentally making some operations uncancellable. They assume that Attachment #307563 [details] from Bug #752057 is already applied (it's a simple double-free fix).
(In reply to Simon McVittie from comment #5) > local-metadata: try per-track XDG thumbnails before per-album media art One thing that this doesn't address is the may_resolve() vfunc, which currently says we need both GRL_METADATA_KEY_ARTIST and GRL_METADATA_KEY_ALBUM to be able to resolve GRL_METADATA_KEY_THUMBNAIL. This is no longer true: we *might* now be able to resolve THUMBNAIL anyway, or we might need ARTIST and ALBUM anyway; it depends. In particular, I have one file that doesn't index correctly due to an apparent Tracker bug, so Grilo doesn't tell me its artist or album, but Tumbler is able to thumbnail it. I would hope that Grilo would give me the cached thumbnail generated by Tumbler. How should I encode this in the may_resolve() vfunc / what is that vfunc used for? Would it be OK to say we may_resolve GRL_METADATA_KEY_THUMBNAIL for all audio files?
Review of attachment 307564 [details] [review]: ::: src/local-metadata/grl-local-metadata.c @@ +818,3 @@ cancellable = g_cancellable_new (); + grl_operation_set_data_full (resolve_data->rs->operation_id, + cancellable, /* transfer ownership */ Move this comment to one above the function call, for example: "The operation owns the cancellable".
Review of attachment 307565 [details] [review]: Looks good otherwise. ::: src/local-metadata/grl-local-metadata.c @@ +535,3 @@ + cancellable = grl_operation_get_data (resolve_data->rs->operation_id); + + if (cancellable) { No need for braces for one-line conditionals. @@ +541,3 @@ + cancellable = g_cancellable_new (); + grl_operation_set_data_full (resolve_data->rs->operation_id, + cancellable, /* transfer ownership */ Ditto the previous patch.
Review of attachment 307566 [details] [review]: > Some XDG thumbnailers (including at least Nautilus and Tumbler) are It's not nautilus doing that, but totem's video thumbnailer.
(In reply to Bastien Nocera from comment #7) > Move this comment to one above the function call, for example: > "The operation owns the cancellable". This is accepted-commit_now with that change, right?
(In reply to Simon McVittie from comment #10) > (In reply to Bastien Nocera from comment #7) > > Move this comment to one above the function call, for example: > > "The operation owns the cancellable". > > This is accepted-commit_now with that change, right? Yes.
Comment on attachment 307564 [details] [review] local-metadata: only free cancellable when operation finishes 67ea3ac, with the comment adjusted as you said
Comment on attachment 307565 [details] [review] local-metadata: consolidate code for allocating a cancellable 6041724, with the comment adjusted and the extra braces removed
Comment on attachment 307566 [details] [review] local-metadata: try per-track XDG thumbnails before per-album media art 42a9391
Fixed in master for 0.2.15
(In reply to Simon McVittie from comment #6) > (In reply to Simon McVittie from comment #5) > > local-metadata: try per-track XDG thumbnails before per-album media art > > One thing that this doesn't address is the may_resolve() vfunc, which > currently says we need both GRL_METADATA_KEY_ARTIST and > GRL_METADATA_KEY_ALBUM to be able to resolve GRL_METADATA_KEY_THUMBNAIL. > > This is no longer true: we *might* now be able to resolve THUMBNAIL anyway, > or we might need ARTIST and ALBUM anyway; it depends. In particular, I have > one file that doesn't index correctly due to an apparent Tracker bug, so > Grilo doesn't tell me its artist or album, but Tumbler is able to thumbnail > it. I would hope that Grilo would give me the cached thumbnail generated by > Tumbler. > > How should I encode this in the may_resolve() vfunc / what is that vfunc > used for? Would it be OK to say we may_resolve GRL_METADATA_KEY_THUMBNAIL > for all audio files? I don’t think these questions/changes to may_resolve() were resolved?
(In reply to Philip Withnall from comment #16) > (In reply to Simon McVittie from comment #6) > > How should I encode this in the may_resolve() vfunc / what is that vfunc > > used for? Would it be OK to say we may_resolve GRL_METADATA_KEY_THUMBNAIL > > for all audio files? > > I don’t think these questions/changes to may_resolve() were resolved? Yes, you're quite right. I'd appreciate any feedback on these from people who understand that API better.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/grilo/issues/63.