diff mbox

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

Message ID 8844fbc9-a92b-aa79-38eb-a012da7288d5@gyani.pro
State Superseded
Headers show

Commit Message

Gyan Doshi Nov. 13, 2019, 4:40 p.m. UTC
As suggested in the review of an earlier patch*, this one drops frames 
till a TC is seen.

Testing results to follow.

Thanks,
Gyan

*https://patchwork.ffmpeg.org/patch/14081/
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(+)

Comments

Ilinca Tudose Nov. 13, 2019, 5:30 p.m. UTC | #1
Hi,

We tested this with several types of tapes. The results are as follows:

Out of 58 captures on Betacam tapes 55 had 0 frame offset and 3 had an
offset of 1 frame. All 3 were captured using the flag "-wait_for_tc 1". Out
of the 58 captures roughly half were captured with and half without
"-wait_for_tc
1".

Out of 26 HDCAM captures (50/50 with/without "-wait_for_tc 1") all 26 had 0
TC offset.

Let me know if there's any questions re testing. The testing setup is the
same as previously mentioned.

Thanks,
ilinca

On Wed, Nov 13, 2019 at 5:40 PM Gyan <ffmpeg@gyani.pro> wrote:

> As suggested in the review of an earlier patch*, this one drops frames
> till a TC is seen.
>
> Testing results to follow.
>
> Thanks,
> Gyan
>
> *https://patchwork.ffmpeg.org/patch/14081/
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Ilinca Tudose Nov. 13, 2019, 6:04 p.m. UTC | #2
My previous email was incorrect about "-wait_for_tc 1". The 2 commands used
were:

(1)
ffmpeg -loglevel trace  -timecode_format rp188any  -wait_for_tc 1 -f
decklink -probesize 15M -analyzeduration 5M -i "DeckLink Quad (1)" -t 30
-preset veryfast output.mov

(2)
ffmpeg -loglevel trace  -timecode_format rp188any  -wait_for_tc 1 -f
decklink -i "DeckLink Quad (1)" -t 30 -preset veryfast output.mov

The second command is the one that produced 1 frame offset in 3 of the
captures. The first command had 0 frames offset for all files tried.

On Wed, Nov 13, 2019 at 6:30 PM Ilinca Tudose <ilincat@google.com> wrote:

> Hi,
>
> We tested this with several types of tapes. The results are as follows:
>
> Out of 58 captures on Betacam tapes 55 had 0 frame offset and 3 had an
> offset of 1 frame. All 3 were captured using the flag "-wait_for_tc 1".
> Out of the 58 captures roughly half were captured with and half without "-wait_for_tc
> 1".
>
> Out of 26 HDCAM captures (50/50 with/without "-wait_for_tc 1") all 26 had
> 0 TC offset.
>
> Let me know if there's any questions re testing. The testing setup is the
> same as previously mentioned.
>
> Thanks,
> ilinca
>
> On Wed, Nov 13, 2019 at 5:40 PM Gyan <ffmpeg@gyani.pro> wrote:
>
>> As suggested in the review of an earlier patch*, this one drops frames
>> till a TC is seen.
>>
>> Testing results to follow.
>>
>> Thanks,
>> Gyan
>>
>> *https://patchwork.ffmpeg.org/patch/14081/
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
>
>
>
Ilinca Tudose Nov. 14, 2019, 3:33 p.m. UTC | #3
This also works when passing the -format_code parameter.

On Wed, Nov 13, 2019 at 7:04 PM Ilinca Tudose <ilincat@google.com> wrote:

> My previous email was incorrect about "-wait_for_tc 1". The 2 commands
> used were:
>
> (1)
> ffmpeg -loglevel trace  -timecode_format rp188any  -wait_for_tc 1 -f
> decklink -probesize 15M -analyzeduration 5M -i "DeckLink Quad (1)" -t 30
> -preset veryfast output.mov
>
> (2)
> ffmpeg -loglevel trace  -timecode_format rp188any  -wait_for_tc 1 -f
> decklink -i "DeckLink Quad (1)" -t 30 -preset veryfast output.mov
>
> The second command is the one that produced 1 frame offset in 3 of the
> captures. The first command had 0 frames offset for all files tried.
>
> On Wed, Nov 13, 2019 at 6:30 PM Ilinca Tudose <ilincat@google.com> wrote:
>
>> Hi,
>>
>> We tested this with several types of tapes. The results are as follows:
>>
>> Out of 58 captures on Betacam tapes 55 had 0 frame offset and 3 had an
>> offset of 1 frame. All 3 were captured using the flag "-wait_for_tc 1".
>> Out of the 58 captures roughly half were captured with and half without "-wait_for_tc
>> 1".
>>
>> Out of 26 HDCAM captures (50/50 with/without "-wait_for_tc 1") all 26
>> had 0 TC offset.
>>
>> Let me know if there's any questions re testing. The testing setup is the
>> same as previously mentioned.
>>
>> Thanks,
>> ilinca
>>
>> On Wed, Nov 13, 2019 at 5:40 PM Gyan <ffmpeg@gyani.pro> wrote:
>>
>>> As suggested in the review of an earlier patch*, this one drops frames
>>> till a TC is seen.
>>>
>>> Testing results to follow.
>>>
>>> Thanks,
>>> Gyan
>>>
>>> *https://patchwork.ffmpeg.org/patch/14081/
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel@ffmpeg.org
>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>>> To unsubscribe, visit link above, or email
>>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>>
>>
>>
>>
>
> --
>
> ---
> Ilinca Tudose | Technical Solutions Consultant, Digitization Operations |
> go/digitops <https://goto.google.com/digitops>
> Google Germany GmbH
> Erika-Mann-Str. 33
> 80636 München
>
> Registergericht und -nummer: Hamburg, HRB 86891
>
> Sitz der Gesellschaft: Hamburg
>
> Geschäftsführer: Paul Manicle, Halimah DeLaine Prado Diese E-Mail ist
> vertraulich. Wenn Sie nicht der richtige Adressat sind, leiten Sie diese
> bitte nicht weiter, informieren Sie den Absender und löschen Sie die E-Mail
> und alle Anhänge. Vielen Dank. This e-mail is confidential. If you are not
> the right addressee please do not forward it, please inform the sender, and
> please erase this e-mail including any attachments. Thanks. Der Inhalt
> dieser E-Mail spiegelt den derzeitigen Stand der Verhandlungen wider und
> dient als Basis für weitere Gespräche. Er soll keine rechtlich verbindliche
> Verpflichtung begründen. Eine solche Verpflichtung wird allein durch einen
> zwischen allen beteiligten Parteien abgeschlossenen, schriftlichen Vertrag
> geschaffen. The above terms reflect a potential business arrangement, are
> provided solely as a basis for further discussion, and are not intended to
> be and do not constitute a legally binding obligation. No legally binding
> obligations will be created, implied, or inferred until an agreement in
> final form is executed in writing by all parties involved.
>
Marton Balint Nov. 14, 2019, 9:32 p.m. UTC | #4
On Wed, 13 Nov 2019, Ilinca Tudose wrote:

> My previous email was incorrect about "-wait_for_tc 1". The 2 commands used
> were:
>
> (1)
> ffmpeg -loglevel trace  -timecode_format rp188any  -wait_for_tc 1 -f
> decklink -probesize 15M -analyzeduration 5M -i "DeckLink Quad (1)" -t 30
> -preset veryfast output.mov
>
> (2)
> ffmpeg -loglevel trace  -timecode_format rp188any  -wait_for_tc 1 -f
> decklink -i "DeckLink Quad (1)" -t 30 -preset veryfast output.mov
>
> The second command is the one that produced 1 frame offset in 3 of the
> captures. The first command had 0 frames offset for all files tried.

These two command lines are only different in probesize and
analyzeduration. I don't understand how these two options can make any
difference regarding the TC offset. Can you figure out which one actually 
causes the issue?

Thanks,
Marton


>
> On Wed, Nov 13, 2019 at 6:30 PM Ilinca Tudose <ilincat@google.com> wrote:
>
>> Hi,
>>
>> We tested this with several types of tapes. The results are as follows:
>>
>> Out of 58 captures on Betacam tapes 55 had 0 frame offset and 3 had an
>> offset of 1 frame. All 3 were captured using the flag "-wait_for_tc 1".
>> Out of the 58 captures roughly half were captured with and half without "-wait_for_tc
>> 1".
>>
>> Out of 26 HDCAM captures (50/50 with/without "-wait_for_tc 1") all 26 had
>> 0 TC offset.
>>
>> Let me know if there's any questions re testing. The testing setup is the
>> same as previously mentioned.
>>
>> Thanks,
>> ilinca
>>
>> On Wed, Nov 13, 2019 at 5:40 PM Gyan <ffmpeg@gyani.pro> wrote:
>>
>>> As suggested in the review of an earlier patch*, this one drops frames
>>> till a TC is seen.
>>>
>>> Testing results to follow.
>>>
>>> Thanks,
>>> Gyan
>>>
>>> *https://patchwork.ffmpeg.org/patch/14081/
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel@ffmpeg.org
>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>>> To unsubscribe, visit link above, or email
>>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>>
>>
>>
>>
>
> -- 
>
> ---
> Ilinca Tudose | Technical Solutions Consultant, Digitization Operations |
> go/digitops <https://goto.google.com/digitops>
> Google Germany GmbH
> Erika-Mann-Str. 33
> 80636 München
>
> Registergericht und -nummer: Hamburg, HRB 86891
>
> Sitz der Gesellschaft: Hamburg
>
> Geschäftsführer: Paul Manicle, Halimah DeLaine Prado Diese E-Mail ist
> vertraulich. Wenn Sie nicht der richtige Adressat sind, leiten Sie diese
> bitte nicht weiter, informieren Sie den Absender und löschen Sie die E-Mail
> und alle Anhänge. Vielen Dank. This e-mail is confidential. If you are not
> the right addressee please do not forward it, please inform the sender, and
> please erase this e-mail including any attachments. Thanks. Der Inhalt
> dieser E-Mail spiegelt den derzeitigen Stand der Verhandlungen wider und
> dient als Basis für weitere Gespräche. Er soll keine rechtlich verbindliche
> Verpflichtung begründen. Eine solche Verpflichtung wird allein durch einen
> zwischen allen beteiligten Parteien abgeschlossenen, schriftlichen Vertrag
> geschaffen. The above terms reflect a potential business arrangement, are
> provided solely as a basis for further discussion, and are not intended to
> be and do not constitute a legally binding obligation. No legally binding
> obligations will be created, implied, or inferred until an agreement in
> final form is executed in writing by all parties involved.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Marton Balint Nov. 14, 2019, 9:53 p.m. UTC | #5
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.

> 
> 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.

LGTM if docs is added.

Thanks,
Marton
Gyan Doshi Nov. 15, 2019, 6:46 a.m. UTC | #6
On 15-11-2019 03:02 am, Marton Balint wrote:
>
>
> On Wed, 13 Nov 2019, Ilinca Tudose wrote:
>
>> My previous email was incorrect about "-wait_for_tc 1". The 2 
>> commands used
>> were:
>>
>> (1)
>> ffmpeg -loglevel trace  -timecode_format rp188any  -wait_for_tc 1 -f
>> decklink -probesize 15M -analyzeduration 5M -i "DeckLink Quad (1)" -t 30
>> -preset veryfast output.mov
>>
>> (2)
>> ffmpeg -loglevel trace  -timecode_format rp188any  -wait_for_tc 1 -f
>> decklink -i "DeckLink Quad (1)" -t 30 -preset veryfast output.mov
>>
>> The second command is the one that produced 1 frame offset in 3 of the
>> captures. The first command had 0 frames offset for all files tried.
>
> These two command lines are only different in probesize and
> analyzeduration. I don't understand how these two options can make any
> difference regarding the TC offset. Can you figure out which one 
> actually causes the issue?

It's not related. Initially,before my patch, they were trying to capture 
TC with format_code set. Because of the lag, the probe limits would 
usually be exhausted before the TC arrived.
With my initial patch in July, which compensated for the lag, extended 
probe was still required. With the current one, it's not.

Ilinca can supply details but the offset occurs infrequently and on 
repeating the capture, would not show an offset. BM Decklink support has 
been unable to reproduce the issue with their capture utility.

Gyan
diff mbox

Patch

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;
             ++ctx->dropped;
         }
     }
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 },
 };