Message ID | CAB0OVGqewYRGmmnkgUW19b3Skj-1C+joqn9BQ8Czai22V+QH3Q@mail.gmail.com |
---|---|
State | Accepted |
Headers | show |
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
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
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 [...]
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". >
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 [...]
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". >
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