[vlc-devel] [PATCH 3/4] videotoolbox: use the new push model
Thomas Guillem
thomas at gllm.fr
Thu Jan 9 10:54:46 CET 2020
On Thu, Jan 9, 2020, at 10:27, Steve Lhomme wrote:
>
> 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.
Already done by p_dec->fmt_out.video = p_dec->fmt_in.video;
>
> > + }
> > +
> > + 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.
This struct is moved to the video context. I don't see how I can do a commit that use a video context but not the decoder device.
>
> > 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.
This callback is used by videotoolbox.m
> 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
> >
> _______________________________________________
> 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