GNOME Bugzilla – Bug 742878
decklink: Add initial support for 10bit YUV modes
Last modified: 2016-10-31 14:20:56 UTC
See summary. This does not completely work yet because set_caps() in the source is called *after* PAUSED->PLAYING (but we need to have the video stream enabled by then). Also it's not clear if we need to probe for 10 bit support or if it is always supported. Probably we have to probe.
Created attachment 294458 [details] [review] decklink: Add initial support for 10bit YUV modes
(In reply to Sebastian Dröge (slomo) from comment #0) > Also it's not clear if we need to probe for 10 bit support > or if it is always supported. Probably we have to probe. According all the technical specifications of DeckLink cards [0], it supports 10 bit. [0] https://www.blackmagicdesign.com/products/decklink/techspecs/W-DLK-01
The question is also if it automatically converts, or if the 8bit/10bit setting must be the same as for the capture source or the output.
Same question for 16/32 bit audio btw
(In reply to Sebastian Dröge (slomo) from comment #3) > The question is also if it automatically converts, or if the 8bit/10bit > setting must be the same as for the capture source or the output. I will investigate it. Anyway, I'm trying to apply the patch to the latest version of gstreamer git. In the present state what the expected behavior of this patch?
Created attachment 303042 [details] [review] Just updating the slomo patch to the latest git version Still doesn't completely work.
hi, any idea what might still be missing in the patch?
We need to negotiate with the hardware if a 10 bit mode is actually possible or not, or make it a property similar to the modes.
As far as I know, there is no way to query the hardware whether 10 bit modes are supported. I suspect that 10 bit is always supported, since SDI signals always carry a 10-bit signal. Not sure about HDMI, though. i think it would make most sense to have an additional property to set the requested bit-depth.
Should do the same for the 16/32 bit PCM audio setting
Created attachment 326080 [details] [review] 0001-decklink-Add-initial-10bit-support-for-YUV-modes.patch Rewrote this patch on latest git. But now the bit depth is determined based on a configurable property rather than caps.
Created attachment 326256 [details] [review] 0001-decklink-Add-initial-10bit-support-for-YUV-modes.patch Updated version. Please review and feedback.
Review of attachment 326256 [details] [review]: ::: sys/decklink/gstdecklink.h @@ +121,3 @@ + GST_DECKLINK_VIDEO_10BIT_RGBXLE, /* bmdFormat10BitRGBXLE */ + GST_DECKLINK_VIDEO_10BIT_RGBX, /* bmdFormat10BitRGBX */ +} GstDecklinkVideoFormatType; GstDecklinkVideoFormat and GST_DECKLINK_VIDEO_FORMAT_AUTO, etc? Seems more consistent ::: sys/decklink/gstdecklinkvideosink.cpp @@ +208,3 @@ + g_object_class_install_property (gobject_class, PROP_VIDEO_FORMAT, + g_param_spec_enum ("video-format-type", "Video format type", And then here also video-format only @@ +481,3 @@ + switch (self->video_format) { + case GST_DECKLINK_VIDEO_AUTO: For AUTO in the sink it might make sense to allow all formats in get_caps() and then select the appropriate format based on the caps in set_caps(). As an additional patch I guess, the same applies for the existing mode property. @@ +488,3 @@ + bpp = 4; + break; + default: It would probably be good if a GST_ELEMENT_ERROR() would happen here... but also in set_property() already a GST_WARNING or so about selecting an unsupported format. Handling the RGB formats that are handled by the source already should be easy though ::: sys/decklink/gstdecklinkvideosrc.cpp @@ +387,3 @@ + mode_caps = gst_decklink_mode_get_caps (mode, format); + + if (gst_caps_is_empty (mode_caps)) { Wouldn't this always return non-empty caps? @@ +569,2 @@ caps = gst_decklink_mode_get_caps (f->mode, f->format); + if (gst_caps_is_empty (caps)) { And here. Isn't the actual problem you want to check here the case when mode!=AUTO && f->mode!=mode (and the same with the format)? That is, we captured a frame different to what is configured on the element
Created attachment 326325 [details] [review] 0001-decklink-Add-initial-10bit-support-for-YUV-modes.patch
8-bit YUV has hardcoded tentacles everywhere. Now I found one in gst_decklink_find_mode_for_caps which makes my previous patch obsolete. A new one is in the oven right now, will post it when I find it to be properly baked.
Created attachment 326338 [details] [review] 0001-decklink-Add-initial-10bit-support-for-YUV-modes.patch New version uploaded, please review again.
Review of attachment 326338 [details] [review]: Maybe gst-indent would've been better as a separate commit :) Otherwise almost good to go, just some simple things ::: sys/decklink/gstdecklink.cpp @@ +130,3 @@ + {GST_DECKLINK_VIDEO_FORMAT_10BIT_RGBX, "10bit-rgbx", "bmdFormat10BitRGBX"}, + {0, NULL, NULL} + }; I think it would be good to comment out the ones here that we don't support yet. They can stay in the enum, but at least they won't show up in gst-inspect for now then ::: sys/decklink/gstdecklinkvideosink.cpp @@ +208,3 @@ + g_object_class_install_property (gobject_class, PROP_VIDEO_FORMAT, + g_param_spec_enum ("video-format-type", "Video format type", video-format, not video-format-type ::: sys/decklink/gstdecklinkvideosrc.cpp @@ +164,3 @@ + g_object_class_install_property (gobject_class, PROP_VIDEO_FORMAT, + g_param_spec_enum ("video-format-type", "Video format type", video-format, not video-format-type
Created attachment 326453 [details] [review] 0001-decklink-Add-initial-10bit-support-for-YUV-modes.patch Thanks, fixed that (and one or two more small issues that I found) and attached the new version.
Created attachment 326454 [details] [review] 0002-decklink-Fix-swapped-name-and-nick-in-GEnumValues.patch I also realized that all GEnumValues in gstdecklink.cpp had the name and nick swapped, so I fixed that too as a separate commit on top of the previous one.
Generally, from my tests, mode=auto is enforcing one specific pixel format, so it goes best with pixel-format=auto, otherwise good luck figuring out what the source is sending. On the other hand, if you enable a specific mode, you can also change the pixel format (but mode=non-auto and pixel-format=auto will still work).
Review of attachment 326453 [details] [review]: 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
Review of attachment 326453 [details] [review]: ::: sys/decklink/gstdecklinkvideosrc.cpp @@ +362,3 @@ + format = self->caps_format; + ret = self->input->input->EnableVideoInput (mode->mode, format, flags); Here we should probably complain if only one of the two properties is auto... because we will get whatever the driver decides then for the other property, not what was set via property. If I understand the decklink API correctly. Should at least be a warning, if not an error.
Created attachment 326954 [details] [review] 0001-decklink-Add-initial-10bit-support-for-YUV-modes.patch
Created attachment 326955 [details] [review] 0002-decklink-Fix-swapped-name-and-nick-in-GEnumValues.patch Thanks for the feedback, new patch versions attached. FYI, there was no bug in video sink, as the video format is kept in self->info and we take it into account in prepare - added comment.
Review of attachment 326954 [details] [review]: ::: 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
Review of attachment 326954 [details] [review]: ::: sys/decklink/gstdecklink.cpp @@ +229,3 @@ + {bmdFormat12BitRGBLE, 12}, + {bmdFormat10BitRGBXLE, 10}, + {bmdFormat10BitRGBX */ The others should get a FIXME for the bpp, needs research
Created attachment 327222 [details] [review] 0001-decklink-Add-initial-10bit-support-for-YUV-modes.patch
Created attachment 327223 [details] [review] 0002-decklink-Fix-swapped-name-and-nick-in-GEnumValues.patch
(In reply to Sebastian Dröge (slomo) from comment #25) > Review of attachment 326954 [details] [review] [review]: > > ::: sys/decklink/gstdecklinkvideosrc.cpp > @@ +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 It might or might not. It will work if you accidentally picked the correct format. Worst case, you get a black frame. I prefer to keep it, because nothing explodes (black frame is also the result of choosing the wrong mode, for instance), and under specific circumstances it might work. Otherwise, I've corrected the patch and uploaded the new version. Please feed back.
commit a4ae4494345361bf91e2981ac05a53a53210dc1e Author: Vivia Nikolaidou <vivia@ahiru.eu> Date: Wed Apr 20 15:11:44 2016 +0300 decklink: Fix swapped name and nick in GEnumValues https://bugzilla.gnome.org/show_bug.cgi?id=742878 commit 832764d2fd9480e5512a44e382b2b8f1c1fbfe17 Author: Vivia Nikolaidou <vivia@ahiru.eu> Date: Thu Apr 14 18:26:33 2016 +0300 decklink: Add initial 10bit support for YUV modes https://bugzilla.gnome.org/show_bug.cgi?id=742878