Emma Worley (HE12025-04-22):
> Adds a generic hash table with the DXV encoder as an initial use case.
>
> Signed-off-by: Emma Worley <e...@emma.gg>
> ---
> libavcodec/Makefile | 2 +
> libavcodec/hashtable.c | 192 +++++++++++++++++++++++++++++++++++
> libavcodec/hashtable.h | 91 +++++++++++++++++
Thanks.
Since it is no longer public and the only user for now has small entries
and a bounded total size, I would suggest to change back all the
unsigned integers that I-do-not-remember-who had you replace with size_t
and see the effect on speed. I suspect it might be non-negligible. You
can use a typedef to make switching between size_t and unsigned easy.
> libavcodec/tests/hashtable.c | 110 ++++++++++++++++++++
> 4 files changed, 395 insertions(+)
> create mode 100644 libavcodec/hashtable.c
> create mode 100644 libavcodec/hashtable.h
> create mode 100644 libavcodec/tests/hashtable.c
>
> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> index 7bd1dbec9a..8071c59378 100644
> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -42,6 +42,7 @@ OBJS = ac3_parser.o
> \
> dv_profile.o \
> encode.o \
> get_buffer.o \
> + hashtable.o \
> imgconvert.o \
> jni.o \
> lcevcdec.o \
> @@ -1321,6 +1322,7 @@ TESTPROGS = avcodec
> \
> bitstream_le \
> celp_math \
> codec_desc \
> + hashtable \
> htmlsubtitles \
> jpeg2000dwt \
> mathops \
> diff --git a/libavcodec/hashtable.c b/libavcodec/hashtable.c
> new file mode 100644
> index 0000000000..4d6d4a9506
> --- /dev/null
> +++ b/libavcodec/hashtable.c
> @@ -0,0 +1,192 @@
> +/*
> + * Generic hashtable
> + * Copyright (C) 2024 Connor Worley <connorbwor...@gmail.com>
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301
> USA
> + */
> +
> +#include <stdint.h>
> +#include <string.h>
> +
> +#include "libavutil/crc.h"
> +#include "libavutil/error.h"
> +#include "libavutil/mem.h"
> +#include "hashtable.h"
> +
> +#define ALIGN _Alignof(size_t)
> +
> +struct FFHashtableContext {
> + size_t key_size;
> + size_t key_size_aligned;
> + size_t val_size;
> + size_t val_size_aligned;
> + size_t entry_size;
> + size_t max_entries;
> + size_t utilization;
Nit: nb_entries?
> + const AVCRC *crc;
> + uint8_t *table;
> + uint8_t *swapbuf;
> +};
> +
> +#define ENTRY_PSL(entry) (entry)
Except for the definition of the next macro, it is always used with
*(size_t*) in front, you might as well make it part of the macro.
> +#define ENTRY_OCC(entry) (ENTRY_PSL(entry) + FFALIGN(sizeof(size_t), ALIGN))
Is it a flag that tells if the bucket is occupied? Maybe add a comment.
Also, you could make *ENTRY_PSL(entry) one more than it is currently,
zero for an empty bucket. It saves one field. And less memory without
more complex code should be faster.
> +#define ENTRY_KEY(entry) (ENTRY_OCC(entry) + FFALIGN(sizeof(size_t), ALIGN))
> +#define ENTRY_VAL(entry) (ENTRY_KEY(entry) + ctx->key_size_aligned)
> +
> +#define KEYS_EQUAL(k1, k2) !memcmp(k1, k2, ctx->key_size)
Not necessary here but usually a good practice to protect macros with
parentheses.
> +
> +int ff_hashtable_alloc(struct FFHashtableContext **ctx, size_t key_size,
> size_t val_size, size_t max_entries)
> +{
> + struct FFHashtableContext *res = av_malloc(sizeof(struct
> FFHashtableContext));
> + if (!res)
> + return AVERROR(ENOMEM);
> + res->key_size = key_size;
> + res->key_size_aligned = FFALIGN(key_size, ALIGN);
> + res->val_size = val_size;
> + res->val_size_aligned = FFALIGN(val_size, ALIGN);
> + res->entry_size = FFALIGN(sizeof(size_t), ALIGN)
> + + FFALIGN(sizeof(size_t), ALIGN)
> + + res->key_size_aligned
> + + res->val_size_aligned;
> + res->max_entries = max_entries;
> + res->utilization = 0;
> + res->crc = av_crc_get_table(AV_CRC_32_IEEE);
> + if (!res->crc) {
> + ff_hashtable_freep(&res);
> + return AVERROR_BUG;
> + }
> + res->table = av_calloc(res->max_entries, res->entry_size);
> + if (!res->table) {
> + ff_hashtable_freep(&res);
> + return AVERROR(ENOMEM);
> + }
> + res->swapbuf = av_calloc(2, res->key_size_aligned +
> res->val_size_aligned);
> + if (!res->swapbuf) {
> + ff_hashtable_freep(&res);
> + return AVERROR(ENOMEM);
> + }
Since they will not be resized individually, you can allocate these in
the same segment as the table itself.
> + *ctx = res;
> + return 0;
> +}
> +
> +static size_t hash_key(const struct FFHashtableContext *ctx, const void *key)
> +{
> + return av_crc(ctx->crc, 0, key, ctx->key_size) % ctx->max_entries;
> +}
> +
> +int ff_hashtable_get(const struct FFHashtableContext *ctx, const void *key,
> void *val)
> +{
> + size_t hash = hash_key(ctx, key);
> +
> + if (!ctx->utilization)
> + return 0;
Does this optimization make a lot of difference? If not, utilization can
be removed entirely.
I expect the compiler will do the right thing, but better compute hash
after checking utilization anyway.
> +
> + for (size_t psl = 0; psl < ctx->max_entries; psl++) {
> + size_t wrapped_index = (hash + psl) % ctx->max_entries;
> + uint8_t *entry = ctx->table + wrapped_index * ctx->entry_size;
> + if (!*ENTRY_OCC(entry) || *(size_t*)ENTRY_PSL(entry) < psl)
> + return 0;
> + if (KEYS_EQUAL(ENTRY_KEY(entry), key)) {
> + memcpy(val, ENTRY_VAL(entry), ctx->val_size);
For a generic hash table, I would recommend returning a pointer to the
internal memory, valid until the next modification of the table. But
since the keys are smaller than pointers...
> + return 1;
> + }
> + }
> + return 0;
> +}
> +
> +int ff_hashtable_set(struct FFHashtableContext *ctx, const void *key, const
> void *val)
> +{
> + int swapping = 0;
> + size_t psl = 0;
A few comments explaining the semantic of psl would probably be useful.
> + size_t hash = hash_key(ctx, key);
> + uint8_t *set = ctx->swapbuf;
> + uint8_t *tmp = ctx->swapbuf + ctx->key_size_aligned +
> ctx->val_size_aligned;
> +
> + memcpy(set, key, ctx->key_size);
> + memcpy(set + ctx->key_size_aligned, val, ctx->val_size);
> +
> + for (size_t i = 0; i < ctx->max_entries; i++) {
> + size_t wrapped_index = (hash + i) % ctx->max_entries;
if (++wrapped_index == max_entries) wrapped_index = 0 might be faster.
> + uint8_t *entry = ctx->table + wrapped_index * ctx->entry_size;
> + if (!*ENTRY_OCC(entry) || (!swapping && KEYS_EQUAL(ENTRY_KEY(entry),
> set))) {
> + if (!*ENTRY_OCC(entry))
> + ctx->utilization++;
> + *(size_t*)ENTRY_PSL(entry) = psl;
> + *ENTRY_OCC(entry) = 1;
> + memcpy(ENTRY_KEY(entry), set, ctx->key_size_aligned +
> ctx->val_size);
> + return 1;
> + }
> + if (*(size_t*)ENTRY_PSL(entry) < psl) {
> + if (ctx->utilization == ctx->max_entries)
> + return 0;
> + swapping = 1;
> + // set needs to swap with entry
> + memcpy(tmp, ENTRY_KEY(entry), ctx->key_size_aligned +
> ctx->val_size);
> + memcpy(ENTRY_KEY(entry), set, ctx->key_size_aligned +
> ctx->val_size);
I suspect copying until val_size_aligned might be faster.
Also, FFSWAP()ing the contents of entry and set (by blocks of size_t)
would make tmp and associated memory unnecessary. But I cannot predict
if it would make things slower or faster.
I kind of hoped to find a way to get rid of swapbuf entirely, but I did
not.
> + FFSWAP(uint8_t*, set, tmp);
> + FFSWAP(size_t, psl, *(size_t*)ENTRY_PSL(entry));
> + }
> + psl++;
> + }
> + return 0;
> +}
> +
> +int ff_hashtable_delete(struct FFHashtableContext *ctx, const void *key)
> +{
> + uint8_t *next_entry;
> + size_t hash = hash_key(ctx, key);
> +
> + if (!ctx->utilization)
> + return 0;
> +
> + for (size_t psl = 0; psl < ctx->max_entries; psl++) {
> + size_t wrapped_index = (hash + psl) % ctx->max_entries;
> + uint8_t *entry = ctx->table + wrapped_index * ctx->entry_size;
> + if (!*ENTRY_OCC(entry) || *(size_t*)ENTRY_PSL(entry) < psl)
> + return 0;
> + if (KEYS_EQUAL(ENTRY_KEY(entry), key)) {
> + *ENTRY_OCC(entry) = 0;
> + for (psl++; psl < ctx->max_entries; psl++) {
> + wrapped_index = (hash + psl) % ctx->max_entries;
> + next_entry = ctx->table + wrapped_index * ctx->entry_size;
> + if (!*ENTRY_OCC(next_entry) ||
> !*(size_t*)ENTRY_PSL(next_entry)) {
> + ctx->utilization--;
> + return 1;
> + }
> + memcpy(entry, next_entry, ctx->entry_size);
> + (*(size_t*)ENTRY_PSL(entry))--;
> + *ENTRY_OCC(next_entry) = 0;
> + entry = next_entry;
> + }
> + }
> + };
> + return 0;
> +}
I need to wrap this mail. I assume testing confirms the code does what
it should.
> +
> +void ff_hashtable_clear(struct FFHashtableContext *ctx)
> +{
> + memset(ctx->table, 0, ctx->entry_size * ctx->max_entries);
> +}
> +
> +void ff_hashtable_freep(struct FFHashtableContext **ctx)
> +{
> + if (*ctx) {
> + av_freep(&(*ctx)->table);
> + av_freep(&(*ctx)->swapbuf);
> + }
> + av_freep(ctx);
> +}
> diff --git a/libavcodec/hashtable.h b/libavcodec/hashtable.h
> new file mode 100644
> index 0000000000..fd28d3bdd7
> --- /dev/null
> +++ b/libavcodec/hashtable.h
> @@ -0,0 +1,91 @@
> +/*
> + * Generic hashtable
> + * Copyright (C) 2024 Connor Worley <connorbwor...@gmail.com>
Some argued against having names in the copyright notice of the public
headers. This is no longer public, but it could become.
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301
> USA
> + */
> +
> +#ifndef AVCODEC_HASHTABLE_H
> +#define AVCODEC_HASHTABLE_H
> +
> +#include <stddef.h>
> +
> +/* Implements a hash table using Robin Hood open addressing.
> + * See: https://cs.uwaterloo.ca/research/tr/1986/CS-86-14.pdf
> + */
> +
> +struct FFHashtableContext;
We usually typedef too.
> +
> +/**
> + * Creates a fixed-sized Robin Hood hash table.
We usually use some kind of weird impersonal / imperative: Create. Same
below.
> + *
> + * @param ctx context to allocate and initialize
> + * @param key_size size of key type in bytes
> + * @param val_size size of value type in bytes
> + * @param max_entries maximum number of key-value pairs to store
> + *
> + * @return zero on success, nonzero on error
> + */
> +int ff_hashtable_alloc(struct FFHashtableContext **ctx, size_t key_size,
> size_t val_size, size_t max_entries);
> +
> +/**
> + * Looks up a value from a hash table given a key.
> + *
> + * @param ctx hash table context
> + * @param key pointer to key data
> + * @param val destination pointer for value data
> + *
> + * @return 1 if the key is found, zero if the key is not found
> + */
> +int ff_hashtable_get(const struct FFHashtableContext *ctx, const void *key,
> void *val);
> +
> +/**
> + * Stores a value in a hash table given a key.
> + *
> + * @param ctx hash table context
> + * @param key pointer to key data
> + * @param val pointer for value data
> + *
> + * @return 1 if the key is written, zero if the key is not written due to
> the hash table reaching max capacity
> + */
> +int ff_hashtable_set(struct FFHashtableContext *ctx, const void *key, const
> void *val);
> +
> +/**
> + * Deletes a value from a hash table given a key.
> + *
> + * @param ctx hash table context
> + * @param key pointer to key data
> + *
> + * @return 1 if the key is deleted, zero if the key is not deleted due to
> not being found
> + */
> +int ff_hashtable_delete(struct FFHashtableContext *ctx, const void *key);
> +
> +/**
> + * Deletes all values from a hash table.
> + *
> + * @param ctx hash table context
> + */
> +void ff_hashtable_clear(struct FFHashtableContext *ctx);
> +
> +/**
> + * Frees a hash table.
> + *
> + * @param ctx hash table context
> + */
> +void ff_hashtable_freep(struct FFHashtableContext **ctx);
> +
> +#endif
> diff --git a/libavcodec/tests/hashtable.c b/libavcodec/tests/hashtable.c
> new file mode 100644
> index 0000000000..367baf4f96
> --- /dev/null
> +++ b/libavcodec/tests/hashtable.c
> @@ -0,0 +1,110 @@
> +/*
> + * Generic hashtable tests
> + * Copyright (C) 2024 Connor Worley <connorbwor...@gmail.com>
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301
> USA
> + */
> +
> +#include <stdint.h>
> +
> +#include "libavutil/avassert.h"
> +#include "libavcodec/hashtable.h"
> +
> +int main(void)
> +{
> + struct FFHashtableContext *ctx;
> + uint8_t k;
> + uint64_t v;
> +
> + // impossibly large allocation should fail gracefully
> + av_assert0(ff_hashtable_alloc(&ctx, -1, -1, -1) < 0);
> +
> + // hashtable can store up to 3 uint8_t->uint64_t entries
> + av_assert0(!ff_hashtable_alloc(&ctx, sizeof(k), sizeof(v), 3));
> +
> + // unsuccessful deletes return 0
> + k = 1;
> + av_assert0(!ff_hashtable_delete(ctx, &k));
> +
> + // unsuccessful gets return 0
> + k = 1;
> + av_assert0(!ff_hashtable_get(ctx, &k, &v));
> +
> + // successful sets returns 1
> + k = 1;
> + v = 1;
> + av_assert0(ff_hashtable_set(ctx, &k, &v));
> +
> + // get should now contain 1
> + k = 1;
> + v = 0;
> + av_assert0(ff_hashtable_get(ctx, &k, &v));
> + av_assert0(v == 1);
> +
> + // updating sets should return 1
> + k = 1;
> + v = 2;
> + av_assert0(ff_hashtable_set(ctx, &k, &v));
> +
> + // get should now contain 2
> + k = 1;
> + v = 0;
> + av_assert0(ff_hashtable_get(ctx, &k, &v));
> + av_assert0(v == 2);
> +
> + // fill the table
> + k = 2;
> + v = 2;
> + av_assert0(ff_hashtable_set(ctx, &k, &v));
> + k = 3;
> + v = 3;
> + av_assert0(ff_hashtable_set(ctx, &k, &v));
> +
> + // inserting sets on a full table should return 0
> + k = 4;
> + v = 4;
> + av_assert0(!ff_hashtable_set(ctx, &k, &v));
> +
> + // updating sets on a full table should return 1
> + k = 1;
> + v = 4;
> + av_assert0(ff_hashtable_set(ctx, &k, &v));
> + v = 0;
> + av_assert0(ff_hashtable_get(ctx, &k, &v));
> + av_assert0(v == 4);
> +
> + // successful deletes should return 1
> + k = 1;
> + av_assert0(ff_hashtable_delete(ctx, &k));
> +
> + // get should now return 0
> + av_assert0(!ff_hashtable_get(ctx, &k, &v));
> +
> + // sanity check remaining keys
> + k = 2;
> + v = 0;
> + av_assert0(ff_hashtable_get(ctx, &k, &v));
> + av_assert0(v == 2);
> + k = 3;
> + v = 0;
> + av_assert0(ff_hashtable_get(ctx, &k, &v));
> + av_assert0(v == 3);
> +
> + ff_hashtable_freep(&ctx);
> +
> + return 0;
> +}
I do not consider any of my suggestions blocking. Thanks for the
patches.
Regards,
--
Nicolas George
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
