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

[Michael Niedermayer]

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

Is this line used more by developers/user reading it or scripts?
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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who would give up essential Liberty, to purchase a little
temporary Safety, deserve neither Liberty nor Safety -- Benjamin Franklin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190811/e3e59c56/attachment.sig>