[FFmpeg-devel] [PATCH] avdevice/decklink_dec: extracting and outputing klv from vanc

Zivkovic, Milos zivkovic at teralogics.com
Mon Jun 15 12:16:27 EEST 2020


Hello,

I appreciate the comments.

Missing docs for the new option.
>

Does this refer to .texi files? I'll update the docs.


> Avoid these if possible. Yes, this is C++ code but only because the
> decklink
> API requires it. Also it gives me a compile error, and I am not familiar
> enough
> with C++ to figure out what is going on
>

It makes everything much easier (just like using std::atomic for ref
counting does).
It also doesn't change the needed version of C++ since std::atomic is
available in C++11 and above.
However, that error message is concerning and I'd like to fix it. Which
compiler is it?

You only use this to avoid calling ->Release() right? If that is the case,
> then
> I'd avoid this as well, I am not sure about the compiler support of such
> templating.
>

Yes, it's used to avoid manually releasing COM pointers which can become
quite a hassle especially if multiple COM objects are involved.
Error handling is simplified as well since there's no need to worry which
COM object(s) should be released.
As far as compiler support goes, this is the most basic C++ template that's
supported by basically every C++ compiler in the last 20+ years.
I'm willing to remove it if you still feel strongly that it's unnecessary.

This interface is only available from DeckLink SDK 10.10 or so, but you
> have not
> bumped the version requirement in configure.
>

Wasn't sure which files should be edited for this.
I'll get the exact SDK version that introduces it and bump it in configure.

Is the sorting really needed? Aren't they supposed to be sorted in the
> VANC?
>

Well, they should be, but it's not guaranteed so this is just a safety net.
If it was guaranteed there would be no need for PSC fields.

Maybe avoid the error message and increase ctx->dropped like other similar
> code
> does it.
>

Will do.

Mixed code and declaration.
>

I'll move that block below the declaration.

Lose the error message for ENOMEM.
>

I was just following what the other code that adds streams does.

AV_OPT_TYPE_BOOL
>

I'll change that asap.

Thanks,
Milos

On Sun, Jun 14, 2020 at 4:27 PM Marton Balint <cus at passwd.hu> wrote:

>
> On Tue, 9 Jun 2020, Zivkovic, Milos wrote:
>
> > Hello,
> >
> > I've attached a patch that adds support for extraction of KLV data from
> SDI
> > VANC.
> > Code is enabled with a special *output_klv* option and is otherwise
> unused.
> >
> > This was tested with an off the shelf UDP -> SDI decoder that preserves
> KLV
> > and inserts them in VANC, following the guidelines in MISB ST 0605.
> >
> > I'm working on another patch that will enable *avdevice/decklink_enc* to
> do
> > that as well, and it will also require *output_klv* option to be used.
>
> > From 7371747b86dd3de4889c28d8b6536e8aff08be2b Mon Sep 17 00:00:00 2001
> > From: Milos Zivkovic <zivkovic at teralogics.com>
> > Date: Mon, 8 Jun 2020 11:56:37 +0200
> > Subject: [PATCH] avdevice/decklink_dec: extracting and outputing klv
> from vanc
> >
> > Signed-off-by: Milos Zivkovic <zivkovic at teralogics.com>
> > ---
> >  libavdevice/decklink_common.h   |   2 +
> >  libavdevice/decklink_common_c.h |   1 +
> >  libavdevice/decklink_dec.cpp    | 160
> +++++++++++++++++++++++++++++++++++++++-
> >  libavdevice/decklink_dec_c.c    |   1 +
> >  4 files changed, 163 insertions(+), 1 deletion(-)
>
> Missing docs for the new option.
>
> >
> > diff --git a/libavdevice/decklink_common.h
> b/libavdevice/decklink_common.h
> > index 27ce6a8..2d57b85 100644
> > --- a/libavdevice/decklink_common.h
> > +++ b/libavdevice/decklink_common.h
> > @@ -120,12 +120,14 @@ struct decklink_ctx {
> >      unsigned int dropped;
> >      AVStream *audio_st;
> >      AVStream *video_st;
> > +    AVStream *klv_st;
> >      AVStream *teletext_st;
> >      uint16_t cdp_sequence_num;
> >
> >      /* Options */
> >      int list_devices;
> >      int list_formats;
> > +    int output_klv;
> >      int64_t teletext_lines;
> >      double preroll;
> >      int duplex_mode;
> > diff --git a/libavdevice/decklink_common_c.h
> b/libavdevice/decklink_common_c.h
> > index 88b1eae..953aebe 100644
> > --- a/libavdevice/decklink_common_c.h
> > +++ b/libavdevice/decklink_common_c.h
> > @@ -40,6 +40,7 @@ struct decklink_cctx {
> >      /* Options */
> >      int list_devices;
> >      int list_formats;
> > +    int output_klv;
> >      int64_t teletext_lines;
> >      double preroll;
> >      int audio_channels;
> > diff --git a/libavdevice/decklink_dec.cpp b/libavdevice/decklink_dec.cpp
> > index 82106aa..7b39dd5 100644
> > --- a/libavdevice/decklink_dec.cpp
> > +++ b/libavdevice/decklink_dec.cpp
> > @@ -22,6 +22,8 @@
> >   */
> >
> >  #include <atomic>
> > +#include <vector>
> > +#include <algorithm>
>
> Avoid these if possible. Yes, this is C++ code but only because the
> decklink
> API requires it. Also it gives me a compile error, and I am not familiar
> enough
> with C++ to figure out what is going on:
>
> cc1plus: warning: command line option ‘-Wdeclaration-after-statement’ is
> valid for C/ObjC but not for C++
> cc1plus: warning: command line option ‘-Wmissing-prototypes’ is valid for
> C/ObjC but not for C++
> cc1plus: warning: command line option ‘-Wno-pointer-to-int-cast’ is valid
> for C/ObjC but not for C++
> cc1plus: warning: command line option ‘-Wstrict-prototypes’ is valid for
> C/ObjC but not for C++
> cc1plus: warning: command line option ‘-Wno-pointer-sign’ is valid for
> C/ObjC but not for C++
> cc1plus: warning: ‘-Werror=’ argument
> ‘-Werror=implicit-function-declaration’ is not valid for C++
> cc1plus: warning: ‘-Werror=’ argument ‘-Werror=missing-prototypes’ is not
> valid for C++
> cc1plus: warning: command line option ‘-std=c11’ is valid for C/ObjC but
> not for C++
> In file included from /usr/include/c++/7/ext/new_allocator.h:33:0,
>                   from
> /usr/include/c++/7/x86_64-suse-linux/bits/c++allocator.h:33,
>                   from /usr/include/c++/7/bits/allocator.h:46,
>                   from /usr/include/c++/7/vector:61,
>                   from libavdevice/decklink_dec.cpp:25:
> ./new:1:1: error: expected unqualified-id before ‘[’ token
>   [PACKET]
>   ^
> In file included from /usr/include/c++/7/bits/allocator.h:46:0,
>                   from /usr/include/c++/7/vector:61,
>                   from libavdevice/decklink_dec.cpp:25:
> /usr/include/c++/7/x86_64-suse-linux/bits/c++allocator.h:48:41: error:
> ‘new_allocator’ in namespace ‘__gnu_cxx’ does not name a template type
>       using __allocator_base = __gnu_cxx::new_allocator<_Tp>;
>                                           ^~~~~~~~~~~~~
> In file included from /usr/include/c++/7/vector:61:0,
>                   from libavdevice/decklink_dec.cpp:25:
> /usr/include/c++/7/bits/allocator.h:108:45: error: expected template-name
> before ‘<’ token
>       class allocator: public __allocator_base<_Tp>
>
> [...]
>
> >  using std::atomic;
> >
> >  /* Include internal.h first to avoid conflict between winsock.h (used by
> > @@ -583,6 +585,143 @@ static int avpacket_queue_get(AVPacketQueue *q,
> AVPacket *pkt, int block)
> >      return ret;
> >  }
> >
> > +namespace
> > +{
> > +    template<typename T>
> > +    struct com_ptr
> > +    {
> > +        T* self;
> > +
> > +        com_ptr()
> > +            : self(nullptr)
> > +        {
> > +        }
> > +
> > +        ~com_ptr()
> > +        {
> > +            destroy();
> > +        }
> > +
> > +        void destroy()
> > +        {
> > +            if (self)
> > +                self->Release();
> > +
> > +            self = nullptr;
> > +        }
> > +
> > +        T* get()
> > +        {
> > +            return self;
> > +        }
> > +
> > +        const T* get() const
> > +        {
> > +            return self;
> > +        }
> > +
> > +        T* operator->()
> > +        {
> > +            return get();
> > +        }
> > +
> > +        const T* operator->() const
> > +        {
> > +            return get();
> > +        }
> > +    };
> > +}
>
> You only use this to avoid calling ->Release() right? If that is the case,
> then
> I'd avoid this as well, I am not sure about the compiler support of such
> templating.
>
> > +
> > +static void handle_klv(AVFormatContext *avctx, decklink_ctx *ctx,
> IDeckLinkVideoInputFrame *videoFrame, int64_t pts)
> > +{
> > +    // defined by MISB ST 0605
> > +    const uint8_t KLV_DID = 0x44;
> > +    const uint8_t KLV_IN_VANC_SDID = 0x04;
> > +    const uint8_t KLV_IN_HANC_SDID = 0x14;
> > +
> > +    struct KLVPacket
> > +    {
> > +        uint16_t sequence_counter;
> > +        std::vector<uint8_t> data;
> > +    };
> > +
> > +    size_t total_size = 0;
> > +    std::vector<std::vector<KLVPacket>> klv_packets(256);
> > +
> > +    com_ptr<IDeckLinkVideoFrameAncillaryPackets> packets;
> > +    if
> (videoFrame->QueryInterface(IID_IDeckLinkVideoFrameAncillaryPackets,
> (void**)&packets.self) != S_OK)
> > +        return;
>
> This interface is only available from DeckLink SDK 10.10 or so, but you
> have not
> bumped the version requirement in configure.
>
> > +
> > +    com_ptr<IDeckLinkAncillaryPacketIterator> it;
> > +    if (packets->GetPacketIterator(&it.self) != S_OK)
> > +        return;
> > +
> > +    com_ptr<IDeckLinkAncillaryPacket> packet;
> > +    while (it->Next(&packet.self) == S_OK) {
> > +        uint8_t *data = nullptr;
> > +        uint32_t size = 0;
> > +
> > +        if (packet->GetDID() == KLV_DID && packet->GetSDID() ==
> KLV_IN_VANC_SDID) {
> > +             av_log(avctx, AV_LOG_DEBUG, "Found KLV VANC packet on
> line: %d\n", packet->GetLineNumber());
> > +
> > +            if (packet->GetBytes(bmdAncillaryPacketFormatUInt8, (const
> void**) &data, &size) == S_OK) {
> > +                // MID and PSC
> > +                if (size > 3) {
> > +                    uint8_t mid = data[0];
> > +                    uint16_t psc = data[1] << 8 | data[2];
> > +
> > +                    av_log(avctx, AV_LOG_DEBUG, "KLV with MID: %d and
> PSC: %d\n", mid, psc);
> > +
> > +                    uint32_t data_len = size - 3;
> > +                    total_size += data_len;
> > +
> > +                    KLVPacket packet{ psc };
> > +                    packet.data.resize(data_len);
> > +                    memcpy(packet.data.data(), data + 3, data_len);
> > +
> > +                    klv_packets[mid].push_back(std::move(packet));
> > +                }
> > +            }
> > +        }
> > +
> > +        packet.destroy();
> > +    }
> > +
> > +    if (total_size > 0) {
> > +        std::vector<uint8_t> klv;
> > +        klv.reserve(total_size);
> > +
> > +        for (size_t i = 0; i < klv_packets.size(); ++i) {
> > +            auto& list = klv_packets[i];
> > +
> > +            if (list.empty())
> > +                continue;
> > +
> > +            av_log(avctx, AV_LOG_DEBUG, "Joining MID: %d\n", i);
> > +
> > +            std::sort(list.begin(), list.end(), [](const KLVPacket&
> lhs, const KLVPacket& rhs) {
> > +                return lhs.sequence_counter < rhs.sequence_counter;
> > +            });
>
> Is the sorting really needed? Aren't they supposed to be sorted in the
> VANC?
>
> > +
> > +            for (auto& packet : list)
> > +                klv.insert(klv.end(), packet.data.begin(),
> packet.data.end());
> > +        }
> > +
> > +        AVPacket klv_packet;
> > +        av_init_packet(&klv_packet);
> > +        klv_packet.pts = pts;
> > +        klv_packet.dts = pts;
> > +        klv_packet.flags |= AV_PKT_FLAG_KEY;
> > +        klv_packet.stream_index = ctx->klv_st->index;
> > +        klv_packet.data = klv.data();
> > +        klv_packet.size = klv.size();
> > +
> > +        if (avpacket_queue_put(&ctx->queue, &klv_packet) < 0) {
> > +            av_log(avctx, AV_LOG_INFO, "dropping KLV VANC packet\n");
>
> Maybe avoid the error message and increase ctx->dropped like other similar
> code
> does it.
>
> > +        }
> > +    }
> > +}
> > +
> >  class decklink_input_callback : public IDeckLinkInputCallback
> >  {
> >  public:
> > @@ -816,6 +955,10 @@ HRESULT
> decklink_input_callback::VideoInputFrameArrived(
> >          //fprintf(stderr,"Video Frame size %d ts %d\n", pkt.size,
> pkt.pts);
> >
> >          if (!no_video) {
> > +            if (ctx->output_klv) {
> > +                handle_klv(avctx, ctx, videoFrame, pkt.pts);
> > +            }
> > +
>
> Mixed code and declaration.
>
> >              IDeckLinkVideoFrameAncillary *vanc;
> >              AVPacket txt_pkt;
> >              uint8_t txt_buf0[3531]; // 35 * 46 bytes decoded teletext
> lines + 1 byte data_identifier + 1920 bytes OP47 decode buffer
> > @@ -973,7 +1116,6 @@ static int decklink_autodetect(struct decklink_cctx
> *cctx) {
> >      } else {
> >          return -1;
> >      }
> > -
> >  }
> >
> >  extern "C" {
> > @@ -1012,6 +1154,7 @@ av_cold int
> ff_decklink_read_header(AVFormatContext *avctx)
> >          return AVERROR(ENOMEM);
> >      ctx->list_devices = cctx->list_devices;
> >      ctx->list_formats = cctx->list_formats;
> > +    ctx->output_klv = cctx->output_klv;
> >      ctx->teletext_lines = cctx->teletext_lines;
> >      ctx->preroll      = cctx->preroll;
> >      ctx->duplex_mode  = cctx->duplex_mode;
> > @@ -1202,6 +1345,21 @@ av_cold int
> ff_decklink_read_header(AVFormatContext *avctx)
> >
> >      ctx->video_st=st;
> >
> > +    if (ctx->output_klv) {
> > +        st = avformat_new_stream(avctx, NULL);
> > +        if (!st) {
> > +            av_log(avctx, AV_LOG_ERROR, "Cannot add KLV stream\n");
>
> Lose the error message for ENOMEM.
>
> > +            ret = AVERROR(ENOMEM);
> > +            goto error;
> > +        }
> > +        st->codecpar->codec_type = AVMEDIA_TYPE_DATA;
> > +        st->time_base.den        = ctx->bmd_tb_den;
> > +        st->time_base.num        = ctx->bmd_tb_num;
> > +        st->codecpar->codec_id   = AV_CODEC_ID_SMPTE_KLV;
> > +        avpriv_set_pts_info(st, 64, 1, 1000000);  /* 64 bits pts in us
> */
> > +        ctx->klv_st = st;
> > +    }
> > +
> >      if (ctx->teletext_lines) {
> >          st = avformat_new_stream(avctx, NULL);
> >          if (!st) {
> > diff --git a/libavdevice/decklink_dec_c.c b/libavdevice/decklink_dec_c.c
> > index b598769..8beb629 100644
> > --- a/libavdevice/decklink_dec_c.c
> > +++ b/libavdevice/decklink_dec_c.c
> > @@ -39,6 +39,7 @@ static const AVOption options[] = {
> >      { "argb",          NULL,   0,  AV_OPT_TYPE_CONST, { .i64 = 32
>                  }, 0, 0, DEC, "raw_format"},
> >      { "bgra",          NULL,   0,  AV_OPT_TYPE_CONST, { .i64 =
> MKBETAG('B','G','R','A') }, 0, 0, DEC, "raw_format"},
> >      { "rgb10",         NULL,   0,  AV_OPT_TYPE_CONST, { .i64 =
> MKBETAG('r','2','1','0') }, 0, 0, DEC, "raw_format"},
> > +    { "output_klv",     "output klv if present in vanc",
> OFFSET(output_klv), AV_OPT_TYPE_INT,  { .i64 = 0}, 0, 1,   DEC },
>
> AV_OPT_TYPE_BOOL
>
> >      { "teletext_lines", "teletext lines bitmask",
> OFFSET(teletext_lines), AV_OPT_TYPE_INT64, { .i64 = 0   }, 0,
> 0x7ffffffffLL, DEC, "teletext_lines"},
> >      { "standard",     NULL,
>  0,  AV_OPT_TYPE_CONST, { .i64 = 0x7fff9fffeLL}, 0, 0,    DEC,
> "teletext_lines"},
> >      { "all",          NULL,
>  0,  AV_OPT_TYPE_CONST, { .i64 = 0x7ffffffffLL}, 0, 0,    DEC,
> "teletext_lines"},
> > --
> > 1.8.3.1
> >
>
> Thanks,
> Marton
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".


More information about the ffmpeg-devel mailing list