[FFmpeg-devel] [PATCH 06/11] decklink: Add support for using libklvanc from within capture module

[Devin Heitmueller]

Hello Marton,

Thank you for the feedback.  Comments inline.

>> +        vanc_ctx->callback_context = &cb_ctx;
>> +        int ret = klvanc_packet_parse(vanc_ctx, lineNr, decoded_words, sizeof(decoded_words) / (sizeof(uint16_t)));
> 
> A parity error also causes a negative return value? Or parity errors only makes the library ignore the packet, and error values are meaning ENOMEM?

A parity error will cause the CRC to fail on the given packet.  Packets that fail the CRC check won’t invoke the callback (there is a special variable to override this, as well as a counter that can be inspected for the number of checksum errors encountered).  We don’t make any effort to distinguish between parity errors and checksum failures, since checksum failure already detects parity errors.

> 
> What happens if multiple packets are in a single line and one packet has parity errors, but the other does not. We should be able to use the result from the packet without errors. So does this function returns success if any of the callbacks succeeded?

The klvanc_packet_parse() can invoke the callback multiple times in a single call, to handle cases that multiple VANC packets are found on the same line.  The return value is the number of packets found, regardless of whether they failed the checksum validation or have a callback registered for their VANC type.

>> 
>> +#if CONFIG_LIBKLVANC
>> +                            int ret = klvanc_handle_line(avctx, ctx->vanc_ctx,
>> +                                                         buf, videoFrame->GetWidth(), i, &pkt);
>> +                            if (ret != 0)
>> +                                av_log(avctx, AV_LOG_ERROR, "Error parsing VANC for line %d\n", i);
>> +#else
> 
> For now, you should allow both klvanc and ffmpeg parsing of the VANC, so the #else does not seem right.

I’m not against this, in particular given libklvanc doesn’t currently support OP47 (it’s on my todo list).  However, how do you propose we handle cases where functionality overlaps between the two?  In particular, we don’t want both klvanc and the internal routine creating EIA-708 side_data.

I’m about to leave the country for a week.  Please don’t interpret my failure to respond to email(s) as disinterest.  I still very much want to get this functionality merged (and I’ve got about a dozen patches queued up behind these for various features/functionality).

Devin