Thanks! :) Can you attach the patch(es) in "git format-patch" format directly to this bug report? That allows direct reviewing of them here inside Bugzilla.

I can, but was planning on waiting until I have it basically working.  Do you want me to attach what I have so far?

What is missing?

There is still quite a bit needed.  The basic support for grabbing the VANC data from the decklink API is there, but that data further decoded into caption data, so those captions can then be processed by the mpeg2 encoder.  VANC can have a lot of other data besides Closed Captions, and there is some basic support for extracting those other types there in case someone wants to expand on this work.

I have updated the github repo with my current code.  It pushes the VANC metadata out with the video buffer from decklinkvideosrc, and that metadata is being seen by gst_ffmpegvidenc_handle_frame(), but is getting garbled along the way:

The data seems to be added to the metadata correctly:

"gst_buffer_add_vanc_meta did 353 sdid 257 checksum 141 first 662 len 43"

But ffmpeg decodes it as:

"gst_VANCPacket_add       did 1611456576 sdid 32740 checksum 9312 first 22736 len 2"

Further, in gst_decklink_video_src_create(), I have to comment out:

// gst_buffer_add_video_time_code_meta (*buffer, f->tc);

or the VANC metadata is NOT seen by gst_ffmpegvidenc_handle_frame()

This tells me that I am not adding the VANC metadata correctly, but I have not been able to figure out what I am doing wrong.

If you are in the mood to try it, I am running it like:

export GST_DEBUG=*:WARN,decklinkvideosrc:DEBUG
export DEVICE=2
gst-launch-1.0 decklinkvideosrc device-number=${DEVICE} mode=12 video-format=2 enable-vanc=1 ! queue ! autovideoconvert ! deinterlace mode=2 ! queue ! avenc_mpeg2video bitrate=800000 ! mpegvideoparse ! mpegtsmux name=mux ! filesink location=video.ts decklinkaudiosrc device-number=${DEVICE} ! audioconvert ! avenc_ac3 bitrate=640000 ! ac3parse ! queue ! mux.

Otherwise, if you can think of what I might be doing wrong when adding the metadata to the buffer, I would appreciate a hint.

https://github.com/jpoet/gst-plugins-bad/blob/VANC/gst-libs/gst/vanc/gstvancmeta.h
https://github.com/jpoet/gst-plugins-bad/blob/VANC/gst-libs/gst/vanc/gstvancmeta.c

Thanks,

(In reply to John Poet from comment #4)
> There is still quite a bit needed.  The basic support for grabbing the VANC
> data from the decklink API is there, but that data further decoded into
> caption data, so those captions can then be processed by the mpeg2 encoder. 
> VANC can have a lot of other data besides Closed Captions, and there is some
> basic support for extracting those other types there in case someone wants
> to expand on this work.

Let's keep this simple and a step-by-step approach.

I would suggest that we get the GstMeta and decklink parts in already, and then discuss the next parts about actually using the meta (and having decoding functions, etc) as a next step in another bug.

Also for decoding we should not depend on gst-libav/ffmpeg (unconditionally) as that will make it not usable for a big fraction of users who can't ship ffmpeg.

> This tells me that I am not adding the VANC metadata correctly, but I have
> not been able to figure out what I am doing wrong.

Yes that sounds like it. Adding another meta to a buffer should never affect any other metas, it's just an array of metas in the end and you can even add the same meta multiple times.


If you agree with what I said above, let's get the meta and decklink part in as a first step, and get those parts work correct first :) Can you attach 2-3 patches for that here? That's easier for review then

Created attachment 339416 [details] [review]
Add GstMeta for VANC (from decklinkvideosrc)

Foundation for propagating VANC information from sources such as decklinkvideosrc to consumers such as encoders.

Created attachment 339417 [details] [review]
Extract VANC from decklinkvideosrc and add it as metadata

Use the Blackmagic API to extract VANC information and add it as metadata to the video frame.

Created attachment 339418 [details] [review]
Basis for extracing VANC metadata and encoding it.

Extract VANC metadata.  Eventually will be used to encode closed captions into mpeg2.

This currently does not work.  The metadata is corrupt when it gets here, and I probably need help figuring out why.

After applying the three patches, the 'master' version of gstreamer can be run like:

export GST_DEBUG=*:WARN,decklinkvideosrc:DEBUG
export DEVICE=2
gst-launch-1.0 decklinkvideosrc device-number=${DEVICE} mode=12 video-format=2 enable-vanc=1 ! queue ! autovideoconvert ! deinterlace mode=2 ! queue ! avenc_mpeg2video bitrate=800000 ! mpegvideoparse ! mpegtsmux name=mux ! filesink location=video.ts decklinkaudiosrc device-number=${DEVICE} ! audioconvert ! avenc_ac3 bitrate=640000 ! ac3parse ! queue ! mux.

When you run it like that, you will see that the VANC metadata that libav sees is quite different from when was added by decklinkvideosrc.

Review of attachment 339416 [details] [review]:

Various comments here, but I don't see any mistakes that could break the checksum. Are any copies happening in your case?

::: gst-libs/gst/vanc/gstvancmeta.c
@@ +1,1 @@
+#include "gstvancmeta.h"

Before that

#ifndef HAVE_CONFIG_H
#include "config.h"
#endif


Also copyright/license header boilerplate in all new .h and .c files

@@ +5,3 @@
+{
+  static volatile GType type;
+  static const gchar *tags[] = { "VANC", NULL };

Should probably have empty tags. The VANC data applies to the buffer, no matter into what it is transformed to

@@ +19,3 @@
+  GstVANCMeta *vmeta = (GstVANCMeta *) meta;
+  vmeta->data_id = vmeta->dbn_sdid = 0;
+  vmeta->vanc_data = NULL;

Also init the buffer and checksum. Why do you need the buffer in the meta btw?

@@ +34,3 @@
+    GstMetaTransformCopy *copy = data;
+
+    if (!copy->region) {

Does it matter if it's a region copy or not? Should always be copied IMHO (also for other transforms)

@@ +42,3 @@
+        return FALSE;
+
+      dmeta->buffer = dest;

You're not doing anything with the buffer elsewhere, better just get rid of it

@@ +83,3 @@
+  if (g_once_init_enter (&meta_info)) {
+    const GstMetaInfo *mi = gst_meta_register (GST_VANC_META_API_TYPE,
+        "VANCMeta",

Gst prefix

@@ +110,3 @@
+  vmeta->vanc_data = g_array_sized_new (FALSE, FALSE, sizeof (guint16), size);
+
+  g_array_append_vals (vmeta->vanc_data, data, size);

Why an GArray here? Maybe just have a GBytes (preferred) with the data, or two fields (guint16 *, gsize)

::: gst-libs/gst/vanc/gstvancmeta.h
@@ +14,3 @@
+ * @data_id: VANC data ID
+ * @dbn_sdid: VANC dbn sdata ID
+ * @vanc_data: VANC data

GArray of? :)

@@ +21,3 @@
+  GstBuffer       *buffer;
+  gint             data_id;
+  gint             dbn_sdid;

Maybe more descriptive names for these. Also are they signed integers of undefined size, or does the standard define something about them?

@@ +41,3 @@
+
+#define gst_buffer_get_vanc_meta(b)                             \
+  ((VANCMeta*)gst_buffer_get_meta((b), GST_VANC_META_API_TYPE))

Wrong cast here, Gst prefix is missing

Review of attachment 339417 [details] [review]:

Mostly style issues :)

::: sys/decklink/gstdecklinkvanc.cpp
@@ +1,1 @@
+#include <vector>

#ifdef HAVE_CONFIG_H
#include "config.h"
#endif

at the very top, also copyright/license boilerplace

@@ +58,3 @@
+}
+
+static void convertANCToUInt16(std::vector<uint16_t> &data, BMDPixelFormat fmt,

We generally prefer to not use C++ types unless needed. Looks like you just want to use a GstByteWriter inside this function and return the result

@@ +59,3 @@
+
+static void convertANCToUInt16(std::vector<uint16_t> &data, BMDPixelFormat fmt,
+                               BMDDisplayMode dm, uint8_t * const * vanc,

Use GLib types, guint8, guint16

@@ +66,3 @@
+  int row_length = 0;
+
+  if (bmdFormat10BitYUV != fmt)

Only valid for 10 bit? Why?

@@ +96,3 @@
+    case bmdMode2k2398:      row_length = 2048;  break;
+    case bmdMode2k24:        row_length = 2048;  break;
+    case bmdMode2k25:        row_length = 2048;  break;

default case maybe, also you can just do for less duplication
case ...:
case ...:
  row_length = ...;
  break;

@@ +169,3 @@
+        (data[idx + 1] & (~0x3)) != (0x3ff & (~0x3)) ||
+        (data[idx + 2] & (~0x3)) != (0x3ff & (~0x3))) {
+      continue;

Why? (Add a comment)

@@ +189,3 @@
+    if (usableVANC(did, dbn_sdid)) {
+      gst_buffer_add_vanc_meta (buffer, did, dbn_sdid, &data[idx + 6],
+                                data_count, chksum_e);

Here you would add potentially multiple metas to a single buffer (and your getter in gstvancmeta.h only allows to get the first)

@@ +201,3 @@
+}
+
+static int vancTable[][7] = {

const

::: sys/decklink/gstdecklinkvanc.h
@@ +1,2 @@
+#ifndef _Decklink_VANC_H_
+#define _Decklink_VANC_H_

__GST_DECKLINK_VANC_H__ to stay consistent. Also G_BEGIN_DECLS / END_DECLS, and copyright/license boilerplate (also in .c file)

@@ +7,3 @@
+bool ProcessVANC(GstDecklinkVideoSrc *self,
+                 IDeckLinkVideoFrameAncillary *vanc_frame,
+                 GstBuffer * buffer);

Proper namespacing please

::: sys/decklink/gstdecklinkvideosrc.cpp
@@ +186,3 @@
+      g_param_spec_boolean ("enable-vanc", "Enable VANC processing",
+                            "Create VANC metatdata",
+                            0, GParamFlags (G_PARAM_READWRITE)));

Missing parenthesis around GParamFlags for consistency, also you want the other flags as above. And FALSE instead of 0

@@ +199,3 @@
+  gst_element_class_set_static_metadata (element_class, "Decklink VANC Source",
+      "Video/Subtitle", "Decklink VANC Source",
+      "John Poet <jppoet@digital-nirvana.com");

There can only be a single metadata per element class

@@ +679,3 @@
   GST_BUFFER_TIMESTAMP (*buffer) = f->capture_time;
   GST_BUFFER_DURATION (*buffer) = f->capture_duration;
+//  gst_buffer_add_video_time_code_meta (*buffer, f->tc);   Have to remove this or the VANC metadata does not show up in gst_ffmpegvidenc_handle_vanc().   Why?????????????

Keep it in, the problem is going to be elsewhere

@@ +684,3 @@
+    if (f->format != bmdFormat10BitYUV)
+      GST_WARNING_OBJECT (self, "VANC requires 10bit YUV (v210)");
+    else {

{} before the else for symmetry

@@ +694,3 @@
+      }
+      else
+        GST_WARNING_OBJECT (self, "No Ancillary data vailable.");

And {} here

Review of attachment 339418 [details] [review]:

::: ext/libav/gstavvidenc.c
@@ +610,3 @@
+  GstVANCMeta *vmeta = (GstVANCMeta *) meta;
+
+  // Are VANC captions available?

No C99/C++ comments in C code (but this is only debugging/testing code right now, the whole patch, so ignore that)

Unfortunately I can't test any of this here as I have nothing that can feed my decklink cards with VANC :)

My guess for the problem is related to that you can add multiple metas and mixup between them. Check if/where copying of the meta happens in your pipeline, check the pointer values of the metas and track all that to make sure you really look at the same thing.

Created attachment 339838 [details] [review]
Add GstMeta for VANC (from decklinkvideosrc)

Created attachment 339839 [details] [review]
Extract VANC from decklinkvideosrc and add it as metadata

Created attachment 339840 [details] [review]
Basis for extracing VANC metadata and encoding it.

The apparent data corruption was because of a typo in gst_ffmpegvidenc_handle_vanc()

  GstVANCMeta *vmeta = (GstVANCMeta *) meta;

should have been

  GstVANCMeta *vmeta = (GstVANCMeta *) *meta;

In gst_decklink_video_src_create() I discovered that moving 

  gst_buffer_add_video_time_code_meta (*buffer, f->tc);

down below the routine which add GstVANCMeta fixes the problem with the VANC meta now showing up at the receiving end.  Does this mean that adding the time_code_meta needs to be done last?

I have update all three patches with new versions.  These mostly address your concerns.  Your remaining comments mostly have to do with commenting variables and such.  I will try to address those, but I have to admit that most of the low-level VANC code is not mine, and I am adapting it from work by ex-coworkers.  I will see if I can figure out why they did some of the stuff they did.

Created attachment 339845 [details] [review]
Add GstMeta for VANC (from decklinkvideosrc)

Created attachment 339846 [details] [review]
Basis for extracing VANC metadata and encoding it.

Created attachment 339847 [details] [review]
Extract VANC from decklinkvideosrc and add it as metadata

Review of attachment 339845 [details] [review]:

Now only cosmetical bits left :) And documentation

::: gst-libs/gst/vanc/Makefile.am
@@ +21,3 @@
+	$(GST_PLUGINS_BASE_CFLAGS) \
+	-DGST_USE_UNSTABLE_API \
+	$(GST_CXXFLAGS)

No CXXFLAGS, this is plain C

::: gst-libs/gst/vanc/gstvancmeta.c
@@ +77,3 @@
+
+  } else {
+    GST_WARNING ("gst_vanc_meta_transform type not supported");

I think you should do the same for all transforms, there is no possible transform that would make the vanc metadata invalid. It's like a separate stream, right?

@@ +96,3 @@
+gst_vanc_meta_get_info (void)
+{
+  static const GstMetaInfo *meta_info = NULL;

Not const

@@ +112,3 @@
+GstVANCMeta *
+gst_buffer_add_vanc_meta (GstBuffer * buffer,
+    gint did, gint dbn_sdid, guint16 * data, gsize size, guint16 checksum)

data_id vs did

@@ +127,3 @@
+  vmeta->vanc_data = g_array_sized_new (FALSE, FALSE, sizeof (guint16), size);
+
+  g_array_append_vals (vmeta->vanc_data, data, size);

GBytes, or storage as (guint16 *, gsize) pair. GArray is too much overhead for data that is not going to be modified in 99.999% of the cases

::: gst-libs/gst/vanc/gstvancmeta.h
@@ +45,3 @@
+  gint             dbn_sdid;  // Secondary Data ID (type 2)
+  guint16          checksum;  // Checksum of raw ancillary data
+  GArray          *vanc_data; // Raw ancillary data

No C99/C++ comments (// vs /* */)

A GBytes instead of a GArray still seems nicer for the data

@@ +55,3 @@
+
+GstVANCMeta * gst_buffer_add_vanc_meta (GstBuffer * buffer,
+                                        gint did, gint dbn_sdid,

It's called data_id above, and did only here

::: pkgconfig/gstreamer-plugins-bad-uninstalled.pc.in
@@ +11,3 @@
 Version: @VERSION@
 Requires: gstreamer-@GST_API_VERSION@
+Libs: -L@abs_top_builddir@/gst-libs/gst/audio/.libs -L@abs_top_builddir@/gst-libs/gst/basecamerabinsrc/.libs -L@abs_top_builddir@/gst-libs/gst/codecparsers/.libs -L@abs_top_builddir@/gst-libs/gst/gl/.libs -L@abs_top_builddir@/gst-libs/gst/insertbin/.libs -L@abs_top_builddir@/gst-libs/gst/interfaces/.libs -L@abs_top_builddir@/gst-libs/gst/mpegts/.libs -L@abs_top_builddir@/gst-libs/gst/player/.libs -L@abs_top_builddir@/gst-libs/gst/signalprocessor/.libs -L@abs_top_builddir@/gst-libs/gst/video/.libs -L@abs_top_builddir@/gst-libs/gst/vanc/.libs -L@abs_top_builddir@/gst-libs/gst/wayland/.libs

Should also add a (two) pkg-config files for the vanc library alone

Review of attachment 339846 [details] [review]:

debug-only patch at this point, so let's make sure we don't accidentially commit it :)

Review of attachment 339847 [details] [review]:

Basically same comments as last time in this one

::: sys/decklink/gstdecklinkvanc.cpp
@@ +265,3 @@
+{
+  guint8 *vanc[50];
+  const int vanc_cnt = sizeof(vanc) / sizeof(guint8 *);

G_N_ELEMENTS(vanc)

@@ +294,3 @@
+
+  BMDPixelFormat fmt = vanc_frame->GetPixelFormat();
+  std::vector<guint16> tmp_data;

Don't use a C++ vector, basically same comments as last time here :)

::: sys/decklink/gstdecklinkvideosrc.cpp
@@ +707,1 @@
   gst_buffer_add_video_time_code_meta (*buffer, f->tc);

How many vanc metas are added? Adding a meta to a buffer is literally just appending an item to a linked list. In any order, none of the things should disappear.

Can you check in the gst_buffer_add_meta() code what goes wrong there?

(In reply to Sebastian Dröge (slomo) from comment #21)

> @@ +96,3 @@
> +gst_vanc_meta_get_info (void)
> +{
> +  static const GstMetaInfo *meta_info = NULL;
> 
> Not const

Ignore this part

(In reply to Sebastian Dröge (slomo) from comment #21)
> Review of attachment 339845 [details] [review] [review]:
> 
> ::: gst-libs/gst/vanc/gstvancmeta.c
> @@ +77,3 @@
> +
> +  } else {
> +    GST_WARNING ("gst_vanc_meta_transform type not supported");
> 
> I think you should do the same for all transforms, there is no possible
> transform that would make the vanc metadata invalid. It's like a separate
> stream, right?

If I take out support for the COPY transform, nothing shows up at the receiving end.  Or, am I misunderstanding what you are asking here?

> ::: pkgconfig/gstreamer-plugins-bad-uninstalled.pc.in
> @@ +11,3 @@
>  Version: @VERSION@
>  Requires: gstreamer-@GST_API_VERSION@
> +Libs: -L@abs_top_builddir@/gst-libs/gst/audio/.libs
> -L@abs_top_builddir@/gst-libs/gst/basecamerabinsrc/.libs
> -L@abs_top_builddir@/gst-libs/gst/codecparsers/.libs
> -L@abs_top_builddir@/gst-libs/gst/gl/.libs
> -L@abs_top_builddir@/gst-libs/gst/insertbin/.libs
> -L@abs_top_builddir@/gst-libs/gst/interfaces/.libs
> -L@abs_top_builddir@/gst-libs/gst/mpegts/.libs
> -L@abs_top_builddir@/gst-libs/gst/player/.libs
> -L@abs_top_builddir@/gst-libs/gst/signalprocessor/.libs
> -L@abs_top_builddir@/gst-libs/gst/video/.libs
> -L@abs_top_builddir@/gst-libs/gst/vanc/.libs
> -L@abs_top_builddir@/gst-libs/gst/wayland/.libs
> 
> Should also add a (two) pkg-config files for the vanc library alone

You want me to create:
pkgconfig/gstreamer-vanc.pc
pkgconfig/gstreamer-vanc-1.0-uninstalled.pc
?  If so, Should it be libvanc or just vanc?

(In reply to John Poet from comment #25)
> (In reply to Sebastian Dröge (slomo) from comment #21)
> > Review of attachment 339845 [details] [review] [review] [review]:
> > 
> > ::: gst-libs/gst/vanc/gstvancmeta.c
> > @@ +77,3 @@
> > +
> > +  } else {
> > +    GST_WARNING ("gst_vanc_meta_transform type not supported");
> > 
> > I think you should do the same for all transforms, there is no possible
> > transform that would make the vanc metadata invalid. It's like a separate
> > stream, right?
> 
> If I take out support for the COPY transform, nothing shows up at the
> receiving end.  Or, am I misunderstanding what you are asking here?

I meant doing what you do for COPY right now for all transforms.

> > ::: pkgconfig/gstreamer-plugins-bad-uninstalled.pc.in
> > @@ +11,3 @@
> >  Version: @VERSION@
> >  Requires: gstreamer-@GST_API_VERSION@
> > +Libs: -L@abs_top_builddir@/gst-libs/gst/audio/.libs
> > -L@abs_top_builddir@/gst-libs/gst/basecamerabinsrc/.libs
> > -L@abs_top_builddir@/gst-libs/gst/codecparsers/.libs
> > -L@abs_top_builddir@/gst-libs/gst/gl/.libs
> > -L@abs_top_builddir@/gst-libs/gst/insertbin/.libs
> > -L@abs_top_builddir@/gst-libs/gst/interfaces/.libs
> > -L@abs_top_builddir@/gst-libs/gst/mpegts/.libs
> > -L@abs_top_builddir@/gst-libs/gst/player/.libs
> > -L@abs_top_builddir@/gst-libs/gst/signalprocessor/.libs
> > -L@abs_top_builddir@/gst-libs/gst/video/.libs
> > -L@abs_top_builddir@/gst-libs/gst/vanc/.libs
> > -L@abs_top_builddir@/gst-libs/gst/wayland/.libs
> > 
> > Should also add a (two) pkg-config files for the vanc library alone
> 
> You want me to create:
> pkgconfig/gstreamer-vanc.pc
> pkgconfig/gstreamer-vanc-1.0-uninstalled.pc
> ?  If so, Should it be libvanc or just vanc?

Yes (the unversioned .pc.in are the files you add, the others are generated). Should be gstreamer-vanc

(In reply to Sebastian Dröge (slomo) from comment #23)
> Review of attachment 339847 [details] [review] [review]:
> ::: sys/decklink/gstdecklinkvideosrc.cpp
> @@ +707,1 @@
>    w (*buffer, f->tc);
> 
> How many vanc metas are added? Adding a meta to a buffer is literally just
> appending an item to a linked list. In any order, none of the things should
> disappear.


In my tests, I am seeing between 3 and 5 per frame.  All of them show up at the receiving end when gst_buffer_add_video_time_code_meta does not get in the way.


> Can you check in the gst_buffer_add_meta() code what goes wrong there?


My gst_buffer_foreach_meta() function was returning FALSE if it was not a GstVANCMeta.  This caused the foreach function to abort processing.

Can't we just chuck the meta into gstvideo somewhere? The V is for Video right? And there are no external deps?

Also thought GBytes for data, or GstBuffer, but not GArray.

(video in -base or in -bad)

I have a lot of "helper" functions not included in these patches (yet). I had planned on these functions being part of libgstvanc.  If libgstvanc is removed, where would you like them?

These functions facilitate parsing captions from the ancillary data, so they can be injected into the ffmpeg user data.

Technically, I have a "helper" class which is currently written in C++.  I am getting a strong hint that you are going to make me port it to 1990 era C.  I am not looking forward to that, but I understand your reasons.  As I am sure you have figured out, before this project I had not written C code in about 25 years.

Created attachment 339982 [details] [review]
Add GstMeta for VANC (from decklinkvideosrc)

Created attachment 339983 [details] [review]
Basis for extracing VANC metadata and encoding it.

Created attachment 339984 [details] [review]
Extract VANC from decklinkvideosrc and add it as metadata

You should be happier with the latest version...

Review of attachment 339982 [details] [review]:

Seems good to me, just minor things and need of documentation :)

::: gst-libs/gst/vanc/gstvancmeta.c
@@ +100,3 @@
+GstVANCMeta *
+gst_buffer_add_vanc_meta (GstBuffer * buffer, gint data_id, gint dbn_sdid,
+    const guint16 * data, gsize size, guint16 checksum)

Needs documentation, e.g. to specify that the size is in 16 bit words, not bytes

@@ +104,3 @@
+  GstVANCMeta *vmeta;
+
+  g_return_val_if_fail (GST_IS_BUFFER (buffer), NULL);

g_return_val_if_fail (data != NULL, NULL)
g_return_val_if_fail (size == 0, NULL)

::: gst-libs/gst/vanc/gstvancmeta.h
@@ +55,3 @@
+
+GstVANCMeta * gst_buffer_add_vanc_meta (GstBuffer * buffer,
+                                        gint did, gint dbn_sdid,

"did" while the field in the meta is called "data_id"

@@ +64,3 @@
+#if 0
+#define gst_buffer_get_vanc_meta(b)                             \
+  ((VANCMeta*)gst_buffer_get_meta((b), GST_VANC_META_API_TYPE))

Why commented out? Might make sense to add API that gets the number of VANC metas, and then allows getting them by index. Or some kind of iterator API that only gives VANCs

Review of attachment 339984 [details] [review]:

::: sys/decklink/gstdecklinkvanc.cpp
@@ +171,3 @@
+      guint32 w1 = ((guint32*) ptr) [1];
+      guint32 w2 = ((guint32*) ptr) [2];
+      guint32 w3 = ((guint32*) ptr) [3];

Might want to use a GstByteReader for going over the samples. For bounds checking, keeping track of position, etc.

Also the above code is endianness dependent. It will behave different on little endian and big endian systems. You probably want something like gst_byte_reader_get_uint32_le() here, or GST_READ_UINT32_LE() or similar (I assume your code is tested to work on little endian systems)

@@ +223,3 @@
+
+  for (gsize idx = 0; idx < data_size - min_adp_size; ++idx) {
+    // Find start of packet

Might want to use the GstByteReader API for reading over all the data, instead of keeping track of position and doing bounds checking yourself

@@ +305,3 @@
+
+  BMDPixelFormat fmt = vanc_frame->GetPixelFormat();
+  GstByteWriter *writer = NULL;

Can be stack allocated, and you could initialize it with some sensible size already

::: sys/decklink/gstdecklinkvanc.h
@@ +21,3 @@
+
+#ifndef _Decklink_VANC_H_
+#define _Decklink_VANC_H_

__GST_DECKLINK_VANC_H__

@@ +28,3 @@
+bool gst_processVANC(GstDecklinkVideoSrc *self,
+                     IDeckLinkVideoFrameAncillary *vanc_frame,
+                     GstBuffer * buffer);

gst_decklink_process_vanc(), gst_decklink_vanc_init() I guess. Also G_GNUC_INTERNAL

::: sys/decklink/gstdecklinkvideosrc.cpp
@@ +198,3 @@
       "Sebastian Dröge <sebastian@centricular.com>");
 
+  gst_element_class_set_static_metadata (element_class, "Decklink VANC Source",

Merge the names with the above call to set_static_metadata(). Only one per element is allowed, a second call overwrites the old ones

@@ +201,3 @@
+      "Video/Subtitle", "Decklink VANC Source",
+      "Gavin Hurlbut <gavin@digital-nirvana.com>, "
+      "John Poet <jppoet@digital-nirvana.com");

Only add people here who actually wrote the thing :)

Beyond the decklink API only supporting VANC, is there any reason this effort shouldn't be more general and also support horizontal metadata? For example, I use horizontal metadata with deltacast cards.

(In reply to Tim-Philipp Müller from comment #28)
> Can't we just chuck the meta into gstvideo somewhere? The V is for Video
> right? And there are no external deps?

I finally have some time to work on this again.

For this, I assume you mean to add
 struct _GstVANCMeta {}
etc, to gst-plugins-base/gst-libs/gst/video/gstvideometa.h
instead of creating a new library in 
gst-plugins-bad/gst-libs/gst/vanc

I can do that, if that is what you want.  I initially created it in gst-plugins-bad because I assumed it would need to be "promoted" to gst-plugins-base after some period of time.

The library part is covered by https://bugzilla.gnome.org/show_bug.cgi?id=794901 now, and this can be used by the Decklink source then.

Created attachment 370484 [details] [review]
decklinkvideosrc: Add support for extracting Closed Caption

If the "output-cc" property is set to TRUE and there is CC present
in the VBI Ancillary Data, they will be extracted and set on the
outgoing buffer as GstVideoCaptionMeta.

Only CDP packets are supported.

Review of attachment 370484 [details] [review]:

::: sys/decklink/gstdecklink.cpp
@@ +236,3 @@
+#define PAL 12, 11, true, "bt601", FALSE
+#define HD 1, 1, true, "bt709", TRUE
+#define UHD 1, 1, true, "bt2020", TRUE

For NTSC and PAL there's nothing?

::: sys/decklink/gstdecklinkvideosrc.cpp
@@ +790,3 @@
+  GstVideoFormat videoformat;
+  // FIXME : Move to a central location
+  GstVideoVBIParser *parser = NULL;

Yes storing that in the instance would seem better

@@ +849,3 @@
+  after = gst_util_get_timestamp ();
+  GST_LOG ("Processing ANC took %" GST_TIME_FORMAT,
+      GST_TIME_ARGS (after - before));

Should this stay or is it debug code?

Created attachment 370694 [details] [review]
decklinkvideosrc: Add support for extracting Closed Caption

If the "output-cc" property is set to TRUE and there is CC present
in the VBI Ancillary Data, they will be extracted and set on the
outgoing buffer as GstVideoCaptionMeta.

Only CDP packets are supported.

That last patch handles all comments mentioned in review

Comment on attachment 370694 [details] [review]
decklinkvideosrc: Add support for extracting Closed Caption

Attachment 370694 [details] pushed as 44390d9 - decklinkvideosrc: Add support for extracting Closed Caption