Hi Aaron, > On Dec 30, 2017, at 2:34 AM, Aaron Levinson <alevinsn_...@levland.net> wrote: > > Patch title: "suppoort" -> "support", also "decklink" -> “DeckLink"
Ok. >> ff_decklink_cleanup(avctx); >> avpacket_queue_end(&ctx->queue); >> + klvanc_context_destroy(ctx->vanc_ctx); > > Shouldn't this last line be wrapped in #if CONFIG_LIBKLVANC? I suggest > building this on Linux with and without libklvanc, and I also suggest > building it (and ideally testing it) on Windows without libklvanc as well. > DeckLink is also supported on OS/X. Yup, that was a mistake I made preparing the latest patch. The #ifdef guard got left out. I do typically test-compile on both Linux and OS X both with and without the library present, but that process got skipped this time around (which was an error on my part). I have confirmed though that was the only build error when the library isn’t present. > >> av_freep(&cctx->ctx); >> @@ -1193,6 +1315,17 @@ av_cold int ff_decklink_read_header(AVFormatContext >> *avctx) >> avpacket_queue_init (avctx, &ctx->queue); >> +#if CONFIG_LIBKLVANC >> + if (klvanc_context_create(&ctx->vanc_ctx) < 0) { >> + av_log(avctx, AV_LOG_ERROR, "Cannot create VANC library context\n"); >> + ret = AVERROR(ENOMEM); > > Perhaps appropriate to use the return value of klvanc_context_create() as > input to AVERROR(). I think it’s usually bad practice to blindly return what a third party library returns. Usually the error should be converted into some ffmpeg specific error code which the caller might actually know what to do with. In this case I just return ENOMEM since that’s the only actual failure case possible in the call. Devin _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel