[FFmpeg-devel] [PATCH 15/30] lavc/libx265: restructure handling reordered_opaque

[Anton Khirnov]

Current code stores a pointer to allocated data in libx265 and frees it
when the encoded packet is retrieved. This will leak if the packet is
never retrieved, e.g. if the encoder is closed without being flushed.

Restructure the code such that only indices to an array stored in our
private data are given to libx265. This ensures no allocated memory can
be lost.
---
 libavcodec/libx265.c | 85 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 73 insertions(+), 12 deletions(-)

diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c
index e87dea604d..25de3c669b 100644
--- a/libavcodec/libx265.c
+++ b/libavcodec/libx265.c
@@ -27,6 +27,7 @@
 #include <x265.h>
 #include <float.h>
 
+#include "libavutil/avassert.h"
 #include "libavutil/internal.h"
 #include "libavutil/common.h"
 #include "libavutil/opt.h"
@@ -39,6 +40,12 @@
 #include "atsc_a53.h"
 #include "sei.h"
 
+typedef struct ReorderedData {
+    int64_t reordered_opaque;
+
+    int in_use;
+} ReorderedData;
+
 typedef struct libx265Context {
     const AVClass *class;
 
@@ -59,6 +66,9 @@ typedef struct libx265Context {
     int udu_sei;
     int a53_cc;
 
+    ReorderedData *rd;
+    int         nb_rd;
+
     /**
      * If the encoder does not support ROI then warn the first time we
      * encounter a frame with ROI side data.
@@ -81,6 +91,40 @@ static int is_keyframe(NalUnitType naltype)
     }
 }
 
+static int rd_get(libx265Context *ctx)
+{
+    const int add = 16;
+
+    ReorderedData *tmp;
+    int idx;
+
+    for (int i = 0; i < ctx->nb_rd; i++)
+        if (!ctx->rd[i].in_use) {
+            ctx->rd[i].in_use = 1;
+            return i;
+        }
+
+    tmp = av_realloc_array(ctx->rd, ctx->nb_rd + add, sizeof(*ctx->rd));
+    if (!tmp)
+        return AVERROR(ENOMEM);
+    memset(tmp + ctx->nb_rd, 0, sizeof(*tmp) * add);
+
+    ctx->rd     = tmp;
+    ctx->nb_rd += add;
+
+    idx                 = ctx->nb_rd - add;
+    ctx->rd[idx].in_use = 1;
+
+    return idx;
+}
+
+static void rd_release(libx265Context *ctx, int idx)
+{
+    av_assert0(idx >= 0 && idx < ctx->nb_rd);
+
+    memset(&ctx->rd[idx], 0, sizeof(ctx->rd[idx]));
+}
+
 static av_cold int libx265_encode_close(AVCodecContext *avctx)
 {
     libx265Context *ctx = avctx->priv_data;
@@ -88,6 +132,8 @@ static av_cold int libx265_encode_close(AVCodecContext *avctx)
     ctx->api->param_free(ctx->params);
     av_freep(&ctx->sei_data);
 
+    av_freep(&ctx->rd);
+
     if (ctx->encoder)
         ctx->api->encoder_close(ctx->encoder);
 
@@ -499,12 +545,18 @@ static av_cold int libx265_encode_set_roi(libx265Context *ctx, const AVFrame *fr
     return 0;
 }
 
-static void free_picture(x265_picture *pic)
+static void free_picture(libx265Context *ctx, x265_picture *pic)
 {
     x265_sei *sei = &pic->userSEI;
     for (int i = 0; i < sei->numPayloads; i++)
         av_free(sei->payloads[i].payload);
-    av_freep(&pic->userData);
+
+    if (pic->userData) {
+        int idx = (int)(intptr_t)pic->userData - 1;
+        rd_release(ctx, idx);
+        pic->userData = NULL;
+    }
+
     av_freep(&pic->quantOffsets);
     sei->numPayloads = 0;
 }
@@ -549,13 +601,18 @@ static int libx265_encode_frame(AVCodecContext *avctx, AVPacket *pkt,
             return ret;
 
         if (pic->reordered_opaque) {
-            x265pic.userData = av_malloc(sizeof(pic->reordered_opaque));
-            if (!x265pic.userData) {
-                free_picture(&x265pic);
-                return AVERROR(ENOMEM);
+            ReorderedData *rd;
+            int rd_idx = rd_get(ctx);
+
+            if (rd_idx < 0) {
+                free_picture(ctx, &x265pic);
+                return rd_idx;
             }
 
-            memcpy(x265pic.userData, &pic->reordered_opaque, sizeof(pic->reordered_opaque));
+            x265pic.userData = (void*)(intptr_t)(rd_idx + 1);
+
+            rd = &ctx->rd[rd_idx];
+            rd->reordered_opaque = pic->reordered_opaque;
         }
 
         if (ctx->a53_cc) {
@@ -574,7 +631,7 @@ static int libx265_encode_frame(AVCodecContext *avctx, AVPacket *pkt,
                         (sei->numPayloads + 1) * sizeof(*sei_payload));
                 if (!tmp) {
                     av_free(sei_data);
-                    free_picture(&x265pic);
+                    free_picture(ctx, &x265pic);
                     return AVERROR(ENOMEM);
                 }
                 ctx->sei_data = tmp;
@@ -600,7 +657,7 @@ static int libx265_encode_frame(AVCodecContext *avctx, AVPacket *pkt,
                         &ctx->sei_data_size,
                         (sei->numPayloads + 1) * sizeof(*sei_payload));
                 if (!tmp) {
-                    free_picture(&x265pic);
+                    free_picture(ctx, &x265pic);
                     return AVERROR(ENOMEM);
                 }
                 ctx->sei_data = tmp;
@@ -608,7 +665,7 @@ static int libx265_encode_frame(AVCodecContext *avctx, AVPacket *pkt,
                 sei_payload = &sei->payloads[sei->numPayloads];
                 sei_payload->payload = av_memdup(side_data->data, side_data->size);
                 if (!sei_payload->payload) {
-                    free_picture(&x265pic);
+                    free_picture(ctx, &x265pic);
                     return AVERROR(ENOMEM);
                 }
                 sei_payload->payloadSize = side_data->size;
@@ -680,8 +737,12 @@ static int libx265_encode_frame(AVCodecContext *avctx, AVPacket *pkt,
     ff_side_data_set_encoder_stats(pkt, x265pic_out.frameData.qp * FF_QP2LAMBDA, NULL, 0, pict_type);
 
     if (x265pic_out.userData) {
-        memcpy(&avctx->reordered_opaque, x265pic_out.userData, sizeof(avctx->reordered_opaque));
-        av_freep(&x265pic_out.userData);
+        int idx = (int)(intptr_t)x265pic_out.userData - 1;
+        ReorderedData *rd = &ctx->rd[idx];
+
+        avctx->reordered_opaque = rd->reordered_opaque;
+
+        rd_release(ctx, idx);
     } else
         avctx->reordered_opaque = 0;
 
-- 
2.35.1