diff mbox

[FFmpeg-devel,v1,6/6] avfilter/vf_random: seeds is uint32, it's enough to use int32_t

Message ID 20191011061444.4988-6-lance.lmwang@gmail.com
State New
Headers show

Commit Message

Lance Wang Oct. 11, 2019, 6:14 a.m. UTC
From: Limin Wang <lance.lmwang@gmail.com>

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavfilter/vf_random.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Andreas Rheinhardt Oct. 11, 2019, 6:20 a.m. UTC | #1
lance.lmwang@gmail.com:
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  libavfilter/vf_random.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavfilter/vf_random.c b/libavfilter/vf_random.c
> index 1937eae771..9f2153be61 100644
> --- a/libavfilter/vf_random.c
> +++ b/libavfilter/vf_random.c
> @@ -33,7 +33,7 @@ typedef struct RandomContext {
>  
>      AVLFG lfg;
>      int nb_frames;
> -    int64_t random_seed;
> +    int32_t random_seed;
>      int nb_frames_filled;
>      AVFrame *frames[MAX_FRAMES];
>      int64_t pts[MAX_FRAMES];
> 
    { "seed",   "set the seed",                  OFFSET(random_seed),
AV_OPT_TYPE_INT64, {.i64=-1}, -1, UINT32_MAX, FLAGS },

So in addition to the complete range of an uint32_t one also needs
another value that instructs init to get a random seed of its own.

- Andreas
Lance Wang Oct. 11, 2019, 6:35 a.m. UTC | #2
On Fri, Oct 11, 2019 at 06:20:00AM +0000, Andreas Rheinhardt wrote:
> lance.lmwang@gmail.com:
> > From: Limin Wang <lance.lmwang@gmail.com>
> > 
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> >  libavfilter/vf_random.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/libavfilter/vf_random.c b/libavfilter/vf_random.c
> > index 1937eae771..9f2153be61 100644
> > --- a/libavfilter/vf_random.c
> > +++ b/libavfilter/vf_random.c
> > @@ -33,7 +33,7 @@ typedef struct RandomContext {
> >  
> >      AVLFG lfg;
> >      int nb_frames;
> > -    int64_t random_seed;
> > +    int32_t random_seed;
> >      int nb_frames_filled;
> >      AVFrame *frames[MAX_FRAMES];
> >      int64_t pts[MAX_FRAMES];
> > 
>     { "seed",   "set the seed",                  OFFSET(random_seed),
> AV_OPT_TYPE_INT64, {.i64=-1}, -1, UINT32_MAX, FLAGS },
> 
> So in addition to the complete range of an uint32_t one also needs
> another value that instructs init to get a random seed of its own.

yes, the max range is UINT32_MAX, so no need to use int64_t


> 
> - Andreas
> _______________________________________________
> 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".
Andreas Rheinhardt Oct. 11, 2019, 6:59 a.m. UTC | #3
Limin Wang:
> On Fri, Oct 11, 2019 at 06:20:00AM +0000, Andreas Rheinhardt wrote:
>> lance.lmwang@gmail.com:
>>> From: Limin Wang <lance.lmwang@gmail.com>
>>>
>>> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
>>> ---
>>>  libavfilter/vf_random.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/libavfilter/vf_random.c b/libavfilter/vf_random.c
>>> index 1937eae771..9f2153be61 100644
>>> --- a/libavfilter/vf_random.c
>>> +++ b/libavfilter/vf_random.c
>>> @@ -33,7 +33,7 @@ typedef struct RandomContext {
>>>  
>>>      AVLFG lfg;
>>>      int nb_frames;
>>> -    int64_t random_seed;
>>> +    int32_t random_seed;
>>>      int nb_frames_filled;
>>>      AVFrame *frames[MAX_FRAMES];
>>>      int64_t pts[MAX_FRAMES];
>>>
>>     { "seed",   "set the seed",                  OFFSET(random_seed),
>> AV_OPT_TYPE_INT64, {.i64=-1}, -1, UINT32_MAX, FLAGS },
>>
>> So in addition to the complete range of an uint32_t one also needs
>> another value that instructs init to get a random seed of its own.
> 
> yes, the max range is UINT32_MAX, so no need to use int64_t
> 
First of all, simply changing the value to 32 bit is not right -- you
would also have to change the type in the corresponding AVOption (it
might seem to work on little-endian systems, but it certainly doesn't
on big-endian systems; actually using -1 as random_seed would probably
already not work on little-endian systems either). Second, there are
more legal values for random_seed than values representable in 32 bit,
hence one has to use a type with more than 32 bit.

- Andreas
Lance Wang Oct. 11, 2019, 7:22 a.m. UTC | #4
On Fri, Oct 11, 2019 at 06:59:00AM +0000, Andreas Rheinhardt wrote:
> Limin Wang:
> > On Fri, Oct 11, 2019 at 06:20:00AM +0000, Andreas Rheinhardt wrote:
> >> lance.lmwang@gmail.com:
> >>> From: Limin Wang <lance.lmwang@gmail.com>
> >>>
> >>> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> >>> ---
> >>>  libavfilter/vf_random.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/libavfilter/vf_random.c b/libavfilter/vf_random.c
> >>> index 1937eae771..9f2153be61 100644
> >>> --- a/libavfilter/vf_random.c
> >>> +++ b/libavfilter/vf_random.c
> >>> @@ -33,7 +33,7 @@ typedef struct RandomContext {
> >>>  
> >>>      AVLFG lfg;
> >>>      int nb_frames;
> >>> -    int64_t random_seed;
> >>> +    int32_t random_seed;
> >>>      int nb_frames_filled;
> >>>      AVFrame *frames[MAX_FRAMES];
> >>>      int64_t pts[MAX_FRAMES];
> >>>
> >>     { "seed",   "set the seed",                  OFFSET(random_seed),
> >> AV_OPT_TYPE_INT64, {.i64=-1}, -1, UINT32_MAX, FLAGS },
> >>
> >> So in addition to the complete range of an uint32_t one also needs
> >> another value that instructs init to get a random seed of its own.
> > 
> > yes, the max range is UINT32_MAX, so no need to use int64_t
> > 
> First of all, simply changing the value to 32 bit is not right -- you
> would also have to change the type in the corresponding AVOption (it

I haven't see .i32 type for AVOption

> might seem to work on little-endian systems, but it certainly doesn't
> on big-endian systems; actually using -1 as random_seed would probably
> already not work on little-endian systems either). Second, there are
> more legal values for random_seed than values representable in 32 bit,
> hence one has to use a type with more than 32 bit.

random_seed = -1, it'll get the seed by av_get_random_seed(), it'll
return 32bit.

else 

uint32_t seed
     seed = s->random_seed;
     av_lfg_init(&s->lfg, seed);

the seed is uint32_t still, so user can't use with more than 32bit
for the current code.

> 
> - Andreas
> _______________________________________________
> 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".
Andreas Rheinhardt Oct. 11, 2019, 7:34 a.m. UTC | #5
Limin Wang:
> On Fri, Oct 11, 2019 at 06:59:00AM +0000, Andreas Rheinhardt wrote:
>> Limin Wang:
>>> On Fri, Oct 11, 2019 at 06:20:00AM +0000, Andreas Rheinhardt wrote:
>>>> lance.lmwang@gmail.com:
>>>>> From: Limin Wang <lance.lmwang@gmail.com>
>>>>>
>>>>> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
>>>>> ---
>>>>>  libavfilter/vf_random.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/libavfilter/vf_random.c b/libavfilter/vf_random.c
>>>>> index 1937eae771..9f2153be61 100644
>>>>> --- a/libavfilter/vf_random.c
>>>>> +++ b/libavfilter/vf_random.c
>>>>> @@ -33,7 +33,7 @@ typedef struct RandomContext {
>>>>>  
>>>>>      AVLFG lfg;
>>>>>      int nb_frames;
>>>>> -    int64_t random_seed;
>>>>> +    int32_t random_seed;
>>>>>      int nb_frames_filled;
>>>>>      AVFrame *frames[MAX_FRAMES];
>>>>>      int64_t pts[MAX_FRAMES];
>>>>>
>>>>     { "seed",   "set the seed",                  OFFSET(random_seed),
>>>> AV_OPT_TYPE_INT64, {.i64=-1}, -1, UINT32_MAX, FLAGS },
>>>>
>>>> So in addition to the complete range of an uint32_t one also needs
>>>> another value that instructs init to get a random seed of its own.
>>>
>>> yes, the max range is UINT32_MAX, so no need to use int64_t
>>>
>> First of all, simply changing the value to 32 bit is not right -- you
>> would also have to change the type in the corresponding AVOption (it
> 
> I haven't see .i32 type for AVOption
> 
>> might seem to work on little-endian systems, but it certainly doesn't
>> on big-endian systems; actually using -1 as random_seed would probably
>> already not work on little-endian systems either). Second, there are
>> more legal values for random_seed than values representable in 32 bit,
>> hence one has to use a type with more than 32 bit.
> 
> random_seed = -1, it'll get the seed by av_get_random_seed(), it'll
> return 32bit.
> 
> else 
> 
> uint32_t seed
>      seed = s->random_seed;
>      av_lfg_init(&s->lfg, seed);
> 
> the seed is uint32_t still, so user can't use with more than 32bit
> for the current code.
> 
There are more than 2^32 legal values the user can input for
random_seed: The 2^32 values which should be directly used as random
seed and the special value -1. You need a type with more than 32 bits
to distinguish -1 from UINT32_MAX.

- Andreas
Lance Wang Oct. 11, 2019, 7:43 a.m. UTC | #6
On Fri, Oct 11, 2019 at 07:34:00AM +0000, Andreas Rheinhardt wrote:
> Limin Wang:
> > On Fri, Oct 11, 2019 at 06:59:00AM +0000, Andreas Rheinhardt wrote:
> >> Limin Wang:
> >>> On Fri, Oct 11, 2019 at 06:20:00AM +0000, Andreas Rheinhardt wrote:
> >>>> lance.lmwang@gmail.com:
> >>>>> From: Limin Wang <lance.lmwang@gmail.com>
> >>>>>
> >>>>> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> >>>>> ---
> >>>>>  libavfilter/vf_random.c | 2 +-
> >>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/libavfilter/vf_random.c b/libavfilter/vf_random.c
> >>>>> index 1937eae771..9f2153be61 100644
> >>>>> --- a/libavfilter/vf_random.c
> >>>>> +++ b/libavfilter/vf_random.c
> >>>>> @@ -33,7 +33,7 @@ typedef struct RandomContext {
> >>>>>  
> >>>>>      AVLFG lfg;
> >>>>>      int nb_frames;
> >>>>> -    int64_t random_seed;
> >>>>> +    int32_t random_seed;
> >>>>>      int nb_frames_filled;
> >>>>>      AVFrame *frames[MAX_FRAMES];
> >>>>>      int64_t pts[MAX_FRAMES];
> >>>>>
> >>>>     { "seed",   "set the seed",                  OFFSET(random_seed),
> >>>> AV_OPT_TYPE_INT64, {.i64=-1}, -1, UINT32_MAX, FLAGS },
> >>>>
> >>>> So in addition to the complete range of an uint32_t one also needs
> >>>> another value that instructs init to get a random seed of its own.
> >>>
> >>> yes, the max range is UINT32_MAX, so no need to use int64_t
> >>>
> >> First of all, simply changing the value to 32 bit is not right -- you
> >> would also have to change the type in the corresponding AVOption (it
> > 
> > I haven't see .i32 type for AVOption
> > 
> >> might seem to work on little-endian systems, but it certainly doesn't
> >> on big-endian systems; actually using -1 as random_seed would probably
> >> already not work on little-endian systems either). Second, there are
> >> more legal values for random_seed than values representable in 32 bit,
> >> hence one has to use a type with more than 32 bit.
> > 
> > random_seed = -1, it'll get the seed by av_get_random_seed(), it'll
> > return 32bit.
> > 
> > else 
> > 
> > uint32_t seed
> >      seed = s->random_seed;
> >      av_lfg_init(&s->lfg, seed);
> > 
> > the seed is uint32_t still, so user can't use with more than 32bit
> > for the current code.
> > 
> There are more than 2^32 legal values the user can input for
> random_seed: The 2^32 values which should be directly used as random
> seed and the special value -1. You need a type with more than 32 bits
> to distinguish -1 from UINT32_MAX.

OK, thanks for reviewing and feedback, I haven't think it too much at
first, let keep it 64bit anyway.

> 
> - Andreas
> _______________________________________________
> 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

diff --git a/libavfilter/vf_random.c b/libavfilter/vf_random.c
index 1937eae771..9f2153be61 100644
--- a/libavfilter/vf_random.c
+++ b/libavfilter/vf_random.c
@@ -33,7 +33,7 @@  typedef struct RandomContext {
 
     AVLFG lfg;
     int nb_frames;
-    int64_t random_seed;
+    int32_t random_seed;
     int nb_frames_filled;
     AVFrame *frames[MAX_FRAMES];
     int64_t pts[MAX_FRAMES];