[FFmpeg-devel] [PATCH 2/2] lavc/vp9: fix the race in exporting video enc params

[Anton Khirnov]

The parameters are currently attached directly to the decoded frame
after it is decoded. This is racy, since the frame is shared with other
threads, as reported e.g. by the fate-vp9-encparams test in TSAN build.

Instead, write the params to a refcounted AVBuffer. Each output frame
then gets its own reference to this buffer, which should be thread-safe.
---
 libavcodec/vp9.c | 93 ++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 83 insertions(+), 10 deletions(-)

diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c
index 7ef10f7a80..f9df898733 100644
--- a/libavcodec/vp9.c
+++ b/libavcodec/vp9.c
@@ -41,6 +41,10 @@
 
 #define VP9_SYNCCODE 0x498342
 
+typedef struct VP9TFPriv {
+    AVBufferRef *enc_par;
+} VP9TFPriv;
+
 #if HAVE_THREADS
 DEFINE_OFFSET_ARRAY(VP9Context, vp9_context, pthread_init_cnt,
                     (offsetof(VP9Context, progress_mutex)),
@@ -100,6 +104,13 @@ static void vp9_frame_unref(AVCodecContext *avctx, VP9Frame *f)
     f->hwaccel_picture_private = NULL;
 }
 
+static void vp9_tf_priv_free(void *opaque, uint8_t *data)
+{
+    VP9TFPriv *p = (VP9TFPriv*)data;
+    av_buffer_unref(&p->enc_par);
+    av_freep(&data);
+}
+
 static int vp9_frame_alloc(AVCodecContext *avctx, VP9Frame *f)
 {
     VP9Context *s = avctx->priv_data;
@@ -139,6 +150,19 @@ static int vp9_frame_alloc(AVCodecContext *avctx, VP9Frame *f)
         }
     }
 
+    if (avctx->export_side_data & AV_CODEC_EXPORT_DATA_VIDEO_ENC_PARAMS) {
+        VP9TFPriv *p = av_mallocz(sizeof(*p));
+        if (!p)
+            goto fail;
+
+        f->tf.priv_buf = av_buffer_create((uint8_t*)p, sizeof(*p), vp9_tf_priv_free,
+                                          NULL, AV_BUFFER_FLAG_READONLY);
+        if (!f->tf.priv_buf) {
+            av_freep(&p);
+            goto fail;
+        }
+    }
+
     return 0;
 
 fail:
@@ -1497,7 +1521,9 @@ int loopfilter_proc(AVCodecContext *avctx)
 
 static int vp9_export_enc_params(VP9Context *s, VP9Frame *frame)
 {
+    VP9TFPriv *p = (VP9TFPriv*)frame->tf.priv_buf->data;
     AVVideoEncParams *par;
+    size_t par_size;
     unsigned int tile, nb_blocks = 0;
 
     if (s->s.h.segmentation.enabled) {
@@ -1505,11 +1531,16 @@ static int vp9_export_enc_params(VP9Context *s, VP9Frame *frame)
             nb_blocks += s->td[tile].nb_block_structure;
     }
 
-    par = av_video_enc_params_create_side_data(frame->tf.f,
-        AV_VIDEO_ENC_PARAMS_VP9, nb_blocks);
+    par = av_video_enc_params_alloc(AV_VIDEO_ENC_PARAMS_VP9, nb_blocks, &par_size);
     if (!par)
         return AVERROR(ENOMEM);
 
+    p->enc_par = av_buffer_create((uint8_t *)par, par_size, NULL, NULL, 0);
+    if (!p->enc_par) {
+        av_freep(&par);
+        return AVERROR(ENOMEM);
+    }
+
     par->qp             = s->s.h.yac_qi;
     par->delta_qp[0][0] = s->s.h.ydc_qdelta;
     par->delta_qp[1][0] = s->s.h.uvdc_qdelta;
@@ -1547,6 +1578,38 @@ static int vp9_export_enc_params(VP9Context *s, VP9Frame *frame)
     return 0;
 }
 
+static int vp9_output_frame(AVFrame *dst, const ThreadFrame *src)
+{
+    int ret = 0;
+
+    ret = av_frame_ref(dst, src->f);
+    if (ret < 0)
+        return ret;
+
+    if (src->priv_buf) {
+        VP9TFPriv *p = (VP9TFPriv*)src->priv_buf->data;
+
+        if (p->enc_par) {
+            AVBufferRef *buf = av_buffer_ref(p->enc_par);
+            if (!buf) {
+                ret = AVERROR(ENOMEM);
+                goto finish;
+            }
+
+            if (!av_frame_new_side_data_from_buf(dst, AV_FRAME_DATA_VIDEO_ENC_PARAMS, buf)) {
+                av_buffer_unref(&buf);
+                ret = AVERROR(ENOMEM);
+                goto finish;
+            }
+        }
+    }
+
+finish:
+    if (ret < 0)
+        av_frame_unref(dst);
+    return ret;
+}
+
 static int vp9_decode_frame(AVCodecContext *avctx, void *frame,
                             int *got_frame, AVPacket *pkt)
 {
@@ -1561,11 +1624,14 @@ static int vp9_decode_frame(AVCodecContext *avctx, void *frame,
     if ((ret = decode_frame_header(avctx, data, size, &ref)) < 0) {
         return ret;
     } else if (ret == 0) {
-        if (!s->s.refs[ref].f->buf[0]) {
+        ThreadFrame *tf = &s->s.refs[ref];
+        if (!tf->f->buf[0]) {
             av_log(avctx, AV_LOG_ERROR, "Requested reference %d not available\n", ref);
             return AVERROR_INVALIDDATA;
         }
-        if ((ret = av_frame_ref(frame, s->s.refs[ref].f)) < 0)
+        ff_thread_await_progress(tf, INT_MAX, 0);
+        ret = vp9_output_frame(frame, tf);
+        if (ret < 0)
             return ret;
         ((AVFrame *)frame)->pts = pkt->pts;
         ((AVFrame *)frame)->pkt_dts = pkt->dts;
@@ -1743,6 +1809,17 @@ static int vp9_decode_frame(AVCodecContext *avctx, void *frame,
             ff_thread_finish_setup(avctx);
         }
     } while (s->pass++ == 1);
+
+    /* this must be done before report_progress(INT_MAX), since the exported
+     * data may be read in other threads */
+    if (avctx->export_side_data & AV_CODEC_EXPORT_DATA_VIDEO_ENC_PARAMS) {
+        ret = vp9_export_enc_params(s, &s->s.frames[CUR_FRAME]);
+        if (ret < 0) {
+            ff_thread_report_progress(&s->s.frames[CUR_FRAME].tf, INT_MAX, 0);
+            return ret;
+        }
+    }
+
     ff_thread_report_progress(&s->s.frames[CUR_FRAME].tf, INT_MAX, 0);
 
     if (s->td->error_info < 0) {
@@ -1750,11 +1827,6 @@ static int vp9_decode_frame(AVCodecContext *avctx, void *frame,
         s->td->error_info = 0;
         return AVERROR_INVALIDDATA;
     }
-    if (avctx->export_side_data & AV_CODEC_EXPORT_DATA_VIDEO_ENC_PARAMS) {
-        ret = vp9_export_enc_params(s, &s->s.frames[CUR_FRAME]);
-        if (ret < 0)
-            return ret;
-    }
 
 finish:
     // ref frame setup
@@ -1767,7 +1839,8 @@ finish:
     }
 
     if (!s->s.h.invisible) {
-        if ((ret = av_frame_ref(frame, s->s.frames[CUR_FRAME].tf.f)) < 0)
+        ret = vp9_output_frame(frame, &s->s.frames[CUR_FRAME].tf);
+        if (ret < 0)
             return ret;
         *got_frame = 1;
     }
-- 
2.34.1