hoyon / mpv-mpris

MPRIS plugin for mpv
MIT License
608 stars 35 forks source link

add support for embedded cover art #94

Closed gnojus closed 1 year ago

gnojus commented 1 year ago

This adds a dependency on libavformat to extract the cover art.

Fixes #91

hoyon commented 1 year ago

@gnojus The CI build looks like it's failing. Can you take a look at it? Probably just need to install libavformat in the github actions config.

I'm also wondering if we should be caching the art data as the metadata function usually gets called at least twice per file as mpv usually updates the media-title property after playback has already started. But maybe that's overkill and reading the file twice is fine.

gnojus commented 1 year ago

Added libavformat-dev to workflow, should pass now. What do you mean by caching - just skip setting of the picture if the code is ran second time? I'm not that familiar with whole mpv/dbus interface thing.

hoyon commented 1 year ago

What do you mean by caching

I mean having a global data structure which stores a mapping between file path and album art url (be it a youtube url, a file path or a jpeg). So that the image file doesn't need to be extracted when the metadata changes.

When testing this on my machine the create_metadata function gets called 3 or 4 times when a file starts playing as mpv sends the metadata updated signal multiple times. Having a cache will mean repeated calls to add_metadata_art with the same path won't have to extract the same image again.

jman-schief commented 1 year ago

@gnojus After this patch, will libavformat be a hard-dependency for mpv-mpris? In terms of added runtime dependencies, how much does libavformat pull in?

I didn't yet try rebuilding mpv-mpris after this patch, I'm wondering if it would make sense to make this an optional compile flag.

thanks

gnojus commented 1 year ago

@gnojus After this patch, will libavformat be a hard-dependency for mpv-mpris?

Yes, but it's also a dependency of mpv, so one shouldn't need to install it manually.

I could see the argument for lower cpu/ram usage, but that would also work as a runtime flag.

gnojus commented 1 year ago

@hoyon added some basic caching, with the last known path.