[FFmpeg-devel,01/11] libavdevice/decklink: Add support for EIA-708 output over SDI

Hook in libklvanc and use it for output of EIA-708 captions over
SDI.  The bulk of this patch is just general support for ancillary
data for the Decklink SDI module - the real work for construction
of the EIA-708 CDP and VANC line construction is done by libklvanc.

Libklvanc can be found at: https://github.com/stoth68000/libklvanc

Updated to reflect feedback from Marton Balint <cus@passwd.hu>,
Carl Eugen Hoyos <ceffmpeg@gmail.com>, and Aaron Levinson
<alevinsn_dev@levland.net>.

Signed-off-by: Devin Heitmueller <dheitmueller@ltnglobal.com>
---
 configure                       |   4 +
 libavcodec/v210enc.c            |   9 ++
 libavdevice/decklink_common.cpp |  16 +++-
 libavdevice/decklink_common.h   |  10 +++
 libavdevice/decklink_enc.cpp    | 178 ++++++++++++++++++++++++++++++++++++++--
 5 files changed, 206 insertions(+), 11 deletions(-)

On Mon, Jan 08, 2018 at 20:16:48 -0500, Devin Heitmueller wrote:

> +        if (ctx->supports_vanc == 0 || ctx->dlo->DoesSupportVideoMode(ctx->bmd_mode, ctx->raw_format,

In other places you (or the DeckLink code) use
    if (!ctx->supports_vanc || [...]
which I believe is preferred.

> +    if (ctx->supports_vanc == 0)

Same here.

> +        if (ret != 0)
> +            return AVERROR(ENOMEM);

Same here.

> +        ret = klvanc_set_framerate_EIA_708B(pkt, ctx->bmd_tb_num, ctx->bmd_tb_den);
> +        if (ret != 0) {
> +            av_log(avctx, AV_LOG_ERROR, "Invalid framerate specified: %lld/%lld\n",

Same here, and others.

> +        for (size_t i = 0; i < cc_count; i++) {

Declare i outside the clause.

> +    for (int i = 0; i < vanc_lines.num_lines; i++) {

Same here.

> +        if (line == NULL)

if (!line)

These comments apply to several of the other patches as well.

Moritz

On Wed, Jan 10, 2018 at 14:29:43 +0100, Moritz Barsnick wrote:
> > +        for (size_t i = 0; i < cc_count; i++) {
> Declare i outside the clause.
> > +    for (int i = 0; i < vanc_lines.num_lines; i++) {
> Same here.

Sorry, this rule may not apply to C++ files, as it's part of all C++
standards and all compilers should support it. Others may know more.

Moritz

Hello Moritz,

> On Wed, Jan 10, 2018 at 14:29:43 +0100, Moritz Barsnick wrote:
>>> +        for (size_t i = 0; i < cc_count; i++) {
>> Declare i outside the clause.
>>> +    for (int i = 0; i < vanc_lines.num_lines; i++) {
>> Same here.
> 
> Sorry, this rule may not apply to C++ files, as it's part of all C++
> standards and all compilers should support it. Others may know more.


For loop declaration has been part of the C standard since C99.  I’m not against changing it if required, but I’ve already revised the patch series multiple times and would prefer that demands for such stylistic cleanup either be pointed out in the first review or I can submit such changes as subsequent patches.

If the patch needs to be revised for some good technical reason, I will incorporate these changes.

Devin

On Wed, Jan 10, 2018 at 09:11:35 -0500, Devin Heitmueller wrote:
> For loop declaration has been part of the C standard since C99.

I'm just quoting other similar discussions on this list. This has been
a topic several (hundred? ;-)) times the last few months. It peaked in
this discussion which was apparently never closed:
http://ffmpeg.org/pipermail/ffmpeg-devel/2017-November/219405.html

> I’m not against changing it if required, but I’ve already revised the
> patch series multiple times and would prefer that demands for such
> stylistic cleanup either be pointed out in the first review or I can
> submit such changes as subsequent patches.

I don't care, I'm justing pointing it out, as others don't seem to care
(enough/anymore/and were more concentrated on the technical aspects) or
have missed it. Comments of these sorts have come before (elsewhere of
course).

Moritz

On Mon, 8 Jan 2018, Devin Heitmueller wrote:

> Hook in libklvanc and use it for output of EIA-708 captions over
> SDI.  The bulk of this patch is just general support for ancillary
> data for the Decklink SDI module - the real work for construction
> of the EIA-708 CDP and VANC line construction is done by libklvanc.
>
> Libklvanc can be found at: https://github.com/stoth68000/libklvanc
>
> Updated to reflect feedback from Marton Balint <cus@passwd.hu>,
> Carl Eugen Hoyos <ceffmpeg@gmail.com>, and Aaron Levinson
> <alevinsn_dev@levland.net>.
>
> Signed-off-by: Devin Heitmueller <dheitmueller@ltnglobal.com>
> ---
> configure                       |   4 +
> libavcodec/v210enc.c            |   9 ++
> libavdevice/decklink_common.cpp |  16 +++-
> libavdevice/decklink_common.h   |  10 +++
> libavdevice/decklink_enc.cpp    | 178 ++++++++++++++++++++++++++++++++++++++--
> 5 files changed, 206 insertions(+), 11 deletions(-)
>

[...]

> diff --git a/libavdevice/decklink_enc.cpp b/libavdevice/decklink_enc.cpp
> index c06ca46..faa382a 100644
> --- a/libavdevice/decklink_enc.cpp
> +++ b/libavdevice/decklink_enc.cpp
> @@ -38,17 +38,25 @@ extern "C" {
> 
> #include "decklink_common.h"
> #include "decklink_enc.h"
> -
> +#if CONFIG_LIBKLVANC
> +#include "libklvanc/vanc.h"
> +#include "libklvanc/vanc-lines.h"
> +#include "libklvanc/pixels.h"
> +#endif
> 
> /* DeckLink callback class declaration */
> class decklink_frame : public IDeckLinkVideoFrame
> {
> public:
>     decklink_frame(struct decklink_ctx *ctx, AVFrame *avframe, AVCodecID codec_id, int height, int width) :
> -        _ctx(ctx), _avframe(avframe), _avpacket(NULL), _codec_id(codec_id), _height(height), _width(width),  _refs(1) { }
> +        _ctx(ctx), _avframe(avframe), _avpacket(NULL), _codec_id(codec_id), _ancillary(NULL), _height(height), _width(width),  _refs(1) { }
>     decklink_frame(struct decklink_ctx *ctx, AVPacket *avpacket, AVCodecID codec_id, int height, int width) :
> -        _ctx(ctx), _avframe(NULL), _avpacket(avpacket), _codec_id(codec_id), _height(height), _width(width), _refs(1) { }
> -
> +        _ctx(ctx), _avframe(NULL), _avpacket(avpacket), _codec_id(codec_id), _ancillary(NULL), _height(height), _width(width), _refs(1) { }
> +    virtual ~decklink_frame()
> +    {
> +        if (_ancillary)
> +            _ancillary->Release();
> +    };
>     virtual long           STDMETHODCALLTYPE GetWidth      (void)          { return _width; }
>     virtual long           STDMETHODCALLTYPE GetHeight     (void)          { return _height; }
>     virtual long           STDMETHODCALLTYPE GetRowBytes   (void)
> @@ -87,8 +95,22 @@ public:
>     }
>
>     virtual HRESULT STDMETHODCALLTYPE GetTimecode     (BMDTimecodeFormat format, IDeckLinkTimecode **timecode) { return S_FALSE; }
> -    virtual HRESULT STDMETHODCALLTYPE GetAncillaryData(IDeckLinkVideoFrameAncillary **ancillary)               { return S_FALSE; }
> -
> +    virtual HRESULT STDMETHODCALLTYPE GetAncillaryData(IDeckLinkVideoFrameAncillary **ancillary)
> +    {
> +        *ancillary = _ancillary;
> +        if (_ancillary) {
> +            _ancillary->AddRef();
> +            return S_OK;
> +        } else {
> +            return S_FALSE;
> +        }
> +    }
> +    virtual HRESULT STDMETHODCALLTYPE SetAncillaryData(IDeckLinkVideoFrameAncillary *ancillary)
> +    {
> +        _ancillary = ancillary;

I guess if you want to follow the existing logic, you have to 
release _ancillary here if it was already set before assigning a new 
value.

> +        _ancillary->AddRef();
> +        return S_OK;
> +    }
>     virtual HRESULT STDMETHODCALLTYPE QueryInterface(REFIID iid, LPVOID *ppv) { return E_NOINTERFACE; }
>     virtual ULONG   STDMETHODCALLTYPE AddRef(void)                            { return ++_refs; }
>     virtual ULONG   STDMETHODCALLTYPE Release(void)
> @@ -106,6 +128,7 @@ public:
>     AVFrame *_avframe;
>     AVPacket *_avpacket;
>     AVCodecID _codec_id;
> +    IDeckLinkVideoFrameAncillary *_ancillary;
>     int _height;
>     int _width;
> 
> @@ -156,10 +179,13 @@ static int decklink_setup_video(AVFormatContext *avctx, AVStream *st)
>                    " Only AV_PIX_FMT_UYVY422 is supported.\n");
>             return -1;
>         }
> +        ctx->raw_format = bmdFormat8BitYUV;
>     } else if (c->codec_id != AV_CODEC_ID_V210) {
>         av_log(avctx, AV_LOG_ERROR, "Unsupported codec type!"
>                " Only V210 and wrapped frame with AV_PIX_FMT_UYVY422 are supported.\n");
>         return -1;
> +    } else {
> +        ctx->raw_format = bmdFormat10BitYUV;
>     }
>
>     if (ff_decklink_set_configs(avctx, DIRECTION_OUT) < 0) {
> @@ -173,7 +199,7 @@ static int decklink_setup_video(AVFormatContext *avctx, AVStream *st)
>         return -1;
>     }
>     if (ctx->dlo->EnableVideoOutput(ctx->bmd_mode,
> -                                    bmdVideoOutputFlagDefault) != S_OK) {
> +                                    ctx->supports_vanc ? bmdVideoOutputVANC : bmdVideoOutputFlagDefault) != S_OK) {
>         av_log(avctx, AV_LOG_ERROR, "Could not enable video output!\n");
>         return -1;
>     }
> @@ -264,11 +290,132 @@ av_cold int ff_decklink_write_trailer(AVFormatContext *avctx)
>     pthread_mutex_destroy(&ctx->mutex);
>     pthread_cond_destroy(&ctx->cond);
> 
> +#if CONFIG_LIBKLVANC
> +    klvanc_context_destroy(ctx->vanc_ctx);
> +#endif
> +
>     av_freep(&cctx->ctx);
>
>     return 0;
> }
> 
> +#if CONFIG_LIBKLVANC
> +static int decklink_construct_vanc(AVFormatContext *avctx, struct decklink_ctx *ctx,
> +                                   AVPacket *pkt, decklink_frame *frame)
> +{
> +    struct klvanc_line_set_s vanc_lines = { 0 };
> +    int ret, size;
> +
> +    if (ctx->supports_vanc == 0)
> +        return 0;
> +
> +    const uint8_t *data = av_packet_get_side_data(pkt, AV_PKT_DATA_A53_CC, &size);
> +    if (data) {
> +        struct klvanc_packet_eia_708b_s *pkt;
> +        uint16_t *cdp;
> +        uint16_t len;
> +        uint8_t cc_count = size / 3;
> +
> +        ret = klvanc_create_eia708_cdp(&pkt);
> +        if (ret != 0)
> +            return AVERROR(ENOMEM);
> +
> +        ret = klvanc_set_framerate_EIA_708B(pkt, ctx->bmd_tb_num, ctx->bmd_tb_den);
> +        if (ret != 0) {
> +            av_log(avctx, AV_LOG_ERROR, "Invalid framerate specified: %lld/%lld\n",
> +                   ctx->bmd_tb_num, ctx->bmd_tb_den);
> +            klvanc_destroy_eia708_cdp(pkt);
> +            return AVERROR(EINVAL);
> +        }
> +
> +        if (cc_count > KLVANC_MAX_CC_COUNT) {
> +            av_log(avctx, AV_LOG_ERROR, "Illegal cc_count received: %d\n", cc_count);
> +            cc_count = KLVANC_MAX_CC_COUNT;
> +        }
> +
> +        /* CC data */
> +        pkt->header.ccdata_present = 1;
> +        pkt->ccdata.cc_count = cc_count;
> +        for (size_t i = 0; i < cc_count; i++) {
> +            if (data [3*i] & 0x40)
> +                pkt->ccdata.cc[i].cc_valid = 1;
> +            pkt->ccdata.cc[i].cc_type = data[3*i] & 0x03;
> +            pkt->ccdata.cc[i].cc_data[0] = data[3*i+1];
> +            pkt->ccdata.cc[i].cc_data[1] = data[3*i+2];
> +        }
> +
> +        klvanc_finalize_EIA_708B(pkt, ctx->cdp_sequence_num++);
> +        ret = klvanc_convert_EIA_708B_to_words(pkt, &cdp, &len);
> +        if (ret != 0) {
> +            av_log(avctx, AV_LOG_ERROR, "Failed converting 708 packet to words\n");
> +            return AVERROR(ENOMEM);
> +        }
> +        klvanc_destroy_eia708_cdp(pkt);

This destroy should be before the if so pkt is freed in case of 
an error as well.

> +
> +        ret = klvanc_line_insert(ctx->vanc_ctx, &vanc_lines, cdp, len, 11, 0);

cdp needs freeing here, no?

> +        if (ret != 0) {
> +            av_log(avctx, AV_LOG_ERROR, "VANC line insertion failed\n");
> +            return AVERROR(ENOMEM);
> +        }
> +    }
> +
> +    IDeckLinkVideoFrameAncillary *vanc;
> +    int result = ctx->dlo->CreateAncillaryData(bmdFormat10BitYUV, &vanc);
> +    if (result != S_OK) {
> +        av_log(avctx, AV_LOG_ERROR, "Failed to create vanc\n");

Are you leaking the contents of vanc_lines here? I am not familiar with 
libklvan library functions/structures, so I might be wrong, but this seems 
suspicious.

> +        return -1;
> +    }
> +
> +    /* Now that we've got all the VANC lines in a nice orderly manner, generate the
> +       final VANC sections for the Decklink output */
> +    for (int i = 0; i < vanc_lines.num_lines; i++) {
> +        struct klvanc_line_s *line = vanc_lines.lines[i];
> +        uint16_t *out_line;
> +        int real_line;
> +        int out_len;
> +        void *buf;
> +
> +        if (line == NULL)
> +            break;
> +
> +        real_line = line->line_number;
> +#if 0
> +        /* FIXME: include hack for certain Decklink cards which mis-represent
> +           line numbers for pSF frames */
> +        if (decklink_sys->b_psf_interlaced)
> +            real_line = Calculate1080psfVancLine(line->line_number);
> +#endif
> +        result = vanc->GetBufferForVerticalBlankingLine(real_line, &buf);
> +        if (result != S_OK) {
> +            av_log(avctx, AV_LOG_ERROR, "Failed to get VANC line %d: %d", real_line, result);
> +            klvanc_line_free(line);
> +            continue;
> +        }
> +
> +        /* Generate the full line taking into account all VANC packets on that line */
> +        result = klvanc_generate_vanc_line(ctx->vanc_ctx, line, &out_line, &out_len, ctx->bmd_width);
> +        if (result != 0) {
> +            av_log(avctx, AV_LOG_ERROR, "Failed to generate VANC line\n");
> +            klvanc_line_free(line);
> +            continue;
> +        }
> +
> +        /* Repack the 16-bit ints into 10-bit, and push into final buffer */
> +        klvanc_y10_to_v210(out_line, (uint8_t *) buf, out_len);
> +        free(out_line);
> +        klvanc_line_free(line);
> +    }
> +
> +    result = frame->SetAncillaryData(vanc);
> +    vanc->Release();
> +    if (result != S_OK) {
> +        av_log(avctx, AV_LOG_ERROR, "Failed to set vanc: %d", result);
> +        return AVERROR(EIO);
> +    }
> +    return 0;
> +}
> +#endif
> +
> static int decklink_write_video_packet(AVFormatContext *avctx, AVPacket *pkt)
> {
>     struct decklink_cctx *cctx = (struct decklink_cctx *)avctx->priv_data;
> @@ -279,6 +426,9 @@ static int decklink_write_video_packet(AVFormatContext *avctx, AVPacket *pkt)
>     decklink_frame *frame;
>     buffercount_type buffered;
>     HRESULT hr;
> +#if CONFIG_LIBKLVANC
> +    int ret;
> +#endif
>
>     if (st->codecpar->codec_id == AV_CODEC_ID_WRAPPED_AVFRAME) {
>         if (tmp->format != AV_PIX_FMT_UYVY422 ||
> @@ -303,6 +453,13 @@ static int decklink_write_video_packet(AVFormatContext *avctx, AVPacket *pkt)
>         }
>
>         frame = new decklink_frame(ctx, avpacket, st->codecpar->codec_id, ctx->bmd_height, ctx->bmd_width);
> +
> +#if CONFIG_LIBKLVANC
> +        ret = decklink_construct_vanc(avctx, ctx, pkt, frame);
> +        if (ret != 0) {
> +            av_log(avctx, AV_LOG_ERROR, "Failed to construct VANC\n");
> +        }
> +#endif
>     }
>
>     if (!frame) {
> @@ -393,6 +550,13 @@ 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
> +    if (klvanc_context_create(&ctx->vanc_ctx) < 0) {
> +        av_log(avctx, AV_LOG_ERROR, "Cannot create VANC library context\n");
> +        return AVERROR(ENOMEM);
> +    }
> +    ctx->supports_vanc = 1;
> +#endif
>
>     /* List available devices and exit. */
>     if (ctx->list_devices) {
> -- 
> 1.8.3.1

Regards,
Marton