| Message ID | 20190501164550.29777-1-cus@passwd.hu |
|---|---|
| State | Accepted |
| Commit | 15b8f36be17c45f17b662090ee2039c93eff9635 |
| Headers | show |
On Wed, 1 May 2019, Marton Balint wrote: > Apparently in the new SDK one cannot query if VANC output is supported, so we > will fall back to non-VANC output if enabling the video output with VANC fails. > > Fixes ticket #7867. Applied. Regards, Marton
Hello Marton, > On May 5, 2019, at 2:28 PM, Marton Balint <cus@passwd.hu> wrote: > > > On Wed, 1 May 2019, Marton Balint wrote: > >> Apparently in the new SDK one cannot query if VANC output is supported, so we >> will fall back to non-VANC output if enabling the video output with VANC fails. >> >> Fixes ticket #7867. > > Applied. I know it’s a bit late for a review given I’m only seeing this after it’s been applied. However, has it been confirmed that the new logic works with decklink SDKs older than version 11? If not, then the old logic probably needs to stay and the new logic probably needs to be #ifdef’d based on the SDK version. If we’re talking about increasing the minimum SDK version for ffmpeg to build against, that’s also an option (although given version 11 is very new I wouldn’t particularly be in favor of that). Also, have you ascertained that the change in question works with more than one model of card? What card did you encounter this issue with, and what other cards did you test with? I’m just concerned about the possibility that you committed a change to address an issue with some particular card, and we don’t know what the effects are on other cards. Regards, Devin --- Devin Heitmueller - LTN Global Communications dheitmueller@ltnglobal.com
On Sun, 5 May 2019, Devin Heitmueller wrote: > Hello Marton, > >> On May 5, 2019, at 2:28 PM, Marton Balint <cus@passwd.hu> wrote: >> >> >> On Wed, 1 May 2019, Marton Balint wrote: >> >>> Apparently in the new SDK one cannot query if VANC output is supported, so we >>> will fall back to non-VANC output if enabling the video output with VANC fails. >>> >>> Fixes ticket #7867. >> >> Applied. > > I know it’s a bit late for a review given I’m only seeing this after > it’s been applied. However, has it been confirmed that the new logic > works with decklink SDKs older than version 11? If not, then the old > logic probably needs to stay and the new logic probably needs to be > #ifdef’d based on the SDK version. If we’re talking about increasing > the minimum SDK version for ffmpeg to build against, that’s also an > option (although given version 11 is very new I wouldn’t particularly be > in favor of that). There were two code hunks. The one which called DoesSupportVideoMode was an already ifdefed part to SDK version 11 only. The other hunk (the fallback to non-VANC mode if enabling output with VANC fails) seemed useful/harmless for all SDK versions, so I saw no reason to complicate it with ifdefry. > Also, have you ascertained that the change in question works with more > than one model of card? What card did you encounter this issue with, > and what other cards did you test with? I’m just concerned about the > possibility that you committed a change to address an issue with some > particular card, and we don’t know what the effects are on other cards. As far as I see the added code can't cause regressions for any HW/SDK combo. If you see something in it where it can, then please let me know. Thanks, Marton
diff --git a/libavdevice/decklink_common.cpp b/libavdevice/decklink_common.cpp index c3a1d5588c..659aa9be3f 100644 --- a/libavdevice/decklink_common.cpp +++ b/libavdevice/decklink_common.cpp @@ -273,21 +273,15 @@ int ff_decklink_set_format(AVFormatContext *avctx, #if BLACKMAGIC_DECKLINK_API_VERSION >= 0x0b000000 if (direction == DIRECTION_IN) { if (ctx->dli->DoesSupportVideoMode(ctx->video_input, ctx->bmd_mode, (BMDPixelFormat) cctx->raw_format, - bmdVideoInputFlagDefault, + bmdSupportedVideoModeDefault, &support) != S_OK) return -1; } else { BMDDisplayMode actualMode = ctx->bmd_mode; - if (!ctx->supports_vanc || ctx->dlo->DoesSupportVideoMode(bmdVideoConnectionUnspecified, ctx->bmd_mode, ctx->raw_format, - bmdVideoOutputVANC, - &actualMode, &support) != S_OK || !support || ctx->bmd_mode != actualMode) { - /* Try without VANC enabled */ - if (ctx->dlo->DoesSupportVideoMode(bmdVideoConnectionUnspecified, ctx->bmd_mode, ctx->raw_format, - bmdVideoOutputFlagDefault, - &actualMode, &support) != S_OK || !support || ctx->bmd_mode != actualMode) { - return -1; - } - ctx->supports_vanc = 0; + if (ctx->dlo->DoesSupportVideoMode(bmdVideoConnectionUnspecified, ctx->bmd_mode, ctx->raw_format, + bmdSupportedVideoModeDefault, + &actualMode, &support) != S_OK || !support || ctx->bmd_mode != actualMode) { + return -1; } } diff --git a/libavdevice/decklink_enc.cpp b/libavdevice/decklink_enc.cpp index 8b621d0054..04b06aee3a 100644 --- a/libavdevice/decklink_enc.cpp +++ b/libavdevice/decklink_enc.cpp @@ -197,8 +197,11 @@ static int decklink_setup_video(AVFormatContext *avctx, AVStream *st) " Check available formats with -list_formats 1.\n"); return -1; } - if (ctx->dlo->EnableVideoOutput(ctx->bmd_mode, - ctx->supports_vanc ? bmdVideoOutputVANC : bmdVideoOutputFlagDefault) != S_OK) { + if (ctx->supports_vanc && ctx->dlo->EnableVideoOutput(ctx->bmd_mode, bmdVideoOutputVANC) != S_OK) { + av_log(avctx, AV_LOG_WARNING, "Could not enable video output with VANC! Trying without...\n"); + ctx->supports_vanc = 0; + } + if (!ctx->supports_vanc && ctx->dlo->EnableVideoOutput(ctx->bmd_mode, bmdVideoOutputFlagDefault) != S_OK) { av_log(avctx, AV_LOG_ERROR, "Could not enable video output!\n"); return -1; }
Apparently in the new SDK one cannot query if VANC output is supported, so we will fall back to non-VANC output if enabling the video output with VANC fails. Fixes ticket #7867. Signed-off-by: Marton Balint <cus@passwd.hu> --- libavdevice/decklink_common.cpp | 16 +++++----------- libavdevice/decklink_enc.cpp | 7 +++++-- 2 files changed, 10 insertions(+), 13 deletions(-)