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 optional cache key for MediaItem art #1001

Open
austinried opened this issue Mar 4, 2023 · 7 comments · May be fixed by #1005
Open

Add optional cache key for MediaItem art #1001

austinried opened this issue Mar 4, 2023 · 7 comments · May be fixed by #1005

Comments

@austinried
Copy link

Feature proposal

I would like to be able to specify an optional cacheKey when defining a MediaItem, so that when art for that item is retrieved from the cache manager I can control what key is used.

Motivating use case(s)

My app uses custom cache keys for images because the URLs used to get them are not guaranteed to be stable.

I can provide a PR if this works for you, as I already have a working branch with this feature in my fork here: https://github.com/austinried/audio_service/tree/features/mediaitem-artcachekey

@ryanheise
Copy link
Owner

If the URls are not stable, then your approach sounds a bit fragile, since you can't predict whether audio_service will resolve the URLs before they expire. You would be better off doing this in a more controlled manner where you obtain your perishable URLs, then preemptively download them to local files in your app's local store, and then simply pass file:// URIs into artUri.

Aside from that, I'm not comfortable with adding a field to MediaItem that isn't actually understood by the native platform, and indeed isn't included in toMessage() for that purpose. Typically non-standard fields should be included in extras but as I said above, the idea of unstable URIs just seems like it is not going to work with the way media sessions work, and the preemptive downloading approach seems safer.

@austinried
Copy link
Author

The URLs don't expire, they're token based and can change because the salt used to generate them changes, or because a user changes their password, etc. I don't want to re-download the image every time in these cases and I'd like to use the cache managers I'm already using with these custom cache keys. I also don't really want to implement my own image pre-caching system outside the cache manager as it does a pretty good job of it already.

If adding a field to MediaItem isn't a good way to do it, an alternative approach I considered was to have an additional param on AudioService.init() for a cacheKeyResolver function that could take a media item and return a cache key. Would that be acceptable?

@ryanheise
Copy link
Owner

I suppose that could work. There is also "maybe" a possibility to implement a custom cache manager (although I don't have the API in front of me right now to confirm that).

@austinried
Copy link
Author

That's what I'm already doing now actually, although not heavily, so I can customize the amount of time things are kept for and a few other things, and I pass that cache manager to audio_service and use it in my app to display the same images from cache with cached_network_image. There are potentially a lot of images I'm putting on a user's device so I want to avoid caching them more than once.

I'm not sure it's possible to customize the cache key resolution in the cache manager without really reworking how it's built, since it allow you to pass a cache key on your own with the URI. Other packages like cached_network_image that also rely on flutter_cache_manager allow passing the cache key when you pass the URI so I figure it'd be reasonable to allow some way to do it here too.

@ryanheise
Copy link
Owner

Understood. I suppose that the cacheKeyResolver approach could be more flexible, since if your cache key can be derived from the URI, it wouldn't require storing extra fields, but if not, you could just store your key into the extras and use cacheKeyResolver to pull it out of the extras.

I don't want to rush API design decisions, so perhaps you can also think on these options and see what you feel is going to be best taking into account all of the considerations.

@austinried
Copy link
Author

austinried commented Mar 4, 2023

Cool, I have another branch with the cacheKeyResolver function but haven't tried implementing it in my app yet, I'll give it a go and see how it feels, and then open a PR if it seems alright.

@austinried austinried linked a pull request Mar 23, 2023 that will close this issue
8 tasks
@Zensonaton
Copy link

Been testing that PR in my app, and it works flawlessly.

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 a pull request may close this issue.

3 participants