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

Make use of libklvanc from within the decklink capture module,
initially for EIA-708 and AFD.  Support for other VANC types will
come in subsequent patches.

Signed-off-by: Devin Heitmueller <dheitmueller@ltnglobal.com>
---
 libavdevice/decklink_dec.cpp | 135 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 135 insertions(+)

On 11/16/2017 6:34 PM, Devin Heitmueller wrote:
> Make use of libklvanc from within the decklink capture module,
> initially for EIA-708 and AFD.  Support for other VANC types will
> come in subsequent patches.
> 
> Signed-off-by: Devin Heitmueller <dheitmueller@ltnglobal.com>
> ---
>  libavdevice/decklink_dec.cpp | 135 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 135 insertions(+)
> 
> diff --git a/libavdevice/decklink_dec.cpp b/libavdevice/decklink_dec.cpp
> index 11b7e60..bea9213 100644
> --- a/libavdevice/decklink_dec.cpp
> +++ b/libavdevice/decklink_dec.cpp
> @@ -3,6 +3,7 @@
>   * Copyright (c) 2013-2014 Luca Barbato, Deti Fliegl
>   * Copyright (c) 2014 Rafaël Carré
>   * Copyright (c) 2017 Akamai Technologies, Inc.
> + * Copyright (c) 2017 LTN Global Communications, Inc.
>   *
>   * This file is part of FFmpeg.
>   *
> @@ -669,10 +670,128 @@ error:
>      return ret;
>  }
>  
> +    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?

> +    if (av_packet_add_side_data(cb_ctx->pkt, AV_PKT_DATA_A53_CC, cc, pkt->ccdata.cc_count * 3) < 0)
> +        av_free(cc);

Ditto here.

> +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.

Same for other occurrences.

> +    /* 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?

> +        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..

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

Huh?

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

No error checking possible?

>                  }
> +
>                  vanc->Release();

Stray change.

> +#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?

- Derek

On 11/16/2017 3:34 PM, Devin Heitmueller wrote:
> Make use of libklvanc from within the decklink capture module,
> initially for EIA-708 and AFD.  Support for other VANC types will
> come in subsequent patches.
> 
> Signed-off-by: Devin Heitmueller <dheitmueller@ltnglobal.com>
> ---
>  libavdevice/decklink_dec.cpp | 135 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 135 insertions(+)
> 
> diff --git a/libavdevice/decklink_dec.cpp b/libavdevice/decklink_dec.cpp
> index 11b7e60..bea9213 100644
> --- a/libavdevice/decklink_dec.cpp
> +++ b/libavdevice/decklink_dec.cpp
> @@ -3,6 +3,7 @@
>   * Copyright (c) 2013-2014 Luca Barbato, Deti Fliegl
>   * Copyright (c) 2014 Rafaël Carré
>   * Copyright (c) 2017 Akamai Technologies, Inc.
> + * Copyright (c) 2017 LTN Global Communications, Inc.
>   *
>   * This file is part of FFmpeg.
>   *
> @@ -669,10 +670,128 @@ error:
>      return ret;
>  }
>  
> +#if CONFIG_LIBKLVANC
> +/* VANC Callbacks */
> +struct vanc_cb_ctx {
> +    AVFormatContext *avctx;
> +    AVPacket *pkt;
> +};
> +static int cb_AFD(void *callback_context, struct klvanc_context_s *ctx,
> +                  struct klvanc_packet_afd_s *pkt)
> +{
> +    struct vanc_cb_ctx *cb_ctx = (struct vanc_cb_ctx *)callback_context;
> +    uint8_t *afd;
> +
> +    afd = (uint8_t *)av_malloc(1);
> +    if (afd == NULL)
> +        return AVERROR(ENOMEM);
> +
> +    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);

For this, av_packet_new_side_data() seems more adequate than av_malloc()
+ av_packet_add_side_data().

Also, you should propagate the errors av_packet_{add,new}_side_data return.

> +
> +    return 0;
> +}
> +
> +static int cb_EIA_708B(void *callback_context, struct klvanc_context_s *ctx,
> +                       struct klvanc_packet_eia_708b_s *pkt)
> +{
> +    struct vanc_cb_ctx *cb_ctx = (struct vanc_cb_ctx *)callback_context;
> +    decklink_cctx *cctx = (struct decklink_cctx *)cb_ctx->avctx->priv_data;
> +    struct decklink_ctx *decklink_ctx = (struct decklink_ctx *)cctx->ctx;
> +
> +    uint16_t expected_cdp;
> +    uint8_t *cc;
> +
> +    if (!pkt->checksum_valid)
> +        return 0;
> +
> +    if (!pkt->header.ccdata_present)
> +        return 0;
> +
> +    expected_cdp = decklink_ctx->cdp_sequence_num + 1;
> +    decklink_ctx->cdp_sequence_num = pkt->header.cdp_hdr_sequence_cntr;
> +    if (pkt->header.cdp_hdr_sequence_cntr != expected_cdp) {
> +        av_log(cb_ctx->avctx, AV_LOG_DEBUG,
> +               "CDP counter inconsistent.  Received=0x%04x Expected=%04x\n",
> +               pkt->header.cdp_hdr_sequence_cntr, expected_cdp);
> +        return 0;
> +    }
> +
> +    cc = (uint8_t *)av_malloc(pkt->ccdata.cc_count * 3);
> +    if (cc == NULL)
> +        return AVERROR(ENOMEM);
> +
> +    for (int i = 0; i < pkt->ccdata.cc_count; i++) {
> +        cc[3*i] = 0xf8 | (pkt->ccdata.cc[i].cc_valid ? 0x04 : 0x00) |
> +                  (pkt->ccdata.cc[i].cc_type & 0x03);
> +        cc[3*i+1] = pkt->ccdata.cc[i].cc_data[0];
> +        cc[3*i+2] = pkt->ccdata.cc[i].cc_data[1];
> +    }
> +
> +    if (av_packet_add_side_data(cb_ctx->pkt, AV_PKT_DATA_A53_CC, cc, pkt->ccdata.cc_count * 3) < 0)
> +        av_free(cc);

Same as above.

> +
> +    return 0;
> +}
> +
> +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,
> +};
> +/* End: VANC Callbacks */
> +
> +/* Take one line of V210 from VANC, colorspace convert and feed it to the
> + * VANC parser. We'll expect our VANC message callbacks to happen on this
> + * same calling thread.
> + */
> +static void klvanc_handle_line(AVFormatContext *avctx, struct klvanc_context_s *vanc_ctx,
> +                               unsigned char *buf, unsigned int uiWidth, unsigned int lineNr,
> +                               AVPacket *pkt)
> +{
> +    /* 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;
> +
> +    /* Convert Blackmagic pixel format to nv20.
> +     * src pointer gets mangled during conversion, hence we need its own
> +     * ptr instead of passing vbiBufferPtr.
> +     * decoded_words should be atleast 2 * uiWidth.
> +     */
> +    uint16_t decoded_words[16384];
> +
> +    /* On output each pixel will be decomposed into three 16-bit words (one for Y, U, V) */
> +    memset(&decoded_words[0], 0, sizeof(decoded_words));
> +    uint16_t *p_anc = decoded_words;
> +    if (klvanc_v210_line_to_nv20_c(src, p_anc, sizeof(decoded_words), (uiWidth / 6) * 6) < 0)
> +        return;
> +
> +    if (vanc_ctx) {
> +        struct vanc_cb_ctx cb_ctx = {
> +            .avctx = avctx,
> +            .pkt = pkt
> +        };
> +        vanc_ctx->callback_context = &cb_ctx;
> +        int ret = klvanc_packet_parse(vanc_ctx, lineNr, decoded_words, sizeof(decoded_words) / (sizeof(unsigned short)));
> +        if (ret < 0) {
> +            /* No VANC on this line */
> +        }
> +    }
> +}
> +#endif
> +
>  HRESULT decklink_input_callback::VideoInputFrameArrived(
>      IDeckLinkVideoInputFrame *videoFrame, IDeckLinkAudioInputPacket *audioFrame)
>  {
>      decklink_cctx *cctx = (struct decklink_cctx *)avctx->priv_data;
> +    struct decklink_ctx *ctx = (struct decklink_ctx *)cctx->ctx;
>      void *frameBytes;
>      void *audioFrameBytes;
>      BMDTimeValue frameTime;
> @@ -774,15 +893,21 @@ HRESULT decklink_input_callback::VideoInputFrameArrived(
>                      for (i = vanc_line_numbers[idx].vanc_start; i <= vanc_line_numbers[idx].vanc_end; i++) {
>                          uint8_t *buf;
>                          if (vanc->GetBufferForVerticalBlankingLine(i, (void**)&buf) == S_OK) {
> +#if CONFIG_LIBKLVANC
> +                            klvanc_handle_line(avctx, ctx->vanc_ctx,
> +                                               buf, videoFrame->GetWidth(), i, &pkt);
> +#else
>                              uint16_t luma_vanc[MAX_WIDTH_VANC];
>                              extract_luma_from_v210(luma_vanc, buf, videoFrame->GetWidth());
>                              txt_buf = get_metadata(avctx, luma_vanc, videoFrame->GetWidth(),
>                                                     txt_buf, sizeof(txt_buf0) - (txt_buf - txt_buf0), &pkt);
> +#endif
>                          }
>                          if (i == vanc_line_numbers[idx].field0_vanc_end)
>                              i = vanc_line_numbers[idx].field1_vanc_start - 1;
>                      }
>                  }
> +
>                  vanc->Release();
>                  if (txt_buf - txt_buf0 > 1) {
>                      int stuffing_units = (4 - ((45 + txt_buf - txt_buf0) / 46) % 4) % 4;
> @@ -900,6 +1025,7 @@ av_cold int ff_decklink_read_close(AVFormatContext *avctx)
>  
>      ff_decklink_cleanup(avctx);
>      avpacket_queue_end(&ctx->queue);
> +    klvanc_context_destroy(ctx->vanc_ctx);
>  
>      av_freep(&cctx->ctx);
>  
> @@ -1128,6 +1254,15 @@ 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");
> +    } else {
> +        ctx->vanc_ctx->verbose = 0;
> +        ctx->vanc_ctx->callbacks = &callbacks;
> +    }
> +#endif
> +
>      if (decklink_start_input (avctx) != S_OK) {
>          av_log(avctx, AV_LOG_ERROR, "Cannot start input stream\n");
>          ret = AVERROR(EIO);
>

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

Hello James,

Thanks for reviewing.

>> +    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);
> 
> For this, av_packet_new_side_data() seems more adequate than av_malloc()
> + av_packet_add_side_data().
> 
> Also, you should propagate the errors av_packet_{add,new}_side_data return.

Good catch.  Yes, I will move to using av_packet_new_side_data() and fix the return.

Thanks,

Devin

On 11/29/2017 7:17 PM, Devin Heitmueller wrote:
>> 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.

I have no strong feelings either way.

>> 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.

The internet seems to claim it's a GNU extension. Will this code ever possibly
be built with something that isn't GCC or Clang?

>> 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?

My mail must have got mangled while I was editing it. Ignore this, I think.

>> 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).

Is this aligment a guarantee by the module?

> 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).

[...]

- Derek