Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add support for embedded cover art #94

Merged
merged 4 commits into from
Aug 30, 2023
Merged

add support for embedded cover art #94

merged 4 commits into from
Aug 30, 2023

Conversation

gnojus
Copy link
Contributor

@gnojus gnojus commented Aug 19, 2023

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

Fixes #91

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

Fixes hoyon#91
mpris.c Outdated
}
}
if (!packet.size) {
return;
Copy link
Contributor

@olifre olifre Aug 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is another:

avformat_close_input(&formatContext);

needed here? If so, probably also in the earlier return case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch, thanks. Rewrote the function.

Rewrite try_put_embedded_art to correctly close avformat_input
@hoyon
Copy link
Owner

hoyon commented Aug 21, 2023

@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
Copy link
Contributor Author

gnojus commented Aug 21, 2023

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
Copy link
Owner

hoyon commented Aug 21, 2023

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
Copy link

@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
Copy link
Contributor Author

gnojus commented Aug 22, 2023

@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
Copy link
Contributor Author

gnojus commented Aug 22, 2023

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

Copy link
Owner

@hoyon hoyon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@hoyon hoyon merged commit f759c86 into hoyon:master Aug 30, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cover Image in notification
4 participants