Message ID | AM7PR03MB6660BDC5A4B1729039D130A88F9B9@AM7PR03MB6660.eurprd03.prod.outlook.com |
---|---|
State | Accepted |
Commit | bbf00916e45ac8754f208c0584834cb330a89da2 |
Headers | show |
Series | [FFmpeg-devel,1/4] fftools/cmdutils: Fix undefined 1 << 31 | expand |
Context | Check | Description |
---|---|---|
andriy/make_x86 | success | Make finished |
andriy/make_fate_x86 | success | Make fate finished |
andriy/make_ppc | success | Make finished |
andriy/make_fate_ppc | success | Make fate finished |
On Thu, Nov 18, 2021 at 10:07:36AM +0100, Andreas Rheinhardt wrote: > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > fftools/cmdutils.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c > index 594eeef379..f80c361eba 100644 > --- a/fftools/cmdutils.c > +++ b/fftools/cmdutils.c > @@ -1823,7 +1823,7 @@ int show_sample_fmts(void *optctx, const char *opt, const char *arg) > int show_dispositions(void *optctx, const char *opt, const char *arg) > { > for (int i = 0; i < 32; i++) { > - const char *str = av_disposition_to_string(1 << i); > + const char *str = av_disposition_to_string(1U << i); is it intended to have 1U<<31 cast to int and then checked as in if (disposition <= 0) return NULL; ? [...]
Michael Niedermayer: > On Thu, Nov 18, 2021 at 10:07:36AM +0100, Andreas Rheinhardt wrote: >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >> --- >> fftools/cmdutils.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c >> index 594eeef379..f80c361eba 100644 >> --- a/fftools/cmdutils.c >> +++ b/fftools/cmdutils.c >> @@ -1823,7 +1823,7 @@ int show_sample_fmts(void *optctx, const char *opt, const char *arg) >> int show_dispositions(void *optctx, const char *opt, const char *arg) >> { >> for (int i = 0; i < 32; i++) { >> - const char *str = av_disposition_to_string(1 << i); >> + const char *str = av_disposition_to_string(1U << i); > > is it intended to have 1U<<31 cast to int and then checked as in > if (disposition <= 0) > return NULL; > > ? > I agree that the types here (and also in ff_ctz (the relevant GCC builtins use unsigned types) are unfortunate and AVStream.disposition as well as the other types involved here should be unsigned. Shall I change av_disposition_to_string to unsigned and schedule AVStream.disposition to unsigned on the next bump? Or shall I just use "i < 31" above? - Andreas
On Thu, Nov 18, 2021 at 09:15:05PM +0100, Andreas Rheinhardt wrote: > Michael Niedermayer: > > On Thu, Nov 18, 2021 at 10:07:36AM +0100, Andreas Rheinhardt wrote: > >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > >> --- > >> fftools/cmdutils.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c > >> index 594eeef379..f80c361eba 100644 > >> --- a/fftools/cmdutils.c > >> +++ b/fftools/cmdutils.c > >> @@ -1823,7 +1823,7 @@ int show_sample_fmts(void *optctx, const char *opt, const char *arg) > >> int show_dispositions(void *optctx, const char *opt, const char *arg) > >> { > >> for (int i = 0; i < 32; i++) { > >> - const char *str = av_disposition_to_string(1 << i); > >> + const char *str = av_disposition_to_string(1U << i); > > > > is it intended to have 1U<<31 cast to int and then checked as in > > if (disposition <= 0) > > return NULL; > > > > ? > > > > I agree that the types here (and also in ff_ctz (the relevant GCC > builtins use unsigned types) are unfortunate and AVStream.disposition as > well as the other types involved here should be unsigned. Shall I change > av_disposition_to_string to unsigned and schedule AVStream.disposition > to unsigned on the next bump? Or shall I just use "i < 31" above? i have no strong preferrance, i see 20 ATM so a 31 limit seems fine but if the type is changed because 31 is not enough, maybe we should go to 64. thx [...]
On 11/18/2021 6:07 AM, Andreas Rheinhardt wrote: > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > fftools/cmdutils.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c > index 594eeef379..f80c361eba 100644 > --- a/fftools/cmdutils.c > +++ b/fftools/cmdutils.c > @@ -1823,7 +1823,7 @@ int show_sample_fmts(void *optctx, const char *opt, const char *arg) > int show_dispositions(void *optctx, const char *opt, const char *arg) > { > for (int i = 0; i < 32; i++) { > - const char *str = av_disposition_to_string(1 << i); > + const char *str = av_disposition_to_string(1U << i); LGTM. > if (str) > printf("%s\n", str); > } >
Quoting Andreas Rheinhardt (2021-11-18 21:15:05) > Michael Niedermayer: > > On Thu, Nov 18, 2021 at 10:07:36AM +0100, Andreas Rheinhardt wrote: > >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > >> --- > >> fftools/cmdutils.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c > >> index 594eeef379..f80c361eba 100644 > >> --- a/fftools/cmdutils.c > >> +++ b/fftools/cmdutils.c > >> @@ -1823,7 +1823,7 @@ int show_sample_fmts(void *optctx, const char *opt, const char *arg) > >> int show_dispositions(void *optctx, const char *opt, const char *arg) > >> { > >> for (int i = 0; i < 32; i++) { > >> - const char *str = av_disposition_to_string(1 << i); > >> + const char *str = av_disposition_to_string(1U << i); > > > > is it intended to have 1U<<31 cast to int and then checked as in > > if (disposition <= 0) > > return NULL; > > > > ? > > > > I agree that the types here (and also in ff_ctz (the relevant GCC > builtins use unsigned types) are unfortunate and AVStream.disposition as > well as the other types involved here should be unsigned. Shall I change > av_disposition_to_string to unsigned and schedule AVStream.disposition > to unsigned on the next bump? Or shall I just use "i < 31" above? That's actually a typo, I remember intending to write i < 31, but apparently failed. If you want to change the type then that's fine too, I think uint64_t would be appropriate, given that over half the space is already used.
Andreas Rheinhardt: > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > fftools/cmdutils.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c > index 594eeef379..f80c361eba 100644 > --- a/fftools/cmdutils.c > +++ b/fftools/cmdutils.c > @@ -1823,7 +1823,7 @@ int show_sample_fmts(void *optctx, const char *opt, const char *arg) > int show_dispositions(void *optctx, const char *opt, const char *arg) > { > for (int i = 0; i < 32; i++) { > - const char *str = av_disposition_to_string(1 << i); > + const char *str = av_disposition_to_string(1U << i); > if (str) > printf("%s\n", str); > } > I know I have promised to make the disposition field (and the av_disposition_to_string() function) use 64bits for the disposition; yet I have forgotten this and now it is too late to change it. Sorry for this. (Fortunately we will probably not run out of bits during this major version period.) So I will just apply and backport this patch as-is tonight unless someone disagrees with this. - Andreas
diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c index 594eeef379..f80c361eba 100644 --- a/fftools/cmdutils.c +++ b/fftools/cmdutils.c @@ -1823,7 +1823,7 @@ int show_sample_fmts(void *optctx, const char *opt, const char *arg) int show_dispositions(void *optctx, const char *opt, const char *arg) { for (int i = 0; i < 32; i++) { - const char *str = av_disposition_to_string(1 << i); + const char *str = av_disposition_to_string(1U << i); if (str) printf("%s\n", str); }
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- fftools/cmdutils.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)