[FFmpeg-devel] [PATCH 1/2] lavc/videotoolbox: do not pass AVCodecContext to decoder output callback

[FFmpeg-devel] [PATCH 1/2] lavc/videotoolbox: do not pass AVCodecContext to decoder output callback

[Anton Khirnov]

The opaque parameter for the callback is set in videotoolbox_start(),
called when the hwaccel is initialized. When frame threading is used,
avctx will be the context corresponding to the frame thread currently
doing the decoding. Using this same codec context in all subsequent
invocations of the decoder callback (even those triggered by a different
frame thread) is unsafe, and broken after
cc867f2c09d2b69cee8a0eccd62aff002cbbfe11, since each frame thread now
cleans up its hwaccel state after decoding each frame.

Fix this by passing hwaccel_priv_data as the opaque parameter, which
exists in a single instance forwarded between all frame threads.

The only other use of AVCodecContext in the decoder output callback is
as a logging context. For this purpose, store a logging context in
hwaccel_priv_data.
---
Initial version of this patch tested by Marvin on IRC.
Someone please test this final version, as I don't have the HW to do it
myself.
---
 libavcodec/videotoolbox.c | 10 ++++++----
 libavcodec/vt_internal.h  |  2 ++
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/libavcodec/videotoolbox.c b/libavcodec/videotoolbox.c
index ce83c2594a..d61d310600 100644
--- a/libavcodec/videotoolbox.c
+++ b/libavcodec/videotoolbox.c
@@ -690,8 +690,7 @@ static void videotoolbox_decoder_callback(void *opaque,
                                           CMTime pts,
                                           CMTime duration)
 {
-    AVCodecContext *avctx = opaque;
-    VTContext *vtctx = avctx->internal->hwaccel_priv_data;
+    VTContext *vtctx = opaque;
 
     if (vtctx->frame) {
         CVPixelBufferRelease(vtctx->frame);
@@ -699,7 +698,8 @@ static void videotoolbox_decoder_callback(void *opaque,
     }
 
     if (!image_buffer) {
-        av_log(avctx, status ? AV_LOG_WARNING : AV_LOG_DEBUG, "vt decoder cb: output image buffer is null: %i\n", status);
+        av_log(vtctx->logctx, status ? AV_LOG_WARNING : AV_LOG_DEBUG,
+               "vt decoder cb: output image buffer is null: %i\n", status);
         return;
     }
 
@@ -949,7 +949,7 @@ static int videotoolbox_start(AVCodecContext *avctx)
                                                      videotoolbox->cv_pix_fmt_type);
 
     decoder_cb.decompressionOutputCallback = videotoolbox_decoder_callback;
-    decoder_cb.decompressionOutputRefCon   = avctx;
+    decoder_cb.decompressionOutputRefCon   = avctx->internal->hwaccel_priv_data;
 
     status = VTDecompressionSessionCreate(NULL,                      // allocator
                                           videotoolbox->cm_fmt_desc, // videoFormatDescription
@@ -1179,6 +1179,8 @@ int ff_videotoolbox_common_init(AVCodecContext *avctx)
     AVHWFramesContext *hw_frames;
     int err;
 
+    vtctx->logctx = avctx;
+
     // Old API - do nothing.
     if (avctx->hwaccel_context)
         return 0;
diff --git a/libavcodec/vt_internal.h b/libavcodec/vt_internal.h
index 54a11fd1b5..9502d7c7dc 100644
--- a/libavcodec/vt_internal.h
+++ b/libavcodec/vt_internal.h
@@ -45,6 +45,8 @@ typedef struct VTContext {
     // Current H264 parameters (used to trigger decoder restart on SPS changes).
     uint8_t                     sps[3];
     bool                        reconfig_needed;
+
+    void *logctx;
 } VTContext;
 
 int ff_videotoolbox_alloc_frame(AVCodecContext *avctx, AVFrame *frame);
-- 
2.35.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] lavc/videotoolbox: deprecate write-only output_callback

[Anton Khirnov]

This field has never been used for anything, so stop setting it and
deprecate it.
---
 libavcodec/version_major.h | 1 +
 libavcodec/videotoolbox.c  | 2 --
 libavcodec/videotoolbox.h  | 5 +++++
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/libavcodec/version_major.h b/libavcodec/version_major.h
index 1ec815a7bc..d9386792de 100644
--- a/libavcodec/version_major.h
+++ b/libavcodec/version_major.h
@@ -51,5 +51,6 @@
 #define FF_API_IDCT_NONE           (LIBAVCODEC_VERSION_MAJOR < 60)
 #define FF_API_SVTAV1_OPTS         (LIBAVCODEC_VERSION_MAJOR < 60)
 #define FF_API_AYUV_CODECID        (LIBAVCODEC_VERSION_MAJOR < 60)
+#define FF_API_VT_OUTPUT_CALLBACK  (LIBAVCODEC_VERSION_MAJOR < 60)
 
 #endif /* AVCODEC_VERSION_MAJOR_H */
diff --git a/libavcodec/videotoolbox.c b/libavcodec/videotoolbox.c
index d61d310600..1b1be8ddb4 100644
--- a/libavcodec/videotoolbox.c
+++ b/libavcodec/videotoolbox.c
@@ -1377,8 +1377,6 @@ static AVVideotoolboxContext *av_videotoolbox_alloc_context_with_pix_fmt(enum AV
     AVVideotoolboxContext *ret = av_mallocz(sizeof(*ret));
 
     if (ret) {
-        ret->output_callback = videotoolbox_decoder_callback;
-
         OSType cv_pix_fmt_type = av_map_videotoolbox_format_from_pixfmt2(pix_fmt, full_range);
         if (cv_pix_fmt_type == 0) {
             cv_pix_fmt_type = kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange;
diff --git a/libavcodec/videotoolbox.h b/libavcodec/videotoolbox.h
index af2db0d580..fd8a5b7982 100644
--- a/libavcodec/videotoolbox.h
+++ b/libavcodec/videotoolbox.h
@@ -37,6 +37,8 @@
 
 #include "libavcodec/avcodec.h"
 
+#include "libavutil/attributes.h"
+
 /**
  * This struct holds all the information that needs to be passed
  * between the caller and libavcodec for initializing Videotoolbox decoding.
@@ -50,11 +52,14 @@ typedef struct AVVideotoolboxContext {
      */
     VTDecompressionSessionRef session;
 
+#if FF_API_VT_OUTPUT_CALLBACK
     /**
      * The output callback that must be passed to the session.
      * Set by av_videottoolbox_default_init()
      */
+    attribute_deprecated
     VTDecompressionOutputCallback output_callback;
+#endif
 
     /**
      * CVPixelBuffer Format Type that Videotoolbox will use for decoded frames.
-- 
2.35.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 1/2] lavc/videotoolbox: do not pass AVCodecContext to decoder output callback

[Thilo Borgmann]

Am 15.09.22 um 15:56 schrieb Anton Khirnov:
> The opaque parameter for the callback is set in videotoolbox_start(),
> called when the hwaccel is initialized. When frame threading is used,
> avctx will be the context corresponding to the frame thread currently
> doing the decoding. Using this same codec context in all subsequent
> invocations of the decoder callback (even those triggered by a different
> frame thread) is unsafe, and broken after
> cc867f2c09d2b69cee8a0eccd62aff002cbbfe11, since each frame thread now
> cleans up its hwaccel state after decoding each frame.
> 
> Fix this by passing hwaccel_priv_data as the opaque parameter, which
> exists in a single instance forwarded between all frame threads.
> 
> The only other use of AVCodecContext in the decoder output callback is
> as a logging context. For this purpose, store a logging context in
> hwaccel_priv_data.
> ---
> Initial version of this patch tested by Marvin on IRC.
> Someone please test this final version, as I don't have the HW to do it
> myself.
> ---
>   libavcodec/videotoolbox.c | 10 ++++++----
>   libavcodec/vt_internal.h  |  2 ++
>   2 files changed, 8 insertions(+), 4 deletions(-)

Confirmed, fixes the crash for me as well!

Thx,
Thilo

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