GNOME Bugzilla – Bug 773660
decklinkvideosink: Add support for hardware keying
Last modified: 2017-08-15 08:10:45 UTC
Since declkinkvideosink can now accept pixel formats with alpha, it would be good if the Decklink card's internal keyer could be enabled. I have a patch which does this, but I cannot work out how to get the new property to display correctly. Ideally, it would show the 3 possible modes (Off, Internal and External) selected by an integer. The selection does work; it just does not display nicely. Can I submit the patch as it is or could anyone advise how to correctly set the properties? I tried using the current sink properties as a guide without much success.
Please do, we will help you with the last detail.
Created attachment 338812 [details] Archive of 4 patch files enabling the DeckLink hardware keyer. Diff reports for the 4 files modified to enable the DeckLink hardware keyer.
The new property does not display correctly - I would appreciate help getting this right.
Comment on attachment 338812 [details] Archive of 4 patch files enabling the DeckLink hardware keyer. Can you attach the patches in "git format-patch" format as plaintext here, each patch as a separate attachment? git bz can help with that
Created attachment 338879 [details] [review] Decklink keying patch 1
Created attachment 338880 [details] [review] Decklink keying patch 2
Created attachment 338881 [details] [review] Decklink keying patch 3
Created attachment 338882 [details] [review] Decklink keying patch 4
Created attachment 357322 [details] [review] Patch to enable the Decklink hardware keyer
I have updated the Decklink hardware keying patch for review.
Review of attachment 357322 [details] [review]: Please attach it in "git format-patch" format, or use git-bz :) And include a proper commit message Apart from that and some minor things this looks good, thanks :) ::: sys/decklink/gstdecklink.cpp @@ +322,3 @@ +{ + BMDKeyerMode format; + GstDecklinkKeyerMode gstformat; Not really "format", more like "mode" and "gstmode" maybe @@ +1236,3 @@ + { + GST_WARNING + ("selected device does not have a keyer interface: 0x%08x", ret); This should probably only be a warning if keying is actually enabled later ::: sys/decklink/gstdecklinkvideosink.cpp @@ +417,3 @@ + { + self->output->keyer->Enable (true); + self->output->keyer->SetLevel (255); Maybe the level should also get a property?
Created attachment 357375 [details] [review] Revised patch to enable the Decklink hardware keyer I'm not sure what to do regarding the keyer interface missing warning. I don't have any hardware without a keyer for test.
Review of attachment 357375 [details] [review]: ::: sys/decklink/gstdecklink.cpp @@ +1236,3 @@ + { + GST_WARNING + ("selected device does not have a keyer interface: 0x%08x", ret); Instead of warning here ... ::: sys/decklink/gstdecklinkvideosink.cpp @@ +421,3 @@ + /* enable or disable keyer */ + if (self->keyer_mode == bmdKeyerModeOff) + self->output->keyer->Disable (); ... you check if ->keyer == NULL and warn here if mode!=off This would currently crash instead
Created attachment 357582 [details] [review] Further revised patch to enable the Decklink hardware keyer. This patch now checks the keyer interface exists before using it.
Review of attachment 357582 [details] [review]: Thanks! ::: sys/decklink/gstdecklinkvideosink.cpp @@ +436,3 @@ + else if (self->keyer_mode!= bmdKeyerModeOff) + GST_WARNING_OBJECT (self, "Failed to set keyer to mode %d", + self->keyer_mode); Some code style problems here, but I'll fix them before merging
commit 0cd1bf13e8b7fb284dce97256a75e6162f42ecf6 (HEAD -> master) Author: Dave Johnstone <dave@digits.tv> Date: Tue Aug 15 10:27:03 2017 +0930 decklinkvideosink: Add support for Decklink hardware keying Add two properties (keyer-mode and keyer-level) to control the built-in hardware keyer of Decklink cards. https://bugzilla.gnome.org/show_bug.cgi?id=773660