[vlc-devel] [PATCH 3/4] videotoolbox: use the new push model
Steve Lhomme
robux4 at ycbcr.xyz
Thu Jan 9 10:27:46 CET 2020
On 2020-01-08 16:52, Thomas Guillem wrote:
> Deprecate the "videotoolbox" bool variable. This module should be
> enabled/disabled by the "dec-dev" variable like any other hw modules.
>
> The vt decoder device is just used to enable/disable hw decoding on Apple and
> act like other hw module. Indeed, VideoToolBox doesn't need any context from
> the window to initialize.
> ---
> modules/codec/videotoolbox.m | 157 ++++++++++++++++++------------
> modules/codec/vt_utils.c | 32 +++---
> modules/codec/vt_utils.h | 7 +-
> modules/video_chroma/cvpx.c | 4 +-
> modules/video_filter/ci_filters.m | 2 +-
> 5 files changed, 113 insertions(+), 89 deletions(-)
>
> diff --git a/modules/codec/videotoolbox.m b/modules/codec/videotoolbox.m
> index 83c343fc4bb..feff262b570 100644
> --- a/modules/codec/videotoolbox.m
> +++ b/modules/codec/videotoolbox.m
> @@ -71,6 +71,7 @@ const CFStringRef kVTVideoDecoderSpecification_RequireHardwareAcceleratedVideoDe
>
> static int OpenDecoder(vlc_object_t *);
> static void CloseDecoder(vlc_object_t *);
> +static int OpenDecDevice(vlc_decoder_device *device, vout_window_t *window);
>
> #define VT_ENABLE_TEXT N_("Enable hardware acceleration")
> #define VT_REQUIRE_HW_DEC N_("Use Hardware decoders only")
> @@ -86,9 +87,11 @@ set_capability("video decoder",800)
> set_callbacks(OpenDecoder, CloseDecoder)
>
> add_obsolete_bool("videotoolbox-temporal-deinterlacing")
> -add_bool("videotoolbox", true, VT_ENABLE_TEXT, NULL, false)
> +add_obsolete_bool("videotoolbox")
> add_bool("videotoolbox-hw-decoder-only", true, VT_REQUIRE_HW_DEC, VT_REQUIRE_HW_DEC, false)
> add_string("videotoolbox-cvpx-chroma", "", VT_FORCE_CVPX_CHROMA, VT_FORCE_CVPX_CHROMA_LONG, true);
> +add_submodule ()
> + set_callback_dec_device(OpenDecDevice, 1)
> vlc_module_end()
>
> #pragma mark - local prototypes
> @@ -184,12 +187,12 @@ typedef struct decoder_sys_t
> bool b_drop_blocks;
> date_t pts;
>
> + vlc_video_context *vctx;
> struct pic_holder *pic_holder;
> } decoder_sys_t;
>
> struct pic_holder
> {
> - bool closed;
> vlc_mutex_t lock;
> vlc_cond_t wait;
> uint8_t nb_field_out;
> @@ -1315,20 +1318,68 @@ static void StopVideoToolbox(decoder_t *p_dec, bool closing)
>
> #pragma mark - module open and close
>
> +static void pic_holder_Destroy(void *priv)
> +{
> + struct pic_holder *pic_holder = priv;
>
> -static int OpenDecoder(vlc_object_t *p_this)
> + vlc_mutex_destroy(&pic_holder->lock);
> + vlc_cond_destroy(&pic_holder->wait);
> +}
> +
> +static int
> +CreateVideoContext(decoder_t *p_dec)
> {
> - decoder_t *p_dec = (decoder_t *)p_this;
> + decoder_sys_t *p_sys = p_dec->p_sys;
>
> - if (!var_InheritBool(p_dec, "videotoolbox"))
> - return VLC_EGENERIC;
> + p_dec->fmt_out.video = p_dec->fmt_in.video;
> + p_dec->fmt_out.video.p_palette = NULL;
>
> -#if TARGET_OS_IPHONE
> - if (unlikely([[UIDevice currentDevice].systemVersion floatValue] < 8.0)) {
> - msg_Warn(p_dec, "decoder skipped as OS is too old");
> + /* Most likely used chroma but can be reconfigured in the future */
> + p_dec->fmt_out.i_codec = VLC_CODEC_CVPX_NV12;
> + p_dec->fmt_out.video.i_chroma = p_dec->fmt_out.i_codec;
> +
> + if (!p_dec->fmt_out.video.i_sar_num || !p_dec->fmt_out.video.i_sar_den)
> + {
> + p_dec->fmt_out.video.i_sar_num = 1;
> + p_dec->fmt_out.video.i_sar_den = 1;
You should use the SAR from fmt_in if it's set.
> + }
> +
> + vlc_decoder_device *dec_dev = decoder_GetDecoderDevice(p_dec);
We might put the SAR and empty width/height handling in
decoder_GetDecoderDevice() since all decoders need to do it anyway.
> + if (!dec_dev || dec_dev->type != VLC_DECODER_DEVICE_VIDEOTOOLBOX)
> + {
> + msg_Warn(p_dec, "Could not find an VIDEOTOOLBOX decoder device: %p", dec_dev);
> return VLC_EGENERIC;
> }
> -#endif
> +
> + static const struct vlc_video_context_operations ops =
> + {
> + .destroy = pic_holder_Destroy,
> + };
Usually I prefer unnamed initializers for the structure so we don't miss
new callbacks when they are added (especially with the right compiler
flags). I have 2 additions coming for vlc_video_context_operations. It
would be nice if the CI could tell us when new things are not handled in
the code.
> + p_sys->vctx =
> + vlc_video_context_Create(dec_dev, VLC_VIDEO_CONTEXT_VIDEOTOOLBOX,
> + sizeof(struct pic_holder), &ops);
> + vlc_decoder_device_Release(dec_dev);
> +
> + if (!p_sys->vctx)
> + return VLC_ENOMEM;
> +
> + p_sys->pic_holder =
> + vlc_video_context_GetPrivate(p_sys->vctx,
> + VLC_VIDEO_CONTEXT_VIDEOTOOLBOX);
> + assert(p_sys->pic_holder);
> +
> + vlc_mutex_init(&p_sys->pic_holder->lock);
> + vlc_cond_init(&p_sys->pic_holder->wait);
> + p_sys->pic_holder->nb_field_out = 0;
> + p_sys->pic_holder->field_reorder_max = p_sys->i_pic_reorder_max * 2;
> +
> + return VLC_SUCCESS;
> +}
> +
> +static int OpenDecoder(vlc_object_t *p_this)
> +{
> + int i_ret;
> + decoder_t *p_dec = (decoder_t *)p_this;
>
> /* Fail if this module already failed to decode this ES */
> if (var_Type(p_dec, "videotoolbox-failed") != 0)
> @@ -1371,20 +1422,13 @@ static int OpenDecoder(vlc_object_t *p_this)
> free(cvpx_chroma);
> }
>
> - p_sys->pic_holder = malloc(sizeof(struct pic_holder));
> - if (!p_sys->pic_holder)
> + i_ret = CreateVideoContext(p_dec);
> + if (i_ret != VLC_SUCCESS)
> {
> free(p_sys);
> - return VLC_ENOMEM;
> + return i_ret;
> }
>
> - vlc_mutex_init(&p_sys->pic_holder->lock);
> - vlc_cond_init(&p_sys->pic_holder->wait);
> - p_sys->pic_holder->nb_field_out = 0;
> - p_sys->pic_holder->closed = false;
> - p_sys->pic_holder->field_reorder_max = p_sys->i_pic_reorder_max * 2;
> - p_sys->b_vt_need_keyframe = false;
> -
I think the code moving should be done in a separate (prior) patch so
it's clearer what you add/remove to push the video context.
> vlc_mutex_init(&p_sys->lock);
>
> p_dec->pf_decode = DecodeBlock;
> @@ -1440,7 +1484,7 @@ static int OpenDecoder(vlc_object_t *p_this)
> return VLC_EGENERIC;
> }
>
> - int i_ret = StartVideoToolbox(p_dec);
> + i_ret = StartVideoToolbox(p_dec);
> if (i_ret == VLC_SUCCESS) {
> PtsInit(p_dec);
> msg_Info(p_dec, "Using Video Toolbox to decode '%4.4s'",
> @@ -1452,13 +1496,6 @@ static int OpenDecoder(vlc_object_t *p_this)
> return i_ret;
> }
>
> -static void pic_holder_clean(struct pic_holder *pic_holder)
> -{
> - vlc_mutex_destroy(&pic_holder->lock);
> - vlc_cond_destroy(&pic_holder->wait);
> - free(pic_holder);
> -}
> -
> static void CloseDecoder(vlc_object_t *p_this)
> {
> decoder_t *p_dec = (decoder_t *)p_this;
> @@ -1471,17 +1508,8 @@ static void CloseDecoder(vlc_object_t *p_this)
>
> vlc_mutex_destroy(&p_sys->lock);
>
> - vlc_mutex_lock(&p_sys->pic_holder->lock);
> - if (p_sys->pic_holder->nb_field_out == 0)
> - {
> - vlc_mutex_unlock(&p_sys->pic_holder->lock);
> - pic_holder_clean(p_sys->pic_holder);
> - }
> - else
> - {
> - p_sys->pic_holder->closed = true;
> - vlc_mutex_unlock(&p_sys->pic_holder->lock);
> - }
> + vlc_video_context_Release(p_sys->vctx);
> +
> free(p_sys);
> }
>
> @@ -1596,20 +1624,12 @@ static int ConfigureVout(decoder_t *p_dec)
> decoder_sys_t *p_sys = p_dec->p_sys;
>
> /* return our proper VLC internal state */
> - p_dec->fmt_out.video = p_dec->fmt_in.video;
> - p_dec->fmt_out.video.p_palette = NULL;
> p_dec->fmt_out.i_codec = 0;
>
> if(p_sys->pf_configure_vout &&
> !p_sys->pf_configure_vout(p_dec))
> return VLC_EGENERIC;
>
> - if (!p_dec->fmt_out.video.i_sar_num || !p_dec->fmt_out.video.i_sar_den)
> - {
> - p_dec->fmt_out.video.i_sar_num = 1;
> - p_dec->fmt_out.video.i_sar_den = 1;
> - }
> -
> if (!p_dec->fmt_out.video.i_visible_width || !p_dec->fmt_out.video.i_visible_height)
> {
> p_dec->fmt_out.video.i_visible_width = p_dec->fmt_out.video.i_width;
> @@ -2069,7 +2089,7 @@ static int UpdateVideoFormat(decoder_t *p_dec, CVPixelBufferRef imageBuffer)
> return -1;
> }
> p_dec->fmt_out.video.i_chroma = p_dec->fmt_out.i_codec;
> - if (decoder_UpdateVideoFormat(p_dec) != 0)
> + if (decoder_UpdateVideoOutput(p_dec, p_sys->vctx) != 0)
> {
> p_sys->vtsession_status = VTSESSION_STATUS_ABORT;
> return -1;
> @@ -2078,23 +2098,16 @@ static int UpdateVideoFormat(decoder_t *p_dec, CVPixelBufferRef imageBuffer)
> }
>
> static void
> -pic_holder_on_cvpx_released(CVPixelBufferRef cvpx, void *data, unsigned nb_fields)
> +video_context_OnPicReleased(vlc_video_context *vctx, unsigned nb_fields)
> {
> - struct pic_holder *pic_holder = data;
> + struct pic_holder *pic_holder =
> + vlc_video_context_GetPrivate(vctx, VLC_VIDEO_CONTEXT_VIDEOTOOLBOX);
>
> vlc_mutex_lock(&pic_holder->lock);
> assert((int) pic_holder->nb_field_out - nb_fields >= 0);
> pic_holder->nb_field_out -= nb_fields;
> - if (pic_holder->nb_field_out == 0 && pic_holder->closed)
> - {
> - vlc_mutex_unlock(&pic_holder->lock);
> - pic_holder_clean(pic_holder);
> - }
> - else
> - {
> - vlc_cond_broadcast(&pic_holder->wait);
> - vlc_mutex_unlock(&pic_holder->lock);
> - }
> + vlc_cond_signal(&pic_holder->wait);
> + vlc_mutex_unlock(&pic_holder->lock);
> }
>
> static void
> @@ -2216,8 +2229,8 @@ static void DecoderCallback(void *decompressionOutputRefCon,
> p_pic->b_top_field_first = p_info->b_top_field_first;
> }
>
> - if (cvpxpic_attach_with_cb(p_pic, imageBuffer, pic_holder_on_cvpx_released,
> - p_sys->pic_holder) != VLC_SUCCESS)
> + if (cvpxpic_attach(p_pic, imageBuffer, p_sys->vctx,
> + video_context_OnPicReleased) != VLC_SUCCESS)
> {
> vlc_mutex_lock(&p_sys->lock);
> goto end;
> @@ -2255,3 +2268,23 @@ end:
> vlc_mutex_unlock(&p_sys->lock);
> return;
> }
> +
> +static int
> +OpenDecDevice(vlc_decoder_device *device, vout_window_t *window)
> +{
> +#if TARGET_OS_IPHONE
> + if (unlikely([[UIDevice currentDevice].systemVersion floatValue] < 8.0)) {
> + msg_Warn(device, "decoder skipped as OS is too old");
> + return VLC_EGENERIC;
> + }
> +#endif
> +
> + static const struct vlc_decoder_device_operations ops =
> + {
> + .close = NULL,
> + };
> + device->ops = &ops;
> + device->type = VLC_DECODER_DEVICE_VIDEOTOOLBOX;
> +
> + return VLC_SUCCESS;
> +}
> diff --git a/modules/codec/vt_utils.c b/modules/codec/vt_utils.c
> index cac13891993..c23ff75b08f 100644
> --- a/modules/codec/vt_utils.c
> +++ b/modules/codec/vt_utils.c
> @@ -49,8 +49,7 @@ struct cvpxpic_ctx
> unsigned nb_fields;
>
> vlc_atomic_rc_t rc;
> - void (*on_released_cb)(CVPixelBufferRef, void *, unsigned);
> - void *on_released_data;
> + void (*on_released_cb)(vlc_video_context *vctx, unsigned);
> };
>
> static void
> @@ -62,7 +61,7 @@ cvpxpic_destroy_cb(picture_context_t *opaque)
> {
> CFRelease(ctx->cvpx);
> if (ctx->on_released_cb)
> - ctx->on_released_cb(ctx->cvpx, ctx->on_released_data, ctx->nb_fields);
> + ctx->on_released_cb(opaque->vctx, ctx->nb_fields);
> free(opaque);
> }
> }
> @@ -72,14 +71,16 @@ cvpxpic_copy_cb(struct picture_context_t *opaque)
> {
> struct cvpxpic_ctx *ctx = (struct cvpxpic_ctx *)opaque;
> vlc_atomic_rc_inc(&ctx->rc);
> + if (opaque->vctx)
> + vlc_video_context_Hold(opaque->vctx);
> return opaque;
> }
>
> static int
> cvpxpic_attach_common(picture_t *p_pic, CVPixelBufferRef cvpx,
> void (*pf_destroy)(picture_context_t *),
> - void (*on_released_cb)(CVPixelBufferRef, void *, unsigned),
> - void *on_released_data)
> + vlc_video_context *vctx,
> + void (*on_released_cb)(vlc_video_context *vctx, unsigned))
> {
> struct cvpxpic_ctx *ctx = malloc(sizeof(struct cvpxpic_ctx));
> if (ctx == NULL)
> @@ -88,15 +89,15 @@ cvpxpic_attach_common(picture_t *p_pic, CVPixelBufferRef cvpx,
> return VLC_ENOMEM;
> }
> ctx->s = (picture_context_t) {
> - pf_destroy, cvpxpic_copy_cb,
> - NULL // no video context for now
> + pf_destroy, cvpxpic_copy_cb, vctx,
> };
> ctx->cvpx = CVPixelBufferRetain(cvpx);
> ctx->nb_fields = p_pic->i_nb_fields;
> vlc_atomic_rc_init(&ctx->rc);
>
> + if (vctx)
> + vlc_video_context_Hold(vctx);
> ctx->on_released_cb = on_released_cb;
Looking at the existing code, it seems this callback is always NULL.
You may do the whole cvpxpic_attach() cleaning in a separate (later) patch.
> - ctx->on_released_data = on_released_data;
>
> p_pic->context = &ctx->s;
>
> @@ -104,17 +105,10 @@ cvpxpic_attach_common(picture_t *p_pic, CVPixelBufferRef cvpx,
> }
>
> int
> -cvpxpic_attach(picture_t *p_pic, CVPixelBufferRef cvpx)
> +cvpxpic_attach(picture_t *p_pic, CVPixelBufferRef cvpx, vlc_video_context *vctx,
> + void (*on_released_cb)(vlc_video_context *vctx, unsigned))
> {
> - return cvpxpic_attach_common(p_pic, cvpx, cvpxpic_destroy_cb, NULL, NULL);
> -}
> -
> -int cvpxpic_attach_with_cb(picture_t *p_pic, CVPixelBufferRef cvpx,
> - void (*on_released_cb)(CVPixelBufferRef, void *, unsigned),
> - void *on_released_data)
> -{
> - return cvpxpic_attach_common(p_pic, cvpx, cvpxpic_destroy_cb, on_released_cb,
> - on_released_data);
> + return cvpxpic_attach_common(p_pic, cvpx, cvpxpic_destroy_cb, vctx, on_released_cb);
> }
>
> CVPixelBufferRef
> @@ -219,7 +213,7 @@ cvpxpic_unmap(picture_t *mapped_pic)
> return NULL;
> }
>
> - cvpxpic_attach(hw_pic, cvpxpic_get_ref(mapped_pic));
> + cvpxpic_attach(hw_pic, cvpxpic_get_ref(mapped_pic), NULL, NULL);
> picture_CopyProperties(hw_pic, mapped_pic);
> picture_Release(mapped_pic);
> return hw_pic;
> diff --git a/modules/codec/vt_utils.h b/modules/codec/vt_utils.h
> index 24cd4aa8983..e981237ed6c 100644
> --- a/modules/codec/vt_utils.h
> +++ b/modules/codec/vt_utils.h
> @@ -34,11 +34,8 @@ void cfdict_set_int32(CFMutableDictionaryRef dict, CFStringRef key, int value);
> * The cvpx ref will be released when the picture is released
> * @return VLC_SUCCESS or VLC_ENOMEM
> */
> -int cvpxpic_attach(picture_t *p_pic, CVPixelBufferRef cvpx);
> -
> -int cvpxpic_attach_with_cb(picture_t *p_pic, CVPixelBufferRef cvpx,
> - void (*on_released_cb)(CVPixelBufferRef, void *, unsigned nb_fields),
> - void *on_released_data);
> +int cvpxpic_attach(picture_t *p_pic, CVPixelBufferRef cvpx, vlc_video_context *vctx,
> + void (*on_released_cb)(vlc_video_context *vctx, unsigned));
>
> /*
> * Get the cvpx buffer attached to a picture
> diff --git a/modules/video_chroma/cvpx.c b/modules/video_chroma/cvpx.c
> index 7a83b982d29..a1e6e2c8974 100644
> --- a/modules/video_chroma/cvpx.c
> +++ b/modules/video_chroma/cvpx.c
> @@ -214,7 +214,7 @@ static picture_t *SW_TO_CVPX_Filter(filter_t *p_filter, picture_t *src)
> Copy(p_filter, mapped_dst, src, __MIN(height, src->format.i_visible_height));
>
> /* Attach the CVPX to a new opaque picture */
> - cvpxpic_attach(dst, cvpxpic_get_ref(mapped_dst));
> + cvpxpic_attach(dst, cvpxpic_get_ref(mapped_dst), NULL, NULL);
>
> /* Unlock and unmap the dst picture */
> picture_Release(mapped_dst);
> @@ -365,7 +365,7 @@ Filter(filter_t *filter, picture_t *src)
> return NULL;
> }
>
> - cvpxpic_attach(dst, dst_cvpx);
> + cvpxpic_attach(dst, dst_cvpx, NULL, NULL);
>
> picture_CopyProperties(dst, src);
> picture_Release(src);
> diff --git a/modules/video_filter/ci_filters.m b/modules/video_filter/ci_filters.m
> index 236fc3452ae..2bf1ff69744 100644
> --- a/modules/video_filter/ci_filters.m
> +++ b/modules/video_filter/ci_filters.m
> @@ -369,7 +369,7 @@ Filter(filter_t *filter, picture_t *src)
> if (!cvpx)
> goto error;
>
> - if (cvpxpic_attach(dst, cvpx))
> + if (cvpxpic_attach(dst, cvpx, NULL, NULL))
> {
> CFRelease(cvpx);
> goto error;
> --
> 2.20.1
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>
More information about the vlc-devel
mailing list