From afa1a07a424d4d33ef382d869bec105954a83b55 Mon Sep 17 00:00:00 2001 From: ejurgensen Date: Fri, 21 Feb 2020 19:51:35 +0100 Subject: [PATCH] [artwork] Fix handling of cache + enable online srcs via config * Don't save artwork for permanent items (file + Spotify) to the stash. The stash is only for short term artwork. * If a request comes with a different max_w/max then search the online source again. * Make artwork requests thread-safe by mutex protecting the search history. * Add config option --- src/artwork.c | 186 ++++++++++++++++++++++++++++++++++--------------- src/conffile.c | 1 + src/settings.c | 8 +-- src/settings.h | 2 +- 4 files changed, 135 insertions(+), 62 deletions(-) diff --git a/src/artwork.c b/src/artwork.c index 4fbf6781..cd5fbdde 100644 --- a/src/artwork.c +++ b/src/artwork.c @@ -30,6 +30,7 @@ #include #include #include +#include #include "db.h" #include "misc.h" @@ -78,6 +79,7 @@ enum artwork_cache NEVER = 0, // No caching of any results ON_SUCCESS = 1, // Cache if artwork found ON_FAILURE = 2, // Cache if artwork not found (so we don't keep asking) + STASH = 4, // Only cache "short term": Cache holds just one image, useful for streams }; /* This struct contains the data available to the handler, as well as a char @@ -87,7 +89,7 @@ enum artwork_cache * changes. */ struct artwork_ctx { - // Handler should output path here if artwork is local + // Handler should output path or URL to artwork here char path[PATH_MAX]; // Handler should output artwork data to this evbuffer struct evbuffer *evbuf; @@ -139,25 +141,31 @@ enum parse_result { struct online_source { // Name of online source - char *name; + const char *name; + const bool is_default; + + const char *setting_name; // How to authorize (using the Authorize http header) - char *auth_header; - char *auth_key; - char *auth_secret; + const char *auth_header; + const char *auth_key; + const char *auth_secret; // How to search for artwork - char *search_endpoint; - char *search_param; + const char *search_endpoint; + const char *search_param; struct { const char *key; const char *template; } query_parts[8]; - // Cache previous artwork searches, so we can avoid futile requests + // Remember previous artwork searches, used to avoid futile requests struct { + pthread_mutex_t mutex; int last_id; uint32_t last_hash; + int last_max_w; + int last_max_h; int last_response_code; char *last_artwork_url; time_t last_timestamp; @@ -245,7 +253,7 @@ static struct artwork_source artwork_item_source[] = .name = "stream", .handler = source_item_stream_get, .data_kinds = (1 << DATA_KIND_HTTP), - .cache = NEVER, + .cache = STASH, }, { .name = "pipe", @@ -281,7 +289,7 @@ static struct artwork_source artwork_item_source[] = .name = "Spotify search web api (streams)", .handler = source_item_spotifywebapi_search_get, .data_kinds = (1 << DATA_KIND_HTTP) | (1 << DATA_KIND_PIPE), - .cache = NEVER, + .cache = STASH, }, { .name = "Discogs (files)", @@ -293,7 +301,7 @@ static struct artwork_source artwork_item_source[] = .name = "Discogs (streams)", .handler = source_item_discogs_get, .data_kinds = (1 << DATA_KIND_HTTP) | (1 << DATA_KIND_PIPE), - .cache = NEVER, + .cache = STASH, }, { // The Cover Art Archive seems rather slow, so low priority @@ -307,7 +315,7 @@ static struct artwork_source artwork_item_source[] = .name = "Cover Art Archive (streams)", .handler = source_item_coverartarchive_get, .data_kinds = (1 << DATA_KIND_HTTP) | (1 << DATA_KIND_PIPE), - .cache = NEVER, + .cache = STASH, }, { .name = NULL, @@ -326,6 +334,8 @@ static enum parse_result response_jparse_musicbrainz(char **artwork_url, json_ob static struct online_source spotify_source = { .name = "Spotify", + .is_default = true, + .setting_name = "use_artwork_source_spotify", .auth_header = "Bearer $SECRET$", .search_endpoint = "https://api.spotify.com/v1/search", .search_param = "type=track&limit=1&$QUERY$", @@ -336,11 +346,14 @@ static struct online_source spotify_source = { NULL, NULL }, }, .response_jparse = response_jparse_spotify, + .search_history = { .mutex = PTHREAD_MUTEX_INITIALIZER }, }; static struct online_source discogs_source = { .name = "Discogs", + .is_default = false, + .setting_name = "use_artwork_source_discogs", .auth_header = "Discogs key=$KEY$, secret=$SECRET$", .auth_key = "ivzUxlkUiwpptDKpSCHF", .auth_secret = "CYLZyExtlznKCupoIIhTpHVDReLunhUo", @@ -354,11 +367,14 @@ static struct online_source discogs_source = { NULL, NULL }, }, .response_jparse = response_jparse_discogs, + .search_history = { .mutex = PTHREAD_MUTEX_INITIALIZER }, }; static struct online_source musicbrainz_source = { .name = "Musicbrainz", + .is_default = false, + .setting_name = "use_artwork_source_coverartarchive", .search_endpoint = "http://musicbrainz.org/ws/2/release-group/", .search_param = "limit=1&fmt=json&$QUERY$", .query_parts = @@ -368,6 +384,7 @@ static struct online_source musicbrainz_source = { NULL, NULL }, }, .response_jparse = response_jparse_musicbrainz, + .search_history = { .mutex = PTHREAD_MUTEX_INITIALIZER }, }; @@ -829,8 +846,8 @@ artwork_get_bydir(struct evbuffer *evbuf, char *dir, int max_w, int max_h, char return artwork_get(evbuf, path, NULL, max_w, max_h, false); } -/* Retrieves artwork from an URL. Will use cache as appropriate, and will - * rescale if needed. +/* Retrieves artwork from an URL, will rescale if needed. Checks the cache stash + * before making a request. * * @out artwork Image data * @in url URL of the artwork @@ -852,7 +869,7 @@ artwork_get_byurl(struct evbuffer *artwork, const char *url, int max_w, int max_ { ret = artwork_evbuf_rescale(artwork, raw, max_w, max_h); if (ret < 0) - goto error; + goto error; evbuffer_free(raw); return format; @@ -862,8 +879,6 @@ artwork_get_byurl(struct evbuffer *artwork, const char *url, int max_w, int max_ if (format < 0) goto error; - cache_artwork_stash(raw, url, format); - ret = artwork_evbuf_rescale(artwork, raw, max_w, max_h); if (ret < 0) goto error; @@ -1092,32 +1107,59 @@ online_source_request_url_make(char *url, size_t url_size, struct online_source return -1; } +static char * +online_source_search_check_last(struct online_source *src, uint32_t hash, int max_w, int max_h) +{ + char *last_artwork_url = NULL; + bool is_same; + + pthread_mutex_lock(&src->search_history.mutex); + + is_same = (hash == src->search_history.last_hash) && + (max_w == src->search_history.last_max_w) && + (max_h == src->search_history.last_max_h); + + if (is_same) + last_artwork_url = safe_strdup(src->search_history.last_artwork_url); + + pthread_mutex_unlock(&src->search_history.mutex); + + return last_artwork_url; +} + static bool online_source_is_failing(struct online_source *src, int id) { + bool is_failing; + + pthread_mutex_lock(&src->search_history.mutex); + // If the last request was more than ONLINE_SEARCH_COOLDOWN_TIME ago we will always try again if (time(NULL) > src->search_history.last_timestamp + ONLINE_SEARCH_COOLDOWN_TIME) - return false; - + is_failing = false; // We won't try again if the source was not replying as expected - if (src->search_history.last_response_code != HTTP_OK) - return true; - + else if (src->search_history.last_response_code != HTTP_OK) + is_failing = true; // The playback source has changed since the last search, let's give it a chance // (internet streams can feed us with garbage search metadata, but will not change id) - if (id != src->search_history.last_id) - return false; - + else if (id != src->search_history.last_id) + is_failing = false; // We allow up to ONLINE_SEARCH_FAILURES_MAX for the same track id before declaring failure - if (src->search_history.count_failures < ONLINE_SEARCH_FAILURES_MAX) - return false; + else if (src->search_history.count_failures < ONLINE_SEARCH_FAILURES_MAX) + is_failing = false; + else + is_failing = true; - return true; + pthread_mutex_unlock(&src->search_history.mutex); + + return is_failing; } static void online_source_history_update(struct online_source *src, int id, uint32_t request_hash, int response_code, const char *artwork_url) { + pthread_mutex_lock(&src->search_history.mutex); + src->search_history.last_id = id; src->search_history.last_hash = request_hash; src->search_history.last_response_code = response_code; @@ -1130,6 +1172,8 @@ online_source_history_update(struct online_source *src, int id, uint32_t request src->search_history.count_failures = 0; else src->search_history.count_failures++; + + pthread_mutex_unlock(&src->search_history.mutex); } static char * @@ -1143,6 +1187,8 @@ online_source_search(struct online_source *src, struct artwork_ctx *ctx) char auth_header[256]; int ret; + DPRINTF(E_SPAM, L_ART, "Trying %s for %s\n", src->name, ctx->dbmfi->path); + ret = online_source_request_url_make(url, sizeof(url), src, ctx); if (ret < 0) { @@ -1152,11 +1198,9 @@ online_source_search(struct online_source *src, struct artwork_ctx *ctx) // Be nice to our peer + improve response times by not repeating search requests hash = djb_hash(url, strlen(url)); - if (hash == src->search_history.last_hash) - { - artwork_url = safe_strdup(src->search_history.last_artwork_url); - return artwork_url; - } + artwork_url = online_source_search_check_last(src, hash, ctx->max_w, ctx->max_h); + if (artwork_url) + return artwork_url; // If our recent searches have been futile we may give the source a break if (online_source_is_failing(src, ctx->id)) @@ -1214,20 +1258,37 @@ online_source_search(struct online_source *src, struct artwork_ctx *ctx) } static bool -online_source_is_enabled(const char *setting_name) +online_source_is_enabled(struct online_source *src) { struct settings_category *category; - struct settings_option *option; + cfg_t *lib; + const char *name; + int n_cfg; + int i; - category = settings_category_get("artwork"); - if (!category) - return false; + lib = cfg_getsec(cfg, "library"); - option = settings_option_get(category, setting_name); - if (!option) - return false; + n_cfg = cfg_size(lib, "artwork_online_sources"); + if (n_cfg == 0) // User didn't set anything in the config file, use settings + { + category = settings_category_get("artwork"); + if (!category) + return src->is_default; - return settings_option_getbool(option); +// if (!settings_option_is_set(category, src->setting_name)) +// return src->is_default; + + return settings_option_getbool(settings_option_get(category, src->setting_name)); + } + + for (i = 0; i < n_cfg; i++) + { + name = cfg_getnstr(lib, "artwork_online_sources", i); + if (strcasecmp(name, src->name) == 0) + return true; + } + + return src->is_default; } @@ -1479,13 +1540,15 @@ source_item_discogs_get(struct artwork_ctx *ctx) char *url; int ret; - if (!online_source_is_enabled("enable_discogs")) + if (!online_source_is_enabled(&discogs_source)) return ART_E_NONE; url = online_source_search(&discogs_source, ctx); if (!url) return ART_E_NONE; + snprintf(ctx->path, sizeof(ctx->path), "%s", url); + ret = artwork_get_byurl(ctx->evbuf, url, ctx->max_w, ctx->max_h); free(url); @@ -1498,7 +1561,7 @@ source_item_coverartarchive_get(struct artwork_ctx *ctx) char *url; int ret; - if (!online_source_is_enabled("enable_coverartarchive")) + if (!online_source_is_enabled(&musicbrainz_source)) return ART_E_NONE; // We search Musicbrainz to get the Musicbrainz ID, which we need to get the @@ -1507,7 +1570,8 @@ source_item_coverartarchive_get(struct artwork_ctx *ctx) if (!url) return ART_E_NONE; - // TODO add support in http client for 307 redirects + snprintf(ctx->path, sizeof(ctx->path), "%s", url); + ret = artwork_get_byurl(ctx->evbuf, url, ctx->max_w, ctx->max_h); free(url); @@ -1526,7 +1590,7 @@ source_item_libspotify_get(struct artwork_ctx *ctx) ret = spotify_artwork_get(raw, ctx->dbmfi->path, ctx->max_w, ctx->max_h); if (ret < 0) { - DPRINTF(E_WARN, L_ART, "No artwork from Spotify for %s\n", ctx->dbmfi->path); + DPRINTF(E_WARN, L_ART, "No artwork from libspotify for %s\n", ctx->dbmfi->path); evbuffer_free(raw); return ART_E_NONE; } @@ -1534,7 +1598,7 @@ source_item_libspotify_get(struct artwork_ctx *ctx) ret = artwork_evbuf_rescale(ctx->evbuf, raw, ctx->max_w, ctx->max_h); if (ret < 0) { - DPRINTF(E_LOG, L_ART, "Could not rescale Spotify artwork for '%s'\n", ctx->dbmfi->path); + DPRINTF(E_LOG, L_ART, "Could not rescale libspotify artwork for '%s'\n", ctx->dbmfi->path); evbuffer_free(raw); return ART_E_ERROR; } @@ -1569,7 +1633,7 @@ source_item_spotifywebapi_search_get(struct artwork_ctx *ctx) char *url; int ret; - if (!online_source_is_enabled("enable_spotify")) + if (!online_source_is_enabled(&spotify_source)) return ART_E_NONE; spotifywebapi_access_token_get(&info); @@ -1583,6 +1647,8 @@ source_item_spotifywebapi_search_get(struct artwork_ctx *ctx) if (!url) return ART_E_NONE; + snprintf(ctx->path, sizeof(ctx->path), "%s", url); + ret = artwork_get_byurl(ctx->evbuf, url, ctx->max_w, ctx->max_h); free(url); @@ -1719,7 +1785,7 @@ process_items(struct artwork_ctx *ctx, int item_mode) if (ret > 0) { DPRINTF(E_DBG, L_ART, "Artwork for '%s' found in source '%s'\n", dbmfi.title, artwork_item_source[i].name); - ctx->cache = (artwork_item_source[i].cache & ON_SUCCESS); + ctx->cache = artwork_item_source[i].cache; db_query_end(&ctx->qp); return ret; } @@ -1774,7 +1840,7 @@ process_group(struct artwork_ctx *ctx) if (ret > 0) { DPRINTF(E_DBG, L_ART, "Artwork for group %" PRIi64 " found in source '%s'\n", ctx->persistentid, artwork_group_source[i].name); - ctx->cache = (artwork_group_source[i].cache & ON_SUCCESS); + ctx->cache = artwork_group_source[i].cache; return ret; } else if (ret == ART_E_ABORT) @@ -1805,7 +1871,7 @@ artwork_get_item(struct evbuffer *evbuf, int id, int max_w, int max_h) char filter[32]; int ret; - DPRINTF(E_DBG, L_ART, "Artwork request for item %d\n", id); + DPRINTF(E_DBG, L_ART, "Artwork request for item %d (max_w=%d, max_h=%d)\n", id, max_w, max_h); if (id == DB_MEDIA_FILE_NON_PERSISTENT_ID) return -1; @@ -1832,8 +1898,10 @@ artwork_get_item(struct evbuffer *evbuf, int id, int max_w, int max_h) ret = process_items(&ctx, 1); if (ret > 0) { - if (ctx.cache == ON_SUCCESS) + if (ctx.cache & ON_SUCCESS) cache_artwork_add(CACHE_ARTWORK_INDIVIDUAL, id, max_w, max_h, ret, ctx.path, evbuf); + if (ctx.cache & STASH) + cache_artwork_stash(evbuf, ctx.path, ret); return ret; } @@ -1844,15 +1912,17 @@ artwork_get_item(struct evbuffer *evbuf, int id, int max_w, int max_h) ret = process_group(&ctx); if (ret > 0) { - if (ctx.cache == ON_SUCCESS) + if (ctx.cache & ON_SUCCESS) cache_artwork_add(CACHE_ARTWORK_GROUP, ctx.persistentid, max_w, max_h, ret, ctx.path, evbuf); + if (ctx.cache & STASH) + cache_artwork_stash(evbuf, ctx.path, ret); return ret; } DPRINTF(E_DBG, L_ART, "No artwork found for item %d\n", id); - if (ctx.cache == ON_FAILURE) + if (ctx.cache & ON_FAILURE) cache_artwork_add(CACHE_ARTWORK_GROUP, ctx.persistentid, max_w, max_h, 0, "", evbuf); return -1; @@ -1864,7 +1934,7 @@ artwork_get_group(struct evbuffer *evbuf, int id, int max_w, int max_h) struct artwork_ctx ctx; int ret; - DPRINTF(E_DBG, L_ART, "Artwork request for group %d\n", id); + DPRINTF(E_DBG, L_ART, "Artwork request for group %d (max_w=%d, max_h=%d)\n", id, max_w, max_h); memset(&ctx, 0, sizeof(struct artwork_ctx)); @@ -1887,15 +1957,17 @@ artwork_get_group(struct evbuffer *evbuf, int id, int max_w, int max_h) ret = process_group(&ctx); if (ret > 0) { - if (ctx.cache == ON_SUCCESS) + if (ctx.cache & ON_SUCCESS) cache_artwork_add(CACHE_ARTWORK_GROUP, ctx.persistentid, max_w, max_h, ret, ctx.path, evbuf); + if (ctx.cache & STASH) + cache_artwork_stash(evbuf, ctx.path, ret); return ret; } DPRINTF(E_DBG, L_ART, "No artwork found for group %d\n", id); - if (ctx.cache == ON_FAILURE) + if (ctx.cache & ON_FAILURE) cache_artwork_add(CACHE_ARTWORK_GROUP, ctx.persistentid, max_w, max_h, 0, "", evbuf); return -1; diff --git a/src/conffile.c b/src/conffile.c index 4e40405c..650380f1 100644 --- a/src/conffile.c +++ b/src/conffile.c @@ -92,6 +92,7 @@ static cfg_opt_t sec_library[] = CFG_STR("name_radio", "Radio", CFGF_NONE), CFG_STR_LIST("artwork_basenames", "{artwork,cover,Folder}", CFGF_NONE), CFG_BOOL("artwork_individual", cfg_false, CFGF_NONE), + CFG_STR_LIST("artwork_online_sources", NULL, CFGF_NONE), CFG_STR_LIST("filetypes_ignore", "{.db,.ini,.db-journal,.pdf,.metadata}", CFGF_NONE), CFG_STR_LIST("filepath_ignore", NULL, CFGF_NONE), CFG_BOOL("filescan_disable", cfg_false, CFGF_NONE), diff --git a/src/settings.c b/src/settings.c index 8e2f89d5..699b8431 100644 --- a/src/settings.c +++ b/src/settings.c @@ -33,9 +33,9 @@ static struct settings_option webinterface_options[] = static struct settings_option artwork_options[] = { - { "enable_spotify", SETTINGS_TYPE_BOOL }, - { "enable_discogs", SETTINGS_TYPE_BOOL }, - { "enable_coverartarchive", SETTINGS_TYPE_BOOL }, + { "use_artwork_source_spotify", SETTINGS_TYPE_BOOL }, + { "use_artwork_source_discogs", SETTINGS_TYPE_BOOL }, + { "use_artwork_source_coverartarchive", SETTINGS_TYPE_BOOL }, }; static struct settings_category categories[] = @@ -105,6 +105,7 @@ settings_option_get(struct settings_category *category, const char *name) return NULL; } + int settings_option_getint(struct settings_option *option) { @@ -114,7 +115,6 @@ settings_option_getint(struct settings_option *option) return db_admin_getint(option->name); } - bool settings_option_getbool(struct settings_option *option) { diff --git a/src/settings.h b/src/settings.h index bc4f7b24..0d972608 100644 --- a/src/settings.h +++ b/src/settings.h @@ -42,10 +42,10 @@ settings_option_get_byindex(struct settings_category *category, int index); struct settings_option * settings_option_get(struct settings_category *category, const char *name); + int settings_option_getint(struct settings_option *option); - bool settings_option_getbool(struct settings_option *option);