diff mbox

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

Message ID CAB0OVGqewYRGmmnkgUW19b3Skj-1C+joqn9BQ8Czai22V+QH3Q@mail.gmail.com
State Accepted
Headers show

Commit Message

Carl Eugen Hoyos Aug. 10, 2019, 12:47 p.m. UTC 
Hi!

Attached patch fixes a compilation warning when compiling without
FF_API_UNSANITIZED_BITRATES.

Please comment, Carl Eugen

Comments

James Almer Aug. 10, 2019, 3:01 p.m. UTC | #1 
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@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.

> +#endif
>             cpb->max_bitrate, cpb->min_bitrate, cpb->avg_bitrate,
>             cpb->buffer_size,
>             cpb->vbv_delay);
> -- 2.22.0
Carl Eugen Hoyos Aug. 10, 2019, 9:51 p.m. UTC | #2 
Am Sa., 10. Aug. 2019 um 17:01 Uhr schrieb James Almer <jamrial@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@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.

Thank you, Carl Eugen
Michael Niedermayer Aug. 11, 2019, 4:08 p.m. UTC | #3 
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@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@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

./ffmpeg -v verbose  -i matrixbench_mpeg2.mpg  -t 1 test.avi

Thanks

[...]
James Almer Aug. 11, 2019, 4:38 p.m. UTC | #4 
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@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@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...

> 
> ./ffmpeg -v verbose  -i matrixbench_mpeg2.mpg  -t 1 test.avi
> 
> Thanks
> 
> [...]
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Michael Niedermayer Aug. 11, 2019, 5:07 p.m. UTC | #5 
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@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@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

[...]
James Almer Aug. 11, 2019, 5:12 p.m. UTC | #6 
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@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@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-request@ffmpeg.org with subject "unsubscribe".
>
diff mbox

Patch

From 48f7c57037206fe734bd45dc12c6140220afe34f Mon Sep 17 00:00:00 2001
From: Carl Eugen Hoyos <ceffmpeg@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,
+#endif
            cpb->max_bitrate, cpb->min_bitrate, cpb->avg_bitrate,
            cpb->buffer_size,
            cpb->vbv_delay);
-- 
2.22.0