[FFmpeg-devel] [PATCH] avdevice/decklink_dec: extracting and outputing klv from vanc
Devin Heitmueller
devin.heitmueller at ltnglobal.com
Wed Jun 17 23:22:09 EEST 2020
Hello Milos,
A few additional comments in addition to what Marton found. Please in
the future send patches as in-line rather than attachments.
On Sun, Jun 14, 2020 at 10:27 AM 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
This is referencing the incorrect specification. KLV data over SDI is
formally defined in SMPTE RP 214-2002. While MISB ST 0605 is
informative and in fact provides perhaps an easier to understand
explanation of the packet structure, it is not the formal reference
and is oriented specifically around the carriage of timestamp metadata
represented as KLV.
> > + const uint8_t KLV_DID = 0x44;
> > + const uint8_t KLV_IN_VANC_SDID = 0x04;
> > + const uint8_t KLV_IN_HANC_SDID = 0x14;
There is no support for HANC in the decklink output. Remove the above
unused constant.
> > +
> > + 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.
We already have a self-contained parser for other VANC packet types
(i.e. teletext, captions). I would rather just see that reused and
have another "else if (did == 0x44 && sdid == 0x04)" simply added to
the existing get_metadata() function than implement a completely
different API for VANC handling. This eliminates the requirement on
the newer SDK and provides more consistent handling relative to the
other VANC types currently supported.
> > +
> > + 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?
I had the same question. Other standards such as SMPTE 2010 require
the packets to be in order (since they simply denote
start/middle/end). Milos, have you seen any cases where reordering is
actually required prior to reassembly?
>
> > +
> > + for (auto& packet : list)
> > + klv.insert(klv.end(), packet.data.begin(), packet.data.end());
> > + }
Should you be tracking the PSC values to check for missing packets
(e.g. checksum errors)? If you get PSC values 1, 2, 4, then you would
concatenate those three packets together without noticing that packet
3 was never provided. The result would be announcing a partial packet
up the stack.
> > +
> > + 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
Personally I would prefer "enable_klv" over "output_klv", as using the
term "output" when describing a property of an input device could be
confusing to some users.
> > { "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
> >
Devin
--
Devin Heitmueller, Senior Software Engineer
LTN Global Communications
o: +1 (301) 363-1001
w: https://ltnglobal.com e: devin.heitmueller at ltnglobal.com
More information about the ffmpeg-devel
mailing list