[FFmpeg-devel] [PATCH 2/5] avcodec/videotoolboxenc: Remove unused variable

[FFmpeg-devel] [PATCH 2/5] avcodec/videotoolboxenc: Remove unused variable

[Zhao Zhili]

From: Zhao Zhili <zhilizhao@tencent.com>

---
 libavcodec/videotoolboxenc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/libavcodec/videotoolboxenc.c b/libavcodec/videotoolboxenc.c
index 91ee507050..03e3c9ad42 100644
--- a/libavcodec/videotoolboxenc.c
+++ b/libavcodec/videotoolboxenc.c
@@ -229,7 +229,6 @@ typedef struct BufNode {
     CMSampleBufferRef cm_buffer;
     ExtraSEI *sei;
     struct BufNode* next;
-    int error;
 } BufNode;
 
 typedef struct VTEncContext {
-- 
2.42.0

_______________________________________________
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/5] avcodec/videotoolboxenc: Use BufNode as sourceFrameRefcon

[Zhao Zhili]

From: Zhao Zhili <zhilizhao@tencent.com>

ExtraSEI is used as the sourceFrameRefcon of VTCompressionSessionEncodeFrame.
It cannot hold other information which is necessary to fix another issue
in the following patch.

This patch also fixed leak of ExtraSEI on the error path of
vtenc_output_callback.
---
 libavcodec/videotoolboxenc.c | 77 ++++++++++++++++++++++++------------
 1 file changed, 51 insertions(+), 26 deletions(-)

diff --git a/libavcodec/videotoolboxenc.c b/libavcodec/videotoolboxenc.c
index 03e3c9ad42..213bfa8b49 100644
--- a/libavcodec/videotoolboxenc.c
+++ b/libavcodec/videotoolboxenc.c
@@ -347,8 +347,7 @@ static void set_async_error(VTEncContext *vtctx, int err)
 
     while (info) {
         BufNode *next = info->next;
-        CFRelease(info->cm_buffer);
-        av_free(info);
+        vtenc_free_buf_node(info);
         info = next;
     }
 
@@ -438,19 +437,8 @@ static int vtenc_q_pop(VTEncContext *vtctx, bool wait, CMSampleBufferRef *buf, E
     return 0;
 }
 
-static void vtenc_q_push(VTEncContext *vtctx, CMSampleBufferRef buffer, ExtraSEI *sei)
+static void vtenc_q_push(VTEncContext *vtctx, BufNode *info)
 {
-    BufNode *info = av_malloc(sizeof(BufNode));
-    if (!info) {
-        set_async_error(vtctx, AVERROR(ENOMEM));
-        return;
-    }
-
-    CFRetain(buffer);
-    info->cm_buffer = buffer;
-    info->sei = sei;
-    info->next = NULL;
-
     pthread_mutex_lock(&vtctx->lock);
 
     if (!vtctx->q_head) {
@@ -726,6 +714,22 @@ static int set_extradata(AVCodecContext *avctx, CMSampleBufferRef sample_buffer)
     return 0;
 }
 
+static void vtenc_free_buf_node(BufNode *info)
+{
+    if (!info)
+        return;
+
+    if (info->sei) {
+        av_free(info->sei->data);
+        av_free(info->sei);
+    }
+
+    if (info->cm_buffer)
+        CFRelease(info->cm_buffer);
+
+    av_free(info);
+}
+
 static void vtenc_output_callback(
     void *ctx,
     void *sourceFrameCtx,
@@ -735,13 +739,15 @@ static void vtenc_output_callback(
 {
     AVCodecContext *avctx = ctx;
     VTEncContext   *vtctx = avctx->priv_data;
-    ExtraSEI *sei = sourceFrameCtx;
+    BufNode *info = sourceFrameCtx;
 
     if (vtctx->async_error) {
+        vtenc_free_buf_node(info);
         return;
     }
 
     if (status) {
+        vtenc_free_buf_node(info);
         av_log(avctx, AV_LOG_ERROR, "Error encoding frame: %d\n", (int)status);
         set_async_error(vtctx, AVERROR_EXTERNAL);
         return;
@@ -751,15 +757,19 @@ static void vtenc_output_callback(
         return;
     }
 
+    CFRetain(sample_buffer);
+    info->cm_buffer = sample_buffer;
+
     if (!avctx->extradata && (avctx->flags & AV_CODEC_FLAG_GLOBAL_HEADER)) {
         int set_status = set_extradata(avctx, sample_buffer);
         if (set_status) {
+            vtenc_free_buf_node(info);
             set_async_error(vtctx, set_status);
             return;
         }
     }
 
-    vtenc_q_push(vtctx, sample_buffer, sei);
+    vtenc_q_push(vtctx, info);
 }
 
 static int get_length_code_size(
@@ -2565,19 +2575,23 @@ static int vtenc_send_frame(AVCodecContext *avctx,
                             const AVFrame  *frame)
 {
     CMTime time;
-    CFDictionaryRef frame_dict;
+    CFDictionaryRef frame_dict = NULL;
     CVPixelBufferRef cv_img = NULL;
     AVFrameSideData *side_data = NULL;
+    BufNode *node = av_mallocz(sizeof(*node));
     ExtraSEI *sei = NULL;
-    int status = create_cv_pixel_buffer(avctx, frame, &cv_img);
+    int status;
 
-    if (status) return status;
+    if (!node)
+        return AVERROR(ENOMEM);
+
+    status = create_cv_pixel_buffer(avctx, frame, &cv_img);
+    if (status)
+        goto out;
 
     status = create_encoder_dict_h264(frame, &frame_dict);
-    if (status) {
-        CFRelease(cv_img);
-        return status;
-    }
+    if (status)
+        goto out;
 
 #if CONFIG_ATSC_A53
     side_data = av_frame_get_side_data(frame, AV_FRAME_DATA_A53_CC);
@@ -2587,6 +2601,7 @@ static int vtenc_send_frame(AVCodecContext *avctx,
             status = AVERROR(ENOMEM);
             goto out;
         }
+        node->sei = sei;
         status = ff_alloc_a53_sei(frame, 0, &sei->data, &sei->size);
         if (status < 0) {
             av_free(sei);
@@ -2602,7 +2617,7 @@ static int vtenc_send_frame(AVCodecContext *avctx,
         time,
         kCMTimeInvalid,
         frame_dict,
-        sei,
+        node,
         NULL
     );
 
@@ -2616,7 +2631,10 @@ static int vtenc_send_frame(AVCodecContext *avctx,
 out:
     if (frame_dict)
         CFRelease(frame_dict);
-    CFRelease(cv_img);
+    if (cv_img)
+        CFRelease(cv_img);
+    if (status)
+        vtenc_free_buf_node(node);
 
     return status;
 }
@@ -2701,6 +2719,10 @@ static int vtenc_populate_extradata(AVCodecContext   *avctx,
     CVPixelBufferRef pix_buf = NULL;
     CMTime time;
     CMSampleBufferRef buf = NULL;
+    BufNode *node = av_mallocz(sizeof(*node));
+
+    if (!node)
+        return AVERROR(ENOMEM);
 
     status = vtenc_create_encoder(avctx,
                                   codec_type,
@@ -2736,7 +2758,7 @@ static int vtenc_populate_extradata(AVCodecContext   *avctx,
                                              time,
                                              kCMTimeInvalid,
                                              NULL,
-                                             NULL,
+                                             node,
                                              NULL);
 
     if (status) {
@@ -2747,6 +2769,7 @@ static int vtenc_populate_extradata(AVCodecContext   *avctx,
         status = AVERROR_EXTERNAL;
         goto pe_cleanup;
     }
+    node = NULL;
 
     //Populates extradata - output frames are flushed and param sets are available.
     status = VTCompressionSessionCompleteFrames(vtctx->session,
@@ -2773,6 +2796,8 @@ pe_cleanup:
     vtctx->frame_ct_out = 0;
 
     av_assert0(status != 0 || (avctx->extradata && avctx->extradata_size > 0));
+    if (!status)
+        vtenc_free_buf_node(node);
 
     return status;
 }
-- 
2.42.0

_______________________________________________
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/5] avcodec/videotoolboxenc: Fix concurrent access to CVPixelBufferRef

[Zhao Zhili]

From: Zhao Zhili <zhilizhao@tencent.com>

For a frame comes from AV_HWDEVICE_TYPE_VIDEOTOOLBOX, it's
CVPixelBufferRef is maintained by a pool. CVPixelBufferRef returned
to the pool when frame buffer reference reached to zero. However,
VTCompressionSessionEncodeFrame also hold a reference to the
CVPixelBufferRef. So a new frame get from av_hwframe_get_buffer
may access a CVPixelBufferRef which still used by the encoder.
It's only after vtenc_output_callback that we can make sure
CVPixelBufferRef has been released by the encoder.

The issue can be tested with sample from trac #10884.
ffmpeg -hwaccel videotoolbox \
	-hwaccel_output_format videotoolbox_vld \
        -i input.mp4 \
	-c:v hevc_videotoolbox \
	-profile:v main \
        -b:v 3M \
        -vf scale_vt=w=iw/2:h=ih/2:color_matrix=bt709:color_primaries=bt709:color_transfer=bt709 \
        -c:a copy \
	-tag:v hvc1 \
	output.mp4

Withtout the patch, there are some out of order images in output.mp4.
---
 libavcodec/videotoolboxenc.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/libavcodec/videotoolboxenc.c b/libavcodec/videotoolboxenc.c
index 213bfa8b49..9bb9b0b457 100644
--- a/libavcodec/videotoolboxenc.c
+++ b/libavcodec/videotoolboxenc.c
@@ -228,6 +228,7 @@ typedef struct ExtraSEI {
 typedef struct BufNode {
     CMSampleBufferRef cm_buffer;
     ExtraSEI *sei;
+    AVBufferRef *frame_buf;
     struct BufNode* next;
 } BufNode;
 
@@ -727,6 +728,7 @@ static void vtenc_free_buf_node(BufNode *info)
     if (info->cm_buffer)
         CFRelease(info->cm_buffer);
 
+    av_buffer_unref(&info->frame_buf);
     av_free(info);
 }
 
@@ -741,6 +743,7 @@ static void vtenc_output_callback(
     VTEncContext   *vtctx = avctx->priv_data;
     BufNode *info = sourceFrameCtx;
 
+    av_buffer_unref(&info->frame_buf);
     if (vtctx->async_error) {
         vtenc_free_buf_node(info);
         return;
@@ -2459,7 +2462,8 @@ static int copy_avframe_to_pixel_buffer(AVCodecContext   *avctx,
 
 static int create_cv_pixel_buffer(AVCodecContext   *avctx,
                                   const AVFrame    *frame,
-                                  CVPixelBufferRef *cv_img)
+                                  CVPixelBufferRef *cv_img,
+                                  BufNode          *node)
 {
     int plane_count;
     int color;
@@ -2478,6 +2482,12 @@ static int create_cv_pixel_buffer(AVCodecContext   *avctx,
         av_assert0(*cv_img);
 
         CFRetain(*cv_img);
+        if (frame->buf[0]) {
+            node->frame_buf = av_buffer_ref(frame->buf[0]);
+            if (!node->frame_buf)
+                return AVERROR(ENOMEM);
+        }
+
         return 0;
     }
 
@@ -2585,7 +2595,7 @@ static int vtenc_send_frame(AVCodecContext *avctx,
     if (!node)
         return AVERROR(ENOMEM);
 
-    status = create_cv_pixel_buffer(avctx, frame, &cv_img);
+    status = create_cv_pixel_buffer(avctx, frame, &cv_img, node);
     if (status)
         goto out;
 
-- 
2.42.0

_______________________________________________
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] avcodec/videotoolboxenc: Put ExtraSEI inside BufNode directly

[Zhao Zhili]

From: Zhao Zhili <zhilizhao@tencent.com>

Reduce error path and simplify the code.
---
 libavcodec/videotoolboxenc.c | 60 +++++++++++++-----------------------
 1 file changed, 21 insertions(+), 39 deletions(-)

diff --git a/libavcodec/videotoolboxenc.c b/libavcodec/videotoolboxenc.c
index 9bb9b0b457..bd0b55275f 100644
--- a/libavcodec/videotoolboxenc.c
+++ b/libavcodec/videotoolboxenc.c
@@ -227,7 +227,7 @@ typedef struct ExtraSEI {
 
 typedef struct BufNode {
     CMSampleBufferRef cm_buffer;
-    ExtraSEI *sei;
+    ExtraSEI sei;
     AVBufferRef *frame_buf;
     struct BufNode* next;
 } BufNode;
@@ -281,6 +281,18 @@ typedef struct VTEncContext {
     int max_ref_frames;
 } VTEncContext;
 
+static void vtenc_free_buf_node(BufNode *info)
+{
+    if (!info)
+        return;
+
+    av_free(info->sei.data);
+    if (info->cm_buffer)
+        CFRelease(info->cm_buffer);
+    av_buffer_unref(&info->frame_buf);
+    av_free(info);
+}
+
 static int vt_dump_encoder(AVCodecContext *avctx)
 {
     VTEncContext *vtctx = avctx->priv_data;
@@ -388,7 +400,7 @@ static void vtenc_reset(VTEncContext *vtctx)
     }
 }
 
-static int vtenc_q_pop(VTEncContext *vtctx, bool wait, CMSampleBufferRef *buf, ExtraSEI **sei)
+static int vtenc_q_pop(VTEncContext *vtctx, bool wait, CMSampleBufferRef *buf, ExtraSEI *sei)
 {
     BufNode *info;
 
@@ -426,14 +438,12 @@ static int vtenc_q_pop(VTEncContext *vtctx, bool wait, CMSampleBufferRef *buf, E
     pthread_mutex_unlock(&vtctx->lock);
 
     *buf = info->cm_buffer;
+    info->cm_buffer = NULL;
     if (sei && *buf) {
         *sei = info->sei;
-    } else if (info->sei) {
-        if (info->sei->data) av_free(info->sei->data);
-        av_free(info->sei);
+        info->sei = (ExtraSEI) {0};
     }
-    av_free(info);
-
+    vtenc_free_buf_node(info);
 
     return 0;
 }
@@ -715,23 +725,6 @@ static int set_extradata(AVCodecContext *avctx, CMSampleBufferRef sample_buffer)
     return 0;
 }
 
-static void vtenc_free_buf_node(BufNode *info)
-{
-    if (!info)
-        return;
-
-    if (info->sei) {
-        av_free(info->sei->data);
-        av_free(info->sei);
-    }
-
-    if (info->cm_buffer)
-        CFRelease(info->cm_buffer);
-
-    av_buffer_unref(&info->frame_buf);
-    av_free(info);
-}
-
 static void vtenc_output_callback(
     void *ctx,
     void *sourceFrameCtx,
@@ -2589,7 +2582,6 @@ static int vtenc_send_frame(AVCodecContext *avctx,
     CVPixelBufferRef cv_img = NULL;
     AVFrameSideData *side_data = NULL;
     BufNode *node = av_mallocz(sizeof(*node));
-    ExtraSEI *sei = NULL;
     int status;
 
     if (!node)
@@ -2606,15 +2598,8 @@ static int vtenc_send_frame(AVCodecContext *avctx,
 #if CONFIG_ATSC_A53
     side_data = av_frame_get_side_data(frame, AV_FRAME_DATA_A53_CC);
     if (vtctx->a53_cc && side_data && side_data->size) {
-        sei = av_mallocz(sizeof(*sei));
-        if (!sei) {
-            status = AVERROR(ENOMEM);
-            goto out;
-        }
-        node->sei = sei;
-        status = ff_alloc_a53_sei(frame, 0, &sei->data, &sei->size);
+        status = ff_alloc_a53_sei(frame, 0, &node->sei.data, &node->sei.size);
         if (status < 0) {
-            av_free(sei);
             goto out;
         }
     }
@@ -2659,7 +2644,7 @@ static av_cold int vtenc_frame(
     bool get_frame;
     int status;
     CMSampleBufferRef buf = NULL;
-    ExtraSEI *sei = NULL;
+    ExtraSEI sei = {0};
 
     if (frame) {
         status = vtenc_send_frame(avctx, vtctx, frame);
@@ -2700,11 +2685,8 @@ static av_cold int vtenc_frame(
     if (status) goto end_nopkt;
     if (!buf)   goto end_nopkt;
 
-    status = vtenc_cm_to_avpacket(avctx, buf, pkt, sei);
-    if (sei) {
-        if (sei->data) av_free(sei->data);
-        av_free(sei);
-    }
+    status = vtenc_cm_to_avpacket(avctx, buf, pkt, sei.data ? &sei : NULL);
+    av_free(sei.data);
     CFRelease(buf);
     if (status) goto end_nopkt;
 
-- 
2.42.0

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