[FFmpeg-devel] [PATCH]lavf/dump: Fix cpb bitrate types after next major bump

[James Almer]

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 <jamrial at 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 <ceffmpeg at 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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>