[FFmpeg-devel] [PATCH 1/8] libavdevice/decklink: Add support for EIA-708 output over SDI

[Devin Heitmueller]

Hi Aaron,

Comments inline:
> 
> Most of the changes in this set of patches appear to only be relevant in the case that libklvanc is enabled.  Nevertheless, a number of the changes are not wrapped in #if CONFIG_LIBKLVANC, which means that they will be exercised even if libklvanc is not available.  Most of these changes are benign, but the change to use bmdVideoOutputVANC by default may not be benign.  I'm unclear on any side effects of specifying bmdVideoOutputVANC for the output flags instead of bmdVideoOutputFlagDefault.  If this change, for example, exposes a bug in Blackmagic on Windows, where libklvanc is almost certainly not currently available, then this code, which has previously been working on Windows, will suddenly stop working, and for no good reason other than the code is a bit cleaner without #if CONFIG_LIBKLVANC. I'm not saying that such a Blackmagic bug exists (although it wouldn't surprise me if such a bug does exist), but if the user doesn't have libklvanc available when building, as will be the case for almost all users out there, it seems preferable not to expose them to potential issues.  

I’ll do a check and see what is still being used if CONFIG_LIBKLVANC isn’t enabled.  Indeed, the focus has been on Linux and OS X, and I haven’t had a chance to do any work under Windows (and frankly I don’t really have any plans to at this point).

I did make an effort to ensure the code wasn’t being run if the library wasn’t present, but I obviously didn’t give it enough attention.

> I think this is also a good argument for having the libklvanc functionality in FFmpeg proper.

There are numerous arguments for why it’s done as a separate library.  Feel free to read the responses to the other patch submissions.  I’m happy to discuss though in greater detail if desired.

> 
> I'd also argue that, even if CONFIG_LIBKLVANC is available, it should only specify the VANC output flag in the case that VANC will actually be used.  Just because VANC is supported doesn't mean it should be used.

This cannot be determined when enabling the video output in cases such as with AFD and EIA-708, where the data is announced as side data to the AVFrame (i.e. that info isn’t available to us at the write_header phase).  Based on testing with a number of versions of the SDK and platforms, enabling it appears to be quite safe.  The cases where you get into trouble tend to be when generating the actual VANC lines (e.g. overflowing the buffer, etc).
> 
> After this point, assuming that SetAncillaryData() is implemented properly, the reference count for vanc should be 2.  This code owns the "first" reference, and the frame object owns the "second" reference. This code should call Release() on vanc after this point, regardless of the return value of SetAncillaryData().

Ok.  I’ll have to look closer at the reference counting.  I thought it was just cribbed from the decklink samples, but I’ll have to better understand what is going on here.

>> @@ -393,6 +532,9 @@ av_cold int ff_decklink_write_header(AVFormatContext *avctx)
>>      ctx->list_formats = cctx->list_formats;
>>      ctx->preroll      = cctx->preroll;
>>      cctx->ctx = ctx;
>> +#if CONFIG_LIBKLVANC
>> +    klvanc_context_create(&ctx->vanc_ctx);
>> +#endif
> 
> Based on my perusal of the implementation of klvanc_context_create(), the function may fail, yet any failure conditions are not handled here. Admittedly, the only failure case I see in an out of memory error.

Yup, missing an error check.  Will fix.

Devin