[FFmpeg-devel] [PATCH 2/3] avutil/opt: Add AV_OPT_TYPE_UINT64

[Michael Niedermayer]

On Sun, Nov 20, 2016 at 12:00:49PM -0500, Ronald S. Bultje wrote:
> Hi,
> 
> On Sun, Nov 20, 2016 at 9:16 AM, Michael Niedermayer <michael at niedermayer.cc
> > wrote:
> 
> > On Sun, Nov 20, 2016 at 08:56:15AM -0500, Ronald S. Bultje wrote:
> > > Hi,
> > >
> > > On Sun, Nov 20, 2016 at 6:57 AM, Michael Niedermayer
> > <michael at niedermayer.cc
> > > > wrote:
> > >
> > > > @@ -131,6 +132,20 @@ static int write_number(void *obj, const AVOption
> > *o,
> > > > void *dst, double num, int
> > > >          if (intnum == 1 && d == (double)INT64_MAX) *(int64_t *)dst =
> > > > INT64_MAX;
> > > >          else                                       *(int64_t *)dst =
> > > > llrint(d) * intnum;
> > > >          break;}
> > > > +    case AV_OPT_TYPE_UINT64:{
> > > > +        double d = num / den;
> > > > +        // We must special case uint64_t here as llrint() does not
> > > > support values
> > > > +        // outside the int64_t range and there is no portable function
> > > > which does
> > > > +        // "INT64_MAX + 1ULL" is used as it is representable exactly
> > as
> > > > IEEE double
> > > > +        // while INT64_MAX is not
> > > > +        if (intnum == 1 && d == (double)UINT64_MAX) {
> > > > +            *(int64_t *)dst = UINT64_MAX;
> > > > +        } else if (o->max > INT64_MAX + 1ULL && d > INT64_MAX + 1ULL)
> > {
> > > > +            *(uint64_t *)dst = (llrint(d - (INT64_MAX + 1ULL)) +
> > > > (INT64_MAX + 1ULL))*intnum;
> > > > +        } else {
> > > > +            *(int64_t *)dst = llrint(d) * intnum;
> > > > +        }
> > > > +        break;}
> > > >      case AV_OPT_TYPE_FLOAT:
> > > >          *(float *)dst = num * intnum / den;
> > > >          break;
> > >
> > >
> > > For the stupid, like me: what does this do? More specifically, this seems
> > > an integer codepath, but there is a double in there. Why?
> >
> > write_number() has a num/den double argument. If this is used to
> > set a uint64_t to UINT64_MAX things fail as double cannot exactly
> > represent UINT64_MAX.  (the closest it can represent is "UINT64_MAX + 1"
> > So it needs to be handled as a special case. Otherwise it turns into 0
> 
> 
> This sounds incredibly shaky. Is write_number() so fringe that we don't
> need an integer codepath?

write_number() can take int64_t, double and 32/32bit rationals.
Its done compactly by representing the value as a
num/den * intnum
for integer input num/den == 1
for double or rational input intnum == 1
doubles can represent int32 exactly so it can be used simplifying the
code
so there is a integer codepath basically

Ive written this 11 years ago
(860a40c8a7756a3e63e7f9bb0d9caaf742d26402 av_set_number())
which also means it had 11 years of testing on many platforms

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Frequently ignored answer#1 FFmpeg bugs should be sent to our bugtracker. User
questions about the command line tools should be sent to the ffmpeg-user ML.
And questions about how to use libav* should be sent to the libav-user ML.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20161120/31699e04/attachment.sig>