On 8/11/2019 2:07 PM, Michael Niedermayer wrote: > On Sun, Aug 11, 2019 at 01:38:38PM -0300, James Almer wrote: >> On 8/11/2019 1:08 PM, Michael Niedermayer wrote: >>> On Sat, Aug 10, 2019 at 11:51:15PM +0200, Carl Eugen Hoyos wrote: >>>> Am Sa., 10. Aug. 2019 um 17:01 Uhr schrieb James Almer <jamr...@gmail.com>: >>>>> >>>>> On 8/10/2019 9:47 AM, Carl Eugen Hoyos wrote: >>>>>> Hi! >>>>>> >>>>>> Attached patch fixes a compilation warning when compiling without >>>>>> FF_API_UNSANITIZED_BITRATES. >>>>>> >>>>>> Please comment, Carl Eugen >>>>>> >>>>>> >>>>>> 0001-lavf-dump-Fix-cpb-bitrate-type-after-next-major-bump.patch >>>>>> >>>>>> From 48f7c57037206fe734bd45dc12c6140220afe34f Mon Sep 17 00:00:00 2001 >>>>>> From: Carl Eugen Hoyos <ceffm...@gmail.com> >>>>>> Date: Sat, 10 Aug 2019 14:43:58 +0200 >>>>>> Subject: [PATCH] lavf/dump: Fix cpb bitrate type after next major bump. >>>>>> >>>>>> --- >>>>>> libavformat/dump.c | 4 ++++ >>>>>> 1 file changed, 4 insertions(+) >>>>>> >>>>>> diff --git a/libavformat/dump.c b/libavformat/dump.c >>>>>> index 1c44656071..126641259d 100644 >>>>>> --- a/libavformat/dump.c >>>>>> +++ b/libavformat/dump.c >>>>>> @@ -320,7 +320,11 @@ static void dump_cpb(void *ctx, AVPacketSideData >>>>>> *sd) >>>>>> } >>>>>> >>>>>> av_log(ctx, AV_LOG_INFO, >>>>>> +#if FF_API_UNSANITIZED_BITRATES >>>>>> "bitrate max/min/avg: %d/%d/%d buffer size: %d vbv_delay: >>>>>> %"PRId64, >>>>>> +#else >>>>>> + "bitrate max/min/avg: %"PRIu64"/%"PRIu64"/%"PRIu64" buffer >>>>>> size: %d vbv_delay: %"PRId64, >>>>> >>>>> They are int64_t, so PRId64. vbv_delay is what should be PRIu64. >>>>> >>>>> LGTM with that fixed. >>>> >>>> Pushed with that fix. >>> >>> This produces: >>> cpb: bitrate max/min/avg: 0/0/200000 buffer size: 0 vbv_delay: >>> 18446744073709551615 >>> prior: >>> cpb: bitrate max/min/avg: 0/0/200000 buffer size: 0 vbv_delay: -1 >>> >>> This doesnt look intended >> >> The field is an uint64_t one, and the doxy states "UINT64_MAX when >> unknown or unspecified". >> >> I know -1 looks nicer, but if values > INT64_MAX are valid and expected >> for this field, then printing with PRId64 was clearly not the intention. >> What i wonder about however is scripts that already take into >> consideration the buggy printing behavior, looking for -1 while parsing >> the output string from av_dump_format() to report it as undefined. Afaik >> it's not something we should consider, since the proper API usage is >> looking at the side data proper, but i know some workflows using the cli >> exist, so... > > i think if we change what is printed from the original "-1" then it should > be something like "N/A", "unspecified" or the "vbv_delay" should be > ommited altogether from the line. > in the same sense the max bitrate and buffer size in this case maybe > should be changed too
Agree, we don't print a lot of other fields when their values are undefined or "default". The same should be done here. > > Is this line used more by developers/user reading it or scripts? That's what i wondered above, and to be honest, i don't think we should take potential scripts into consideration here. The side data API is there for this purpose, and the output of av_dump_format() is not defined and was never meant to be used for string parsing. > if its humans then i think its better to ommit unknown fields instead > of using special nummeric value with specific meaning defined in some > docs. > > thx > > [...] > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". > _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
