[FFmpeg-devel,1/6] lavu/mem: add av_realloc_reuse() as a replacement for av_fast_realloc()

It uses size_t rather than unsigned for the size and conforms to our
standard naming scheme.
---
 configure           |  5 ++++-
 doc/APIchanges      |  3 +++
 libavutil/mem.c     | 30 ++++++++++++++++++++++++++++++
 libavutil/mem.h     | 37 +++++++++++++++++++++++++++++++++++--
 libavutil/version.h |  3 ++-
 5 files changed, 74 insertions(+), 4 deletions(-)

Le 28 septembre 2022 13:51:43 GMT+03:00, "Rémi Denis-Courmont" <remi@remlab.net> a écrit :
>Le 28 septembre 2022 13:48:49 GMT+03:00, Anton Khirnov <anton@khirnov.net> a écrit :
>>It uses size_t rather than unsigned for the size and conforms to our
>>standard naming scheme.
>>---
>> configure           |  5 ++++-
>> doc/APIchanges      |  3 +++
>> libavutil/mem.c     | 30 ++++++++++++++++++++++++++++++
>> libavutil/mem.h     | 37 +++++++++++++++++++++++++++++++++++--
>> libavutil/version.h |  3 ++-
>> 5 files changed, 74 insertions(+), 4 deletions(-)
>>
>>diff --git a/configure b/configure
>>index 6712d045d9..fd0e6ae032 100755
>>--- a/configure
>>+++ b/configure
>>@@ -4360,7 +4360,10 @@ case "$toolchain" in
>>         target_exec_default="valgrind"
>>         case "$toolchain" in
>>             valgrind-massif)
>>-                target_exec_args="--tool=massif --alloc-fn=av_malloc --alloc-fn=av_mallocz --alloc-fn=av_calloc --alloc-fn=av_fast_padded_malloc --alloc-fn=av_fast_malloc --alloc-fn=av_realloc_f --alloc-fn=av_fast_realloc --alloc-fn=av_realloc"
>>+                target_exec_args="--tool=massif"
>>+                for func in av_malloc av_mallocz av_calloc av_fast_padded_malloc av_fast_malloc av_realloc_f av_fast_realloc av_realloc av_realloc_reuse; do
>>+                    target_exec_args="$target_exec_args --alloc-fn=$func"
>>+                done
>>                 ;;
>>             valgrind-memcheck)
>>                 target_exec_args="--error-exitcode=1 --malloc-fill=0x2a --track-origins=yes --leak-check=full --gen-suppressions=all --suppressions=$source_path/tests/fate-valgrind.supp"
>>diff --git a/doc/APIchanges b/doc/APIchanges
>>index b0a41c9e37..9a735c27e7 100644
>>--- a/doc/APIchanges
>>+++ b/doc/APIchanges
>>@@ -14,6 +14,9 @@ libavutil:     2021-04-27
>> 
>> API changes, most recent first:
>> 
>>+2022-xx-xx - xxxxxxxxxx - lavu 57.38.100 - mem.h
>>+  Add av_realloc_reuse(), deprecate av_fast_realloc().
>>+
>> 2022-09-26 - xxxxxxxxxx - lavc 59.48.100 - avcodec.h
>>   Deprecate avcodec_enum_to_chroma_pos() and avcodec_chroma_pos_to_enum().
>>   Use av_chroma_location_enum_to_pos() or av_chroma_location_pos_to_enum()
>>diff --git a/libavutil/mem.c b/libavutil/mem.c
>>index 18aff5291f..51fdc9155f 100644
>>--- a/libavutil/mem.c
>>+++ b/libavutil/mem.c
>>@@ -502,6 +502,35 @@ void av_memcpy_backptr(uint8_t *dst, int back, int cnt)
>>     }
>> }
>> 
>>+void *av_realloc_reuse(void *ptr, size_t *size, size_t min_size)
>>+{
>>+    size_t max_size;
>>+
>>+    if (min_size <= *size)
>>+        return ptr;
>>+
>>+    max_size = atomic_load_explicit(&max_alloc_size, memory_order_relaxed);
>>+
>>+    if (min_size > max_size) {
>>+        *size = 0;
>>+        return NULL;
>
>Isn't this leaking the existing allocation?
>
>>+    }
>>+
>>+    min_size = FFMIN(max_size, FFMAX(min_size + min_size / 16 + 32, min_size));
>>+
>>+    ptr = av_realloc(ptr, min_size);
>>+    /* we could set this to the unmodified min_size but this is safer
>>+     * if the user lost the ptr and uses NULL now
>>+     */
>>+    if (!ptr)
>>+        min_size = 0;
>>+
>>+    *size = min_size;
>>+
>>+    return ptr;
>>+}
>>+
>>+#if FF_API_FAST_ALLOC
>> void *av_fast_realloc(void *ptr, unsigned int *size, size_t min_size)
>> {
>>     size_t max_size;
>>@@ -531,6 +560,7 @@ void *av_fast_realloc(void *ptr, unsigned int *size, size_t min_size)
>> 
>>     return ptr;
>> }
>>+#endif
>> 
>> static inline void fast_malloc(void *ptr, unsigned int *size, size_t min_size, int zero_realloc)
>> {
>>diff --git a/libavutil/mem.h b/libavutil/mem.h
>>index d91174196c..deb440ca1a 100644
>>--- a/libavutil/mem.h
>>+++ b/libavutil/mem.h
>>@@ -264,7 +264,7 @@ void *av_mallocz_array(size_t nmemb, size_t size) av_malloc_attrib av_alloc_size
>>  * @warning Unlike av_malloc(), the returned pointer is not guaranteed to be
>>  *          correctly aligned. The returned pointer must be freed after even
>>  *          if size is zero.
>>- * @see av_fast_realloc()
>>+ * @see av_realloc_reuse()
>>  * @see av_reallocp()
>>  */
>> void *av_realloc(void *ptr, size_t size) av_alloc_size(2);
>>@@ -346,6 +346,35 @@ av_alloc_size(2, 3) void *av_realloc_array(void *ptr, size_t nmemb, size_t size)
>>  */
>> int av_reallocp_array(void *ptr, size_t nmemb, size_t size);
>> 
>>+/**
>>+ * Reallocate a data buffer, reusing the existing one if it is large enough.
>>+ *
>>+ * This function is similar to av_realloc(), but optimized for cases where the
>>+ * buffer may grow significantly and is not expected to shrink.
>>+ *
>>+ * @param[in] ptr Previously allocated buffer, or `NULL`. If `ptr` is `NULL`, a
>>+ * new uninitialized buffer is allocated. `ptr` is invalidated when this
>>+ * function returns non-`NULL` and must be replaced with its return value.
>>+ *
>>+ * @param[in,out] size Pointer to the allocated size of buffer `ptr`. This
>>+ * function updates `*size` to the new allocated size (which may be larger than
>>+ * `min_size`). `*size` is set to 0 on failure.
>>+ *
>>+ * @param[in] min_size Minimum size in bytes of the returned buffer.
>>+ *
>>+ * @return
>>+ * - An allocated buffer (to be freed with `av_free()`) that is large enough to
>>+ *   hold at least `min_size` bytes. The first `*size` (value on entry to this
>>+ *   function) bytes of the buffer remain the same as the data in `ptr`, the
>>+ *   rest is uninitialized.
>>+ * - `NULL` on failure, then `*size` is set to 0 and ptr remains untouched.
>>+ *
>>+ * @see av_realloc()
>>+ * @see av_fast_malloc()
>>+ */
>>+void *av_realloc_reuse(void *ptr, size_t *size, size_t min_size);
>>+
>>+#if FF_API_FAST_ALLOC
>> /**
>>  * Reallocate the given buffer if it is not large enough, otherwise do nothing.
>>  *
>>@@ -377,13 +406,17 @@ int av_reallocp_array(void *ptr, size_t nmemb, size_t size);
>>  *         error
>>  * @see av_realloc()
>>  * @see av_fast_malloc()
>>+ *
>>+ * @deprecated use av_realloc_reuse()
>>  */
>>+attribute_deprecated
>> void *av_fast_realloc(void *ptr, unsigned int *size, size_t min_size);
>>+#endif
>> 
>> /**
>>  * Allocate a buffer, reusing the given one if large enough.
>>  *
>>- * Contrary to av_fast_realloc(), the current buffer contents might not be
>>+ * Contrary to av_realloc_reuse(), the current buffer contents might not be
>>  * preserved and on error the old buffer is freed, thus no special handling to
>>  * avoid memleaks is necessary.
>>  *
>>diff --git a/libavutil/version.h b/libavutil/version.h
>>index 9c44cef6aa..285a32f3d6 100644
>>--- a/libavutil/version.h
>>+++ b/libavutil/version.h
>>@@ -79,7 +79,7 @@
>>  */
>> 
>> #define LIBAVUTIL_VERSION_MAJOR  57
>>-#define LIBAVUTIL_VERSION_MINOR  37
>>+#define LIBAVUTIL_VERSION_MINOR  38
>> #define LIBAVUTIL_VERSION_MICRO 100
>> 
>> #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
>>@@ -115,6 +115,7 @@
>> #define FF_API_OLD_CHANNEL_LAYOUT       (LIBAVUTIL_VERSION_MAJOR < 58)
>> #define FF_API_AV_FOPEN_UTF8            (LIBAVUTIL_VERSION_MAJOR < 58)
>> #define FF_API_PKT_DURATION             (LIBAVUTIL_VERSION_MAJOR < 58)
>>+#define FF_API_FAST_ALLOC               (LIBAVUTIL_VERSION_MAJOR < 58)
>> 
>> /**
>>  * @}
>
>_______________________________________________
>ffmpeg-devel mailing list
>ffmpeg-devel@ffmpeg.org
>https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>To unsubscribe, visit link above, or email
>ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Nevermind. This follows the realloc() convention that the caller retains the reference on failure.

ons 2022-09-28 klockan 12:48 +0200 skrev Anton Khirnov:
> 
> +/**
> + * Reallocate a data buffer, reusing the existing one if it is large
> enough.
> + *
> + * This function is similar to av_realloc(), but optimized for cases
> where the
> + * buffer may grow significantly and is not expected to shrink.
> + *
> + * @param[in] ptr Previously allocated buffer, or `NULL`. If `ptr`
> is `NULL`, a
> + * new uninitialized buffer is allocated. `ptr` is invalidated when
> this
> + * function returns non-`NULL` and must be replaced with its return
> value.
> + *
> + * @param[in,out] size Pointer to the allocated size of buffer
> `ptr`. This
> + * function updates `*size` to the new allocated size (which may be
> larger than
> + * `min_size`). `*size` is set to 0 on failure.
> + *
> + * @param[in] min_size Minimum size in bytes of the returned buffer.
> + *
> + * @return
> + * - An allocated buffer (to be freed with `av_free()`) that is
> large enough to
> + *   hold at least `min_size` bytes. The first `*size` (value on
> entry to this
> + *   function) bytes of the buffer remain the same as the data in
> `ptr`, the
> + *   rest is uninitialized.
> + * - `NULL` on failure, then `*size` is set to 0 and ptr remains
> untouched.
> + *
> + * @see av_realloc()
> + * @see av_fast_malloc()
> + */
> +void *av_realloc_reuse(void *ptr, size_t *size, size_t min_size);

Isn't it better to return int like av_realloc_array_reuse() and leave
*ptr and *size untouched on error just as it does? If we're in the
business of straightening this all out then having all functions work
the same is less mental load down the line.

/Tomas

Quoting Tomas Härdin (2022-09-28 13:48:01)
> ons 2022-09-28 klockan 12:48 +0200 skrev Anton Khirnov:
> > 
> > +/**
> > + * Reallocate a data buffer, reusing the existing one if it is large
> > enough.
> > + *
> > + * This function is similar to av_realloc(), but optimized for cases
> > where the
> > + * buffer may grow significantly and is not expected to shrink.
> > + *
> > + * @param[in] ptr Previously allocated buffer, or `NULL`. If `ptr`
> > is `NULL`, a
> > + * new uninitialized buffer is allocated. `ptr` is invalidated when
> > this
> > + * function returns non-`NULL` and must be replaced with its return
> > value.
> > + *
> > + * @param[in,out] size Pointer to the allocated size of buffer
> > `ptr`. This
> > + * function updates `*size` to the new allocated size (which may be
> > larger than
> > + * `min_size`). `*size` is set to 0 on failure.
> > + *
> > + * @param[in] min_size Minimum size in bytes of the returned buffer.
> > + *
> > + * @return
> > + * - An allocated buffer (to be freed with `av_free()`) that is
> > large enough to
> > + *   hold at least `min_size` bytes. The first `*size` (value on
> > entry to this
> > + *   function) bytes of the buffer remain the same as the data in
> > `ptr`, the
> > + *   rest is uninitialized.
> > + * - `NULL` on failure, then `*size` is set to 0 and ptr remains
> > untouched.
> > + *
> > + * @see av_realloc()
> > + * @see av_fast_malloc()
> > + */
> > +void *av_realloc_reuse(void *ptr, size_t *size, size_t min_size);
> 
> Isn't it better to return int like av_realloc_array_reuse() and leave
> *ptr and *size untouched on error just as it does? If we're in the
> business of straightening this all out then having all functions work
> the same is less mental load down the line.

I suppose you're right, it's just more work to convert all the callers.
Will do and resend.

Tomas Härdin:
> ons 2022-09-28 klockan 12:48 +0200 skrev Anton Khirnov:
>>
>> +/**
>> + * Reallocate a data buffer, reusing the existing one if it is large
>> enough.
>> + *
>> + * This function is similar to av_realloc(), but optimized for cases
>> where the
>> + * buffer may grow significantly and is not expected to shrink.
>> + *
>> + * @param[in] ptr Previously allocated buffer, or `NULL`. If `ptr`
>> is `NULL`, a
>> + * new uninitialized buffer is allocated. `ptr` is invalidated when
>> this
>> + * function returns non-`NULL` and must be replaced with its return
>> value.
>> + *
>> + * @param[in,out] size Pointer to the allocated size of buffer
>> `ptr`. This
>> + * function updates `*size` to the new allocated size (which may be
>> larger than
>> + * `min_size`). `*size` is set to 0 on failure.
>> + *
>> + * @param[in] min_size Minimum size in bytes of the returned buffer.
>> + *
>> + * @return
>> + * - An allocated buffer (to be freed with `av_free()`) that is
>> large enough to
>> + *   hold at least `min_size` bytes. The first `*size` (value on
>> entry to this
>> + *   function) bytes of the buffer remain the same as the data in
>> `ptr`, the
>> + *   rest is uninitialized.
>> + * - `NULL` on failure, then `*size` is set to 0 and ptr remains
>> untouched.
>> + *
>> + * @see av_realloc()
>> + * @see av_fast_malloc()
>> + */
>> +void *av_realloc_reuse(void *ptr, size_t *size, size_t min_size);
> 
> Isn't it better to return int like av_realloc_array_reuse() and leave
> *ptr and *size untouched on error just as it does? If we're in the
> business of straightening this all out then having all functions work
> the same is less mental load down the line.
> 

IMO size should be unchanged on error, but returning an int is IMO
overblown. I only did it for my version because there is the possibility
of the multiplication overflowing, but that is not the case here.

- Andreas