[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 #339417|none                        |needs-work
             status|                            |

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

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 at 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

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