diff mbox series

[FFmpeg-devel,1/4] fftools/cmdutils: Fix undefined 1 << 31

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

Checks

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

Commit Message

Andreas Rheinhardt Nov. 18, 2021, 9:07 a.m. UTC 
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 fftools/cmdutils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Michael Niedermayer Nov. 18, 2021, 8:06 p.m. UTC | #1 
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;

 ?
 
[...]
Andreas Rheinhardt Nov. 18, 2021, 8:15 p.m. UTC | #2 
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
Michael Niedermayer Nov. 18, 2021, 9:28 p.m. UTC | #3 
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

[...]
James Almer Nov. 18, 2021, 11:01 p.m. UTC | #4 
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);
>       }
>
Anton Khirnov Nov. 19, 2021, 1:12 p.m. UTC | #5 
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 Jan. 10, 2022, 6:08 p.m. UTC | #6 
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 mbox series

Patch

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);
     }