[FFmpeg-devel] [PATCH] avdevice/decklink: Add option to align Capture start time

[Jeyapal, Karthick]

On 9/28/18 2:58 AM, Marton Balint wrote:
>
>
> On Mon, 24 Sep 2018, Karthick J wrote:
>
> > From: Karthick Jeyapal <kjeyapal at akamai.com>
> >
> > This option is useful for maintaining input synchronization across N
> > different hardware devices deployed for 'N-way' redundancy.
> > The system time of different hardware devices should be synchronized
> > with protocols such as NTP or PTP, before using this option.
>
> Here is a review of the patch if you want to proceed with implementing
> the feature in decklink. The introduced code can be simple/small enough
> that I think it is OK to include it if you prefer that.
Thanks for review comments. Please find my relevant replies inlined.
I have submitted a new v2 patch.
>
>
> > ---
> > doc/indevs.texi                 | 10 ++++++++++
> > libavdevice/decklink_common_c.h |  1 +
> > libavdevice/decklink_dec.cpp    | 20 ++++++++++++++++++++
> > libavdevice/decklink_dec_c.c    |  1 +
> > 4 files changed, 32 insertions(+)
> >
> > diff --git a/doc/indevs.texi b/doc/indevs.texi
> > index ed2784b..dfd530a 100644
> > --- a/doc/indevs.texi
> > +++ b/doc/indevs.texi
> > @@ -371,6 +371,16 @@ If set to @option{true}, timestamps are forwarded as they are without removing
> > the initial offset.
> > Defaults to @option{false}.
> > 
> > + at item timestamp_align
> > +Capture start time alignment in seconds. If set to nonzero, input frames are
> > +dropped till the system timestamp aligns with configured value.
> > +Alignment difference of upto one frame duration is tolerated.
> > +This is useful for maintaining input synchronization across N different
> > +hardware devices deployed for 'N-way' redundancy. The system time of different
> > +hardware devices should be synchronized with protocols such as NTP or PTP,
> > +before using this option.
> > +Defaults to @samp{0}.
>
> This approach is not perfect. If frame callbacks are roughly at the same
> time as the alignment, then - because of the jitter of the callbacks -
> segments can be skipped or 1 frame difference is possible. At least
> mention this in the docs.
Done. 
>
> Also I remember having some burst callbacks in case of signal
> loss/return on newer DeckLink models, that might mess this up as well.
>
> An always working synchornization method probably needs a continous SDI
> timecode which is unfortunately not an option in most cases.
>
>
> > +
> > @end table
> > 
> > @subsection Examples
> > diff --git a/libavdevice/decklink_common_c.h b/libavdevice/decklink_common_c.h
> > index 32a5d70..c4a8985 100644
> > --- a/libavdevice/decklink_common_c.h
> > +++ b/libavdevice/decklink_common_c.h
> > @@ -56,6 +56,7 @@ struct decklink_cctx {
> >     int raw_format;
> >     int64_t queue_size;
> >     int copyts;
> > +    int timestamp_align;
>
> Make this int64_t and the option below AV_OPT_TYPE_DURATION.
Done
>
> > };
> > 
> > #endif /* AVDEVICE_DECKLINK_COMMON_C_H */
> > diff --git a/libavdevice/decklink_dec.cpp b/libavdevice/decklink_dec.cpp
> > index 7fabef2..24f5ca9 100644
> > --- a/libavdevice/decklink_dec.cpp
> > +++ b/libavdevice/decklink_dec.cpp
> > @@ -703,6 +703,26 @@ HRESULT decklink_input_callback::VideoInputFrameArrived(
> >         return S_OK;
> >     }
> > 
> > +    // Drop the frames till system's timestamp aligns with the configured value.
> > +    if (0 == ctx->frameCount && cctx->timestamp_align) {
> > +        int64_t current_time_us = av_gettime();
> > +        int64_t align_factor_us = (cctx->timestamp_align * 1000000);
> > +        int remainder = current_time_us % align_factor_us;
> > +        if (videoFrame) {
> > +            videoFrame->GetStreamTime(&frameTime, &frameDuration, 1000000);
> > +        } else if (audioFrame) {
> > +            long sample_count = audioFrame->GetSampleFrameCount();
> > +            frameDuration = (long)(sample_count * 1000000) / bmdAudioSampleRate48kHz;
> > +        } else {
> > +            frameDuration = 0;
> > +        }
> > +        // threshold of one frameDuration
> > +        if(remainder > frameDuration) {
> > +            ++ctx->dropped;
> > +            return S_OK;
> > +        }
>
> You already know the frame rate, so I'd simply write something like:
Yes, that makes it very simple. Thanks for pointing it out with an example. That's really helpful.
>
> if (ctx->frameCount == 0 && ctx->timestamp_align) {
>      AVRational remainder = av_make_q(av_gettime() % cctx->timestamp_align, 1000000);
>      if (av_cmp_q(remainder, st->video_st->time_base) > 0) {
A minor change here. Timebase is reset to 1000000 units. Hence, I am using r_frame_rate for this.
>          ctx->dropped++;
>          return S_OK;
>      }
> }
>
> > +    }
> > +
> >     ctx->frameCount++;
> >     if (ctx->audio_pts_source == PTS_SRC_WALLCLOCK || ctx->video_pts_source == PTS_SRC_WALLCLOCK)
> >         wallclock = av_gettime_relative();
> > diff --git a/libavdevice/decklink_dec_c.c b/libavdevice/decklink_dec_c.c
> > index 6ab3819..bef9c14 100644
> > --- a/libavdevice/decklink_dec_c.c
> > +++ b/libavdevice/decklink_dec_c.c
> > @@ -84,6 +84,7 @@ static const AVOption options[] = {
> >     { "queue_size",    "input queue buffer size",   OFFSET(queue_size),   AV_OPT_TYPE_INT64, { .i64 = (1024 * 1024 * 1024)}, 0, INT64_MAX, DEC },
> >     { "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_INT, { .i64 = 0 }, 0, INT_MAX, DEC },
>
> AV_OPT_TYPE_DURATION.
Done
>
> Regards,
> Marton

Thanks and regards,
Karthick
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel