[Bug 742878] decklink: Add initial support for 10bit YUV modes

[GStreamer (GNOME Bugzilla)]

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

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

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

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

Might be useful to run gst-indent before the patch, but I'll do that. Generally
looks good, just some cleanup possible

::: sys/decklink/gstdecklink.cpp
@@ +122,3 @@
+    {GST_DECKLINK_VIDEO_FORMAT_8BIT_ARGB, "8bit-argb", "bmdFormat8BitARGB"},
+    {GST_DECKLINK_VIDEO_FORMAT_8BIT_BGRA, "8bit-bgra", "bmdFormat8BitBGRA"},
+    /*

Maybe add a comment here saying why they are commented out

@@ +328,3 @@
+  switch (t) {
+    case GST_DECKLINK_VIDEO_FORMAT_AUTO:
+      f = bmdFormat8BitYUV;

Maybe a comment here why we return 8 bit YUV for auto, also a table might be
easier to read and maintain than a big switch-case block

@@ +477,3 @@
+
+  caps = gst_caps_new_empty ();
+  /* FIXME: Loop when we support all the formats */

Why not now already? You could also use the same table you create for
gst_decklink_pixel_format_from_type() here already, if you let the table only
contain the supported ones for now. Which should be enough

static const BMDPixelFormat formats[] = {bmdFormat8BitYUV, ...};
for (i = 0; i < G_N_ELEMENTS(formats); i++)
  caps = gst_caps_merge_structure(caps, ... formats[i]);

::: sys/decklink/gstdecklinkvideosink.cpp
@@ +320,3 @@
       GStreamerVideoOutputCallback (self));

   if (self->mode == GST_DECKLINK_MODE_AUTO) {

If mode!=AUTO && format==AUTO you would fail here. getcaps() is returning all
formats for the mode then, and some will be chosen by upstream... but you won't
parse the format from the caps

@@ +322,3 @@
   if (self->mode == GST_DECKLINK_MODE_AUTO) {
+    BMDPixelFormat f;
+    mode = gst_decklink_find_mode_and_format_for_caps (caps, &f);

Is find_mode_for_caps() (without the format) still used somewhere after this?

@@ +510,3 @@
+  switch (self->info.finfo->format) {
+    case GST_VIDEO_FORMAT_UYVY:
+      format = bmdFormat8BitYUV;

This mapping could probably be moved to gstdecklink.cpp and also used by the
source, and that file already contains something similar.

::: sys/decklink/gstdecklinkvideosrc.cpp
@@ +213,3 @@
       self->mode = (GstDecklinkModeEnum) g_value_get_enum (value);
+      if (self->mode != GST_DECKLINK_MODE_AUTO)
+        self->caps_mode = self->mode;

It's confusing that these are now set as part of a property. I think the way it
was before was so that after the element is restarted, it also forgets about
the format it detected and starts again from the property which might be auto.

But I think the code does the right thing now, just that the naming is
confusing of the variable. Or maybe a comment here saying why we set a caps
thing as part of set_property() and why that is ok

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