[FFmpeg-devel] [PATCH] Limited timecode support for lavd/decklink
Jonathan Morley
jmorley at pixsystem.com
Fri May 25 20:08:22 EEST 2018
> On May 25, 2018, at 8:32 AM, Marton Balint <cus at 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.
>>
>> 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).
>> +
>> 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.
> Regards,
> Marton
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org <mailto:ffmpeg-devel at ffmpeg.org>
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel <http://ffmpeg.org/mailman/listinfo/ffmpeg-devel>
Please have a look at my comments above and let me know about splitting this into two patches and I will resubmit as attachments.
Thank you for taking a look!
More information about the ffmpeg-devel
mailing list