PR #24360 opened by Niklas Haas (haasn) URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/24360 Patch URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/24360.patch
V2 electric boogaloo >From 8b45d50cd5ea10fcf538cbceac5778a06e25b0c9 Mon Sep 17 00:00:00 2001 From: Niklas Haas <[email protected]> Date: Thu, 3 Sep 2026 22:05:13 +0200 Subject: [PATCH 1/3] avformat/libcurl: return CURLcode alongside AVERROR Useful for the next commit. A slight complication arises from the libcurl-internal distinction between CURLcode and CURLMcode, even though the overlap between the two is quite big. Make an effort to map between the two for a few relevant cases; excluding stuff we probably won't see outside a debugging session. Introduce a new helper to ensure these are always updated in lockstep. Sponsored-by: nxtedition AB Signed-off-by: Niklas Haas <[email protected]> --- libavformat/libcurl.c | 44 ++++++++++++++++++++++++++++++------------- 1 file changed, 31 insertions(+), 13 deletions(-) diff --git a/libavformat/libcurl.c b/libavformat/libcurl.c index a2f7e6201e..e7edb3134f 100644 --- a/libavformat/libcurl.c +++ b/libavformat/libcurl.c @@ -150,8 +150,9 @@ struct CurlContext { pthread_cond_t cond; AVFifo *fifo; int paused; /* write callback paused, FIFO was full */ - int status; /* current stream status (AVERROR code) */ int aborted; /* transfer should stop (open was interrupted) */ + int status; /* current stream status (AVERROR code) */ + CURLcode curl_status; /* corresponding libcurl status code */ }; /* Guards lazy creation of a format context's shared loop. */ @@ -176,6 +177,18 @@ static int curlcode_to_averror(CURLcode code) } } +static int curlmcode_to_curlcode(CURLMcode code) +{ + switch (code) { + case CURLM_OK: return CURLE_OK; + case CURLM_UNKNOWN_OPTION: return CURLE_UNKNOWN_OPTION; + case CURLM_OUT_OF_MEMORY: return CURLE_OUT_OF_MEMORY; + case CURLM_ABORTED_BY_CALLBACK: return CURLE_ABORTED_BY_CALLBACK; + case CURLM_UNRECOVERABLE_POLL: return CURLE_UNRECOVERABLE_POLL; + default: return CURLE_FAILED_INIT; + } +} + static int is_recoverable(CURLcode code) { switch (code) { @@ -198,6 +211,15 @@ static int is_recoverable(CURLcode code) /* curl callbacks (run on the loop thread) */ /* ------------------------------------------------------------------------- */ +static void update_status_locked(CurlContext *c, int status, CURLcode code) +{ + if (c->status) + return; + + c->status = status; + c->curl_status = code; +} + static size_t write_callback(char *ptr, size_t size, size_t nmemb, void *userdata) { CurlContext *c = userdata; @@ -309,8 +331,7 @@ static size_t header_callback(char *ptr, size_t size, size_t nitems, void *userd content_start, c->request_start); c->loop->num_errors++; c->stream_ok = 0; - if (!c->status) - c->status = AVERROR(EIO); + update_status_locked(c, AVERROR(EIO), CURLE_OK); pthread_cond_broadcast(&c->cond); pthread_mutex_unlock(&c->mutex); return len; @@ -361,8 +382,7 @@ static size_t header_callback(char *ptr, size_t size, size_t nitems, void *userd } else { c->loop->num_errors++; c->stream_ok = 0; - if (!c->status) - c->status = ff_http_averror(status, AVERROR(EIO)); + update_status_locked(c, ff_http_averror(status, AVERROR(EIO)), CURLE_OK); } c->probed = 1; pthread_cond_broadcast(&c->cond); @@ -418,8 +438,7 @@ static void start_request(CurlContext *c) curl_multi_strerror(res)); c->active = 0; pthread_mutex_lock(&c->mutex); - if (!c->status) - c->status = AVERROR(EIO); + update_status_locked(c, AVERROR(EIO), curlmcode_to_curlcode(res)); pthread_cond_broadcast(&c->cond); pthread_mutex_unlock(&c->mutex); } @@ -460,10 +479,9 @@ static void on_done(CurlContext *c, CURLcode code) received = c->request_received; /* Advance past delivered bytes so a retry or seek resumes at the right offset. */ if (received > INT64_MAX - c->request_start) { - if (!c->status) - c->status = AVERROR(EIO); received = 0; aborted = 1; + update_status_locked(c, AVERROR(EIO), code); pthread_cond_broadcast(&c->cond); } c->request_start += received; @@ -476,9 +494,8 @@ static void on_done(CurlContext *c, CURLcode code) pthread_mutex_lock(&c->mutex); c->probed = 1; c->stream_ok = 0; - if (!c->status) - c->status = curlcode_to_averror(code); c->loop->num_errors++; + update_status_locked(c, curlcode_to_averror(code), code); pthread_cond_broadcast(&c->cond); pthread_mutex_unlock(&c->mutex); return; @@ -499,6 +516,7 @@ static void on_done(CurlContext *c, CURLcode code) } pthread_mutex_lock(&c->mutex); c->status = AVERROR_EOF; + c->curl_status = CURLE_OK; pthread_cond_broadcast(&c->cond); pthread_mutex_unlock(&c->mutex); return; @@ -521,8 +539,7 @@ static void on_done(CurlContext *c, CURLcode code) /* Unhandled generic curl error */ pthread_mutex_lock(&c->mutex); - if (!c->status) - c->status = curlcode_to_averror(code); + update_status_locked(c, curlcode_to_averror(code), code); pthread_cond_broadcast(&c->cond); pthread_mutex_unlock(&c->mutex); } @@ -561,6 +578,7 @@ static void execute_command(CurlLoop *loop, CurlCmd *cmd) av_fifo_reset2(c->fifo); c->paused = 0; c->status = 0; + c->curl_status = 0; pthread_mutex_unlock(&c->mutex); c->request_start = cmd->pos; c->retry_count = 0; -- 2.52.0 >From e05d4271d2731b686e4874bac910fec46e69f06e Mon Sep 17 00:00:00 2001 From: Niklas Haas <[email protected]> Date: Thu, 3 Sep 2026 22:09:03 +0200 Subject: [PATCH 2/3] avformat/libcurl: report accurate error on CURLm failure In particular, this is interesting for ENOMEM. Sponsored-by: nxtedition AB Signed-off-by: Niklas Haas <[email protected]> --- libavformat/libcurl.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/libavformat/libcurl.c b/libavformat/libcurl.c index e7edb3134f..8785661a49 100644 --- a/libavformat/libcurl.c +++ b/libavformat/libcurl.c @@ -177,6 +177,16 @@ static int curlcode_to_averror(CURLcode code) } } +static int curlmcode_to_averror(CURLMcode code) +{ + switch (code) { + case CURLM_OK: return 0; + case CURLM_UNKNOWN_OPTION: return AVERROR(EINVAL); + case CURLM_OUT_OF_MEMORY: return AVERROR(ENOMEM); + default: return AVERROR(EIO); + } +} + static int curlmcode_to_curlcode(CURLMcode code) { switch (code) { @@ -438,7 +448,7 @@ static void start_request(CurlContext *c) curl_multi_strerror(res)); c->active = 0; pthread_mutex_lock(&c->mutex); - update_status_locked(c, AVERROR(EIO), curlmcode_to_curlcode(res)); + update_status_locked(c, curlmcode_to_averror(res), curlmcode_to_curlcode(res)); pthread_cond_broadcast(&c->cond); pthread_mutex_unlock(&c->mutex); } -- 2.52.0 >From 4c19f2c6da8002835ae9132b2a45357e6a793665 Mon Sep 17 00:00:00 2001 From: Niklas Haas <[email protected]> Date: Thu, 3 Sep 2026 22:10:16 +0200 Subject: [PATCH 3/3] avformat/libcurl: move retry handling to the URL thread Instead of the loop worker immediately retrying requests, this change moves both the retry decision and accounting logic to the calling thread instead. The main motivation here is that it allows us to add a configurable delay between retries, e.g. for exponential backoff or respecting server advertised Retry-After headers. There is some ambiquity about when exactly we want to reset retry_count, with the options roughly being: 1. After fully reading a successful request to the end (old behavior) 2. After successfully reading any number of bytes (new behavior) 3. Never / only on explicit ffurl_seek() I think that both 2 and 3 are defensible, but 1 is arguably the only one that's not really useful; so this patch also represents a mild improvement in that regard - the retry counter only starts ticking up if the request fails several times in a row. The reason I think the old behavior is not useful here is because the rate of *requests* is a bit arbitrary and depends on options like -request_size. Sponsored-by: nxtedition AB Signed-off-by: Niklas Haas <[email protected]> --- libavformat/libcurl.c | 51 +++++++++++++++++++++++++++++++------------ 1 file changed, 37 insertions(+), 14 deletions(-) diff --git a/libavformat/libcurl.c b/libavformat/libcurl.c index 8785661a49..1d60b330a0 100644 --- a/libavformat/libcurl.c +++ b/libavformat/libcurl.c @@ -122,14 +122,15 @@ struct CurlContext { int64_t initial_request_size; int max_retries; - int64_t logical_pos; /* next byte url_read() will return, caller side */ + /* URL thread bookkeeping, not touched by loop thread */ + int64_t logical_pos; /* next byte url_read() will return, caller side */ + int retry_count; /* consecutive recoverable failures */ /* Producer bookkeeping, touched only by the loop thread. */ int active; /* currently added to the multi */ int64_t request_start; /* absolute offset the current request began at */ int64_t request_received;/* bytes delivered in the current request */ int64_t request_end; /* expected end of request, or -1 if unknown */ - int retry_count; /* consecutive recoverable failures */ int is_initial; /* using reduced request size */ /* Per-response-block header scratch, loop thread only. */ @@ -515,7 +516,6 @@ static void on_done(CurlContext *c, CURLcode code) return; if (code == CURLE_OK && c->stream_ok) { - c->retry_count = 0; int64_t file_end = c->content_size > 0 ? c->content_size - 1 : -1; if (c->end_off > 0) file_end = FFMIN(file_end, c->end_off - 1); @@ -537,16 +537,6 @@ static void on_done(CurlContext *c, CURLcode code) c->loop->num_errors++; } - /* Resume seekable transfers after a recoverable error. */ - if (c->seekable && is_recoverable(code) && - c->retry_count < c->max_retries) { - c->retry_count++; - av_log(c->h, AV_LOG_WARNING, "Retrying (#%d) from %"PRId64"\n", - c->retry_count, c->request_start); - start_request(c); - return; - } - /* Unhandled generic curl error */ pthread_mutex_lock(&c->mutex); update_status_locked(c, curlcode_to_averror(code), code); @@ -591,7 +581,6 @@ static void execute_command(CurlLoop *loop, CurlCmd *cmd) c->curl_status = 0; pthread_mutex_unlock(&c->mutex); c->request_start = cmd->pos; - c->retry_count = 0; start_request(c); break; } @@ -1011,6 +1000,35 @@ static int wait_for_probe(CurlContext *c) return ret; } +static int retry_request_locked(URLContext *h) +{ + CurlContext *c = h->priv_data; + const int status = c->status; + const CURLcode code = c->curl_status; + pthread_mutex_unlock(&c->mutex); + + if (c->retry_count >= c->max_retries) { + av_log(h, AV_LOG_ERROR, "Maximum number of retries (%d) reached\n", + c->max_retries); + return AVERROR(EIO); + } + + c->retry_count++; + av_log(h, AV_LOG_WARNING, "Retrying (#%d) from %"PRId64" after %s (%s)\n", + c->retry_count, c->logical_pos, av_err2str(status), curl_easy_strerror(code)); + + /** + * Use a synchronous request to ensure that the seek is registered, and + * the reset of c->state is observable, before the next libcurl_read() + * call, otherwise this might hit the exact same retry path a second time. + */ + int ret = curl_dispatch(c->loop, CMD_SEEK, c, c->logical_pos, 1); + if (ret < 0) + return ret; + + return AVERROR(EAGAIN); /* allow caller to handle interrupts and retry */ +} + static int libcurl_open(URLContext *h, const char *url, int flags, AVDictionary **options) { @@ -1104,13 +1122,17 @@ static int libcurl_read(URLContext *h, unsigned char *buf, int size) av_fifo_read(c->fifo, buf, n); /* Resume a paused transfer once the FIFO is at least half empty. */ unpause = c->paused && av_fifo_can_write(c->fifo) * 2 >= c->buffer_size; + c->retry_count = 0; c->logical_pos += n; pthread_mutex_unlock(&c->mutex); if (unpause) curl_dispatch(c->loop, CMD_UNPAUSE, c, 0, 0); return n; } + if (c->status) { + if (is_recoverable(c->curl_status)) { + return retry_request_locked(h); ret = c->status; break; } @@ -1172,6 +1194,7 @@ static int64_t libcurl_seek(URLContext *h, int64_t pos, int whence) * surfaces on the following url_read(). */ curl_dispatch(c->loop, CMD_SEEK, c, newpos, 1); c->logical_pos = newpos; + c->retry_count = 0; return newpos; } -- 2.52.0 _______________________________________________ ffmpeg-devel mailing list -- [email protected] To unsubscribe send an email to [email protected]
