Hi Aaron,
> On Dec 30, 2017, at 2:34 AM, Aaron Levinson <[email protected]> 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
[email protected]
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
