diff mbox

[FFmpeg-devel] avdevice/decklink: add option to drop frames till timecode is seen

Message ID fab42196-1256-5afe-e860-d1f7db9523ad@gyani.pro
State Accepted
Headers show

Commit Message

Gyan Doshi Nov. 15, 2019, 6:47 a.m. UTC 
On 15-11-2019 03:23 am, Marton Balint wrote:
>
>
> On Wed, 13 Nov 2019, Gyan wrote:
>
>> As suggested in the review of an earlier patch*, this one drops 
>> frames till a TC is seen.
>>
>
>> From 05555932db9d14dea215c1b0e5dcac4eac16cd6a Mon Sep 17 00:00:00 2001
>> From: Gyan Doshi <ffmpeg@gyani.pro>
>> Date: Mon, 9 Sep 2019 18:33:09 +0530
>> Subject: [PATCH] avdevice/decklink: add option to drop frames till 
>> timecode is
>>  seen
>>
>> Option wait_for_tc only takes effect if tc_format is set
>> ---
>>  libavdevice/decklink_common.h   |  1 +
>>  libavdevice/decklink_common_c.h |  1 +
>>  libavdevice/decklink_dec.cpp    | 12 ++++++++++++
>>  libavdevice/decklink_dec_c.c    |  1 +
>>  4 files changed, 15 insertions(+)
>
> Docs update is missing for the new option as well as micro version bump.

Added.
>
>>
>> diff --git a/libavdevice/decklink_common.h 
>> b/libavdevice/decklink_common.h
>> index 921818ba41..35422a300b 100644
>> --- a/libavdevice/decklink_common.h
>> +++ b/libavdevice/decklink_common.h
>> @@ -149,6 +149,7 @@ struct decklink_ctx {
>>
>>      int channels;
>>      int audio_depth;
>> +    unsigned long tc_seen;    // used with option wait_for_tc
>>  };
>>
>>  typedef enum { DIRECTION_IN, DIRECTION_OUT} decklink_direction_t;
>> diff --git a/libavdevice/decklink_common_c.h 
>> b/libavdevice/decklink_common_c.h
>> index ca85ec2504..b78630b5fc 100644
>> --- a/libavdevice/decklink_common_c.h
>> +++ b/libavdevice/decklink_common_c.h
>> @@ -58,6 +58,7 @@ struct decklink_cctx {
>>      int copyts;
>>      int64_t timestamp_align;
>>      int timing_offset;
>> +    int wait_for_tc;
>>  };
>>
>>  #endif /* AVDEVICE_DECKLINK_COMMON_C_H */
>> diff --git a/libavdevice/decklink_dec.cpp b/libavdevice/decklink_dec.cpp
>> index 4da9122bff..69be37ed64 100644
>> --- a/libavdevice/decklink_dec.cpp
>> +++ b/libavdevice/decklink_dec.cpp
>> @@ -784,6 +784,8 @@ HRESULT 
>> decklink_input_callback::VideoInputFrameArrived(
>>                              if (packed_metadata) {
>>                                  if (av_packet_add_side_data(&pkt, 
>> AV_PKT_DATA_STRINGS_METADATA, packed_metadata, metadata_len) < 0)
>> av_freep(&packed_metadata);
>> +                                else if (!ctx->tc_seen)
>> +                                    ctx->tc_seen = ctx->frameCount;
>>                              }
>>                          }
>>                      }
>> @@ -793,6 +795,14 @@ HRESULT 
>> decklink_input_callback::VideoInputFrameArrived(
>>              }
>>          }
>>
>> +        if (ctx->tc_format && cctx->wait_for_tc && !ctx->tc_seen) {
>> +
>> +            av_log(avctx, AV_LOG_WARNING, "No TC detected yet. 
>> wait_for_tc set. Dropping. \n");
>> +            av_log(avctx, AV_LOG_WARNING, "Frame received (#%lu) - "
>> +                        "- Frames dropped %u\n", ctx->frameCount, 
>> ++ctx->dropped);
>> +            return S_OK;
>> +        }
>> +
>>          pkt.pts = get_pkt_pts(videoFrame, audioFrame, wallclock, 
>> abs_wallclock, ctx->video_pts_source, ctx->video_st->time_base, 
>> &initial_video_pts, cctx->copyts);
>>          pkt.dts = pkt.pts;
>>
>> @@ -880,6 +890,8 @@ HRESULT 
>> decklink_input_callback::VideoInputFrameArrived(
>>              videoFrame->AddRef();
>>
>>          if (avpacket_queue_put(&ctx->queue, &pkt) < 0) {
>> +            if (ctx->tc_seen == ctx->frameCount)
>> +                ctx->tc_seen = 0;
>
> I don't think this is needed, this can only happen in an ENOMEM case. 
> Keep it if you feel strongly about it, but it looks a rather useless 
> corner case to me.

Removed.
>
> LGTM if docs is added.

Revised patch attached.

Thanks,
Gyan
From d76ed7ec1582a2050c9cfc755c243da22bb3c41a Mon Sep 17 00:00:00 2001
From: Gyan Doshi <ffmpeg@gyani.pro>
Date: Mon, 9 Sep 2019 18:33:09 +0530
Subject: [PATCH v2] avdevice/decklink: add option to drop frames till timecode
 is seen

Option wait_for_tc only takes effect if tc_format is set
---
 doc/indevs.texi                 |  8 ++++++++
 libavdevice/decklink_common.h   |  1 +
 libavdevice/decklink_common_c.h |  1 +
 libavdevice/decklink_dec.cpp    | 10 ++++++++++
 libavdevice/decklink_dec_c.c    |  1 +
 libavdevice/version.h           |  2 +-
 6 files changed, 22 insertions(+), 1 deletion(-)

Comments

Marton Balint Nov. 17, 2019, 4:30 p.m. UTC | #1 
> From: Gyan Doshi <ffmpeg@gyani.pro>
> Date: Mon, 9 Sep 2019 18:33:09 +0530
> Subject: [PATCH v2] avdevice/decklink: add option to drop frames till timecode
>  is seen
> 
> Option wait_for_tc only takes effect if tc_format is set
> ---
>  doc/indevs.texi                 |  8 ++++++++
>  libavdevice/decklink_common.h   |  1 +
>  libavdevice/decklink_common_c.h |  1 +
>  libavdevice/decklink_dec.cpp    | 10 ++++++++++
>  libavdevice/decklink_dec_c.c    |  1 +
>  libavdevice/version.h           |  2 +-
>  6 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/indevs.texi b/doc/indevs.texi
> index 14595774f3..bc791013f2 100644
> --- a/doc/indevs.texi
> +++ b/doc/indevs.texi
> @@ -395,6 +395,14 @@ Either sync could go wrong by 1 frame or in a rarer case
>  @option{timestamp_align} seconds.
>  Defaults to @samp{0}.
> 
> +@item wait_for_tc (@emph{bool})
> +Drop frames till a frame with timecode is received. Sometimes serial timecode
> +isn't received with the first input frame. If that happens, the stored stream
> +timecode will be inaccurate. If this option is set to @option{true}, input frames
> +are dropped till a frame with timecode is received.
> +Option @var{timecode_format} must have been set.

I don't think "must have been" is correct here. Maybe simply write this:

Option @var{timecode_format} must also be specified.

> +Defaults to @option{false}.
> +
>  @end table
>
>  @subsection Examples
> diff --git a/libavdevice/decklink_common.h b/libavdevice/decklink_common.h
> index 921818ba41..35422a300b 100644
> --- a/libavdevice/decklink_common.h
> +++ b/libavdevice/decklink_common.h
> @@ -149,6 +149,7 @@ struct decklink_ctx {
>
>      int channels;
>      int audio_depth;
> +    unsigned long tc_seen;    // used with option wait_for_tc
>  };
>
>  typedef enum { DIRECTION_IN, DIRECTION_OUT} decklink_direction_t;
> diff --git a/libavdevice/decklink_common_c.h b/libavdevice/decklink_common_c.h
> index ca85ec2504..b78630b5fc 100644
> --- a/libavdevice/decklink_common_c.h
> +++ b/libavdevice/decklink_common_c.h
> @@ -58,6 +58,7 @@ struct decklink_cctx {
>      int copyts;
>      int64_t timestamp_align;
>      int timing_offset;
> +    int wait_for_tc;
>  };
>
>  #endif /* AVDEVICE_DECKLINK_COMMON_C_H */
> diff --git a/libavdevice/decklink_dec.cpp b/libavdevice/decklink_dec.cpp
> index 4da9122bff..ab7f28112e 100644
> --- a/libavdevice/decklink_dec.cpp
> +++ b/libavdevice/decklink_dec.cpp
> @@ -784,6 +784,8 @@ HRESULT decklink_input_callback::VideoInputFrameArrived(
>                              if (packed_metadata) {
>                                  if (av_packet_add_side_data(&pkt, AV_PKT_DATA_STRINGS_METADATA, packed_metadata, metadata_len) < 0)
>                                      av_freep(&packed_metadata);
> +                                else if (!ctx->tc_seen)
> +                                    ctx->tc_seen = ctx->frameCount;
>                              }
>                          }
>                      }
> @@ -793,6 +795,14 @@ HRESULT decklink_input_callback::VideoInputFrameArrived(
>              }
>          }
> 
> +        if (ctx->tc_format && cctx->wait_for_tc && !ctx->tc_seen) {
> +
> +            av_log(avctx, AV_LOG_WARNING, "No TC detected yet. wait_for_tc set. Dropping. \n");
> +            av_log(avctx, AV_LOG_WARNING, "Frame received (#%lu) - "
> +                        "- Frames dropped %u\n", ctx->frameCount, ++ctx->dropped);
> +            return S_OK;
> +        }
> +
>          pkt.pts = get_pkt_pts(videoFrame, audioFrame, wallclock, abs_wallclock, ctx->video_pts_source, ctx->video_st->time_base, &initial_video_pts, cctx->copyts);
>          pkt.dts = pkt.pts;
> 
> diff --git a/libavdevice/decklink_dec_c.c b/libavdevice/decklink_dec_c.c
> index 7aceaf934c..99439f91ae 100644
> --- a/libavdevice/decklink_dec_c.c
> +++ b/libavdevice/decklink_dec_c.c
> @@ -85,6 +85,7 @@ static const AVOption options[] = {
>      { "audio_depth",   "audio bitdepth (16 or 32)", OFFSET(audio_depth),  AV_OPT_TYPE_INT,   { .i64 = 16}, 16, 32, DEC },
>      { "decklink_copyts", "copy timestamps, do not remove the initial offset", OFFSET(copyts), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, DEC },
>      { "timestamp_align", "capture start time alignment (in seconds)", OFFSET(timestamp_align), AV_OPT_TYPE_DURATION, { .i64 = 0 }, 0, INT_MAX, DEC },
> +    { "wait_for_tc",     "drop frames till a frame with timecode is received. TC format must be set", OFFSET(wait_for_tc), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, DEC },
>      { NULL },
>  };
> 
> diff --git a/libavdevice/version.h b/libavdevice/version.h
> index e3a583d267..68302908cf 100644
> --- a/libavdevice/version.h
> +++ b/libavdevice/version.h
> @@ -29,7 +29,7 @@
>
>  #define LIBAVDEVICE_VERSION_MAJOR  58
>  #define LIBAVDEVICE_VERSION_MINOR   9
> -#define LIBAVDEVICE_VERSION_MICRO 100
> +#define LIBAVDEVICE_VERSION_MICRO 101
>
>  #define LIBAVDEVICE_VERSION_INT AV_VERSION_INT(LIBAVDEVICE_VERSION_MAJOR, \
>                                                 LIBAVDEVICE_VERSION_MINOR, \
> -- 
> 2.24.0

Otherwise LGTM.

Thanks,
Marton
Gyan Doshi Nov. 18, 2019, 4:33 a.m. UTC | #2 
On 17-11-2019 10:00 pm, Marton Balint wrote:
>
>> From: Gyan Doshi <ffmpeg@gyani.pro>
>> Date: Mon, 9 Sep 2019 18:33:09 +0530
>> Subject: [PATCH v2] avdevice/decklink: add option to drop frames till 
>> timecode
>>  is seen
>>
>> Option wait_for_tc only takes effect if tc_format is set
>> ---
>>  doc/indevs.texi                 |  8 ++++++++
>>  libavdevice/decklink_common.h   |  1 +
>>  libavdevice/decklink_common_c.h |  1 +
>>  libavdevice/decklink_dec.cpp    | 10 ++++++++++
>>  libavdevice/decklink_dec_c.c    |  1 +
>>  libavdevice/version.h           |  2 +-
>>  6 files changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/doc/indevs.texi b/doc/indevs.texi
>> index 14595774f3..bc791013f2 100644
>> --- a/doc/indevs.texi
>> +++ b/doc/indevs.texi
>> @@ -395,6 +395,14 @@ Either sync could go wrong by 1 frame or in a 
>> rarer case
>>  @option{timestamp_align} seconds.
>>  Defaults to @samp{0}.
>>
>> +@item wait_for_tc (@emph{bool})
>> +Drop frames till a frame with timecode is received. Sometimes serial 
>> timecode
>> +isn't received with the first input frame. If that happens, the 
>> stored stream
>> +timecode will be inaccurate. If this option is set to @option{true}, 
>> input frames
>> +are dropped till a frame with timecode is received.
>> +Option @var{timecode_format} must have been set.
>
> I don't think "must have been" is correct here. Maybe simply write this:
>
> Option @var{timecode_format} must also be specified.
>
>> +Defaults to @option{false}.
>> +
>>  @end table
>>
>>  @subsection Examples
>> diff --git a/libavdevice/decklink_common.h 
>> b/libavdevice/decklink_common.h
>> index 921818ba41..35422a300b 100644
>> --- a/libavdevice/decklink_common.h
>> +++ b/libavdevice/decklink_common.h
>> @@ -149,6 +149,7 @@ struct decklink_ctx {
>>
>>      int channels;
>>      int audio_depth;
>> +    unsigned long tc_seen;    // used with option wait_for_tc
>>  };
>>
>>  typedef enum { DIRECTION_IN, DIRECTION_OUT} decklink_direction_t;
>> diff --git a/libavdevice/decklink_common_c.h 
>> b/libavdevice/decklink_common_c.h
>> index ca85ec2504..b78630b5fc 100644
>> --- a/libavdevice/decklink_common_c.h
>> +++ b/libavdevice/decklink_common_c.h
>> @@ -58,6 +58,7 @@ struct decklink_cctx {
>>      int copyts;
>>      int64_t timestamp_align;
>>      int timing_offset;
>> +    int wait_for_tc;
>>  };
>>
>>  #endif /* AVDEVICE_DECKLINK_COMMON_C_H */
>> diff --git a/libavdevice/decklink_dec.cpp b/libavdevice/decklink_dec.cpp
>> index 4da9122bff..ab7f28112e 100644
>> --- a/libavdevice/decklink_dec.cpp
>> +++ b/libavdevice/decklink_dec.cpp
>> @@ -784,6 +784,8 @@ HRESULT 
>> decklink_input_callback::VideoInputFrameArrived(
>>                              if (packed_metadata) {
>>                                  if (av_packet_add_side_data(&pkt, 
>> AV_PKT_DATA_STRINGS_METADATA, packed_metadata, metadata_len) < 0)
>> av_freep(&packed_metadata);
>> +                                else if (!ctx->tc_seen)
>> +                                    ctx->tc_seen = ctx->frameCount;
>>                              }
>>                          }
>>                      }
>> @@ -793,6 +795,14 @@ HRESULT 
>> decklink_input_callback::VideoInputFrameArrived(
>>              }
>>          }
>>
>> +        if (ctx->tc_format && cctx->wait_for_tc && !ctx->tc_seen) {
>> +
>> +            av_log(avctx, AV_LOG_WARNING, "No TC detected yet. 
>> wait_for_tc set. Dropping. \n");
>> +            av_log(avctx, AV_LOG_WARNING, "Frame received (#%lu) - "
>> +                        "- Frames dropped %u\n", ctx->frameCount, 
>> ++ctx->dropped);
>> +            return S_OK;
>> +        }
>> +
>>          pkt.pts = get_pkt_pts(videoFrame, audioFrame, wallclock, 
>> abs_wallclock, ctx->video_pts_source, ctx->video_st->time_base, 
>> &initial_video_pts, cctx->copyts);
>>          pkt.dts = pkt.pts;
>>
>> diff --git a/libavdevice/decklink_dec_c.c b/libavdevice/decklink_dec_c.c
>> index 7aceaf934c..99439f91ae 100644
>> --- a/libavdevice/decklink_dec_c.c
>> +++ b/libavdevice/decklink_dec_c.c
>> @@ -85,6 +85,7 @@ static const AVOption options[] = {
>>      { "audio_depth",   "audio bitdepth (16 or 32)", 
>> OFFSET(audio_depth),  AV_OPT_TYPE_INT,   { .i64 = 16}, 16, 32, DEC },
>>      { "decklink_copyts", "copy timestamps, do not remove the initial 
>> offset", OFFSET(copyts), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, DEC },
>>      { "timestamp_align", "capture start time alignment (in 
>> seconds)", OFFSET(timestamp_align), AV_OPT_TYPE_DURATION, { .i64 = 0 
>> }, 0, INT_MAX, DEC },
>> +    { "wait_for_tc",     "drop frames till a frame with timecode is 
>> received. TC format must be set", OFFSET(wait_for_tc), 
>> AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, DEC },
>>      { NULL },
>>  };
>>
>> diff --git a/libavdevice/version.h b/libavdevice/version.h
>> index e3a583d267..68302908cf 100644
>> --- a/libavdevice/version.h
>> +++ b/libavdevice/version.h
>> @@ -29,7 +29,7 @@
>>
>>  #define LIBAVDEVICE_VERSION_MAJOR  58
>>  #define LIBAVDEVICE_VERSION_MINOR   9
>> -#define LIBAVDEVICE_VERSION_MICRO 100
>> +#define LIBAVDEVICE_VERSION_MICRO 101
>>
>>  #define LIBAVDEVICE_VERSION_INT 
>> AV_VERSION_INT(LIBAVDEVICE_VERSION_MAJOR, \
>> LIBAVDEVICE_VERSION_MINOR, \
>> -- 
>> 2.24.0
>
> Otherwise LGTM.

Docs revised and pushed as d831edc387c17c25372ff317715b6d6bce235c13

Thanks,
Gyan
diff mbox

Patch

diff --git a/doc/indevs.texi b/doc/indevs.texi
index 14595774f3..bc791013f2 100644
--- a/doc/indevs.texi
+++ b/doc/indevs.texi
@@ -395,6 +395,14 @@  Either sync could go wrong by 1 frame or in a rarer case
 @option{timestamp_align} seconds.
 Defaults to @samp{0}.
 
+@item wait_for_tc (@emph{bool})
+Drop frames till a frame with timecode is received. Sometimes serial timecode
+isn't received with the first input frame. If that happens, the stored stream
+timecode will be inaccurate. If this option is set to @option{true}, input frames
+are dropped till a frame with timecode is received.
+Option @var{timecode_format} must have been set.
+Defaults to @option{false}.
+
 @end table
 
 @subsection Examples
diff --git a/libavdevice/decklink_common.h b/libavdevice/decklink_common.h
index 921818ba41..35422a300b 100644
--- a/libavdevice/decklink_common.h
+++ b/libavdevice/decklink_common.h
@@ -149,6 +149,7 @@  struct decklink_ctx {
 
     int channels;
     int audio_depth;
+    unsigned long tc_seen;    // used with option wait_for_tc
 };
 
 typedef enum { DIRECTION_IN, DIRECTION_OUT} decklink_direction_t;
diff --git a/libavdevice/decklink_common_c.h b/libavdevice/decklink_common_c.h
index ca85ec2504..b78630b5fc 100644
--- a/libavdevice/decklink_common_c.h
+++ b/libavdevice/decklink_common_c.h
@@ -58,6 +58,7 @@  struct decklink_cctx {
     int copyts;
     int64_t timestamp_align;
     int timing_offset;
+    int wait_for_tc;
 };
 
 #endif /* AVDEVICE_DECKLINK_COMMON_C_H */
diff --git a/libavdevice/decklink_dec.cpp b/libavdevice/decklink_dec.cpp
index 4da9122bff..ab7f28112e 100644
--- a/libavdevice/decklink_dec.cpp
+++ b/libavdevice/decklink_dec.cpp
@@ -784,6 +784,8 @@  HRESULT decklink_input_callback::VideoInputFrameArrived(
                             if (packed_metadata) {
                                 if (av_packet_add_side_data(&pkt, AV_PKT_DATA_STRINGS_METADATA, packed_metadata, metadata_len) < 0)
                                     av_freep(&packed_metadata);
+                                else if (!ctx->tc_seen)
+                                    ctx->tc_seen = ctx->frameCount;
                             }
                         }
                     }
@@ -793,6 +795,14 @@  HRESULT decklink_input_callback::VideoInputFrameArrived(
             }
         }
 
+        if (ctx->tc_format && cctx->wait_for_tc && !ctx->tc_seen) {
+
+            av_log(avctx, AV_LOG_WARNING, "No TC detected yet. wait_for_tc set. Dropping. \n");
+            av_log(avctx, AV_LOG_WARNING, "Frame received (#%lu) - "
+                        "- Frames dropped %u\n", ctx->frameCount, ++ctx->dropped);
+            return S_OK;
+        }
+
         pkt.pts = get_pkt_pts(videoFrame, audioFrame, wallclock, abs_wallclock, ctx->video_pts_source, ctx->video_st->time_base, &initial_video_pts, cctx->copyts);
         pkt.dts = pkt.pts;
 
diff --git a/libavdevice/decklink_dec_c.c b/libavdevice/decklink_dec_c.c
index 7aceaf934c..99439f91ae 100644
--- a/libavdevice/decklink_dec_c.c
+++ b/libavdevice/decklink_dec_c.c
@@ -85,6 +85,7 @@  static const AVOption options[] = {
     { "audio_depth",   "audio bitdepth (16 or 32)", OFFSET(audio_depth),  AV_OPT_TYPE_INT,   { .i64 = 16}, 16, 32, DEC },
     { "decklink_copyts", "copy timestamps, do not remove the initial offset", OFFSET(copyts), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, DEC },
     { "timestamp_align", "capture start time alignment (in seconds)", OFFSET(timestamp_align), AV_OPT_TYPE_DURATION, { .i64 = 0 }, 0, INT_MAX, DEC },
+    { "wait_for_tc",     "drop frames till a frame with timecode is received. TC format must be set", OFFSET(wait_for_tc), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, DEC },
     { NULL },
 };
 
diff --git a/libavdevice/version.h b/libavdevice/version.h
index e3a583d267..68302908cf 100644
--- a/libavdevice/version.h
+++ b/libavdevice/version.h
@@ -29,7 +29,7 @@ 
 
 #define LIBAVDEVICE_VERSION_MAJOR  58
 #define LIBAVDEVICE_VERSION_MINOR   9
-#define LIBAVDEVICE_VERSION_MICRO 100
+#define LIBAVDEVICE_VERSION_MICRO 101
 
 #define LIBAVDEVICE_VERSION_INT AV_VERSION_INT(LIBAVDEVICE_VERSION_MAJOR, \
                                                LIBAVDEVICE_VERSION_MINOR, \