diff mbox series

[FFmpeg-devel,4/6] avcodec/qsvenc: Combine multiple allocations

Message ID AM7PR03MB66600088D0BC79E5FD08B6C08FA69@AM7PR03MB6660.eurprd03.prod.outlook.com
State New
Headers show
Series [FFmpeg-devel,1/6] avcodec/qsvenc: Fix leak and crash when encoding H.264 due to A53_CC | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

Andreas Rheinhardt Sept. 26, 2021, 6:40 a.m. UTC 
It makes the cleanup code smaller (and reduces the amount of
allocations).

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
Better naming suggestions for the structures welcome.

One could also stop using an av_fifo altogether and use an ordinary
array (that is only allocated once) with FIFO semantics.
Or one could combine the AVPacket and the pointer to the new structure
to one structure, so that one can read and write it from/to the FIFO
in one call.

 libavcodec/qsvenc.c | 111 ++++++++++++++++++--------------------------
 1 file changed, 46 insertions(+), 65 deletions(-)
diff mbox series

Patch

diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c
index 26a94cd419..1650d89a17 100644
--- a/libavcodec/qsvenc.c
+++ b/libavcodec/qsvenc.c
@@ -111,6 +111,19 @@  static const struct {
 #endif
 };
 
+typedef struct QSVTmp {
+    mfxBitstream bs;
+    mfxSyncPoint sync;
+} QSVTmp;
+
+#if QSV_VERSION_ATLEAST(1, 26)
+typedef struct QSVH264Tmp {
+    QSVTmp common;
+    mfxExtAVCEncodedFrameInfo enc_info;
+    mfxExtBuffer *enc_buf;
+} QSVH264Tmp;
+#endif
+
 static const char *print_ratecontrol(mfxU16 rc_mode)
 {
     int i;
@@ -1110,7 +1123,7 @@  static int qsvenc_init_session(AVCodecContext *avctx, QSVEncContext *q)
 
 static inline unsigned int qsv_fifo_item_size(void)
 {
-    return sizeof(AVPacket) + sizeof(mfxSyncPoint*) + sizeof(mfxBitstream*);
+    return sizeof(AVPacket) + sizeof(QSVTmp*);
 }
 
 static inline unsigned int qsv_fifo_size(const AVFifoBuffer* fifo)
@@ -1414,16 +1427,12 @@  static int encode_frame(AVCodecContext *avctx, QSVEncContext *q,
                         const AVFrame *frame)
 {
     AVPacket new_pkt = { 0 };
-    mfxBitstream *bs = NULL;
-#if QSV_VERSION_ATLEAST(1, 26)
-    mfxExtAVCEncodedFrameInfo *enc_info = NULL;
-    mfxExtBuffer **enc_buf = NULL;
-#endif
+    QSVTmp *tmp_struct;
 
     mfxFrameSurface1 *surf = NULL;
-    mfxSyncPoint *sync     = NULL;
     QSVFrame *qsv_frame = NULL;
     mfxEncodeCtrl* enc_ctrl = NULL;
+    size_t alloc_size;
     int ret;
 
     if (frame) {
@@ -1443,34 +1452,36 @@  static int encode_frame(AVCodecContext *avctx, QSVEncContext *q,
                 enc_ctrl->FrameType |= MFX_FRAMETYPE_IDR;
         }
     }
-
     ret = av_new_packet(&new_pkt, q->packet_size);
     if (ret < 0) {
         av_log(avctx, AV_LOG_ERROR, "Error allocating the output packet\n");
         return ret;
     }
-
-    bs = av_mallocz(sizeof(*bs));
-    if (!bs)
-        goto nomem;
-    bs->Data      = new_pkt.data;
-    bs->MaxLength = new_pkt.size;
+    alloc_size = sizeof(QSVTmp);
+#if QSV_VERSION_ATLEAST(1, 26)
+    if (avctx->codec_id == AV_CODEC_ID_H264)
+        alloc_size = sizeof(QSVH264Tmp);
+#endif
+    tmp_struct = av_mallocz(alloc_size);
+    if (!tmp_struct) {
+        ret = AVERROR(ENOMEM);
+        goto free;
+    }
+    tmp_struct->bs.Data      = new_pkt.data;
+    tmp_struct->bs.MaxLength = new_pkt.size;
 
 #if QSV_VERSION_ATLEAST(1, 26)
     if (avctx->codec_id == AV_CODEC_ID_H264) {
-        enc_info = av_mallocz(sizeof(*enc_info));
-        if (!enc_info)
-            goto nomem;
+        QSVH264Tmp *const h264_tmp_struct = (QSVH264Tmp*)tmp_struct;
+        mfxExtAVCEncodedFrameInfo *const enc_info = &h264_tmp_struct->enc_info;
+        mfxExtBuffer **const enc_buf = &h264_tmp_struct->enc_buf;
 
         enc_info->Header.BufferId = MFX_EXTBUFF_ENCODED_FRAME_INFO;
         enc_info->Header.BufferSz = sizeof (*enc_info);
-        bs->NumExtParam = 1;
-        enc_buf = av_mallocz(sizeof(mfxExtBuffer *));
-        if (!enc_buf)
-            goto nomem;
+        tmp_struct->bs.NumExtParam = 1;
         enc_buf[0] = (mfxExtBuffer *)enc_info;
 
-        bs->ExtParam = enc_buf;
+        tmp_struct->bs.ExtParam = enc_buf;
     }
 #endif
 
@@ -1478,12 +1489,9 @@  static int encode_frame(AVCodecContext *avctx, QSVEncContext *q,
         q->set_encode_ctrl_cb(avctx, frame, &qsv_frame->enc_ctrl);
     }
 
-    sync = av_mallocz(sizeof(*sync));
-    if (!sync)
-        goto nomem;
-
     do {
-        ret = MFXVideoENCODE_EncodeFrameAsync(q->session, enc_ctrl, surf, bs, sync);
+        ret = MFXVideoENCODE_EncodeFrameAsync(q->session, enc_ctrl, surf,
+                                              &tmp_struct->bs, &tmp_struct->sync);
         if (ret == MFX_WRN_DEVICE_BUSY)
             av_usleep(500);
     } while (ret == MFX_WRN_DEVICE_BUSY || ret == MFX_WRN_IN_EXECUTION);
@@ -1502,27 +1510,16 @@  static int encode_frame(AVCodecContext *avctx, QSVEncContext *q,
 
     ret = 0;
 
-    if (*sync) {
+    if (tmp_struct->sync) {
         av_fifo_generic_write(q->async_fifo, &new_pkt, sizeof(new_pkt), NULL);
-        av_fifo_generic_write(q->async_fifo, &sync,    sizeof(sync),    NULL);
-        av_fifo_generic_write(q->async_fifo, &bs,      sizeof(bs),    NULL);
+        av_fifo_generic_write(q->async_fifo, &tmp_struct, sizeof(tmp_struct), NULL);
     } else {
 free:
-        av_freep(&sync);
+        av_freep(&tmp_struct);
         av_packet_unref(&new_pkt);
-        av_freep(&bs);
-#if QSV_VERSION_ATLEAST(1, 26)
-        if (avctx->codec_id == AV_CODEC_ID_H264) {
-            av_freep(&enc_info);
-            av_freep(&enc_buf);
-        }
-#endif
     }
 
     return ret;
-nomem:
-    ret = AVERROR(ENOMEM);
-    goto free;
 }
 
 int ff_qsv_encode(AVCodecContext *avctx, QSVEncContext *q,
@@ -1537,20 +1534,19 @@  int ff_qsv_encode(AVCodecContext *avctx, QSVEncContext *q,
     if ((qsv_fifo_size(q->async_fifo) >= q->async_depth) ||
         (!frame && av_fifo_size(q->async_fifo))) {
         AVPacket new_pkt;
+        QSVTmp *tmp_struct;
         mfxBitstream *bs;
-        mfxSyncPoint *sync;
 #if QSV_VERSION_ATLEAST(1, 26)
         mfxExtAVCEncodedFrameInfo *enc_info;
-        mfxExtBuffer **enc_buf;
 #endif
         enum AVPictureType pict_type;
 
         av_fifo_generic_read(q->async_fifo, &new_pkt, sizeof(new_pkt), NULL);
-        av_fifo_generic_read(q->async_fifo, &sync,    sizeof(sync),    NULL);
-        av_fifo_generic_read(q->async_fifo, &bs,      sizeof(bs),      NULL);
+        av_fifo_generic_read(q->async_fifo, &tmp_struct, sizeof(tmp_struct), NULL);
+        bs = &tmp_struct->bs;
 
         do {
-            ret = MFXVideoCORE_SyncOperation(q->session, *sync, 1000);
+            ret = MFXVideoCORE_SyncOperation(q->session, tmp_struct->sync, 1000);
         } while (ret == MFX_WRN_IN_EXECUTION);
 
         new_pkt.dts  = av_rescale_q(bs->DecodeTimeStamp, (AVRational){1, 90000}, avctx->time_base);
@@ -1576,16 +1572,12 @@  int ff_qsv_encode(AVCodecContext *avctx, QSVEncContext *q,
 
 #if QSV_VERSION_ATLEAST(1, 26)
         if (avctx->codec_id == AV_CODEC_ID_H264) {
-            enc_buf = bs->ExtParam;
             enc_info = (mfxExtAVCEncodedFrameInfo *)(*bs->ExtParam);
             ff_side_data_set_encoder_stats(&new_pkt,
                 enc_info->QP * FF_QP2LAMBDA, NULL, 0, pict_type);
-            av_freep(&enc_info);
-            av_freep(&enc_buf);
         }
 #endif
-        av_freep(&bs);
-        av_freep(&sync);
+        av_freep(&tmp_struct);
 
         av_packet_move_ref(pkt, &new_pkt);
 
@@ -1619,23 +1611,12 @@  int ff_qsv_enc_close(AVCodecContext *avctx, QSVEncContext *q)
 
     while (q->async_fifo && av_fifo_size(q->async_fifo)) {
         AVPacket pkt;
-        mfxSyncPoint *sync;
-        mfxBitstream *bs;
+        QSVTmp *tmp_struct;
 
         av_fifo_generic_read(q->async_fifo, &pkt,  sizeof(pkt),  NULL);
-        av_fifo_generic_read(q->async_fifo, &sync, sizeof(sync), NULL);
-        av_fifo_generic_read(q->async_fifo, &bs,   sizeof(bs),   NULL);
+        av_fifo_generic_read(q->async_fifo, &tmp_struct, sizeof(tmp_struct), NULL);
 
-#if QSV_VERSION_ATLEAST(1, 26)
-        if (avctx->codec_id == AV_CODEC_ID_H264) {
-            mfxExtBuffer **enc_buf = bs->ExtParam;
-            mfxExtAVCEncodedFrameInfo *enc_info = (mfxExtAVCEncodedFrameInfo *)(*bs->ExtParam);
-            av_freep(&enc_info);
-            av_freep(&enc_buf);
-        }
-#endif
-        av_freep(&sync);
-        av_freep(&bs);
+        av_freep(&tmp_struct);
         av_packet_unref(&pkt);
     }
     av_fifo_free(q->async_fifo);