PR #25753 opened by Niklas Haas (haasn) URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/25753 Patch URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/25753.patch
>From 163e21524ca81d2dc9510772208bb82cfc28dd09 Mon Sep 17 00:00:00 2001 From: Niklas Haas <[email protected]> Date: Wed, 4 Nov 2026 14:37:22 +0100 Subject: [PATCH 1/4] avformat/shared: rename blocks_cached to blocks_allocated (cosmetic) This better reflects what the field is actually tracking, which is an upper bound on the number of stored blocks. It's an upper bound because we already don't decrement this when retrying a failed block that becomes NONE again. Sponsored-by: nxtedition AB Signed-off-by: Niklas Haas <[email protected]> --- libavformat/shared.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/libavformat/shared.c b/libavformat/shared.c index 38a64412a7d..13453cf8ab7 100644 --- a/libavformat/shared.c +++ b/libavformat/shared.c @@ -130,7 +130,7 @@ typedef struct Spacemap { atomic_ushort block_shift; atomic_ullong filesize; /* byte offset of true EOF, or 0 if unknown */ atomic_uchar hash[HASH_SIZE]; /* hash of resource URI / filename */ - atomic_ullong blocks_cached; /* (lower bound on) the number of blocks cached */ + atomic_ullong blocks_allocated; /* number of blocks allocated (upper bound) */ char reserved[72]; Block blocks[]; @@ -673,7 +673,7 @@ read_block: if (s->read_only || s->write_err || !s->inner) break; /* don't mark block as pending */ else if (s->cache_size_max) { - int64_t cached = atomic_load_explicit(&s->spacemap->blocks_cached, + int64_t cached = atomic_load_explicit(&s->spacemap->blocks_allocated, memory_order_relaxed); if (cached >= s->blocks_max) { av_log(h, AV_LOG_WARNING, "Cache size limit reached (%"PRId64" " @@ -843,8 +843,10 @@ read_block: "offset 0x%"PRIx64", CRC 0x%08X\n", bytes_read, block_id, block_pos, crc); atomic_store_explicit(&block->state, crc, memory_order_release); - if (allocated) - atomic_fetch_add_explicit(&s->spacemap->blocks_cached, 1, memory_order_release); + if (allocated) { + atomic_fetch_add_explicit(&s->spacemap->blocks_allocated, 1, + memory_order_release); + } } } else { RELEASE_PENDING(block, state); -- 2.54.0 >From ee4bb064438a11231e4bae73ca957425bf9dbcfc Mon Sep 17 00:00:00 2001 From: Niklas Haas <[email protected]> Date: Wed, 4 Nov 2026 15:02:02 +0100 Subject: [PATCH 2/4] avformat/shared: move block state transitions to shared helper Also switches to acq_rel/acquire order; which is a bit stronger than before but needed for the following commit Sponsored-by: nxtedition AB Signed-off-by: Niklas Haas <[email protected]> --- libavformat/shared.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/libavformat/shared.c b/libavformat/shared.c index 13453cf8ab7..caf4ea27002 100644 --- a/libavformat/shared.c +++ b/libavformat/shared.c @@ -158,6 +158,16 @@ DEF_SET_ONCE(unsigned int, uint) DEF_SET_ONCE(unsigned short, ushort) DEF_SET_ONCE(unsigned long long, ullong) +static int set_block_state(Spacemap *map, Block *block, unsigned *state, + unsigned new_state) +{ + (void) map; + return atomic_compare_exchange_strong_explicit(&block->state, state, + new_state, + memory_order_acq_rel, + memory_order_acquire); +} + typedef struct SharedContext { AVClass *class; URLContext *inner; @@ -684,11 +694,7 @@ read_block: } } - if (atomic_compare_exchange_strong_explicit(&block->state, &state, - BLOCK_PENDING, - memory_order_acquire, - memory_order_acquire)) - { + if (set_block_state(s->spacemap, block, &state, BLOCK_PENDING)) { /* Acquired pending state, proceed to fetch the block */ acquired = 1; allocated = (state == BLOCK_NONE || state == BLOCK_FAILED); @@ -727,9 +733,7 @@ read_block: do { \ if (acquired) { \ av_assert1(state == BLOCK_PENDING); \ - atomic_compare_exchange_strong_explicit( \ - &block->state, &state, BLOCK_NONE, memory_order_relaxed, \ - memory_order_relaxed); \ + set_block_state(s->spacemap, block, &state, BLOCK_NONE); \ } \ } while (0) @@ -808,10 +812,7 @@ read_block: /* Try to mark block as failed; ignore errors - any mismatch * here will mean that either another thread already marked it * as failed, or successfully cached it in the meantime */ - atomic_compare_exchange_strong_explicit(&block->state, &state, - BLOCK_FAILED, - memory_order_relaxed, - memory_order_relaxed); + set_block_state(s->spacemap, block, &state, BLOCK_FAILED); return ret; } -- 2.54.0 >From 824a9ffa32d80fe424431b0426d2fb0425aa9d92 Mon Sep 17 00:00:00 2001 From: Niklas Haas <[email protected]> Date: Wed, 4 Nov 2026 16:49:59 +0100 Subject: [PATCH 3/4] avformat/shared: keep blocks_allocated in sync with block states This counts blocks as allocated as soon as they are marked PENDING, which more faithfully represents the disk usage - which is what this field is used for. This also fixes two accounting conditions that could result in an off-by-one when a timeout racer publishes a new block. Bump the cache version to ensure that this logic doesn't break when encountering legacy files, since the forced decrement would otherwise possibly decrement the blocks_allocated counter past zero. Sponsored-by: nxtedition AB Signed-off-by: Niklas Haas <[email protected]> --- libavformat/shared.c | 38 +++++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 13 deletions(-) diff --git a/libavformat/shared.c b/libavformat/shared.c index caf4ea27002..91c9698f8c5 100644 --- a/libavformat/shared.c +++ b/libavformat/shared.c @@ -67,7 +67,7 @@ #define HASH_METHOD "SHA512/256" #define HASH_SIZE 32 #define HEADER_MAGIC MKTAG(u'\xFF', 'S', 'h', '$') -#define HEADER_VERSION 3 +#define HEADER_VERSION 4 /** * Hard watershed of consecutive failed blocks before we give up on the cache @@ -161,11 +161,27 @@ DEF_SET_ONCE(unsigned long long, ullong) static int set_block_state(Spacemap *map, Block *block, unsigned *state, unsigned new_state) { - (void) map; - return atomic_compare_exchange_strong_explicit(&block->state, state, - new_state, - memory_order_acq_rel, - memory_order_acquire); + int prev_alloc = *state != BLOCK_NONE && *state != BLOCK_FAILED; + int new_alloc = new_state != BLOCK_NONE && new_state != BLOCK_FAILED; + + /* Increment the block allocation count *before* we acquire the block; + * otherwise another racing process might see a pending block, and mark + * it as failed, decrementing again before our own post-increment + * finishes. */ + if (new_alloc && !prev_alloc) + atomic_fetch_add_explicit(&map->blocks_allocated, 1, memory_order_relaxed); + + int ok = atomic_compare_exchange_strong_explicit(&block->state, state, + new_state, + memory_order_acq_rel, + memory_order_acquire); + + /* If the CAS failed, we need to rollback any change we made. We also + * want to rollback when resetting a block back to FAILED/NONE */ + if (ok ? (prev_alloc && !new_alloc) : (new_alloc && !prev_alloc)) + atomic_fetch_sub_explicit(&map->blocks_allocated, 1, memory_order_relaxed); + + return ok; } typedef struct SharedContext { @@ -610,7 +626,7 @@ static int shared_read(URLContext *h, unsigned char *buf, int size) Block *const block = &s->spacemap->blocks[block_id]; unsigned state = atomic_load_explicit(&block->state, memory_order_acquire); int64_t pending_since = 0; - int verify_read = 0, acquired = 0, allocated = 0; + int verify_read = 0, acquired = 0; retry: switch (state) { @@ -697,7 +713,6 @@ read_block: if (set_block_state(s->spacemap, block, &state, BLOCK_PENDING)) { /* Acquired pending state, proceed to fetch the block */ acquired = 1; - allocated = (state == BLOCK_NONE || state == BLOCK_FAILED); state = BLOCK_PENDING; break; } @@ -843,11 +858,8 @@ read_block: av_log(h, AV_LOG_TRACE, "Cached %d bytes to block 0x%"PRIx64" at " "offset 0x%"PRIx64", CRC 0x%08X\n", bytes_read, block_id, block_pos, crc); - atomic_store_explicit(&block->state, crc, memory_order_release); - if (allocated) { - atomic_fetch_add_explicit(&s->spacemap->blocks_allocated, 1, - memory_order_release); - } + while (!set_block_state(s->spacemap, block, &state, crc)) + ; } } else { RELEASE_PENDING(block, state); -- 2.54.0 >From de0e283460459ab528435523b520e65d2e08b0a5 Mon Sep 17 00:00:00 2001 From: Niklas Haas <[email protected]> Date: Wed, 4 Nov 2026 17:49:42 +0100 Subject: [PATCH 4/4] avformat/shared: also count the number of blocks cached This allows other tools (e.g. third party wrappers) to analyze a cache file to know what percentage of the file is successfully cached, or to statically know that a cache is complete (at which point we can just bypass the underlying protocol and directly read the cached file). This doesn't need a header bump because the default state is 0, which reads identically to a file that has no such metadata yet. It would be nice if we could use this to also skip opening the inner URL in this case, but we still to be able to parse the inner URL's options in this case; which requires some API changes that are out of scope of this change. Sponsored-by: nxtedition AB Signed-off-by: Niklas Haas <[email protected]> --- libavformat/shared.c | 52 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 50 insertions(+), 2 deletions(-) diff --git a/libavformat/shared.c b/libavformat/shared.c index 91c9698f8c5..dbe3aa8e21c 100644 --- a/libavformat/shared.c +++ b/libavformat/shared.c @@ -131,7 +131,8 @@ typedef struct Spacemap { atomic_ullong filesize; /* byte offset of true EOF, or 0 if unknown */ atomic_uchar hash[HASH_SIZE]; /* hash of resource URI / filename */ atomic_ullong blocks_allocated; /* number of blocks allocated (upper bound) */ - char reserved[72]; + atomic_ullong blocks_cached; /* number of blocks definitely cached */ + char reserved[64]; Block blocks[]; } Spacemap; @@ -226,11 +227,53 @@ typedef struct SharedContext { int64_t nb_miss; } SharedContext; +/* Opportunistically update the lower bound on the number of cached blocks */ +static void shared_count_blocks(const SharedContext *s) +{ + Spacemap *const map = s->spacemap; + if (!map) + return; + + const uint64_t size = atomic_load_explicit(&map->filesize, memory_order_relaxed); + const int shift = atomic_load_explicit(&map->block_shift, memory_order_relaxed); + if (!size || !shift) + return; + + const size_t mapped = (s->map_size - sizeof(Spacemap)) / sizeof(Block); + const size_t num_blocks = FFMIN(AV_CEIL_RSHIFT(size, shift), mapped); + unsigned long long prev_cached; + prev_cached = atomic_load_explicit(&map->blocks_cached, memory_order_relaxed); + if (prev_cached >= num_blocks) + return; + + uint64_t cached = 0; + for (size_t i = 0; i < num_blocks; i++) { + switch (atomic_load_explicit(&map->blocks[i].state, memory_order_relaxed)) { + case BLOCK_NONE: + case BLOCK_PENDING: + case BLOCK_FAILED: + break; + default: + cached++; + break; + } + } + + /* atomic_max_explicit(&map->blocks_cached, cached, memory_order_relaxed) */ + while (prev_cached < cached && + !atomic_compare_exchange_weak_explicit(&map->blocks_cached, &prev_cached, + cached, memory_order_relaxed, + memory_order_relaxed)) + ; +} + static int shared_close(URLContext *h) { SharedContext *s = h->priv_data; ffurl_close(s->inner); + shared_count_blocks(s); + av_file_unmap_shared(s->spacemap, s->map_size); if (s->fd != -1) close(s->fd); @@ -353,8 +396,13 @@ static int shared_open(URLContext *h, const char *arg, int flags, AVDictionary * } ret = spacemap_init(h, hash); - if (ret < 0) + if (ret < 0) { + /* Unmap again so shared_close() doesn't try to update it */ + av_file_unmap_shared(s->spacemap, s->map_size); + s->spacemap = NULL; + s->map_size = 0; goto fail; + } /* s->block_shift is fully settled after spacemap_init() */ s->block_size = 1 << s->block_shift; -- 2.54.0 _______________________________________________ ffmpeg-devel mailing list -- [email protected] To unsubscribe send an email to [email protected]
