[FFmpeg-devel] [PATCH 5/6] Add suppoort for using libklvanc from within decklink capture module

[Devin Heitmueller]

Hello Derek,

Comments inline.

>> 
>> +    afd[0] = pkt->hdr.payload[0] >> 3;
>> +    if (av_packet_add_side_data(cb_ctx->pkt, AV_PKT_DATA_AFD, afd, 1) < 0)
>> +        av_free(afd);
> 
> Is there a reason we shouldn't fail hard here?

Not really.  The parser will log an error if the callback returns a nonzero value, but beyond the return value isn’t actively used.  That said, no objection to having it return -1 for clarity.
> 
>> +static struct klvanc_callbacks_s callbacks =
>> +{
>> +    .afd               = cb_AFD,
>> +    .eia_708b          = cb_EIA_708B,
>> +    .eia_608           = NULL,
>> +    .scte_104          = NULL,
>> +    .all               = NULL,
>> +    .kl_i64le_counter  = NULL,
>> +};
> 
> I thought C++ didn't have designated initializers? Maybe my C++ is rusty.

Clang didn’t complain, and g++ only complains if you put them in a non-default order (i.e. "non-trivial designated initializers not supported").  The designated initializers improve readability but aren’t required (since already put the items in the default order).  If there’s a portability concern then I can get rid of them.

> 
> Same for other occurrences.

I’m sorry, but what other occurrences?  I don’t see any other instances in this patch where designated initializers are used — or did I misunderstand your comment?
> 
>> +    /* Convert the vanc line from V210 to CrCB422, then vanc parse it */
>> +
>> +    /* We need two kinds of type pointers into the source vbi buffer */
>> +    /* TODO: What the hell is this, two ptrs? */
>> +    const uint32_t *src = (const uint32_t *)buf;
> 
> Is buf guaranteed to be properly aligned for this, or will cause aliasing problems?

Hmm, good question.  The start of each line will always be aligned on a 48 byte boundary as a result of how the decklink module manages it’s buffers, but I agree that this block of code is a bit messy and needs some cleanup (hence the TODO).

I suspect the original routine was cribbed from OBE (with portions derived from ffmpeg’s v210dec), and the assembly version of the same function probably isn’t as forgiving (although libklvanc doesn’t provide an assembly implementation as this routine isn’t particularly performance sensitive).

> 
>> +        vanc_ctx->callback_context = &cb_ctx;
>> +        int ret = klvanc_packet_parse(vanc_ctx, lineNr, decoded_words, sizeof(decoded_words) / (sizeof(unsigned short)));
> 
> Nobody should be typing 'short' in any C/C++ code in 2017..

Will fix.

> 
>> +        if (ret < 0) {
>> +            /* No VANC on this line */
>> +        }
> 
> Huh?

The parser takes in the complete VANC lines, but it’s possible that those lines are blank and don’t contain any actual VANC packets.  That said, you’re right - a negative return should be treated as an error and the comment in question should only occur if the return value is zero (a positive return value is the number of packets parsed).  Will fix.

> 
>> +#if CONFIG_LIBKLVANC
>> +                            klvanc_handle_line(avctx, ctx->vanc_ctx,
>> +                                               buf, videoFrame->GetWidth(), i, &pkt);
>> +#else
> 
> No error checking possible?

Will fix.

> 
>>                 }
>> +
>>                 vanc->Release();
> 
> Stray change.

Will fix.

> 
>> +#if CONFIG_LIBKLVANC
>> +    if (klvanc_context_create(&ctx->vanc_ctx) < 0) {
>> +        av_log(avctx, AV_LOG_ERROR, "Cannot create VANC library context\n");
>> +    } else {
>> +        ctx->vanc_ctx->verbose = 0;
>> +        ctx->vanc_ctx->callbacks = &callbacks;
>> +    }
>> +#endif
> 
> Should fail hard, no?

Will fix.

Thanks for reviewing,

Devin