[FFmpeg-devel] [PATCH 1/1] avdevice/decklink_dec: Autodetect the video input format
Aaron Levinson
alevinsn_dev at levland.net
Mon Nov 6 06:54:00 EET 2017
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 <kjeyapal at 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 at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
More information about the ffmpeg-devel
mailing list