> On May 25, 2018, at 1:08 PM, Jonathan Morley <jmor...@pixsystem.com> wrote: > >> On May 25, 2018, at 8:32 AM, Marton Balint <c...@passwd.hu >> <mailto:c...@passwd.hu>> wrote: >> >> On Fri, 25 May 2018, Jonathan Morley wrote: >> >>> I apologize in advance for the formatting of this message. My mail >>> configuration impeded my efforts to use ‘—send-email’. >> >> Your mail is corrupted by new lines, attach the .patch file if you cannot >> resolve this. > > Will do! Sorry about that. The mail server’s auth setup kept me from using > ‘—send-email’ directly. I will send future patches as attachments.
Here is a copy of my adjustments for the patch so that I could apply it for testing. https://gist.github.com/dericed/af69d60a99ad235b17722a64ea9413ba <https://gist.github.com/dericed/af69d60a99ad235b17722a64ea9413ba> >>> This commit did pass 'make fate’ though the use of these changes requires >>> decklink hardware so I did not add any new fate tests. By default the new >>> optional behavior is skipped for old behavior. >>> >>> --- >>> libavdevice/decklink_common.cpp | 30 ----------------------------- >>> libavdevice/decklink_common.h | 42 ++++++++++++++++++++++++++++++ >>> +++++++++++ >> >> Why is this huge chunk of function movement needed? If there is a good >> reason to do that, then split it into a separate patch, if there is not, >> then just get rid of it. > > The move here was so that I could make use of the cross platform safe > DECKLINK string methods. I was only able to test with linux and build on mac, > but I wanted to do my best to make a contribution that _could_ work on all > platforms. I can break this into two patches if you prefer. > >>> libavdevice/decklink_common_c.h | 1 + >>> libavdevice/decklink_dec.cpp | 19 +++++++++++++++++++ >>> libavdevice/decklink_dec_c.c | 9 +++++++++ >>> 5 files changed, 71 insertions(+), 30 deletions(-) >>> >>> diff --git a/libavdevice/decklink_common.cpp b/libavdevice/decklink_common. >>> cpp >>> index d8cced7c74..aab9d85b94 100644 >>> --- a/libavdevice/decklink_common.cpp >>> +++ b/libavdevice/decklink_common.cpp >>> @@ -77,36 +77,6 @@ static IDeckLinkIterator >>> *decklink_create_iterator(AVFormatContext >>> *avctx) >>> return iter; >>> } >>> >>> -#ifdef _WIN32 >>> -static char *dup_wchar_to_utf8(wchar_t *w) >>> -{ >>> - char *s = NULL; >>> - int l = WideCharToMultiByte(CP_UTF8, 0, w, -1, 0, 0, 0, 0); >>> - s = (char *) av_malloc(l); >>> - if (s) >>> - WideCharToMultiByte(CP_UTF8, 0, w, -1, s, l, 0, 0); >>> - return s; >>> -} >>> -#define DECKLINK_STR OLECHAR * >>> -#define DECKLINK_STRDUP dup_wchar_to_utf8 >>> -#define DECKLINK_FREE(s) SysFreeString(s) >>> -#elif defined(__APPLE__) >>> -static char *dup_cfstring_to_utf8(CFStringRef w) >>> -{ >>> - char s[256]; >>> - CFStringGetCString(w, s, 255, kCFStringEncodingUTF8); >>> - return av_strdup(s); >>> -} >>> -#define DECKLINK_STR const __CFString * >>> -#define DECKLINK_STRDUP dup_cfstring_to_utf8 >>> -#define DECKLINK_FREE(s) CFRelease(s) >>> -#else >>> -#define DECKLINK_STR const char * >>> -#define DECKLINK_STRDUP av_strdup >>> -/* free() is needed for a string returned by the DeckLink SDL. */ >>> -#define DECKLINK_FREE(s) free((void *) s) >>> -#endif >>> - >>> HRESULT ff_decklink_get_display_name(IDeckLink *This, const char >>> **displayName) >>> { >>> DECKLINK_STR tmpDisplayName; >>> diff --git a/libavdevice/decklink_common.h b/libavdevice/decklink_common.h >>> index 57ee7d1d68..8c5f8e9f06 100644 >>> --- a/libavdevice/decklink_common.h >>> +++ b/libavdevice/decklink_common.h >>> @@ -34,6 +34,36 @@ >>> #define DECKLINK_BOOL bool >>> #endif >>> >>> +#ifdef _WIN32 >>> +static char *dup_wchar_to_utf8(wchar_t *w) >>> +{ >>> + char *s = NULL; >>> + int l = WideCharToMultiByte(CP_UTF8, 0, w, -1, 0, 0, 0, 0); >>> + s = (char *) av_malloc(l); >>> + if (s) >>> + WideCharToMultiByte(CP_UTF8, 0, w, -1, s, l, 0, 0); >>> + return s; >>> +} >>> +#define DECKLINK_STR OLECHAR * >>> +#define DECKLINK_STRDUP dup_wchar_to_utf8 >>> +#define DECKLINK_FREE(s) SysFreeString(s) >>> +#elif defined(__APPLE__) >>> +static char *dup_cfstring_to_utf8(CFStringRef w) >>> +{ >>> + char s[256]; >>> + CFStringGetCString(w, s, 255, kCFStringEncodingUTF8); >>> + return av_strdup(s); >>> +} >>> +#define DECKLINK_STR const __CFString * >>> +#define DECKLINK_STRDUP dup_cfstring_to_utf8 >>> +#define DECKLINK_FREE(s) CFRelease(s) >>> +#else >>> +#define DECKLINK_STR const char * >>> +#define DECKLINK_STRDUP av_strdup >>> +/* free() is needed for a string returned by the DeckLink SDL. */ >>> +#define DECKLINK_FREE(s) free((void *) s) >>> +#endif >>> + >>> class decklink_output_callback; >>> class decklink_input_callback; >>> >>> @@ -64,6 +94,7 @@ struct decklink_ctx { >>> BMDDisplayMode bmd_mode; >>> BMDVideoConnection video_input; >>> BMDAudioConnection audio_input; >>> + BMDTimecodeFormat tc_format; >>> int bmd_width; >>> int bmd_height; >>> int bmd_field_dominance; >>> @@ -140,6 +171,17 @@ static const BMDVideoConnection >>> decklink_video_connection_map[] = { >>> bmdVideoConnectionSVideo, >>> }; >>> >>> +static const BMDTimecodeFormat decklink_timecode_format_map[] = { >>> + (BMDTimecodeFormat)0, >>> + bmdTimecodeRP188VITC1, >>> + bmdTimecodeRP188VITC2, >>> + bmdTimecodeRP188LTC, >>> + bmdTimecodeRP188Any, >>> + bmdTimecodeVITC, >>> + bmdTimecodeVITCField2, >>> + bmdTimecodeSerial, >>> +}; >>> + >>> HRESULT ff_decklink_get_display_name(IDeckLink *This, const char >>> **displayName); >>> int ff_decklink_set_configs(AVFormatContext *avctx, decklink_direction_t >>> direction); >>> 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); >>> diff --git a/libavdevice/decklink_common_c.h b/libavdevice/decklink_common_ >>> c.h >>> index 08e9f9bbd5..32a5d70ee1 100644 >>> --- a/libavdevice/decklink_common_c.h >>> +++ b/libavdevice/decklink_common_c.h >>> @@ -50,6 +50,7 @@ struct decklink_cctx { >>> DecklinkPtsSource video_pts_source; >>> int audio_input; >>> int video_input; >>> + int tc_format; >>> int draw_bars; >>> char *format_code; >>> int raw_format; >>> diff --git a/libavdevice/decklink_dec.cpp b/libavdevice/decklink_dec.cpp >>> index 510637676c..818235bfa6 100644 >>> --- a/libavdevice/decklink_dec.cpp >>> +++ b/libavdevice/decklink_dec.cpp >>> @@ -726,6 +726,23 @@ HRESULT decklink_input_callback:: >>> VideoInputFrameArrived( >>> no_video = 0; >>> } >>> >>> + // Handle Timecode (if requested) >>> + if (ctx->tc_format && !(av_dict_get(ctx->video_st->metadata, >>> "timecode", NULL, 0)) && !no_video) { >>> + IDeckLinkTimecode *timecode; >>> + if (videoFrame->GetTimecode(ctx->tc_format, &timecode) == >>> S_OK) { >>> + DECKLINK_STR timecodeString = NULL; >>> + timecode->GetString(&timecodeString); >>> + const char* tc = DECKLINK_STRDUP(timecodeString); >>> + if (!(av_dict_set(&ctx->video_st->metadata, "timecode", >>> tc, 0))) >> >> Don't you need AV_DICT_DONT_STRDUP_VAL flag here? > > Yes. I agree. > >>> + av_log(avctx, AV_LOG_ERROR, "Unable to set >>> timecode\n"); >>> + if (timecodeString) >>> + DECKLINK_FREE(timecodeString); >>> + timecode->Release(); >>> + } else { >>> + av_log(avctx, AV_LOG_ERROR, "Unable to find timecode.\n"); >>> + } >>> + } >> >> Maybe it makes sense to put this under the else branch of if >> (videoFrame->GetFlags() & bmdFrameHasNoInputSource), because if you have no >> input, you won't have any (valid) timecode… > > I can move this. The logic still checks for !no_video, but moving it should > be more explicit. > > Sadly I no longer have access to the hardware I used when making these > changes and cannot test this kind of change. We have moved on to a > video-for-linux based solution with AJA hardware. (I am currently adding > support for TC to the v4l2 ffmpeg reader). I tested moving this function up a few lines into the else branch as Marton suggested and it worked well for me. I’m running this on a Mac. The timecode values when recording with this match what I get via Blackmagic Media Express, but the drop frame flag does not match. I’m testing with a vitc timecode source which is non-drop frame but the value shows in ffmpeg as drop frame, 07:09:56;19. >>> + >>> 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; >>> >>> @@ -939,6 +956,8 @@ av_cold int ff_decklink_read_header(AVFormatContext >>> *avctx) >>> ctx->teletext_lines = cctx->teletext_lines; >>> ctx->preroll = cctx->preroll; >>> ctx->duplex_mode = cctx->duplex_mode; >>> + if (cctx->tc_format > 0 && (unsigned int)cctx->tc_format < >>> FF_ARRAY_ELEMS(decklink_timecode_format_map)) >>> + ctx->tc_format = decklink_timecode_format_map[cctx->tc_format]; >>> if (cctx->video_input > 0 && (unsigned int)cctx->video_input < >>> FF_ARRAY_ELEMS(decklink_video_connection_map)) >>> ctx->video_input = decklink_video_connection_map[cctx->video_input]; >>> if (cctx->audio_input > 0 && (unsigned int)cctx->audio_input < >>> FF_ARRAY_ELEMS(decklink_audio_connection_map)) >>> diff --git a/libavdevice/decklink_dec_c.c b/libavdevice/decklink_dec_c.c >>> index 47018dc681..6ab3819375 100644 >>> --- a/libavdevice/decklink_dec_c.c >>> +++ b/libavdevice/decklink_dec_c.c >>> @@ -48,6 +48,15 @@ static const AVOption options[] = { >>> { "unset", NULL, 0, >>> AV_OPT_TYPE_CONST, { .i64 = 0}, 0, 0, DEC, "duplex_mode"}, >>> { "half", NULL, 0, >>> AV_OPT_TYPE_CONST, { .i64 = 1}, 0, 0, DEC, "duplex_mode"}, >>> { "full", NULL, 0, >>> AV_OPT_TYPE_CONST, { .i64 = 2}, 0, 0, DEC, "duplex_mode"}, >>> + { "timecode_format", "timecode format", OFFSET(tc_format), >>> AV_OPT_TYPE_INT, { .i64 = 0}, 0, 7, DEC, "tc_format"}, >>> + { "none", NULL, 0, >>> AV_OPT_TYPE_CONST, { .i64 = 0}, 0, 0, DEC, "tc_format"}, >>> + { "rp188vitc", NULL, 0, >>> AV_OPT_TYPE_CONST, { .i64 = 1}, 0, 0, DEC, "tc_format"}, >>> + { "rp188vitc2", NULL, 0, >>> AV_OPT_TYPE_CONST, { .i64 = 2}, 0, 0, DEC, "tc_format"}, >>> + { "rp188ltc", NULL, 0, >>> AV_OPT_TYPE_CONST, { .i64 = 3}, 0, 0, DEC, "tc_format"}, >>> + { "rp188any", NULL, 0, >>> AV_OPT_TYPE_CONST, { .i64 = 4}, 0, 0, DEC, "tc_format"}, >>> + { "vitc", NULL, 0, >>> AV_OPT_TYPE_CONST, { .i64 = 5}, 0, 0, DEC, "tc_format"}, >>> + { "vitc2", NULL, 0, >>> AV_OPT_TYPE_CONST, { .i64 = 6}, 0, 0, DEC, "tc_format"}, >>> + { "serial", NULL, 0, >>> AV_OPT_TYPE_CONST, { .i64 = 7}, 0, 0, DEC, "tc_format"}, >>> { "video_input", "video input", OFFSET(video_input), >>> AV_OPT_TYPE_INT, { .i64 = 0}, 0, 6, DEC, "video_input"}, >>> { "unset", NULL, 0, >>> AV_OPT_TYPE_CONST, { .i64 = 0}, 0, 0, DEC, "video_input"}, >>> { "sdi", NULL, 0, >>> AV_OPT_TYPE_CONST, { .i64 = 1}, 0, 0, DEC, "video_input"}, >>> -- >> >> Documentation update is missing. > > D’oh! Sloppy. Thanks for this work! Dave _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
