Attached are the two patches to address your feedback, Marton. Please advise.
Thanks,
Jon
> On May 25, 2018, at 8:32 AM, Marton Balint <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.
>
>>
>> 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.
>
>> 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?
>
>> + 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...
>
>> +
>> 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.
>
> Regards,
> Marton
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org <mailto:ffmpeg-devel@ffmpeg.org>
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> <http://ffmpeg.org/mailman/listinfo/ffmpeg-devel>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
