[FFmpeg-devel] [PATCH 1/8] avutil/mem: Handle fast allocations near UINT_MAX properly

[FFmpeg-devel] [PATCH 1/8] avutil/mem: Handle fast allocations near UINT_MAX properly

[Andreas Rheinhardt]

av_fast_realloc and av_fast_mallocz? store the size of
the objects they allocate in an unsigned. Yet they overallocate
and currently they can allocate more than UINT_MAX bytes
in case a user has requested a size of about UINT_MAX * 16 / 17
or more if SIZE_MAX > UINT_MAX. In this case it is impossible
to store the true size of the buffer via the unsigned*;
future requests are likely to use the (re)allocation codepath
even if the buffer is actually large enough because of
the incorrect size.

Fix this by ensuring that the actually allocated size
always fits into an unsigned. (This entails erroring out
in case the user requested more than UINT_MAX.)

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavutil/mem.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/libavutil/mem.c b/libavutil/mem.c
index a0c9a42849..18aff5291f 100644
--- a/libavutil/mem.c
+++ b/libavutil/mem.c
@@ -510,6 +510,8 @@ void *av_fast_realloc(void *ptr, unsigned int *size, size_t min_size)
         return ptr;
 
     max_size = atomic_load_explicit(&max_alloc_size, memory_order_relaxed);
+    /* *size is an unsigned, so the real maximum is <= UINT_MAX. */
+    max_size = FFMIN(max_size, UINT_MAX);
 
     if (min_size > max_size) {
         *size = 0;
@@ -542,6 +544,8 @@ static inline void fast_malloc(void *ptr, unsigned int *size, size_t min_size, i
     }
 
     max_size = atomic_load_explicit(&max_alloc_size, memory_order_relaxed);
+    /* *size is an unsigned, so the real maximum is <= UINT_MAX. */
+    max_size = FFMIN(max_size, UINT_MAX);
 
     if (min_size > max_size) {
         av_freep(ptr);
-- 
2.34.1

_______________________________________________
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".

[FFmpeg-devel] [PATCH 2/8] avformat/flvenc: Add deinit function

[Andreas Rheinhardt]

Fixes memleaks when the trailer is never written or when shift_data()
fails when writing the trailer.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
This is the same as
https://patchwork.ffmpeg.org/project/ffmpeg/patch/20191023125944.10292-4-andreas.rheinhardt@gmail.com/

 libavformat/flvenc.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/libavformat/flvenc.c b/libavformat/flvenc.c
index 4e65ba4066..b6135b25bf 100644
--- a/libavformat/flvenc.c
+++ b/libavformat/flvenc.c
@@ -733,7 +733,7 @@ static int flv_write_trailer(AVFormatContext *s)
     int64_t cur_pos = avio_tell(s->pb);
 
     if (build_keyframes_idx) {
-        FLVFileposition *newflv_posinfo, *p;
+        FLVFileposition *newflv_posinfo;
 
         avio_seek(pb, flv->videosize_offset, SEEK_SET);
         put_amf_double(pb, flv->videosize);
@@ -768,19 +768,6 @@ static int flv_write_trailer(AVFormatContext *s)
             put_amf_double(pb, newflv_posinfo->keyframe_timestamp);
         }
 
-        newflv_posinfo = flv->head_filepositions;
-        while (newflv_posinfo) {
-            p = newflv_posinfo->next;
-            if (p) {
-                newflv_posinfo->next = p->next;
-                av_free(p);
-                p = NULL;
-            } else {
-                av_free(newflv_posinfo);
-                newflv_posinfo = NULL;
-            }
-        }
-
         put_amf_string(pb, "");
         avio_w8(pb, AMF_END_OF_OBJECT);
 
@@ -1047,6 +1034,20 @@ static int flv_check_bitstream(AVFormatContext *s, AVStream *st,
     return ret;
 }
 
+static void flv_deinit(AVFormatContext *s)
+{
+    FLVContext *flv = s->priv_data;
+    FLVFileposition *filepos = flv->head_filepositions;
+
+    while (filepos) {
+        FLVFileposition *next = filepos->next;
+        av_free(filepos);
+        filepos = next;
+    }
+    flv->filepositions = flv->head_filepositions = NULL;
+    flv->filepositions_count = 0;
+}
+
 static const AVOption options[] = {
     { "flvflags", "FLV muxer flags", offsetof(FLVContext, flags), AV_OPT_TYPE_FLAGS, {.i64 = 0}, INT_MIN, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM, "flvflags" },
     { "aac_seq_header_detect", "Put AAC sequence header based on stream data", 0, AV_OPT_TYPE_CONST, {.i64 = FLV_AAC_SEQ_HEADER_DETECT}, INT_MIN, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM, "flvflags" },
@@ -1076,6 +1077,7 @@ const AVOutputFormat ff_flv_muxer = {
     .write_header   = flv_write_header,
     .write_packet   = flv_write_packet,
     .write_trailer  = flv_write_trailer,
+    .deinit         = flv_deinit,
     .check_bitstream= flv_check_bitstream,
     .codec_tag      = (const AVCodecTag* const []) {
                           flv_video_codec_ids, flv_audio_codec_ids, 0
-- 
2.34.1

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 2/8] avformat/flvenc: Add deinit function

[Steven Liu]

Andreas Rheinhardt <andreas.rheinhardt@outlook.com> 于2022年7月6日周三 04:27写道：
>
> Fixes memleaks when the trailer is never written or when shift_data()
> fails when writing the trailer.
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> This is the same as
> https://patchwork.ffmpeg.org/project/ffmpeg/patch/20191023125944.10292-4-andreas.rheinhardt@gmail.com/
>
>  libavformat/flvenc.c | 30 ++++++++++++++++--------------
>  1 file changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/libavformat/flvenc.c b/libavformat/flvenc.c
> index 4e65ba4066..b6135b25bf 100644
> --- a/libavformat/flvenc.c
> +++ b/libavformat/flvenc.c
> @@ -733,7 +733,7 @@ static int flv_write_trailer(AVFormatContext *s)
>      int64_t cur_pos = avio_tell(s->pb);
>
>      if (build_keyframes_idx) {
> -        FLVFileposition *newflv_posinfo, *p;
> +        FLVFileposition *newflv_posinfo;
>
>          avio_seek(pb, flv->videosize_offset, SEEK_SET);
>          put_amf_double(pb, flv->videosize);
> @@ -768,19 +768,6 @@ static int flv_write_trailer(AVFormatContext *s)
>              put_amf_double(pb, newflv_posinfo->keyframe_timestamp);
>          }
>
> -        newflv_posinfo = flv->head_filepositions;
> -        while (newflv_posinfo) {
> -            p = newflv_posinfo->next;
> -            if (p) {
> -                newflv_posinfo->next = p->next;
> -                av_free(p);
> -                p = NULL;
> -            } else {
> -                av_free(newflv_posinfo);
> -                newflv_posinfo = NULL;
> -            }
> -        }
> -
>          put_amf_string(pb, "");
>          avio_w8(pb, AMF_END_OF_OBJECT);
>
> @@ -1047,6 +1034,20 @@ static int flv_check_bitstream(AVFormatContext *s, AVStream *st,
>      return ret;
>  }
>
> +static void flv_deinit(AVFormatContext *s)
> +{
> +    FLVContext *flv = s->priv_data;
> +    FLVFileposition *filepos = flv->head_filepositions;
> +
> +    while (filepos) {
> +        FLVFileposition *next = filepos->next;
> +        av_free(filepos);
> +        filepos = next;
> +    }
> +    flv->filepositions = flv->head_filepositions = NULL;
> +    flv->filepositions_count = 0;
> +}
> +
>  static const AVOption options[] = {
>      { "flvflags", "FLV muxer flags", offsetof(FLVContext, flags), AV_OPT_TYPE_FLAGS, {.i64 = 0}, INT_MIN, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM, "flvflags" },
>      { "aac_seq_header_detect", "Put AAC sequence header based on stream data", 0, AV_OPT_TYPE_CONST, {.i64 = FLV_AAC_SEQ_HEADER_DETECT}, INT_MIN, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM, "flvflags" },
> @@ -1076,6 +1077,7 @@ const AVOutputFormat ff_flv_muxer = {
>      .write_header   = flv_write_header,
>      .write_packet   = flv_write_packet,
>      .write_trailer  = flv_write_trailer,
> +    .deinit         = flv_deinit,
>      .check_bitstream= flv_check_bitstream,
>      .codec_tag      = (const AVCodecTag* const []) {
>                            flv_video_codec_ids, flv_audio_codec_ids, 0
> --
> 2.34.1
>
> _______________________________________________
> 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".

 The flvenc part patchset looks ok to me.


Thanks
Steven
_______________________________________________
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".

[FFmpeg-devel] [PATCH 3/8] avutil/mem: Add av_fast_realloc_array()

[Andreas Rheinhardt]

From: Andreas Rheinhardt <andres.rheinhardt@outlook.com>

This is an array-equivalent of av_fast_realloc(). Its advantages
compared to using av_fast_realloc() for allocating arrays are as
follows:

a) It performs its own overflow checks for the multiplication that is
implicit in array allocations. (And it only needs to perform these
checks (as well as the multiplication itself) in case the array needs to
be reallocated.)
b) It allows to limit the number of elements to an upper bound given
by the caller. This allows to restrict the number of allocated elements
to fit into an int and therefore makes this function usable with
counters of this type. It can also be used to avoid overflow checks in
the caller: E.g. setting it to UINT_MAX - 1 elements makes it safe to
increase the desired number of elements in steps of one. And it avoids
overallocations in situations where one already has an upper bound.
c) av_fast_realloc_array() will always allocate in multiples of array
elements; no memory is wasted with partial elements.
d) By returning an int, av_fast_realloc_array() can distinguish between
ordinary allocation failures (meriting AVERROR(ENOMEM)) and failures
because of allocation limits (by returning AVERROR(ERANGE)).
e) It is no longer possible for the user to accidentally lose the
pointer by using ptr = av_fast_realloc(ptr, ...).

Because of e) there is no need to set the number of allocated elements
to zero on failure.

av_fast_realloc() usually allocates size + size / 16 + 32 bytes if size
bytes are desired and if the already existing buffer isn't big enough.
av_fast_realloc_array() instead allocates nb + (nb + 14) / 16. Rounding
up is done in order not to reallocate in steps of one if the current
number is < 16; adding 14 instead of 15 has the effect of only
allocating one element if one element is desired. This is done with an
eye towards applications where arrays might commonly only contain one
element (as happens with the Matroska CueTrackPositions).

Which of the two functions allocates faster depends upon the size of
the elements. E.g. if the elements have a size of 32B and the desired
size is incremented in steps of one, allocations happen at
1, 3, 5, 7, 9, 11, 13, 15, 17, 20, 23, 26 ... for av_fast_realloc(),
1, 2, 4, 6, 8, 10, 12, 14, 16, 18, 21, 24 ... for
av_fast_realloc_array(). For element sizes of 96B, the numbers are
1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 13, 15, 17, 19, 21, 23, 25, 27, 30 ...
for av_fast_realloc() whereas the pattern for av_fast_realloc_array() is
unchanged.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
This patch (and some of the other patches of this patchset)
are mostly the same as the one in these threads:
https://ffmpeg.org/pipermail/ffmpeg-devel/2019-December/254836.html
https://ffmpeg.org/pipermail/ffmpeg-devel/2020-January/255182.html

 doc/APIchanges      |  3 +++
 libavutil/mem.c     | 33 +++++++++++++++++++++++++++++++++
 libavutil/mem.h     | 30 ++++++++++++++++++++++++++++++
 libavutil/version.h |  2 +-
 4 files changed, 67 insertions(+), 1 deletion(-)

diff --git a/doc/APIchanges b/doc/APIchanges
index 20b944933a..f633ae6fee 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -14,6 +14,9 @@ libavutil:     2021-04-27
 
 API changes, most recent first:
 
+2022-07-05 - xxxxxxxxxx - lavu 57.28.100 - mem.h
+  Add av_fast_realloc_array() to simplify reallocating of arrays.
+
 2022-06-12 - xxxxxxxxxx - lavf 59.25.100 - avio.h
   Add avio_vprintf(), similar to avio_printf() but allow to use it
   from within a function taking a variable argument list as input.
diff --git a/libavutil/mem.c b/libavutil/mem.c
index 18aff5291f..6e3942ae63 100644
--- a/libavutil/mem.c
+++ b/libavutil/mem.c
@@ -532,6 +532,39 @@ void *av_fast_realloc(void *ptr, unsigned int *size, size_t min_size)
     return ptr;
 }
 
+int av_fast_realloc_array(void *ptr, size_t *nb_allocated,
+                          size_t min_nb, size_t max_nb, size_t elsize)
+{
+    void *array;
+    size_t nb, max_alloc_size_bytes;
+
+    if (min_nb <= *nb_allocated)
+        return 0;
+
+    max_alloc_size_bytes = atomic_load_explicit(&max_alloc_size, memory_order_relaxed);
+    max_nb = FFMIN(max_nb, max_alloc_size_bytes / elsize);
+
+    if (min_nb > max_nb)
+        return AVERROR(ERANGE);
+
+    nb = min_nb + (min_nb + 14) / 16;
+
+    /* If min_nb is so big that the above calculation overflowed,
+     * just allocate as much as we are allowed to. */
+    nb = nb < min_nb ? max_nb : FFMIN(nb, max_nb);
+
+    memcpy(&array, ptr, sizeof(array));
+
+    array = av_realloc(array, nb * elsize);
+    if (!array)
+        return AVERROR(ENOMEM);
+
+    memcpy(ptr, &array, sizeof(array));
+    *nb_allocated = nb;
+
+    return 0;
+}
+
 static inline void fast_malloc(void *ptr, unsigned int *size, size_t min_size, int zero_realloc)
 {
     size_t max_size;
diff --git a/libavutil/mem.h b/libavutil/mem.h
index d91174196c..f040de08fc 100644
--- a/libavutil/mem.h
+++ b/libavutil/mem.h
@@ -375,11 +375,41 @@ int av_reallocp_array(void *ptr, size_t nmemb, size_t size);
  * @return `ptr` if the buffer is large enough, a pointer to newly reallocated
  *         buffer if the buffer was not large enough, or `NULL` in case of
  *         error
+ * @see av_fast_realloc_array()
  * @see av_realloc()
  * @see av_fast_malloc()
  */
 void *av_fast_realloc(void *ptr, unsigned int *size, size_t min_size);
 
+/**
+ * Reallocate the given array if it is not large enough, otherwise do nothing.
+ *
+ * If `ptr` points to `NULL`, then a new uninitialized array is allocated.
+ *
+ * @param[in,out] ptr           Pointer to `NULL` or pointer to an already
+ *                              allocated array. `*ptr` will be set to point
+ *                              to the new array on success.
+ * @param[in,out] nb_allocated  Pointer to the number of elements of the array
+ *                              `*ptr`. `*nb_allocated` is updated to the new
+ *                              number of allocated elements.
+ * @param[in]     min_nb        Desired minimal number of elements in array `*ptr`
+ * @param[in]     max_nb        Maximal number of elements to allocate.
+ * @param[in]     elsize        Size of a single element of the array.
+ *                              Must not be zero.
+ * @return 0 on success, < 0 on failure. On failure, `*ptr` is not freed and
+ *         `*ptr` as well as `*nb_allocated` are unchanged.
+ * @note `max_nb` can be used to limit allocations and make this function
+ *       usable with counters of types other than size_t. It can also be used
+ *       to avoid overflow checks in callers: E.g. setting it to `UINT_MAX - 1`
+ *       means that incrementing an unsigned int in steps of one need not be
+ *       checked for overflow.
+ * @see av_fast_realloc()
+ * @see av_realloc()
+ * @see av_fast_malloc()
+ */
+int av_fast_realloc_array(void *ptr, size_t *nb_allocated,
+                          size_t min_nb, size_t max_nb, size_t elsize);
+
 /**
  * Allocate a buffer, reusing the given one if large enough.
  *
diff --git a/libavutil/version.h b/libavutil/version.h
index 2e9e02dda8..87178e9e9a 100644
--- a/libavutil/version.h
+++ b/libavutil/version.h
@@ -79,7 +79,7 @@
  */
 
 #define LIBAVUTIL_VERSION_MAJOR  57
-#define LIBAVUTIL_VERSION_MINOR  27
+#define LIBAVUTIL_VERSION_MINOR  28
 #define LIBAVUTIL_VERSION_MICRO 100
 
 #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
-- 
2.34.1

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 3/8] avutil/mem: Add av_fast_realloc_array()

[Tomas Härdin]

tis 2022-07-05 klockan 22:26 +0200 skrev Andreas Rheinhardt:
> From: Andreas Rheinhardt <andres.rheinhardt@outlook.com>
> 
> This is an array-equivalent of av_fast_realloc(). Its advantages
> compared to using av_fast_realloc() for allocating arrays are as
> follows:
> 
> a) It performs its own overflow checks for the multiplication that is
> implicit in array allocations. (And it only needs to perform these
> checks (as well as the multiplication itself) in case the array needs
> to
> be reallocated.)
> b) It allows to limit the number of elements to an upper bound given
> by the caller. This allows to restrict the number of allocated
> elements
> to fit into an int and therefore makes this function usable with
> counters of this type. It can also be used to avoid overflow checks
> in
> the caller: E.g. setting it to UINT_MAX - 1 elements makes it safe to
> increase the desired number of elements in steps of one. And it
> avoids
> overallocations in situations where one already has an upper bound.
> c) av_fast_realloc_array() will always allocate in multiples of array
> elements; no memory is wasted with partial elements.
> d) By returning an int, av_fast_realloc_array() can distinguish
> between
> ordinary allocation failures (meriting AVERROR(ENOMEM)) and failures
> because of allocation limits (by returning AVERROR(ERANGE)).
> e) It is no longer possible for the user to accidentally lose the
> pointer by using ptr = av_fast_realloc(ptr, ...).

If you add an option for clearing the newly allocated memory then this
could work for my av_fast_recalloc() use case in the jpeg2000 decoder.
Or we could have two functions.

Small bikeshed: since the function takes a pointer to a pointer as
argument, av_fast_realloc_arrayp() might be a better name. I had in
mind to similarly rename av_fast_recalloc() to av_fast_recallocp().


> +
> +    nb = min_nb + (min_nb + 14) / 16;

Not +15? Or +0?

> +
> +    /* If min_nb is so big that the above calculation overflowed,
> +     * just allocate as much as we are allowed to. */
> +    nb = nb < min_nb ? max_nb : FFMIN(nb, max_nb);

Looks OK, but an explicit check for overflow is easier to verify

> +
> +    memcpy(&array, ptr, sizeof(array));
> +
> +    array = av_realloc(array, nb * elsize);
> +    if (!array)
> +        return AVERROR(ENOMEM);
> +
> +    memcpy(ptr, &array, sizeof(array));

An optional memset() here would be useful for me

Else it looks OK

/Tomas

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 3/8] avutil/mem: Add av_fast_realloc_array()

[Andreas Rheinhardt]

Tomas Härdin:
> tis 2022-07-05 klockan 22:26 +0200 skrev Andreas Rheinhardt:
>> From: Andreas Rheinhardt <andres.rheinhardt@outlook.com>
>>
>> This is an array-equivalent of av_fast_realloc(). Its advantages
>> compared to using av_fast_realloc() for allocating arrays are as
>> follows:
>>
>> a) It performs its own overflow checks for the multiplication that is
>> implicit in array allocations. (And it only needs to perform these
>> checks (as well as the multiplication itself) in case the array needs
>> to
>> be reallocated.)
>> b) It allows to limit the number of elements to an upper bound given
>> by the caller. This allows to restrict the number of allocated
>> elements
>> to fit into an int and therefore makes this function usable with
>> counters of this type. It can also be used to avoid overflow checks
>> in
>> the caller: E.g. setting it to UINT_MAX - 1 elements makes it safe to
>> increase the desired number of elements in steps of one. And it
>> avoids
>> overallocations in situations where one already has an upper bound.
>> c) av_fast_realloc_array() will always allocate in multiples of array
>> elements; no memory is wasted with partial elements.
>> d) By returning an int, av_fast_realloc_array() can distinguish
>> between
>> ordinary allocation failures (meriting AVERROR(ENOMEM)) and failures
>> because of allocation limits (by returning AVERROR(ERANGE)).
>> e) It is no longer possible for the user to accidentally lose the
>> pointer by using ptr = av_fast_realloc(ptr, ...).
> 
> If you add an option for clearing the newly allocated memory then this
> could work for my av_fast_recalloc() use case in the jpeg2000 decoder.
> Or we could have two functions.
> 

I'd prefer it if the zeroing function were a wrapper around the
non-zeroing function.

> Small bikeshed: since the function takes a pointer to a pointer as
> argument, av_fast_realloc_arrayp() might be a better name. I had in
> mind to similarly rename av_fast_recalloc() to av_fast_recallocp().
> 
> 
>> +
>> +    nb = min_nb + (min_nb + 14) / 16;
> 
> Not +15? Or +0?

"av_fast_realloc_array() instead allocates nb + (nb + 14) / 16. Rounding
up is done in order not to reallocate in steps of one if the current
number is < 16; adding 14 instead of 15 has the effect of only
allocating one element if one element is desired."

> 
>> +
>> +    /* If min_nb is so big that the above calculation overflowed,
>> +     * just allocate as much as we are allowed to. */
>> +    nb = nb < min_nb ? max_nb : FFMIN(nb, max_nb);
> 
> Looks OK, but an explicit check for overflow is easier to verify
> 
>> +
>> +    memcpy(&array, ptr, sizeof(array));
>> +
>> +    array = av_realloc(array, nb * elsize);
>> +    if (!array)
>> +        return AVERROR(ENOMEM);
>> +
>> +    memcpy(ptr, &array, sizeof(array));
> 
> An optional memset() here would be useful for me
> 
> Else it looks OK
> 
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 3/8] avutil/mem: Add av_fast_realloc_array()

[Tomas Härdin]

ons 2022-07-06 klockan 16:46 +0200 skrev Andreas Rheinhardt:
> Tomas Härdin:
> > tis 2022-07-05 klockan 22:26 +0200 skrev Andreas Rheinhardt:
> > > From: Andreas Rheinhardt <andres.rheinhardt@outlook.com>
> > > 
> > > This is an array-equivalent of av_fast_realloc(). Its advantages
> > > compared to using av_fast_realloc() for allocating arrays are as
> > > follows:
> > > 
> > > a) It performs its own overflow checks for the multiplication
> > > that is
> > > implicit in array allocations. (And it only needs to perform
> > > these
> > > checks (as well as the multiplication itself) in case the array
> > > needs
> > > to
> > > be reallocated.)
> > > b) It allows to limit the number of elements to an upper bound
> > > given
> > > by the caller. This allows to restrict the number of allocated
> > > elements
> > > to fit into an int and therefore makes this function usable with
> > > counters of this type. It can also be used to avoid overflow
> > > checks
> > > in
> > > the caller: E.g. setting it to UINT_MAX - 1 elements makes it
> > > safe to
> > > increase the desired number of elements in steps of one. And it
> > > avoids
> > > overallocations in situations where one already has an upper
> > > bound.
> > > c) av_fast_realloc_array() will always allocate in multiples of
> > > array
> > > elements; no memory is wasted with partial elements.
> > > d) By returning an int, av_fast_realloc_array() can distinguish
> > > between
> > > ordinary allocation failures (meriting AVERROR(ENOMEM)) and
> > > failures
> > > because of allocation limits (by returning AVERROR(ERANGE)).
> > > e) It is no longer possible for the user to accidentally lose the
> > > pointer by using ptr = av_fast_realloc(ptr, ...).
> > 
> > If you add an option for clearing the newly allocated memory then
> > this
> > could work for my av_fast_recalloc() use case in the jpeg2000
> > decoder.
> > Or we could have two functions.
> > 
> 
> I'd prefer it if the zeroing function were a wrapper around the
> non-zeroing function.

That should work, the change in allocated entries can be detected

> 
> > Small bikeshed: since the function takes a pointer to a pointer as
> > argument, av_fast_realloc_arrayp() might be a better name. I had in
> > mind to similarly rename av_fast_recalloc() to av_fast_recallocp().
> > 
> > 
> > > +
> > > +    nb = min_nb + (min_nb + 14) / 16;
> > 
> > Not +15? Or +0?
> 
> "av_fast_realloc_array() instead allocates nb + (nb + 14) / 16.
> Rounding
> up is done in order not to reallocate in steps of one if the current
> number is < 16; adding 14 instead of 15 has the effect of only
> allocating one element if one element is desired."

Wups, I actually read that part of the commit message then forgot about
it. I blame sickness

/Tomas

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 3/8] avutil/mem: Add av_fast_realloc_array()

[Andreas Rheinhardt]

Andreas Rheinhardt:
> From: Andreas Rheinhardt <andres.rheinhardt@outlook.com>
> 
> This is an array-equivalent of av_fast_realloc(). Its advantages
> compared to using av_fast_realloc() for allocating arrays are as
> follows:
> 
> a) It performs its own overflow checks for the multiplication that is
> implicit in array allocations. (And it only needs to perform these
> checks (as well as the multiplication itself) in case the array needs to
> be reallocated.)
> b) It allows to limit the number of elements to an upper bound given
> by the caller. This allows to restrict the number of allocated elements
> to fit into an int and therefore makes this function usable with
> counters of this type. It can also be used to avoid overflow checks in
> the caller: E.g. setting it to UINT_MAX - 1 elements makes it safe to
> increase the desired number of elements in steps of one. And it avoids
> overallocations in situations where one already has an upper bound.
> c) av_fast_realloc_array() will always allocate in multiples of array
> elements; no memory is wasted with partial elements.
> d) By returning an int, av_fast_realloc_array() can distinguish between
> ordinary allocation failures (meriting AVERROR(ENOMEM)) and failures
> because of allocation limits (by returning AVERROR(ERANGE)).
> e) It is no longer possible for the user to accidentally lose the
> pointer by using ptr = av_fast_realloc(ptr, ...).
> 
> Because of e) there is no need to set the number of allocated elements
> to zero on failure.
> 
> av_fast_realloc() usually allocates size + size / 16 + 32 bytes if size
> bytes are desired and if the already existing buffer isn't big enough.
> av_fast_realloc_array() instead allocates nb + (nb + 14) / 16. Rounding
> up is done in order not to reallocate in steps of one if the current
> number is < 16; adding 14 instead of 15 has the effect of only
> allocating one element if one element is desired. This is done with an
> eye towards applications where arrays might commonly only contain one
> element (as happens with the Matroska CueTrackPositions).
> 
> Which of the two functions allocates faster depends upon the size of
> the elements. E.g. if the elements have a size of 32B and the desired
> size is incremented in steps of one, allocations happen at
> 1, 3, 5, 7, 9, 11, 13, 15, 17, 20, 23, 26 ... for av_fast_realloc(),
> 1, 2, 4, 6, 8, 10, 12, 14, 16, 18, 21, 24 ... for
> av_fast_realloc_array(). For element sizes of 96B, the numbers are
> 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 13, 15, 17, 19, 21, 23, 25, 27, 30 ...
> for av_fast_realloc() whereas the pattern for av_fast_realloc_array() is
> unchanged.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> This patch (and some of the other patches of this patchset)
> are mostly the same as the one in these threads:
> https://ffmpeg.org/pipermail/ffmpeg-devel/2019-December/254836.html
> https://ffmpeg.org/pipermail/ffmpeg-devel/2020-January/255182.html
> 
>  doc/APIchanges      |  3 +++
>  libavutil/mem.c     | 33 +++++++++++++++++++++++++++++++++
>  libavutil/mem.h     | 30 ++++++++++++++++++++++++++++++
>  libavutil/version.h |  2 +-
>  4 files changed, 67 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 20b944933a..f633ae6fee 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -14,6 +14,9 @@ libavutil:     2021-04-27
>  
>  API changes, most recent first:
>  
> +2022-07-05 - xxxxxxxxxx - lavu 57.28.100 - mem.h
> +  Add av_fast_realloc_array() to simplify reallocating of arrays.
> +
>  2022-06-12 - xxxxxxxxxx - lavf 59.25.100 - avio.h
>    Add avio_vprintf(), similar to avio_printf() but allow to use it
>    from within a function taking a variable argument list as input.
> diff --git a/libavutil/mem.c b/libavutil/mem.c
> index 18aff5291f..6e3942ae63 100644
> --- a/libavutil/mem.c
> +++ b/libavutil/mem.c
> @@ -532,6 +532,39 @@ void *av_fast_realloc(void *ptr, unsigned int *size, size_t min_size)
>      return ptr;
>  }
>  
> +int av_fast_realloc_array(void *ptr, size_t *nb_allocated,
> +                          size_t min_nb, size_t max_nb, size_t elsize)
> +{
> +    void *array;
> +    size_t nb, max_alloc_size_bytes;
> +
> +    if (min_nb <= *nb_allocated)
> +        return 0;
> +
> +    max_alloc_size_bytes = atomic_load_explicit(&max_alloc_size, memory_order_relaxed);
> +    max_nb = FFMIN(max_nb, max_alloc_size_bytes / elsize);
> +
> +    if (min_nb > max_nb)
> +        return AVERROR(ERANGE);
> +
> +    nb = min_nb + (min_nb + 14) / 16;
> +
> +    /* If min_nb is so big that the above calculation overflowed,
> +     * just allocate as much as we are allowed to. */
> +    nb = nb < min_nb ? max_nb : FFMIN(nb, max_nb);
> +
> +    memcpy(&array, ptr, sizeof(array));
> +
> +    array = av_realloc(array, nb * elsize);
> +    if (!array)
> +        return AVERROR(ENOMEM);
> +
> +    memcpy(ptr, &array, sizeof(array));
> +    *nb_allocated = nb;
> +
> +    return 0;
> +}
> +
>  static inline void fast_malloc(void *ptr, unsigned int *size, size_t min_size, int zero_realloc)
>  {
>      size_t max_size;
> diff --git a/libavutil/mem.h b/libavutil/mem.h
> index d91174196c..f040de08fc 100644
> --- a/libavutil/mem.h
> +++ b/libavutil/mem.h
> @@ -375,11 +375,41 @@ int av_reallocp_array(void *ptr, size_t nmemb, size_t size);
>   * @return `ptr` if the buffer is large enough, a pointer to newly reallocated
>   *         buffer if the buffer was not large enough, or `NULL` in case of
>   *         error
> + * @see av_fast_realloc_array()
>   * @see av_realloc()
>   * @see av_fast_malloc()
>   */
>  void *av_fast_realloc(void *ptr, unsigned int *size, size_t min_size);
>  
> +/**
> + * Reallocate the given array if it is not large enough, otherwise do nothing.
> + *
> + * If `ptr` points to `NULL`, then a new uninitialized array is allocated.
> + *
> + * @param[in,out] ptr           Pointer to `NULL` or pointer to an already
> + *                              allocated array. `*ptr` will be set to point
> + *                              to the new array on success.
> + * @param[in,out] nb_allocated  Pointer to the number of elements of the array
> + *                              `*ptr`. `*nb_allocated` is updated to the new
> + *                              number of allocated elements.
> + * @param[in]     min_nb        Desired minimal number of elements in array `*ptr`
> + * @param[in]     max_nb        Maximal number of elements to allocate.
> + * @param[in]     elsize        Size of a single element of the array.
> + *                              Must not be zero.
> + * @return 0 on success, < 0 on failure. On failure, `*ptr` is not freed and
> + *         `*ptr` as well as `*nb_allocated` are unchanged.
> + * @note `max_nb` can be used to limit allocations and make this function
> + *       usable with counters of types other than size_t. It can also be used
> + *       to avoid overflow checks in callers: E.g. setting it to `UINT_MAX - 1`
> + *       means that incrementing an unsigned int in steps of one need not be
> + *       checked for overflow.
> + * @see av_fast_realloc()
> + * @see av_realloc()
> + * @see av_fast_malloc()
> + */
> +int av_fast_realloc_array(void *ptr, size_t *nb_allocated,
> +                          size_t min_nb, size_t max_nb, size_t elsize);
> +
>  /**
>   * Allocate a buffer, reusing the given one if large enough.
>   *
> diff --git a/libavutil/version.h b/libavutil/version.h
> index 2e9e02dda8..87178e9e9a 100644
> --- a/libavutil/version.h
> +++ b/libavutil/version.h
> @@ -79,7 +79,7 @@
>   */
>  
>  #define LIBAVUTIL_VERSION_MAJOR  57
> -#define LIBAVUTIL_VERSION_MINOR  27
> +#define LIBAVUTIL_VERSION_MINOR  28
>  #define LIBAVUTIL_VERSION_MICRO 100
>  
>  #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \

Anton really dislikes the av_fast_* naming and instead wants this to be
called av_realloc_array_reuse(). I don't care either way. Any more
opinions on this (or on the patch itself)?

- Andreas
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 3/8] avutil/mem: Add av_fast_realloc_array()

[Anton Khirnov]

Quoting Andreas Rheinhardt (2022-07-12 16:12:16)
> Anton really dislikes the av_fast_* naming and instead wants this to be
> called av_realloc_array_reuse(). I don't care either way. Any more
> opinions on this (or on the patch itself)?

If people dislike _reuse(), I am open to other reasonable suggestions.
This 'fast' naming sucks because
- it tells you nothing about how this function is "fast"
- it is added at the beginning rather than the end, which is
  against standard namespacing conventions

-- 
Anton Khirnov
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 3/8] avutil/mem: Add av_fast_realloc_array()

[Andreas Rheinhardt]

Anton Khirnov:
> Quoting Andreas Rheinhardt (2022-07-12 16:12:16)
>> Anton really dislikes the av_fast_* naming and instead wants this to be
>> called av_realloc_array_reuse(). I don't care either way. Any more
>> opinions on this (or on the patch itself)?
> 
> If people dislike _reuse(), I am open to other reasonable suggestions.
> This 'fast' naming sucks because
> - it tells you nothing about how this function is "fast"
> - it is added at the beginning rather than the end, which is
>   against standard namespacing conventions
> 

Isn't reusing the basic modus operandi for a reallocation function? So
your suggested name doesn't seem to fit either.
(The actual difference of this function to an ordinary function is that
it overallocates and does not shrink the buffer even if it is too big.)

- Andreas
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 3/8] avutil/mem: Add av_fast_realloc_array()

[Anton Khirnov]

Quoting Andreas Rheinhardt (2022-07-14 14:51:07)
> Anton Khirnov:
> > Quoting Andreas Rheinhardt (2022-07-12 16:12:16)
> >> Anton really dislikes the av_fast_* naming and instead wants this to be
> >> called av_realloc_array_reuse(). I don't care either way. Any more
> >> opinions on this (or on the patch itself)?
> > 
> > If people dislike _reuse(), I am open to other reasonable suggestions.
> > This 'fast' naming sucks because
> > - it tells you nothing about how this function is "fast"
> > - it is added at the beginning rather than the end, which is
> >   against standard namespacing conventions
> > 
> 
> Isn't reusing the basic modus operandi for a reallocation function? So
> your suggested name doesn't seem to fit either.

Ordinary realloc just keeps the data, I wouldn't call that "reuse" since
it will often be a copy. This "fast" realloc OTOH reuses the actual
buffer, same as all the other "fast" mem.h functions.

But feel free to suggest another naming pattern if you can think of one.

-- 
Anton Khirnov
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 3/8] avutil/mem: Add av_fast_realloc_array()

[Andreas Rheinhardt]

Anton Khirnov:
> Quoting Andreas Rheinhardt (2022-07-14 14:51:07)
>> Anton Khirnov:
>>> Quoting Andreas Rheinhardt (2022-07-12 16:12:16)
>>>> Anton really dislikes the av_fast_* naming and instead wants this to be
>>>> called av_realloc_array_reuse(). I don't care either way. Any more
>>>> opinions on this (or on the patch itself)?
>>>
>>> If people dislike _reuse(), I am open to other reasonable suggestions.
>>> This 'fast' naming sucks because
>>> - it tells you nothing about how this function is "fast"
>>> - it is added at the beginning rather than the end, which is
>>>   against standard namespacing conventions
>>>
>>
>> Isn't reusing the basic modus operandi for a reallocation function? So
>> your suggested name doesn't seem to fit either.
> 
> Ordinary realloc just keeps the data, I wouldn't call that "reuse" since
> it will often be a copy. This "fast" realloc OTOH reuses the actual
> buffer, same as all the other "fast" mem.h functions.
> 
> But feel free to suggest another naming pattern if you can think of one.
> 

I see two differences between this function and ordinary realloc: It
never shrinks the buffer and it overallocates. These two properties make
it more likely that these functions can avoid copies more often than
plain realloc (but in contrast to realloc, we can not grow the buffer in
case there is free space after it), but it is nevertheless the same as
realloc.

But I don't really care that much about the name and will therefore use
your name as I can't come up with anything better.
(Of course, I am still open to alternative suggestions.)

- Andreas

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 3/8] avutil/mem: Add av_fast_realloc_array()

[Andreas Rheinhardt]

Andreas Rheinhardt:
> Anton Khirnov:
>> Quoting Andreas Rheinhardt (2022-07-14 14:51:07)
>>> Anton Khirnov:
>>>> Quoting Andreas Rheinhardt (2022-07-12 16:12:16)
>>>>> Anton really dislikes the av_fast_* naming and instead wants this to be
>>>>> called av_realloc_array_reuse(). I don't care either way. Any more
>>>>> opinions on this (or on the patch itself)?
>>>>
>>>> If people dislike _reuse(), I am open to other reasonable suggestions.
>>>> This 'fast' naming sucks because
>>>> - it tells you nothing about how this function is "fast"
>>>> - it is added at the beginning rather than the end, which is
>>>>   against standard namespacing conventions
>>>>
>>>
>>> Isn't reusing the basic modus operandi for a reallocation function? So
>>> your suggested name doesn't seem to fit either.
>>
>> Ordinary realloc just keeps the data, I wouldn't call that "reuse" since
>> it will often be a copy. This "fast" realloc OTOH reuses the actual
>> buffer, same as all the other "fast" mem.h functions.
>>
>> But feel free to suggest another naming pattern if you can think of one.
>>
> 
> I see two differences between this function and ordinary realloc: It
> never shrinks the buffer and it overallocates. These two properties make
> it more likely that these functions can avoid copies more often than
> plain realloc (but in contrast to realloc, we can not grow the buffer in
> case there is free space after it), but it is nevertheless the same as
> realloc.
> 
> But I don't really care that much about the name and will therefore use
> your name as I can't come up with anything better.
> (Of course, I am still open to alternative suggestions.)
> 

Here is a rebased branch that uses av_realloc_array_reuse:
https://github.com/mkver/FFmpeg/commits/fast_realloc

- Andreas

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 3/8] avutil/mem: Add av_fast_realloc_array()

[Tomas Härdin]

mån 2022-09-26 klockan 14:25 +0200 skrev Andreas Rheinhardt:
> Anton Khirnov:
> > Quoting Andreas Rheinhardt (2022-07-14 14:51:07)
> > > Anton Khirnov:
> > > > Quoting Andreas Rheinhardt (2022-07-12 16:12:16)
> > > > > Anton really dislikes the av_fast_* naming and instead wants
> > > > > this to be
> > > > > called av_realloc_array_reuse(). I don't care either way. Any
> > > > > more
> > > > > opinions on this (or on the patch itself)?
> > > > 
> > > > If people dislike _reuse(), I am open to other reasonable
> > > > suggestions.
> > > > This 'fast' naming sucks because
> > > > - it tells you nothing about how this function is "fast"
> > > > - it is added at the beginning rather than the end, which is
> > > >   against standard namespacing conventions
> > > > 
> > > 
> > > Isn't reusing the basic modus operandi for a reallocation
> > > function? So
> > > your suggested name doesn't seem to fit either.
> > 
> > Ordinary realloc just keeps the data, I wouldn't call that "reuse"
> > since
> > it will often be a copy. This "fast" realloc OTOH reuses the actual
> > buffer, same as all the other "fast" mem.h functions.
> > 
> > But feel free to suggest another naming pattern if you can think of
> > one.
> > 
> 
> I see two differences between this function and ordinary realloc: It
> never shrinks the buffer and it overallocates. These two properties
> make
> it more likely that these functions can avoid copies more often than
> plain realloc (but in contrast to realloc, we can not grow the buffer
> in
> case there is free space after it), but it is nevertheless the same
> as
> realloc.
> 
> But I don't really care that much about the name and will therefore
> use
> your name as I can't come up with anything better.
> (Of course, I am still open to alternative suggestions.)
> 
> - Andreas

So this means av_realloc_array_reuse()? Eh, it works. I will add a
function that also zeroes the newly allocated space, what should we
call that? av_realloc_array_reusez()?
av_realloc_array_reuse_zerofill()?

/Tomas

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 3/8] avutil/mem: Add av_fast_realloc_array()

[Tomas Härdin]

[-- Attachment #1: Type: text/plain, Size: 2350 bytes --]

mån 2022-09-26 klockan 16:24 +0200 skrev Tomas Härdin:
> mån 2022-09-26 klockan 14:25 +0200 skrev Andreas Rheinhardt:
> > Anton Khirnov:
> > > Quoting Andreas Rheinhardt (2022-07-14 14:51:07)
> > > > Anton Khirnov:
> > > > > Quoting Andreas Rheinhardt (2022-07-12 16:12:16)
> > > > > > Anton really dislikes the av_fast_* naming and instead
> > > > > > wants
> > > > > > this to be
> > > > > > called av_realloc_array_reuse(). I don't care either way.
> > > > > > Any
> > > > > > more
> > > > > > opinions on this (or on the patch itself)?
> > > > > 
> > > > > If people dislike _reuse(), I am open to other reasonable
> > > > > suggestions.
> > > > > This 'fast' naming sucks because
> > > > > - it tells you nothing about how this function is "fast"
> > > > > - it is added at the beginning rather than the end, which is
> > > > >   against standard namespacing conventions
> > > > > 
> > > > 
> > > > Isn't reusing the basic modus operandi for a reallocation
> > > > function? So
> > > > your suggested name doesn't seem to fit either.
> > > 
> > > Ordinary realloc just keeps the data, I wouldn't call that
> > > "reuse"
> > > since
> > > it will often be a copy. This "fast" realloc OTOH reuses the
> > > actual
> > > buffer, same as all the other "fast" mem.h functions.
> > > 
> > > But feel free to suggest another naming pattern if you can think
> > > of
> > > one.
> > > 
> > 
> > I see two differences between this function and ordinary realloc:
> > It
> > never shrinks the buffer and it overallocates. These two properties
> > make
> > it more likely that these functions can avoid copies more often
> > than
> > plain realloc (but in contrast to realloc, we can not grow the
> > buffer
> > in
> > case there is free space after it), but it is nevertheless the same
> > as
> > realloc.
> > 
> > But I don't really care that much about the name and will therefore
> > use
> > your name as I can't come up with anything better.
> > (Of course, I am still open to alternative suggestions.)
> > 
> > - Andreas
> 
> So this means av_realloc_array_reuse()? Eh, it works. I will add a
> function that also zeroes the newly allocated space, what should we
> call that? av_realloc_array_reusez()?
> av_realloc_array_reuse_zerofill()?

Here's a draft patch that calls it av_reallocz_array_reuse(). Needs a
minor version bump of course

/Tomas

[-- Attachment #2: 0001-avutil-mem-Add-av_reallocz_array_reuse.patch --]
[-- Type: text/x-patch, Size: 3735 bytes --]

From 75e0ca71f782b2b18242815b1fbb079a7bfdb5ba Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <git@haerdin.se>
Date: Tue, 27 Sep 2022 15:37:41 +0200
Subject: [PATCH] avutil/mem: Add av_reallocz_array_reuse()

Like av_realloc_array_reuse() but zeroes the newly allocated memory.
---
 libavutil/mem.c | 24 ++++++++++++++++++++++++
 libavutil/mem.h | 31 +++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+)

diff --git a/libavutil/mem.c b/libavutil/mem.c
index c315a38672..3dd34fcf1b 100644
--- a/libavutil/mem.c
+++ b/libavutil/mem.c
@@ -565,6 +565,30 @@ int av_realloc_array_reuse(void *ptr, size_t *nb_allocated,
     return 0;
 }
 
+int av_reallocz_array_reuse(void *ptr, size_t *nb_allocated,
+                            size_t min_nb, size_t max_nb, size_t elsize)
+{
+    size_t nb_initial, nb_allocated2 = *nb_allocated;
+    void *array;
+    int ret;
+
+    // check if *ptr is NULL. nb_allocated is not guaranteed to have an initial value
+    memcpy(&array, ptr, sizeof(array));
+    nb_initial = array ? *nb_allocated : 0;
+
+    if ((ret = av_realloc_array_reuse(ptr, &nb_allocated2, min_nb, max_nb, elsize)))
+        return ret;
+
+    if (nb_allocated2 > nb_initial) {
+        // new space allocated - zero it!
+        memcpy(&array, ptr, sizeof(array));
+        memset((char*)array + nb_initial*elsize, 0, (nb_allocated2 - nb_initial)*elsize);
+        *nb_allocated = nb_allocated2;
+    }
+
+    return ret;
+}
+
 static inline void fast_malloc(void *ptr, unsigned int *size, size_t min_size, int zero_realloc)
 {
     size_t max_size;
diff --git a/libavutil/mem.h b/libavutil/mem.h
index 3638329108..cfcec37e73 100644
--- a/libavutil/mem.h
+++ b/libavutil/mem.h
@@ -410,6 +410,37 @@ void *av_fast_realloc(void *ptr, unsigned int *size, size_t min_size);
 int av_realloc_array_reuse(void *ptr, size_t *nb_allocated,
                            size_t min_nb, size_t max_nb, size_t elsize);
 
+/**
+ * Reallocate the given array if it is not large enough and fill the newly
+ * allocated space with zeroes, otherwise do nothing.
+ * In other words, like av_realloc_array_reuse() but zeroes the new space.
+ *
+ * If `ptr` points to `NULL`, then a new zeroed array is allocated.
+ *
+ * @param[in,out] ptr           Pointer to `NULL` or pointer to an already
+ *                              allocated array. `*ptr` will be set to point
+ *                              to the new array on success.
+ * @param[in,out] nb_allocated  Pointer to the number of elements of the array
+ *                              `*ptr`. `*nb_allocated` is updated to the new
+ *                              number of allocated elements.
+ * @param[in]     min_nb        Desired minimal number of elements in array `*ptr`
+ * @param[in]     max_nb        Maximal number of elements to allocate.
+ * @param[in]     elsize        Size of a single element of the array.
+ *                              Must not be zero.
+ * @return 0 on success, < 0 on failure. On failure, `*ptr` is not freed and
+ *         `*ptr` as well as `*nb_allocated` are unchanged.
+ * @note `max_nb` can be used to limit allocations and make this function
+ *       usable with counters of types other than size_t. It can also be used
+ *       to avoid overflow checks in callers: E.g. setting it to `UINT_MAX - 1`
+ *       means that incrementing an unsigned int in steps of one need not be
+ *       checked for overflow.
+ * @see av_fast_realloc()
+ * @see av_realloc()
+ * @see av_fast_malloc()
+ */
+int av_reallocz_array_reuse(void *ptr, size_t *nb_allocated,
+                            size_t min_nb, size_t max_nb, size_t elsize);
+
 /**
  * Allocate a buffer, reusing the given one if large enough.
  *
-- 
2.30.2


[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 3/8] avutil/mem: Add av_fast_realloc_array()

[Tomas Härdin]

[-- Attachment #1: Type: text/plain, Size: 2752 bytes --]

tis 2022-09-27 klockan 17:23 +0200 skrev Tomas Härdin:
> mån 2022-09-26 klockan 16:24 +0200 skrev Tomas Härdin:
> > mån 2022-09-26 klockan 14:25 +0200 skrev Andreas Rheinhardt:
> > > Anton Khirnov:
> > > > Quoting Andreas Rheinhardt (2022-07-14 14:51:07)
> > > > > Anton Khirnov:
> > > > > > Quoting Andreas Rheinhardt (2022-07-12 16:12:16)
> > > > > > > Anton really dislikes the av_fast_* naming and instead
> > > > > > > wants
> > > > > > > this to be
> > > > > > > called av_realloc_array_reuse(). I don't care either way.
> > > > > > > Any
> > > > > > > more
> > > > > > > opinions on this (or on the patch itself)?
> > > > > > 
> > > > > > If people dislike _reuse(), I am open to other reasonable
> > > > > > suggestions.
> > > > > > This 'fast' naming sucks because
> > > > > > - it tells you nothing about how this function is "fast"
> > > > > > - it is added at the beginning rather than the end, which
> > > > > > is
> > > > > >   against standard namespacing conventions
> > > > > > 
> > > > > 
> > > > > Isn't reusing the basic modus operandi for a reallocation
> > > > > function? So
> > > > > your suggested name doesn't seem to fit either.
> > > > 
> > > > Ordinary realloc just keeps the data, I wouldn't call that
> > > > "reuse"
> > > > since
> > > > it will often be a copy. This "fast" realloc OTOH reuses the
> > > > actual
> > > > buffer, same as all the other "fast" mem.h functions.
> > > > 
> > > > But feel free to suggest another naming pattern if you can
> > > > think
> > > > of
> > > > one.
> > > > 
> > > 
> > > I see two differences between this function and ordinary realloc:
> > > It
> > > never shrinks the buffer and it overallocates. These two
> > > properties
> > > make
> > > it more likely that these functions can avoid copies more often
> > > than
> > > plain realloc (but in contrast to realloc, we can not grow the
> > > buffer
> > > in
> > > case there is free space after it), but it is nevertheless the
> > > same
> > > as
> > > realloc.
> > > 
> > > But I don't really care that much about the name and will
> > > therefore
> > > use
> > > your name as I can't come up with anything better.
> > > (Of course, I am still open to alternative suggestions.)
> > > 
> > > - Andreas
> > 
> > So this means av_realloc_array_reuse()? Eh, it works. I will add a
> > function that also zeroes the newly allocated space, what should we
> > call that? av_realloc_array_reusez()?
> > av_realloc_array_reuse_zerofill()?
> 
> Here's a draft patch that calls it av_reallocz_array_reuse(). Needs a
> minor version bump of course

This makes me realize something: av_realloc_array_reuse() requires that
*nb_allocated == 0 initially but this isn't specified in the
documentation. Patch attached relaxes this.

/Tomas

[-- Attachment #2: 0001-lavu-mem-Do-not-require-nb_allocated-0-when-ptr-NULL.patch --]
[-- Type: text/x-patch, Size: 1149 bytes --]

From df72691f514e2437b1917d808b6fcd153c393c20 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <git@haerdin.se>
Date: Wed, 28 Sep 2022 11:34:45 +0200
Subject: [PATCH] lavu/mem: Do not require *nb_allocated == 0 when *ptr == NULL

---
 libavutil/mem.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/libavutil/mem.c b/libavutil/mem.c
index 781dcbaded..bd2ee342fe 100644
--- a/libavutil/mem.c
+++ b/libavutil/mem.c
@@ -561,7 +561,10 @@ int av_realloc_array_reuse(void *ptr, size_t *nb_allocated,
     void *array;
     size_t nb, max_alloc_size_bytes;
 
-    if (min_nb <= *nb_allocated)
+    memcpy(&array, ptr, sizeof(array));
+
+    // make no demands on *nb_allocated if *ptr == NULL
+    if (array && min_nb <= *nb_allocated)
         return 0;
 
     max_alloc_size_bytes = atomic_load_explicit(&max_alloc_size, memory_order_relaxed);
@@ -571,7 +574,6 @@ int av_realloc_array_reuse(void *ptr, size_t *nb_allocated,
         return AVERROR(ERANGE);
 
     nb = compute_nb(min_nb, max_nb);
-    memcpy(&array, ptr, sizeof(array));
 
     array = av_realloc(array, nb * elsize);
     if (!array)
-- 
2.30.2


[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 3/8] avutil/mem: Add av_fast_realloc_array()

[Andreas Rheinhardt]

Tomas Härdin:
> tis 2022-09-27 klockan 17:23 +0200 skrev Tomas Härdin:
>> mån 2022-09-26 klockan 16:24 +0200 skrev Tomas Härdin:
>>> mån 2022-09-26 klockan 14:25 +0200 skrev Andreas Rheinhardt:
>>>> Anton Khirnov:
>>>>> Quoting Andreas Rheinhardt (2022-07-14 14:51:07)
>>>>>> Anton Khirnov:
>>>>>>> Quoting Andreas Rheinhardt (2022-07-12 16:12:16)
>>>>>>>> Anton really dislikes the av_fast_* naming and instead
>>>>>>>> wants
>>>>>>>> this to be
>>>>>>>> called av_realloc_array_reuse(). I don't care either way.
>>>>>>>> Any
>>>>>>>> more
>>>>>>>> opinions on this (or on the patch itself)?
>>>>>>>
>>>>>>> If people dislike _reuse(), I am open to other reasonable
>>>>>>> suggestions.
>>>>>>> This 'fast' naming sucks because
>>>>>>> - it tells you nothing about how this function is "fast"
>>>>>>> - it is added at the beginning rather than the end, which
>>>>>>> is
>>>>>>>   against standard namespacing conventions
>>>>>>>
>>>>>>
>>>>>> Isn't reusing the basic modus operandi for a reallocation
>>>>>> function? So
>>>>>> your suggested name doesn't seem to fit either.
>>>>>
>>>>> Ordinary realloc just keeps the data, I wouldn't call that
>>>>> "reuse"
>>>>> since
>>>>> it will often be a copy. This "fast" realloc OTOH reuses the
>>>>> actual
>>>>> buffer, same as all the other "fast" mem.h functions.
>>>>>
>>>>> But feel free to suggest another naming pattern if you can
>>>>> think
>>>>> of
>>>>> one.
>>>>>
>>>>
>>>> I see two differences between this function and ordinary realloc:
>>>> It
>>>> never shrinks the buffer and it overallocates. These two
>>>> properties
>>>> make
>>>> it more likely that these functions can avoid copies more often
>>>> than
>>>> plain realloc (but in contrast to realloc, we can not grow the
>>>> buffer
>>>> in
>>>> case there is free space after it), but it is nevertheless the
>>>> same
>>>> as
>>>> realloc.
>>>>
>>>> But I don't really care that much about the name and will
>>>> therefore
>>>> use
>>>> your name as I can't come up with anything better.
>>>> (Of course, I am still open to alternative suggestions.)
>>>>
>>>> - Andreas
>>>
>>> So this means av_realloc_array_reuse()? Eh, it works. I will add a
>>> function that also zeroes the newly allocated space, what should we
>>> call that? av_realloc_array_reusez()?
>>> av_realloc_array_reuse_zerofill()?
>>
>> Here's a draft patch that calls it av_reallocz_array_reuse(). Needs a
>> minor version bump of course
> 
> This makes me realize something: av_realloc_array_reuse() requires that
> *nb_allocated == 0 initially but this isn't specified in the
> documentation. Patch attached relaxes this.
> 

* @param[in,out] nb_allocated  Pointer to the number of elements of the
array
*                              `*ptr`. `*nb_allocated` is updated to the new
*                              number of allocated elements.

If *ptr == NULL, then the number of elements of said array is (of
course) zero, so *nb_allocated has to be set to that. (But if you think
that this needs to be said explicitly, I can document it.)

- Andreas

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 3/8] avutil/mem: Add av_fast_realloc_array()

[Tomas Härdin]

ons 2022-09-28 klockan 13:06 +0200 skrev Andreas Rheinhardt:
> Tomas Härdin:
> > tis 2022-09-27 klockan 17:23 +0200 skrev Tomas Härdin:
> > > mån 2022-09-26 klockan 16:24 +0200 skrev Tomas Härdin:
> > > > mån 2022-09-26 klockan 14:25 +0200 skrev Andreas Rheinhardt:
> > > > > Anton Khirnov:
> > > > > > Quoting Andreas Rheinhardt (2022-07-14 14:51:07)
> > > > > > > Anton Khirnov:
> > > > > > > > Quoting Andreas Rheinhardt (2022-07-12 16:12:16)
> > > > > > > > > Anton really dislikes the av_fast_* naming and
> > > > > > > > > instead
> > > > > > > > > wants
> > > > > > > > > this to be
> > > > > > > > > called av_realloc_array_reuse(). I don't care either
> > > > > > > > > way.
> > > > > > > > > Any
> > > > > > > > > more
> > > > > > > > > opinions on this (or on the patch itself)?
> > > > > > > > 
> > > > > > > > If people dislike _reuse(), I am open to other
> > > > > > > > reasonable
> > > > > > > > suggestions.
> > > > > > > > This 'fast' naming sucks because
> > > > > > > > - it tells you nothing about how this function is
> > > > > > > > "fast"
> > > > > > > > - it is added at the beginning rather than the end,
> > > > > > > > which
> > > > > > > > is
> > > > > > > >   against standard namespacing conventions
> > > > > > > > 
> > > > > > > 
> > > > > > > Isn't reusing the basic modus operandi for a reallocation
> > > > > > > function? So
> > > > > > > your suggested name doesn't seem to fit either.
> > > > > > 
> > > > > > Ordinary realloc just keeps the data, I wouldn't call that
> > > > > > "reuse"
> > > > > > since
> > > > > > it will often be a copy. This "fast" realloc OTOH reuses
> > > > > > the
> > > > > > actual
> > > > > > buffer, same as all the other "fast" mem.h functions.
> > > > > > 
> > > > > > But feel free to suggest another naming pattern if you can
> > > > > > think
> > > > > > of
> > > > > > one.
> > > > > > 
> > > > > 
> > > > > I see two differences between this function and ordinary
> > > > > realloc:
> > > > > It
> > > > > never shrinks the buffer and it overallocates. These two
> > > > > properties
> > > > > make
> > > > > it more likely that these functions can avoid copies more
> > > > > often
> > > > > than
> > > > > plain realloc (but in contrast to realloc, we can not grow
> > > > > the
> > > > > buffer
> > > > > in
> > > > > case there is free space after it), but it is nevertheless
> > > > > the
> > > > > same
> > > > > as
> > > > > realloc.
> > > > > 
> > > > > But I don't really care that much about the name and will
> > > > > therefore
> > > > > use
> > > > > your name as I can't come up with anything better.
> > > > > (Of course, I am still open to alternative suggestions.)
> > > > > 
> > > > > - Andreas
> > > > 
> > > > So this means av_realloc_array_reuse()? Eh, it works. I will
> > > > add a
> > > > function that also zeroes the newly allocated space, what
> > > > should we
> > > > call that? av_realloc_array_reusez()?
> > > > av_realloc_array_reuse_zerofill()?
> > > 
> > > Here's a draft patch that calls it av_reallocz_array_reuse().
> > > Needs a
> > > minor version bump of course
> > 
> > This makes me realize something: av_realloc_array_reuse() requires
> > that
> > *nb_allocated == 0 initially but this isn't specified in the
> > documentation. Patch attached relaxes this.
> > 
> 
> * @param[in,out] nb_allocated  Pointer to the number of elements of
> the
> array
> *                              `*ptr`. `*nb_allocated` is updated to
> the new
> *                              number of allocated elements.
> 
> If *ptr == NULL, then the number of elements of said array is (of
> course) zero, so *nb_allocated has to be set to that.

Keep in mind av_malloc(0) and av_realloc(ptr, 0) are both legal and
will allocate one byte. Seems it's not a problem in this case luckily

> (But if you think
> that this needs to be said explicitly, I can document it.)

It's best to be explicit IMO, so that callers know they can't just use
an uninitialized size_t on the stack or something.

/Tomas

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 3/8] avutil/mem: Add av_fast_realloc_array()

[Tomas Härdin]

tis 2022-07-12 klockan 16:12 +0200 skrev Andreas Rheinhardt:
> 
> Anton really dislikes the av_fast_* naming and instead wants this to
> be
> called av_realloc_array_reuse(). I don't care either way. Any more
> opinions on this (or on the patch itself)?

That's going to cause some "impedance mismatch" given that the other
functions are still called fast rather than reuse, and changing that
would requite a major bump.

Ideally we'd come up with a coherent naming scheme for all of these
functions because frankly it's a mess at the moment. I tried
summarizing all functions in a spreadsheet, and there's a lot of gaps
functionality wise. Best solution would be to write the cartesian
product of all features in one go, with an accompanying coherent naming
scheme.

{pointer, pointer-to-pointer} * {don't care what the data is, copy old
data, copy and zero newly allocated memory, zero entire buffer} * {size
in bytes, nelem*elemsize}

/Tomas

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 3/8] avutil/mem: Add av_fast_realloc_array()

[Anton Khirnov]

Quoting Tomas Härdin (2022-07-21 23:23:25)
> tis 2022-07-12 klockan 16:12 +0200 skrev Andreas Rheinhardt:
> > 
> > Anton really dislikes the av_fast_* naming and instead wants this to
> > be
> > called av_realloc_array_reuse(). I don't care either way. Any more
> > opinions on this (or on the patch itself)?
> 
> That's going to cause some "impedance mismatch" given that the other
> functions are still called fast rather than reuse, and changing that
> would requite a major bump.

One of my other points was that the existing "fast" functions use
unsigned int rather than size_t, so that's another reason to replace
them.

Adding new functions and using them to replace all uses of the old ones
in our codebase does not need a major bump. Actually removing the old
ones does, but that can be done whenever.

-- 
Anton Khirnov
_______________________________________________
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".

[FFmpeg-devel] [PATCH 4/8] avformat/flvenc: Use array instead of linked list for index

[Andreas Rheinhardt]

Using a linked list had very much overhead (the pointer to the next
entry increased the size of the index entry struct from 16 to 24 bytes,
not to mention the overhead of having separate allocations), so it is
better to (re)allocate a continuous array for the index.
av_fast_realloc_array() is used for this purpose, in order not to
reallocate the array for each entry. It's feature of allowing to
restrict the number of elements of the array is put to good use here:
Each entry will lead to 18 bytes being written and the array is
contained in an element whose size field has a length of three bytes,
so that more than (2^24 - 1) / 18 entries make no sense.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavformat/flvenc.c | 60 +++++++++++++++++---------------------------
 1 file changed, 23 insertions(+), 37 deletions(-)

diff --git a/libavformat/flvenc.c b/libavformat/flvenc.c
index b6135b25bf..426135aa15 100644
--- a/libavformat/flvenc.c
+++ b/libavformat/flvenc.c
@@ -34,6 +34,8 @@
 #include "libavutil/opt.h"
 #include "libavcodec/put_bits.h"
 
+/* Each FLVFileposition entry is written as two AMF double values. */
+#define FILEPOSITION_ENTRY_SIZE (2 * 9)
 
 static const AVCodecTag flv_video_codec_ids[] = {
     { AV_CODEC_ID_FLV1,     FLV_CODECID_H263 },
@@ -73,7 +75,6 @@ typedef enum {
 typedef struct FLVFileposition {
     int64_t keyframe_position;
     double keyframe_timestamp;
-    struct FLVFileposition *next;
 } FLVFileposition;
 
 typedef struct FLVContext {
@@ -107,9 +108,9 @@ typedef struct FLVContext {
     int acurframeindex;
     int64_t keyframes_info_offset;
 
-    int64_t filepositions_count;
     FLVFileposition *filepositions;
-    FLVFileposition *head_filepositions;
+    unsigned filepositions_count;
+    size_t filepositions_allocated;
 
     AVCodecParameters *audio_par;
     AVCodecParameters *video_par;
@@ -548,27 +549,20 @@ static void flv_write_codec_header(AVFormatContext* s, AVCodecParameters* par, i
 
 static int flv_append_keyframe_info(AVFormatContext *s, FLVContext *flv, double ts, int64_t pos)
 {
-    FLVFileposition *position = av_malloc(sizeof(FLVFileposition));
-
-    if (!position) {
-        av_log(s, AV_LOG_WARNING, "no mem for add keyframe index!\n");
-        return AVERROR(ENOMEM);
-    }
-
-    position->keyframe_timestamp = ts;
-    position->keyframe_position = pos;
-
-    if (!flv->filepositions_count) {
-        flv->filepositions = position;
-        flv->head_filepositions = flv->filepositions;
-        position->next = NULL;
-    } else {
-        flv->filepositions->next = position;
-        position->next = NULL;
-        flv->filepositions = flv->filepositions->next;
+    /* The filepositions array is part of a metadata element whose size field
+     * is three bytes long; so bound the number of filepositions in order not
+     * to allocate more than could ever be written. */
+    int ret = av_fast_realloc_array(&flv->filepositions,
+                                    &flv->filepositions_allocated,
+                                    flv->filepositions_count + 1,
+                                    (((1 << 24) - 1) - 10) / FILEPOSITION_ENTRY_SIZE,
+                                    sizeof(*flv->filepositions));
+    if (ret < 0) {
+        av_log(s, AV_LOG_WARNING, "Adding entry to keyframe index failed.\n");
+        return ret;
     }
 
-    flv->filepositions_count++;
+    flv->filepositions[flv->filepositions_count++] = (FLVFileposition){ pos, ts };
 
     return 0;
 }
@@ -733,7 +727,7 @@ static int flv_write_trailer(AVFormatContext *s)
     int64_t cur_pos = avio_tell(s->pb);
 
     if (build_keyframes_idx) {
-        FLVFileposition *newflv_posinfo;
+        const FLVFileposition *flv_posinfo = flv->filepositions;
 
         avio_seek(pb, flv->videosize_offset, SEEK_SET);
         put_amf_double(pb, flv->videosize);
@@ -758,15 +752,13 @@ static int flv_write_trailer(AVFormatContext *s)
         avio_seek(pb, flv->keyframes_info_offset, SEEK_SET);
         put_amf_string(pb, "filepositions");
         put_amf_dword_array(pb, flv->filepositions_count);
-        for (newflv_posinfo = flv->head_filepositions; newflv_posinfo; newflv_posinfo = newflv_posinfo->next) {
-            put_amf_double(pb, newflv_posinfo->keyframe_position + flv->keyframe_index_size);
-        }
+        for (unsigned i = 0; i < flv->filepositions_count; i++)
+            put_amf_double(pb, flv_posinfo[i].keyframe_position + flv->keyframe_index_size);
 
         put_amf_string(pb, "times");
         put_amf_dword_array(pb, flv->filepositions_count);
-        for (newflv_posinfo = flv->head_filepositions; newflv_posinfo; newflv_posinfo = newflv_posinfo->next) {
-            put_amf_double(pb, newflv_posinfo->keyframe_timestamp);
-        }
+        for (unsigned i = 0; i < flv->filepositions_count; i++)
+            put_amf_double(pb, flv_posinfo[i].keyframe_timestamp);
 
         put_amf_string(pb, "");
         avio_w8(pb, AMF_END_OF_OBJECT);
@@ -1037,15 +1029,9 @@ static int flv_check_bitstream(AVFormatContext *s, AVStream *st,
 static void flv_deinit(AVFormatContext *s)
 {
     FLVContext *flv = s->priv_data;
-    FLVFileposition *filepos = flv->head_filepositions;
 
-    while (filepos) {
-        FLVFileposition *next = filepos->next;
-        av_free(filepos);
-        filepos = next;
-    }
-    flv->filepositions = flv->head_filepositions = NULL;
-    flv->filepositions_count = 0;
+    flv->filepositions_allocated = flv->filepositions_count = 0;
+    av_freep(&flv->filepositions);
 }
 
 static const AVOption options[] = {
-- 
2.34.1

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 4/8] avformat/flvenc: Use array instead of linked list for index

[Tomas Härdin]

tis 2022-07-05 klockan 22:26 +0200 skrev Andreas Rheinhardt:
> Using a linked list had very much overhead (the pointer to the next
> entry increased the size of the index entry struct from 16 to 24
> bytes,
> not to mention the overhead of having separate allocations), so it is
> better to (re)allocate a continuous array for the index.
> av_fast_realloc_array() is used for this purpose, in order not to
> reallocate the array for each entry. It's feature of allowing to
> restrict the number of elements of the array is put to good use here:
> Each entry will lead to 18 bytes being written and the array is
> contained in an element whose size field has a length of three bytes,
> so that more than (2^24 - 1) / 18 entries make no sense.
> 

> +    /* The filepositions array is part of a metadata element whose
> size field
> +     * is three bytes long; so bound the number of filepositions in
> order not
> +     * to allocate more than could ever be written. */
> +    int ret = av_fast_realloc_array(&flv->filepositions,
> +                                    &flv->filepositions_allocated,
> +                                    flv->filepositions_count + 1,
> +                                    (((1 << 24) - 1) - 10) /

Why -10? I see it bumps max_nb down from 932067 to 932066, but it make
no difference keeping below 1<<24

Looks like the rest of the patch makes the code faster and easier to
read. Very nice.

/Tomas


_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 4/8] avformat/flvenc: Use array instead of linked list for index

[Andreas Rheinhardt]

Tomas Härdin:
> tis 2022-07-05 klockan 22:26 +0200 skrev Andreas Rheinhardt:
>> Using a linked list had very much overhead (the pointer to the next
>> entry increased the size of the index entry struct from 16 to 24
>> bytes,
>> not to mention the overhead of having separate allocations), so it is
>> better to (re)allocate a continuous array for the index.
>> av_fast_realloc_array() is used for this purpose, in order not to
>> reallocate the array for each entry. It's feature of allowing to
>> restrict the number of elements of the array is put to good use here:
>> Each entry will lead to 18 bytes being written and the array is
>> contained in an element whose size field has a length of three bytes,
>> so that more than (2^24 - 1) / 18 entries make no sense.
>>
> 
>> +    /* The filepositions array is part of a metadata element whose
>> size field
>> +     * is three bytes long; so bound the number of filepositions in
>> order not
>> +     * to allocate more than could ever be written. */
>> +    int ret = av_fast_realloc_array(&flv->filepositions,
>> +                                    &flv->filepositions_allocated,
>> +                                    flv->filepositions_count + 1,
>> +                                    (((1 << 24) - 1) - 10) /
> 
> Why -10? I see it bumps max_nb down from 932067 to 932066, but it make
> no difference keeping below 1<<24
> 

This was designed to make the result of the following computation
(performed in shift_data) to always fit into three bytes:

metadata_size = flv->filepositions_count * 9 * 2 + 10

But now upon rereading I see that there is some change added to this
unconditionally after that. I will also account for that.
(Notice that it is not intended to account for flv->metadata_totalsize
which is added later.)

- Andreas
_______________________________________________
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".

[FFmpeg-devel] [PATCH 5/8] avformat/matroskaenc: Use av_fast_realloc_array for index entries

[Andreas Rheinhardt]

Currently, the Matroska muxer reallocates its array of index entries
each time another entry is added. This is bad performance-wise,
especially on Windows where reallocations are slow. This is solved
by switching to av_fast_realloc_array() which ensures that actual
reallocations will happen only seldomly.

For an (admittedly extreme) example which consists of looping a video
consisting of a single keyframe of size 4KB 540000 times this improved
the time for writing a frame from 23524201 decicycles (516466 runs,
7822 skips) to 225240 decicycles (522122 runs, 2166 skips) on Windows.

(Writing CRC-32 elements was disabled for these tests.)

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavformat/matroskaenc.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
index 1256bdfe36..7c7de612de 100644
--- a/libavformat/matroskaenc.c
+++ b/libavformat/matroskaenc.c
@@ -168,7 +168,8 @@ typedef struct mkv_cuepoint {
 
 typedef struct mkv_cues {
     mkv_cuepoint   *entries;
-    int             num_entries;
+    size_t          num_entries;
+    size_t          allocated_entries;
 } mkv_cues;
 
 struct MatroskaMuxContext;
@@ -257,6 +258,10 @@ typedef struct MatroskaMuxContext {
 /** 4 * (1-byte EBML ID, 1-byte EBML size, 8-byte uint max) */
 #define MAX_CUETRACKPOS_SIZE 40
 
+/** Minimal size of CueTrack, CueClusterPosition and CueRelativePosition,
+ *  and 1 + 1 bytes for the overhead of CueTrackPositions itself. */
+#define MIN_CUETRACKPOS_SIZE (1 + 1 + 3 * (1 + 1 + 1))
+
 /** 2 + 1 Simpletag header, 2 + 1 + 8 Name "DURATION", 23B for TagString */
 #define DURATION_SIMPLETAG_SIZE (2 + 1 + (2 + 1 + 8) + 23)
 
@@ -914,16 +919,20 @@ static int mkv_add_cuepoint(MatroskaMuxContext *mkv, int stream, int64_t ts,
                             int64_t cluster_pos, int64_t relative_pos, int64_t duration)
 {
     mkv_cues *cues = &mkv->cues;
-    mkv_cuepoint *entries = cues->entries;
-    unsigned idx = cues->num_entries;
+    mkv_cuepoint *entries;
+    size_t idx = cues->num_entries;
+    int ret;
 
     if (ts < 0)
         return 0;
 
-    entries = av_realloc_array(entries, cues->num_entries + 1, sizeof(mkv_cuepoint));
-    if (!entries)
-        return AVERROR(ENOMEM);
-    cues->entries = entries;
+    ret = av_fast_realloc_array(&cues->entries, &cues->allocated_entries,
+                                cues->num_entries + 1,
+                                MAX_SUPPORTED_EBML_LENGTH / MIN_CUETRACKPOS_SIZE,
+                                sizeof(*cues->entries));
+    if (ret < 0)
+        return ret;
+    entries = cues->entries;
 
     /* Make sure the cues entries are sorted by pts. */
     while (idx > 0 && entries[idx - 1].pts > ts)
-- 
2.34.1

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 5/8] avformat/matroskaenc: Use av_fast_realloc_array for index entries

[Tomas Härdin]

tis 2022-07-05 klockan 22:26 +0200 skrev Andreas Rheinhardt:
> 
> -    entries = av_realloc_array(entries, cues->num_entries + 1,
> sizeof(mkv_cuepoint));
> -    if (!entries)
> -        return AVERROR(ENOMEM);
> -    cues->entries = entries;
> +    ret = av_fast_realloc_array(&cues->entries, &cues-
> >allocated_entries,
> +                                cues->num_entries + 1,
> +                                MAX_SUPPORTED_EBML_LENGTH /
> MIN_CUETRACKPOS_SIZE,

Looks fine since MAX_SUPPORTED_EBML_LENGTH <= INT_MAX. Even SIZE_MAX /
MIN_CUETRACKPOS_SIZE would work. Maybe we can could switch
MAX_SUPPORTED_EBML_LENGTH to

 #define MAX_SUPPORTED_EBML_LENGTH FFMIN(MAX_EBML_LENGTH, SIZE_MAX)

?

/Tomas

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 5/8] avformat/matroskaenc: Use av_fast_realloc_array for index entries

[Andreas Rheinhardt]

Tomas Härdin:
> tis 2022-07-05 klockan 22:26 +0200 skrev Andreas Rheinhardt:
>>
>> -    entries = av_realloc_array(entries, cues->num_entries + 1,
>> sizeof(mkv_cuepoint));
>> -    if (!entries)
>> -        return AVERROR(ENOMEM);
>> -    cues->entries = entries;
>> +    ret = av_fast_realloc_array(&cues->entries, &cues-
>>> allocated_entries,
>> +                                cues->num_entries + 1,
>> +                                MAX_SUPPORTED_EBML_LENGTH /
>> MIN_CUETRACKPOS_SIZE,
> 
> Looks fine since MAX_SUPPORTED_EBML_LENGTH <= INT_MAX. Even SIZE_MAX /
> MIN_CUETRACKPOS_SIZE would work. Maybe we can could switch
> MAX_SUPPORTED_EBML_LENGTH to
> 
>  #define MAX_SUPPORTED_EBML_LENGTH FFMIN(MAX_EBML_LENGTH, SIZE_MAX)
> 
> ?
> 

To quote the comment for MAX_SUPPORTED_EBML_LENGTH:
"/* The dynamic buffer API we rely upon has a limit of INT_MAX;
 * and so has avio_write(). */"
And I don't get why MAX_SUPPORTED_EBML_LENGTH <= INT_MAX is even
relevant here. (Do you worry that MAX_SUPPORTED_EBML_LENGTH /
MIN_CUETRACKPOS_SIZE might not be representable in a size_t? Thinking
about this, defining it as FFMIN3(MAX_EBML_LENGTH, INT_MAX, SIZE_MAX) is
better.)

- Andreas
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 5/8] avformat/matroskaenc: Use av_fast_realloc_array for index entries

[Tomas Härdin]

ons 2022-07-06 klockan 17:10 +0200 skrev Andreas Rheinhardt:
> Tomas Härdin:
> > tis 2022-07-05 klockan 22:26 +0200 skrev Andreas Rheinhardt:
> > > 
> > > -    entries = av_realloc_array(entries, cues->num_entries + 1,
> > > sizeof(mkv_cuepoint));
> > > -    if (!entries)
> > > -        return AVERROR(ENOMEM);
> > > -    cues->entries = entries;
> > > +    ret = av_fast_realloc_array(&cues->entries, &cues-
> > > > allocated_entries,
> > > +                                cues->num_entries + 1,
> > > +                                MAX_SUPPORTED_EBML_LENGTH /
> > > MIN_CUETRACKPOS_SIZE,
> > 
> > Looks fine since MAX_SUPPORTED_EBML_LENGTH <= INT_MAX. Even
> > SIZE_MAX /
> > MIN_CUETRACKPOS_SIZE would work. Maybe we can could switch
> > MAX_SUPPORTED_EBML_LENGTH to
> > 
> >  #define MAX_SUPPORTED_EBML_LENGTH FFMIN(MAX_EBML_LENGTH, SIZE_MAX)
> > 
> > ?
> > 
> 
> To quote the comment for MAX_SUPPORTED_EBML_LENGTH:
> "/* The dynamic buffer API we rely upon has a limit of INT_MAX;
>  * and so has avio_write(). */"
> And I don't get why MAX_SUPPORTED_EBML_LENGTH <= INT_MAX is even
> relevant here. (Do you worry that MAX_SUPPORTED_EBML_LENGTH /
> MIN_CUETRACKPOS_SIZE might not be representable in a size_t? Thinking
> about this, defining it as FFMIN3(MAX_EBML_LENGTH, INT_MAX, SIZE_MAX)
> is
> better.)

INT_MAX <= SIZE_MAX on all platforms I am aware of. Just thought we
might want to support absolutely gargantuan .mkv files. Leaving it as-
is is fine

/Tomas

_______________________________________________
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".

[FFmpeg-devel] [PATCH 6/8] avcodec/movtextenc: Use av_fast_realloc_array

[Andreas Rheinhardt]

It has the advantage of not overallocating beyond the maximum
amount of entries that can be potentially used (namely UINT16_MAX).

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/movtextenc.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/libavcodec/movtextenc.c b/libavcodec/movtextenc.c
index 728338f2cc..fb19b110b4 100644
--- a/libavcodec/movtextenc.c
+++ b/libavcodec/movtextenc.c
@@ -76,7 +76,7 @@ typedef struct {
     ASSStyle *ass_dialog_style;
     StyleBox *style_attributes;
     unsigned  count;
-    unsigned  style_attributes_bytes_allocated;
+    size_t  style_attributes_allocated;
     StyleBox  style_attributes_temp;
     AVBPrint buffer;
     HighlightBox hlit;
@@ -342,6 +342,8 @@ static av_cold int mov_text_encode_init(AVCodecContext *avctx)
 // Start a new style box if needed
 static int mov_text_style_start(MovTextContext *s)
 {
+    int ret;
+
     // there's an existing style entry
     if (s->style_attributes_temp.style_start == s->text_pos)
         // Still at same text pos, use same entry
@@ -353,10 +355,9 @@ static int mov_text_style_start(MovTextContext *s)
         StyleBox *tmp;
 
         // last style != defaults, end the style entry and start a new one
-        if (s->count + 1 > FFMIN(SIZE_MAX / sizeof(*s->style_attributes), UINT16_MAX) ||
-            !(tmp = av_fast_realloc(s->style_attributes,
-                                    &s->style_attributes_bytes_allocated,
-                                    (s->count + 1) * sizeof(*s->style_attributes)))) {
+        ret = av_fast_realloc_array(&s->style_attributes, &s->style_attributes_allocated,
+                                    s->count + 1, UINT16_MAX, sizeof(*s->style_attributes));
+        if (ret < 0) {
             mov_text_cleanup(s);
             av_bprint_clear(&s->buffer);
             s->box_flags &= ~STYL_BOX;
-- 
2.34.1

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 6/8] avcodec/movtextenc: Use av_fast_realloc_array

[Tomas Härdin]

tis 2022-07-05 klockan 22:26 +0200 skrev Andreas Rheinhardt:
> 
> -        if (s->count + 1 > FFMIN(SIZE_MAX / sizeof(*s-
> >style_attributes), UINT16_MAX) ||
> -            !(tmp = av_fast_realloc(s->style_attributes,
> -                                    &s-
> >style_attributes_bytes_allocated,
> -                                    (s->count + 1) * sizeof(*s-
> >style_attributes)))) {
> +        ret = av_fast_realloc_array(&s->style_attributes, &s-
> >style_attributes_allocated,
> +                                    s->count + 1, UINT16_MAX,
> sizeof(*s->style_attributes));

Looks simple enough. Should ever s->count == UINT16_MAX then this will
fail, as it should

/Tomas

_______________________________________________
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".

[FFmpeg-devel] [PATCH 7/8] avutil/fifo: Simplify growing FIFO

[Andreas Rheinhardt]

In case the data in the FIFO currently wraps around,
move the data from the end of the old buffer to the end
of the new buffer instead of moving the data from the start
of the old buffer partially to the end of the new buffer
and partially to the start of the new buffer.
This simplifies the code.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavutil/fifo.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/libavutil/fifo.c b/libavutil/fifo.c
index 51a5af6f39..53359a2112 100644
--- a/libavutil/fifo.c
+++ b/libavutil/fifo.c
@@ -108,17 +108,12 @@ int av_fifo_grow2(AVFifo *f, size_t inc)
         return AVERROR(ENOMEM);
     f->buffer = tmp;
 
-    // move the data from the beginning of the ring buffer
-    // to the newly allocated space
+    // move the data from the end of the ring buffer
+    // to the end of the newly allocated space
     if (f->offset_w <= f->offset_r && !f->is_empty) {
-        const size_t copy = FFMIN(inc, f->offset_w);
-        memcpy(tmp + f->nb_elems * f->elem_size, tmp, copy * f->elem_size);
-        if (copy < f->offset_w) {
-            memmove(tmp, tmp + copy * f->elem_size,
-                    (f->offset_w - copy) * f->elem_size);
-            f->offset_w -= copy;
-        } else
-            f->offset_w = copy == inc ? 0 : f->nb_elems + copy;
+        memmove(tmp + (f->offset_r + inc) * f->elem_size, tmp + f->offset_r * f->elem_size,
+                (f->nb_elems - f->offset_r) * f->elem_size);
+        f->offset_r += inc;
     }
 
     f->nb_elems += inc;
-- 
2.34.1

_______________________________________________
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".

[FFmpeg-devel] [PATCH 8/8] avutil/fifo: Grow FIFO faster when growing automatically

[Andreas Rheinhardt]

Up until now, when the FIFO is grown automatically, it
would be resized by double the amount needed (if possible,
i.e. if compatible with the auto-grow limit).
This implies that if e.g. the user always writes a single
element to the FIFO, the FIFO will be reallocated once
for every two writes (presuming no reads happen inbetween).
This is potentially quadratic (depending upon the realloc
implementation).

This commit changes this by using av_fast_realloc_array
to realloc the buffer. Its ability to not overallocate
beyond a given size allows to honour the user-specified
auto-grow limit.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavutil/fifo.c | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/libavutil/fifo.c b/libavutil/fifo.c
index 53359a2112..04f4d057ca 100644
--- a/libavutil/fifo.c
+++ b/libavutil/fifo.c
@@ -96,6 +96,19 @@ size_t av_fifo_can_write(const AVFifo *f)
     return f->nb_elems - av_fifo_can_read(f);
 }
 
+static void fifo_readjust_after_growing(AVFifo *f, size_t old_size)
+{
+    const size_t inc = f->nb_elems - old_size;
+    // move the data from the end of the ring buffer
+    // to the end of the newly allocated space
+    if (f->offset_w <= f->offset_r && !f->is_empty) {
+        memmove(f->buffer + (f->offset_r + inc) * f->elem_size,
+                f->buffer + f->offset_r * f->elem_size,
+                (old_size - f->offset_r) * f->elem_size);
+        f->offset_r += inc;
+    }
+}
+
 int av_fifo_grow2(AVFifo *f, size_t inc)
 {
     uint8_t *tmp;
@@ -107,16 +120,8 @@ int av_fifo_grow2(AVFifo *f, size_t inc)
     if (!tmp)
         return AVERROR(ENOMEM);
     f->buffer = tmp;
-
-    // move the data from the end of the ring buffer
-    // to the end of the newly allocated space
-    if (f->offset_w <= f->offset_r && !f->is_empty) {
-        memmove(tmp + (f->offset_r + inc) * f->elem_size, tmp + f->offset_r * f->elem_size,
-                (f->nb_elems - f->offset_r) * f->elem_size);
-        f->offset_r += inc;
-    }
-
     f->nb_elems += inc;
+    fifo_readjust_after_growing(f, f->nb_elems - inc);
 
     return 0;
 }
@@ -133,9 +138,15 @@ static int fifo_check_space(AVFifo *f, size_t to_write)
     can_grow = f->auto_grow_limit > f->nb_elems ?
                f->auto_grow_limit - f->nb_elems : 0;
     if ((f->flags & AV_FIFO_FLAG_AUTO_GROW) && need_grow <= can_grow) {
-        // allocate a bit more than necessary, if we can
-        const size_t inc = (need_grow < can_grow / 2 ) ? need_grow * 2 : can_grow;
-        return av_fifo_grow2(f, inc);
+        // Use av_fast_realloc_array() to allocate in a fast way
+        // while respecting the auto_grow_limit
+        const size_t old_size = f->nb_elems;
+        int ret = av_fast_realloc_array(&f->buffer, &f->nb_elems, f->nb_elems + need_grow,
+                                        f->auto_grow_limit, f->elem_size);
+        if (ret < 0)
+            return ret;
+        fifo_readjust_after_growing(f, old_size);
+        return 0;
     }
 
     return AVERROR(ENOSPC);
-- 
2.34.1

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 1/8] avutil/mem: Handle fast allocations near UINT_MAX properly

[Anton Khirnov]

Quoting Andreas Rheinhardt (2022-07-05 22:09:37)
> av_fast_realloc and av_fast_mallocz? store the size of
> the objects they allocate in an unsigned. Yet they overallocate
> and currently they can allocate more than UINT_MAX bytes
> in case a user has requested a size of about UINT_MAX * 16 / 17
> or more if SIZE_MAX > UINT_MAX. In this case it is impossible
> to store the true size of the buffer via the unsigned*;
> future requests are likely to use the (re)allocation codepath
> even if the buffer is actually large enough because of
> the incorrect size.
> 
> Fix this by ensuring that the actually allocated size
> always fits into an unsigned. (This entails erroring out
> in case the user requested more than UINT_MAX.)

I really dislike this av_fast_* naming.

Given that all these functions use unsigned int for something that
should really be size_t, how about we deprecate them all and replace
with something that has a sane naming convention and uses proper types?

-- 
Anton Khirnov
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 1/8] avutil/mem: Handle fast allocations near UINT_MAX properly

[Andreas Rheinhardt]

Anton Khirnov:
> Quoting Andreas Rheinhardt (2022-07-05 22:09:37)
>> av_fast_realloc and av_fast_mallocz? store the size of
>> the objects they allocate in an unsigned. Yet they overallocate
>> and currently they can allocate more than UINT_MAX bytes
>> in case a user has requested a size of about UINT_MAX * 16 / 17
>> or more if SIZE_MAX > UINT_MAX. In this case it is impossible
>> to store the true size of the buffer via the unsigned*;
>> future requests are likely to use the (re)allocation codepath
>> even if the buffer is actually large enough because of
>> the incorrect size.
>>
>> Fix this by ensuring that the actually allocated size
>> always fits into an unsigned. (This entails erroring out
>> in case the user requested more than UINT_MAX.)
> 
> I really dislike this av_fast_* naming.
> 
> Given that all these functions use unsigned int for something that
> should really be size_t, how about we deprecate them all and replace
> with something that has a sane naming convention and uses proper types?
> 

What name do you suggest?
And what's your opinion of the actual patch?

- Andreas
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 1/8] avutil/mem: Handle fast allocations near UINT_MAX properly

[Anton Khirnov]

Quoting Andreas Rheinhardt (2022-07-06 15:08:05)
> Anton Khirnov:
> > Quoting Andreas Rheinhardt (2022-07-05 22:09:37)
> >> av_fast_realloc and av_fast_mallocz? store the size of
> >> the objects they allocate in an unsigned. Yet they overallocate
> >> and currently they can allocate more than UINT_MAX bytes
> >> in case a user has requested a size of about UINT_MAX * 16 / 17
> >> or more if SIZE_MAX > UINT_MAX. In this case it is impossible
> >> to store the true size of the buffer via the unsigned*;
> >> future requests are likely to use the (re)allocation codepath
> >> even if the buffer is actually large enough because of
> >> the incorrect size.
> >>
> >> Fix this by ensuring that the actually allocated size
> >> always fits into an unsigned. (This entails erroring out
> >> in case the user requested more than UINT_MAX.)
> > 
> > I really dislike this av_fast_* naming.
> > 
> > Given that all these functions use unsigned int for something that
> > should really be size_t, how about we deprecate them all and replace
> > with something that has a sane naming convention and uses proper types?
> > 
> 
> What name do you suggest?

I suggested av_?alloc*_reuse() in a recent thread, since those function
are "fast" by reusing the buffer when possible.

> And what's your opinion of the actual patch?

Seems straightforwardly ok.

-- 
Anton Khirnov
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 1/8] avutil/mem: Handle fast allocations near UINT_MAX properly

[Tomas Härdin]

tis 2022-07-05 klockan 22:09 +0200 skrev Andreas Rheinhardt:
> av_fast_realloc and av_fast_mallocz? store the size of
> the objects they allocate in an unsigned. Yet they overallocate
> and currently they can allocate more than UINT_MAX bytes
> in case a user has requested a size of about UINT_MAX * 16 / 17
> or more if SIZE_MAX > UINT_MAX.

I think you mean if max_alloc_size > UINT_MAX

> In this case it is impossible
> to store the true size of the buffer via the unsigned*;
> future requests are likely to use the (re)allocation codepath
> even if the buffer is actually large enough because of
> the incorrect size.
> 
> Fix this by ensuring that the actually allocated size
> always fits into an unsigned. (This entails erroring out
> in case the user requested more than UINT_MAX.)

Who decided unsigned was a good idea in these functions anyway?

> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>  libavutil/mem.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/libavutil/mem.c b/libavutil/mem.c
> index a0c9a42849..18aff5291f 100644
> --- a/libavutil/mem.c
> +++ b/libavutil/mem.c
> @@ -510,6 +510,8 @@ void *av_fast_realloc(void *ptr, unsigned int
> *size, size_t min_size)
>          return ptr;
>  
>      max_size = atomic_load_explicit(&max_alloc_size,
> memory_order_relaxed);
> +    /* *size is an unsigned, so the real maximum is <= UINT_MAX. */
> +    max_size = FFMIN(max_size, UINT_MAX);
>  
>      if (min_size > max_size) {
>          *size = 0;
> @@ -542,6 +544,8 @@ static inline void fast_malloc(void *ptr,
> unsigned int *size, size_t min_size, i
>      }
>  
>      max_size = atomic_load_explicit(&max_alloc_size,
> memory_order_relaxed);
> +    /* *size is an unsigned, so the real maximum is <= UINT_MAX. */
> +    max_size = FFMIN(max_size, UINT_MAX);
>  
>      if (min_size > max_size) {
>          av_freep(ptr);

Looks OK. This is also why I decided to do formal verification on my
av_fast_recalloc() patch. I only verify part of it, so it's vulnerable
to this also.

This is inspiring me to rework my patch to use size_t instead of
unsigned for *size

/Tomas

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 1/8] avutil/mem: Handle fast allocations near UINT_MAX properly

[Andreas Rheinhardt]

Tomas Härdin:
> tis 2022-07-05 klockan 22:09 +0200 skrev Andreas Rheinhardt:
>> av_fast_realloc and av_fast_mallocz? store the size of
>> the objects they allocate in an unsigned. Yet they overallocate
>> and currently they can allocate more than UINT_MAX bytes
>> in case a user has requested a size of about UINT_MAX * 16 / 17
>> or more if SIZE_MAX > UINT_MAX.
> 
> I think you mean if max_alloc_size > UINT_MAX
> 

Both are correct. I should probably add a note to the commit message
that this whole issue can only be encountered if one has increased the
allocation limit by calling av_max_alloc() before that.

>> In this case it is impossible
>> to store the true size of the buffer via the unsigned*;
>> future requests are likely to use the (re)allocation codepath
>> even if the buffer is actually large enough because of
>> the incorrect size.
>>
>> Fix this by ensuring that the actually allocated size
>> always fits into an unsigned. (This entails erroring out
>> in case the user requested more than UINT_MAX.)
> 
> Who decided unsigned was a good idea in these functions anyway?
> 

git log will tell you.

>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>>  libavutil/mem.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/libavutil/mem.c b/libavutil/mem.c
>> index a0c9a42849..18aff5291f 100644
>> --- a/libavutil/mem.c
>> +++ b/libavutil/mem.c
>> @@ -510,6 +510,8 @@ void *av_fast_realloc(void *ptr, unsigned int
>> *size, size_t min_size)
>>          return ptr;
>>  
>>      max_size = atomic_load_explicit(&max_alloc_size,
>> memory_order_relaxed);
>> +    /* *size is an unsigned, so the real maximum is <= UINT_MAX. */
>> +    max_size = FFMIN(max_size, UINT_MAX);
>>  
>>      if (min_size > max_size) {
>>          *size = 0;
>> @@ -542,6 +544,8 @@ static inline void fast_malloc(void *ptr,
>> unsigned int *size, size_t min_size, i
>>      }
>>  
>>      max_size = atomic_load_explicit(&max_alloc_size,
>> memory_order_relaxed);
>> +    /* *size is an unsigned, so the real maximum is <= UINT_MAX. */
>> +    max_size = FFMIN(max_size, UINT_MAX);
>>  
>>      if (min_size > max_size) {
>>          av_freep(ptr);
> 
> Looks OK. This is also why I decided to do formal verification on my
> av_fast_recalloc() patch. I only verify part of it, so it's vulnerable
> to this also.
> 
> This is inspiring me to rework my patch to use size_t instead of
> unsigned for *size

See also 3/8.

- Andreas
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 1/8] avutil/mem: Handle fast allocations near UINT_MAX properly

[Tomas Härdin]

Any update on this patchset? It's mostly fine as I recall

/Tomas

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 1/8] avutil/mem: Handle fast allocations near UINT_MAX properly

[Tomas Härdin]

tis 2022-07-05 klockan 22:09 +0200 skrev Andreas Rheinhardt:
> av_fast_realloc and av_fast_mallocz? store the size of
> the objects they allocate in an unsigned. Yet they overallocate
> and currently they can allocate more than UINT_MAX bytes
> in case a user has requested a size of about UINT_MAX * 16 / 17
> or more if SIZE_MAX > UINT_MAX. In this case it is impossible
> to store the true size of the buffer via the unsigned*;
> future requests are likely to use the (re)allocation codepath
> even if the buffer is actually large enough because of
> the incorrect size.
> 
> Fix this by ensuring that the actually allocated size
> always fits into an unsigned. (This entails erroring out
> in case the user requested more than UINT_MAX.)
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>  libavutil/mem.c | 4 ++++
>  1 file changed, 4 insertions(+)

Second bump for this and patch 3/8. This is holding up my rebasing my
jpeg2000 patches and indirectly Caleb's htj2k stuff benefiting from
them

/Tomas

_______________________________________________
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".