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

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

::: sys/decklink/gstdecklink.cpp
@@ +223,3 @@
+  {bmdFormat10BitYUV, 10, GST_VIDEO_FORMAT_v210},
+  {bmdFormat8BitARGB, 8, GST_VIDEO_FORMAT_ARGB},
+  {bmdFormat8BitBGRA, 8, GST_VIDEO_FORMAT_BGRA},

I think these values are wrong for bpp? UYVY should be 2 bytes for example,
ARGB 4, v210 should also be 4. See usage in decklinkvideosink.

You might also be able to use the pixel stride of the first component (Y / R)
here, that should be the same value

@@ +359,3 @@
+  guint i;
+
+  for (i = 0; i < G_N_ELEMENTS (formats); i++) {

You can start counting from 1

@@ +364,3 @@
+  }
+  GST_ERROR ("Video format %d not supported", f);
+  return GST_DECKLINK_VIDEO_FORMAT_AUTO;

Isn't this more a g_assert_not_reached()?

@@ +465,3 @@
+
+  caps = gst_caps_new_empty ();
+  for (i = 0; i < G_N_ELEMENTS (formats); i++)

You can start counting from 1

@@ +472,3 @@
+  return caps;
+
+  return caps;

return caps twice

::: sys/decklink/gstdecklinkvideosrc.cpp
@@ +567,3 @@
+      GST_ELEMENT_ERROR (self, CORE, NEGOTIATION,
+          ("Invalid mode in captured frame"),
+          ("Mode set to %d but captured %d", self->caps_mode, f->mode));

This should probably return here with a GST_FLOW_NOT_NEGOTIATED after unlocking
the mutex. And the error message should probably also happen after unlocking
the mutex already

@@ +579,3 @@
+      GST_ELEMENT_ERROR (self, CORE, NEGOTIATION,
+          ("Invalid pixel format in captured frame"),
+          ("Format set to %d but captured %d", self->caps_format, f->format));

And this

@@ +813,3 @@
+          self->video_format != GST_DECKLINK_VIDEO_FORMAT_AUTO) {
+        GST_WARNING_OBJECT (self, "Warning: mode=auto and format!=auto may \
+                            not work");

Will mode!=auto && format==auto work as expected? I would also just make it an
error and return GST_STATE_CHANGE_FAILURE here then

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