[Bug 773863] decklinkvideosrc: add VANC parsing

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=773863

Sebastian Dröge (slomo) <slomo at coaxion.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #339416|none                        |needs-work
             status|                            |

--- Comment #10 from Sebastian Dröge (slomo) <slomo at coaxion.net> ---
Review of attachment 339416:
 --> (https://bugzilla.gnome.org/review?bug=773863&attachment=339416)

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

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.