See comments below.

Aaron Levinson

On 11/5/2017 4:49 PM, Marton Balint wrote:

On Fri, 27 Oct 2017, Jeyapal, Karthick wrote:

Please find the patch attached.


Thanks, below some comments:

From b18679b91a79f5e23a5ad23ae70f3862a34ddfb8 Mon Sep 17 00:00:00 2001
From: Karthick J <kjeya...@akamai.com>
Date: Fri, 27 Oct 2017 12:00:23 +0530
Subject: [PATCH 1/1] avdevice/decklink_dec: Autodetect the video input format

When -format_code is not specified autodetection will happen
---
 doc/indevs.texi                 |  1 +
 libavdevice/decklink_common.cpp | 33 ++++++++++---------
 libavdevice/decklink_common.h   |  8 +++++
 libavdevice/decklink_dec.cpp    | 70 +++++++++++++++++++++++++++++++++++------
 4 files changed, 89 insertions(+), 23 deletions(-)

diff --git a/doc/indevs.texi b/doc/indevs.texi
index d308bbf..74bfcc5 100644
--- a/doc/indevs.texi
+++ b/doc/indevs.texi
@@ -238,6 +238,7 @@ This sets the input video format to the format given by the FourCC. To see
 the supported values of your device(s) use @option{list_formats}.
 Note that there is a FourCC @option{'pal '} that can also be used
 as @option{pal} (3 letters).
+Default behavior is autodetection of the input video format

... video format, if the hardware supports it.


 @item bm_v210
 This is a deprecated option, you can use @option{raw_format} instead.
diff --git a/libavdevice/decklink_common.cpp b/libavdevice/decklink_common.cpp
index 2bd63ac..757f419 100644
--- a/libavdevice/decklink_common.cpp
+++ b/libavdevice/decklink_common.cpp
@@ -148,23 +148,10 @@ static DECKLINK_BOOL field_order_eq(enum AVFieldOrder field_order, BMDFieldDomin
     return false;
 }

-int ff_decklink_set_format(AVFormatContext *avctx,
-                               int width, int height,
-                               int tb_num, int tb_den,
-                               enum AVFieldOrder field_order,
-                               decklink_direction_t direction, int num)
-{
+void ff_decklink_set_duplex_mode(AVFormatContext *avctx) {
     struct decklink_cctx *cctx = (struct decklink_cctx *)avctx->priv_data;
     struct decklink_ctx *ctx = (struct decklink_ctx *)cctx->ctx;
-    BMDDisplayModeSupport support;
-    IDeckLinkDisplayModeIterator *itermode;
-    IDeckLinkDisplayMode *mode;
-    int i = 1;
     HRESULT res;
-
-    av_log(avctx, AV_LOG_DEBUG, "Trying to find mode for frame size %dx%d, frame timing %d/%d, field order %d, direction %d, mode number %d, format code %s\n", -        width, height, tb_num, tb_den, field_order, direction, num, (cctx->format_code) ? cctx->format_code : "(unset)");
-
     if (ctx->duplex_mode) {
         DECKLINK_BOOL duplex_supported = false;

@@ -181,6 +168,24 @@ int ff_decklink_set_format(AVFormatContext *avctx,
             av_log(avctx, AV_LOG_WARNING, "Unable to set duplex mode, because it is not supported.\n");
         }
     }
+}

You factorized this out, but keep in mind that decklink_enc might also use this to set duplex mode. (even if there is no option to do that at the moment). Also
it would make sense to rename the function and also put the input selection
logic here, because even if you autodetect, you should select the input
(sdi/hdmi/etc) first.

This factorization should be a separate patch for easier review.

+
+int ff_decklink_set_format(AVFormatContext *avctx,
+                               int width, int height,
+                               int tb_num, int tb_den,
+                               enum AVFieldOrder field_order,
+                               decklink_direction_t direction, int num)
+{
+    struct decklink_cctx *cctx = (struct decklink_cctx *)avctx->priv_data;
+    struct decklink_ctx *ctx = (struct decklink_ctx *)cctx->ctx;
+    BMDDisplayModeSupport support;
+    IDeckLinkDisplayModeIterator *itermode;
+    IDeckLinkDisplayMode *mode;
+    int i = 1;
+    HRESULT res;
+
+    av_log(avctx, AV_LOG_DEBUG, "Trying to find mode for frame size %dx%d, frame timing %d/%d, field order %d, direction %d, mode number %d, format code %s\n", +        width, height, tb_num, tb_den, field_order, direction, num, (cctx->format_code) ? cctx->format_code : "(unset)");

     if (direction == DIRECTION_IN) {
         int ret;
diff --git a/libavdevice/decklink_common.h b/libavdevice/decklink_common.h
index b6acb01..f38fc14 100644
--- a/libavdevice/decklink_common.h
+++ b/libavdevice/decklink_common.h
@@ -31,6 +31,12 @@
 class decklink_output_callback;
 class decklink_input_callback;

+typedef enum autodetect_state {
+    AUTODETECT_RESET = 0,

Maybe something like AUTODETECT_INACTIVE is a better name?

+    AUTODETECT_START,
+    AUTODETECT_DONE,
+} autodetect_state;
+
 typedef struct AVPacketQueue {
     AVPacketList *first_pkt, *last_pkt;
     int nb_packets;
@@ -95,6 +101,7 @@ struct decklink_ctx {
     pthread_mutex_t mutex;
     pthread_cond_t cond;
     int frames_buffer_available_spots;
+    autodetect_state autodetect;

     int channels;
     int audio_depth;
@@ -134,6 +141,7 @@ static const BMDVideoConnection decklink_video_connection_map[] = {
 };

 HRESULT ff_decklink_get_display_name(IDeckLink *This, const char **displayName);
+void ff_decklink_set_duplex_mode(AVFormatContext *avctx);
 int ff_decklink_set_format(AVFormatContext *avctx, int width, int height, int tb_num, int tb_den, enum AVFieldOrder field_order, decklink_direction_t direction = DIRECTION_OUT, int num = 0);  int ff_decklink_set_format(AVFormatContext *avctx, decklink_direction_t direction, int num);  int ff_decklink_list_devices(AVFormatContext *avctx, struct AVDeviceInfoList *device_list, int show_inputs, int show_outputs);
diff --git a/libavdevice/decklink_dec.cpp b/libavdevice/decklink_dec.cpp
index b4b9e02..374a30f 100644
--- a/libavdevice/decklink_dec.cpp
+++ b/libavdevice/decklink_dec.cpp
@@ -634,6 +634,9 @@ HRESULT decklink_input_callback::VideoInputFrameArrived(
     BMDTimeValue frameDuration;
     int64_t wallclock = 0;

+    if (ctx->autodetect != AUTODETECT_RESET)
+        return S_OK;
+
     ctx->frameCount++;
     if (ctx->audio_pts_source == PTS_SRC_WALLCLOCK || ctx->video_pts_source == PTS_SRC_WALLCLOCK)
         wallclock = av_gettime_relative();
@@ -794,17 +797,51 @@ HRESULT decklink_input_callback::VideoInputFormatChanged(
     BMDVideoInputFormatChangedEvents events, IDeckLinkDisplayMode *mode,
     BMDDetectedVideoInputFormatFlags)
 {
+    pthread_mutex_lock(&ctx->mutex);
+    ctx->bmd_width  = mode->GetWidth();
+    ctx->bmd_height = mode->GetHeight();
+    ctx->bmd_mode  = mode->GetDisplayMode();
+    ctx->bmd_field_dominance = mode->GetFieldDominance();
+    mode->GetFrameRate(&ctx->bmd_tb_num, &ctx->bmd_tb_den);
+    ctx->autodetect = AUTODETECT_DONE;
+    pthread_cond_signal(&ctx->cond);
+    pthread_mutex_unlock(&ctx->mutex);

I'd prefer an approach which only stores bmd_mode and calls
ff_decklink_set_format in the main thread as common code with the
no-autodetection case.

     return S_OK;
 }

-static HRESULT decklink_start_input(AVFormatContext *avctx)
-{
-    struct decklink_cctx *cctx = (struct decklink_cctx *)avctx->priv_data;
+static int decklink_autodetect(struct decklink_cctx *cctx) {
     struct decklink_ctx *ctx = (struct decklink_ctx *)cctx->ctx;
+    timespec ts;
+    int rc;
+    int64_t t;
+
+    ctx->autodetect = AUTODETECT_START;
+    if (ctx->dli->EnableVideoInput(bmdModeNTSC,
+                                   (BMDPixelFormat) cctx->raw_format,

You can specify a fixed pixel format here (e.g. 8bit YUV), raw_format here is
not yet validated.

+ bmdVideoInputEnableFormatDetection) != S_OK) {
+        return -1;
+    }

-    ctx->input_callback = new decklink_input_callback(avctx);
-    ctx->dli->SetCallback(ctx->input_callback);
-    return ctx->dli->StartStreams();
+    if (ctx->dli->StartStreams() != S_OK) {
+        return -1;
+    }
+
+    t = av_gettime();
+    ts = { .tv_sec  =  t / 1000000,
+           .tv_nsec = (t % 1000000) * 1000 };
+    ts.tv_sec += 1;
+    rc = 0;
+    pthread_mutex_lock(&ctx->mutex);
+    while (ctx->autodetect == AUTODETECT_START && rc == 0)
+        rc = pthread_cond_timedwait(&ctx->cond, &ctx->mutex, &ts);

pthread_cond_timedwait is not portable, it has no compatiblity wrapper for w32 threads as far as I know, and using av_gettime() instead of the monotonic clock is also ugly a bit, so in general I'd try to avoid it. Maybe it is enough if you wait for 25 received frames in VideoFrameArrived to be able to detect no signal?

Also, what happens if you specify the proper format as the default? Do you
still get a Format Changed event? So if you get a VideoFrame with bmdNoSignal
flag *not* set, maybe you should finish the autodetection phase?

If one calls EnableVideoInput() with the video mode that would eventually be detected initially, VideoInputFormatChanged() will not be called in this case. That, is, if I do something like:

hr = pDeckLinkInputToUse->EnableVideoInput(bmdModeHD720p5994, bmdFormat8BitYUV, bmdVideoInputEnableFormatDetection);

and have a camera outputting 720p59.94 connected to the DeckLink device, VideoInputFormatChanged() will not be called in this case.

Also, as Marton has indicated, this code needs to build on Windows as well, and by that, I mean, built with MSVC or perhaps ICC. In the VideoInputFrameArrived() callback, you can check for bmdFrameHasNoInputSource to detect the case that there is no longer any video signal. As in:

        BMDFrameFlags flags = videoFrame->GetFlags();
        if (flags & bmdFrameHasNoInputSource)

+    pthread_mutex_unlock(&ctx->mutex);
+    if (rc != 0) {
+        return -1;
+    }
+    ctx->dli->PauseStreams();
+    ctx->dli->FlushStreams();
+
+    return 0;
 }

 extern "C" {
@@ -916,6 +953,13 @@ av_cold int ff_decklink_read_header(AVFormatContext *avctx)
         goto error;
     }

+    avpacket_queue_init (avctx, &ctx->queue);
+
+    ctx->input_callback = new decklink_input_callback(avctx);
+    ctx->dli->SetCallback(ctx->input_callback);
+
+    ff_decklink_set_duplex_mode(avctx);
+
     if (mode_num > 0 || cctx->format_code) {
         if (ff_decklink_set_format(avctx, DIRECTION_IN, mode_num) < 0) {
             av_log(avctx, AV_LOG_ERROR, "Could not set mode number %d or format code %s for %s\n", @@ -923,6 +967,16 @@ av_cold int ff_decklink_read_header(AVFormatContext *avctx)
             ret = AVERROR(EIO);
             goto error;
         }
+    } else {

If autodetection is not supported, you should not do that. Query using
IDeckLinkAttributes::GetFlag(BMDDeckLinkSupportsInputFormatDetection).

+        if (decklink_autodetect(cctx) < 0) {
+            av_log(avctx, AV_LOG_ERROR, "Cannot Autodetect input stream or No signal");
+            ret = AVERROR(EIO);
+            goto error;
+        }
+        av_log(avctx, AV_LOG_INFO, "Autodetected the input mode %dx%d @ %.2f%s fps\n", +            ctx->bmd_width, ctx->bmd_height, (float)ctx->bmd_tb_den/(float)ctx->bmd_tb_num, +            (ctx->bmd_field_dominance==bmdLowerFieldFirst || ctx->bmd_field_dominance==bmdUpperFieldFirst)?"(i)":"");
+        ctx->autodetect = AUTODETECT_RESET;
     }

and if no mode is specified, and autodetetion is not supported, you might as well fail and inform the user. I think previously the code failed later when EnableVideoInput was called with 0 mode...


 #if !CONFIG_LIBZVBI
@@ -1050,9 +1104,7 @@ av_cold int ff_decklink_read_header(AVFormatContext *avctx)
         goto error;
     }

-    avpacket_queue_init (avctx, &ctx->queue);
-
-    if (decklink_start_input (avctx) != S_OK) {
+    if (ctx->dli->StartStreams() != S_OK) {
         av_log(avctx, AV_LOG_ERROR, "Cannot start input stream\n");
         ret = AVERROR(EIO);
         goto error;
--
1.9.1


Regards,
Marton
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Reply via email to