[FFmpeg-devel] [PATCH 1/2] avcodec/libx265: Don't use AVBPrint unnecessarily

[FFmpeg-devel] [PATCH 1/2] avcodec/libx265: Don't use AVBPrint unnecessarily

[Andreas Rheinhardt]

This code uses the AVBPrint API for exactly one av_bprintf()
in a scenario in which a good upper bound for the needed
size of the buffer is available (with said upper bound being
much smaller than sizeof(AVBPrint)). So one can simply use
snprintf() instead. This also avoids the (always-false due to
the current size of the internal AVBPrint buffer) check for
whether the AVBPrint is complete.

Furthermore, the old code used AV_BPRINT_SIZE_AUTOMATIC
which implies that the AVBPrint buffer will never be
(re)allocated and yet it used av_bprint_finalize().
This has of course also been removed.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
This has actually been mentioned in my review of v3
of this patchset:
https://patchwork.ffmpeg.org/project/ffmpeg/patch/20230817214858.184010-13-jeebjp@gmail.com/

 libavcodec/libx265.c | 31 ++++++-------------------------
 1 file changed, 6 insertions(+), 25 deletions(-)

diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c
index 70ec6d3539..d7620878b8 100644
--- a/libavcodec/libx265.c
+++ b/libavcodec/libx265.c
@@ -28,17 +28,14 @@
 #include <float.h>
 
 #include "libavutil/avassert.h"
-#include "libavutil/bprint.h"
 #include "libavutil/buffer.h"
 #include "libavutil/internal.h"
-#include "libavutil/common.h"
 #include "libavutil/mastering_display_metadata.h"
 #include "libavutil/opt.h"
 #include "libavutil/pixdesc.h"
 #include "avcodec.h"
 #include "codec_internal.h"
 #include "encode.h"
-#include "internal.h"
 #include "packet_internal.h"
 #include "atsc_a53.h"
 #include "sei.h"
@@ -182,13 +179,10 @@ static int handle_mdcv(const AVClass **avcl, const x265_api *api,
                        x265_param *params,
                        const AVMasteringDisplayMetadata *mdcv)
 {
-    int ret = AVERROR_BUG;
-    AVBPrint buf;
-    av_bprint_init(&buf, 0, AV_BPRINT_SIZE_AUTOMATIC);
+    char buf[10 /* # of PRId64s */ * 20 /* max strlen for %PRId64 */ + sizeof("G(,)B(,)R(,)WP(,)L(,)")];
 
     // G(%hu,%hu)B(%hu,%hu)R(%hu,%hu)WP(%hu,%hu)L(%u,%u)
-    av_bprintf(
-        &buf,
+    snprintf(buf, sizeof(buf),
         "G(%"PRId64",%"PRId64")B(%"PRId64",%"PRId64")R(%"PRId64",%"PRId64")"
         "WP(%"PRId64",%"PRId64")L(%"PRId64",%"PRId64")",
         av_rescale_q(1, mdcv->display_primaries[1][0], (AVRational){ 1, 50000 }),
@@ -202,28 +196,15 @@ static int handle_mdcv(const AVClass **avcl, const x265_api *api,
         av_rescale_q(1, mdcv->max_luminance,  (AVRational){ 1, 10000 }),
         av_rescale_q(1, mdcv->min_luminance,  (AVRational){ 1, 10000 }));
 
-    if (!av_bprint_is_complete(&buf)) {
-        av_log(avcl, AV_LOG_ERROR,
-          "MDCV string too long for its available space!\n");
-        ret = AVERROR(ENOMEM);
-        goto end;
-    }
-
-    if (api->param_parse(params, "master-display", buf.str) ==
+    if (api->param_parse(params, "master-display", buf) ==
             X265_PARAM_BAD_VALUE) {
         av_log(avcl, AV_LOG_ERROR,
                "Invalid value \"%s\" for param \"master-display\".\n",
-               buf.str);
-        ret = AVERROR(EINVAL);
-        goto end;
+               buf);
+        return AVERROR(EINVAL);
     }
 
-    ret = 0;
-
-end:
-    av_bprint_finalize(&buf, NULL);
-
-    return ret;
+    return 0;
 }
 
 static int handle_side_data(AVCodecContext *avctx, const x265_api *api,
-- 
2.40.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/2] avcodec/libx265: Pass logctx as void*, not AVClass**

[Andreas Rheinhardt]

The latter need not be save, because av_log() expects
to get a pointer to an AVClass-enabled structure
and not only a fake object. If this function were actually
be called in the following way:

const AVClass *avcl = avctx->av_class;
handle_mdcv(&avcl, );

the AVClass's item_name would expect to point to an actual
AVCodecContext, potentially leading to a segfault.

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

diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c
index d7620878b8..45349a85b9 100644
--- a/libavcodec/libx265.c
+++ b/libavcodec/libx265.c
@@ -175,7 +175,7 @@ static av_cold int libx265_param_parse_int(AVCodecContext *avctx,
     return 0;
 }
 
-static int handle_mdcv(const AVClass **avcl, const x265_api *api,
+static int handle_mdcv(void *logctx, const x265_api *api,
                        x265_param *params,
                        const AVMasteringDisplayMetadata *mdcv)
 {
@@ -198,7 +198,7 @@ static int handle_mdcv(const AVClass **avcl, const x265_api *api,
 
     if (api->param_parse(params, "master-display", buf) ==
             X265_PARAM_BAD_VALUE) {
-        av_log(avcl, AV_LOG_ERROR,
+        av_log(logctx, AV_LOG_ERROR,
                "Invalid value \"%s\" for param \"master-display\".\n",
                buf);
         return AVERROR(EINVAL);
@@ -230,7 +230,7 @@ static int handle_side_data(AVCodecContext *avctx, const x265_api *api,
 
     if (mdcv_sd) {
         int ret = handle_mdcv(
-            &avctx->av_class, api, params,
+            avctx, api, params,
             (AVMasteringDisplayMetadata *)mdcv_sd->data);
         if (ret < 0)
             return ret;
-- 
2.40.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 3/4] avutil/frame: Constify av_frame_side_data_get()

[Andreas Rheinhardt]

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
Will bump minor and add an APIChanges entry for patches 3+4
on push.

 libavutil/frame.c | 2 +-
 libavutil/frame.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/libavutil/frame.c b/libavutil/frame.c
index 89db687d9c..8598aa98a2 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -813,7 +813,7 @@ int av_frame_side_data_clone(AVFrameSideData ***sd, int *nb_sd,
     return 0;
 }
 
-const AVFrameSideData *av_frame_side_data_get(const AVFrameSideData **sd,
+const AVFrameSideData *av_frame_side_data_get(const AVFrameSideData * const *sd,
                                               const int nb_sd,
                                               enum AVFrameSideDataType type)
 {
diff --git a/libavutil/frame.h b/libavutil/frame.h
index a7fc909ad8..cf9ffe1ba9 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -1051,7 +1051,7 @@ int av_frame_side_data_clone(AVFrameSideData ***sd, int *nb_sd,
  * @return a pointer to the side data of a given type on success, NULL if there
  *         is no side data with such type in this set.
  */
-const AVFrameSideData *av_frame_side_data_get(const AVFrameSideData **sd,
+const AVFrameSideData *av_frame_side_data_get(const AVFrameSideData * const *sd,
                                               const int nb_sd,
                                               enum AVFrameSideDataType type);
 
-- 
2.40.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 4/4] avutil/frame: Rename av_frame_side_data_get and add wrapper for it

[Andreas Rheinhardt]

av_frame_side_data_get() has a const AVFrameSideData * const *sd
parameter; so calling it with an AVFramesSideData **sd like
AVCodecContext.decoded_side_data (or with a AVFramesSideData * const
*sd) is safe, but the conversion is not performed automatically
in C. All users of this function therefore resort to a cast.

This commit changes this: av_frame_side_data_get() is renamed
to av_frame_side_data_get_c(); furthermore, a static inline
wrapper for it name av_frame_side_data_get() is added
that accepts an AVFramesSideData * const * and converts this
to const AVFramesSideData * const * in a Wcast-qual safe way.

This also allows to remove the casts from the current users.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/libsvtav1.c |  6 ++----
 libavcodec/libx264.c   |  6 ++----
 libavcodec/libx265.c   |  6 ++----
 libavutil/frame.c      |  8 ++++----
 libavutil/frame.h      | 19 +++++++++++++++++--
 5 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/libavcodec/libsvtav1.c b/libavcodec/libsvtav1.c
index 6400a6507a..8fa42d590b 100644
--- a/libavcodec/libsvtav1.c
+++ b/libavcodec/libsvtav1.c
@@ -180,12 +180,10 @@ static void handle_side_data(AVCodecContext *avctx,
                              EbSvtAv1EncConfiguration *param)
 {
     const AVFrameSideData *cll_sd =
-        av_frame_side_data_get(
-            (const AVFrameSideData **)avctx->decoded_side_data,
+        av_frame_side_data_get(avctx->decoded_side_data,
             avctx->nb_decoded_side_data, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL);
     const AVFrameSideData *mdcv_sd =
-        av_frame_side_data_get(
-            (const AVFrameSideData **)avctx->decoded_side_data,
+        av_frame_side_data_get(avctx->decoded_side_data,
             avctx->nb_decoded_side_data,
             AV_FRAME_DATA_MASTERING_DISPLAY_METADATA);
 
diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
index 3d195fa6b6..eadb20d2b3 100644
--- a/libavcodec/libx264.c
+++ b/libavcodec/libx264.c
@@ -1044,12 +1044,10 @@ static void handle_side_data(AVCodecContext *avctx, x264_param_t *params)
 {
 #if CONFIG_LIBX264_HDR10
     const AVFrameSideData *cll_sd =
-        av_frame_side_data_get(
-            (const AVFrameSideData **)avctx->decoded_side_data,
+        av_frame_side_data_get(avctx->decoded_side_data,
             avctx->nb_decoded_side_data, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL);
     const AVFrameSideData *mdcv_sd =
-        av_frame_side_data_get(
-            (const AVFrameSideData **)avctx->decoded_side_data,
+        av_frame_side_data_get(avctx->decoded_side_data,
             avctx->nb_decoded_side_data,
             AV_FRAME_DATA_MASTERING_DISPLAY_METADATA);
 
diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c
index 45349a85b9..d3e74eaacf 100644
--- a/libavcodec/libx265.c
+++ b/libavcodec/libx265.c
@@ -211,12 +211,10 @@ static int handle_side_data(AVCodecContext *avctx, const x265_api *api,
                             x265_param *params)
 {
     const AVFrameSideData *cll_sd =
-        av_frame_side_data_get(
-            (const AVFrameSideData **)avctx->decoded_side_data,
+        av_frame_side_data_get(avctx->decoded_side_data,
             avctx->nb_decoded_side_data, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL);
     const AVFrameSideData *mdcv_sd =
-        av_frame_side_data_get(
-            (const AVFrameSideData **)avctx->decoded_side_data,
+        av_frame_side_data_get(avctx->decoded_side_data,
             avctx->nb_decoded_side_data,
             AV_FRAME_DATA_MASTERING_DISPLAY_METADATA);
 
diff --git a/libavutil/frame.c b/libavutil/frame.c
index 8598aa98a2..7dd37e5490 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -813,9 +813,9 @@ int av_frame_side_data_clone(AVFrameSideData ***sd, int *nb_sd,
     return 0;
 }
 
-const AVFrameSideData *av_frame_side_data_get(const AVFrameSideData * const *sd,
-                                              const int nb_sd,
-                                              enum AVFrameSideDataType type)
+const AVFrameSideData *av_frame_side_data_get_c(const AVFrameSideData * const *sd,
+                                                const int nb_sd,
+                                                enum AVFrameSideDataType type)
 {
     for (int i = 0; i < nb_sd; i++) {
         if (sd[i]->type == type)
@@ -828,7 +828,7 @@ AVFrameSideData *av_frame_get_side_data(const AVFrame *frame,
                                         enum AVFrameSideDataType type)
 {
     return (AVFrameSideData *)av_frame_side_data_get(
-        (const AVFrameSideData **)frame->side_data, frame->nb_side_data,
+        frame->side_data, frame->nb_side_data,
         type
     );
 }
diff --git a/libavutil/frame.h b/libavutil/frame.h
index cf9ffe1ba9..373866c600 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -1051,9 +1051,24 @@ int av_frame_side_data_clone(AVFrameSideData ***sd, int *nb_sd,
  * @return a pointer to the side data of a given type on success, NULL if there
  *         is no side data with such type in this set.
  */
-const AVFrameSideData *av_frame_side_data_get(const AVFrameSideData * const *sd,
+const AVFrameSideData *av_frame_side_data_get_c(const AVFrameSideData * const *sd,
+                                                const int nb_sd,
+                                                enum AVFrameSideDataType type);
+
+/**
+ * Wrapper around av_frame_side_data_get_c() to workaround the limitation
+ * that for any type T the conversion from T * const * to const T * const *
+ * is not performed automatically in C.
+ * @see av_frame_side_data_get_c()
+ */
+static inline
+const AVFrameSideData *av_frame_side_data_get(AVFrameSideData * const *sd,
                                               const int nb_sd,
-                                              enum AVFrameSideDataType type);
+                                              enum AVFrameSideDataType type)
+{
+    return av_frame_side_data_get_c((const AVFrameSideData * const *)sd,
+                                    nb_sd, type);
+}
 
 /**
  * @}
-- 
2.40.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 5/5] avutil/frame: Use av_realloc_array(), improve overflow check

[Andreas Rheinhardt]

Also use sizeof of the proper type, namely sizeof(**sd)
and not sizeof(*sd).

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

diff --git a/libavutil/frame.c b/libavutil/frame.c
index 7dd37e5490..d7a32cdc92 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -721,10 +721,11 @@ static AVFrameSideData *add_side_data_from_buf(AVFrameSideData ***sd,
     if (!buf)
         return NULL;
 
-    if (*nb_sd > INT_MAX / sizeof(*sd) - 1)
+    // *nb_sd + 1 needs to fit into an int and a size_t.
+    if ((unsigned)*nb_sd >= FFMIN(INT_MAX, SIZE_MAX))
         return NULL;
 
-    tmp = av_realloc(*sd, (*nb_sd + 1) * sizeof(*sd));
+    tmp = av_realloc_array(*sd, sizeof(**sd), *nb_sd + 1);
     if (!tmp)
         return NULL;
     *sd = tmp;
-- 
2.40.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/5] avutil/frame: Use av_realloc_array(), improve overflow check

[Jan Ekström]

On Fri, Mar 22, 2024 at 4:41 PM Andreas Rheinhardt
<andreas.rheinhardt@outlook.com> wrote:
>
> Also use sizeof of the proper type, namely sizeof(**sd)
> and not sizeof(*sd).
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>  libavutil/frame.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/libavutil/frame.c b/libavutil/frame.c
> index 7dd37e5490..d7a32cdc92 100644
> --- a/libavutil/frame.c
> +++ b/libavutil/frame.c
> @@ -721,10 +721,11 @@ static AVFrameSideData *add_side_data_from_buf(AVFrameSideData ***sd,
>      if (!buf)
>          return NULL;
>
> -    if (*nb_sd > INT_MAX / sizeof(*sd) - 1)
> +    // *nb_sd + 1 needs to fit into an int and a size_t.
> +    if ((unsigned)*nb_sd >= FFMIN(INT_MAX, SIZE_MAX))
>          return NULL;
>
> -    tmp = av_realloc(*sd, (*nb_sd + 1) * sizeof(*sd));
> +    tmp = av_realloc_array(*sd, sizeof(**sd), *nb_sd + 1);
>      if (!tmp)
>          return NULL;
>      *sd = tmp;
> --

Thanks for noticing these things like the AVClass-usage, figuring out
a workaround for the getter and improving on the overflow check as
well as doing the AVBPrint to snprintf switch.

Will have to think why I thought that AVClass thing was somehow a good idea.

Set LGTM.

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