| Message ID | 20161120115744.28351-2-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Headers | show |
Hi, On Sun, Nov 20, 2016 at 6:57 AM, Michael Niedermayer <michael@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? Ronald
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@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 [...]
Hi, On Sun, Nov 20, 2016 at 9:16 AM, Michael Niedermayer <michael@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@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? Ronald
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@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@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 [...]
On 20.11.2016 12:57, Michael Niedermayer wrote: > Requested-by: wm4 ([FFmpeg-devel] [PATCH] avutil/opt: Support max > INT64_MAX in write_number() with AV_OPT_TYPE_INT64) > Requested-by: ronald ([FFmpeg-devel] [PATCH] avutil/opt: Support max > INT64_MAX in write_number() with AV_OPT_TYPE_INT64) > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavutil/opt.c | 29 +++++++++++++++++++++++++++++ > libavutil/opt.h | 1 + > 2 files changed, 30 insertions(+) > > diff --git a/libavutil/opt.c b/libavutil/opt.c > index 6669356..b6b4d9f 100644 > --- a/libavutil/opt.c > +++ b/libavutil/opt.c > @@ -74,6 +74,7 @@ static int read_number(const AVOption *o, const void *dst, double *num, int *den > case AV_OPT_TYPE_CHANNEL_LAYOUT: > case AV_OPT_TYPE_DURATION: > case AV_OPT_TYPE_INT64: > + case AV_OPT_TYPE_UINT64: > *intnum = *(int64_t *)dst; > return 0; > case AV_OPT_TYPE_FLOAT: > @@ -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; Is there a reason why this uses int64_t, > + } else if (o->max > INT64_MAX + 1ULL && d > INT64_MAX + 1ULL) { > + *(uint64_t *)dst = (llrint(d - (INT64_MAX + 1ULL)) + (INT64_MAX + 1ULL))*intnum; but this uint64_t, > + } else { > + *(int64_t *)dst = llrint(d) * intnum; and this again int64_t? Best regards, Andreas
On Sun, Nov 20, 2016 at 08:55:44PM +0100, Andreas Cadhalpun wrote: > On 20.11.2016 12:57, Michael Niedermayer wrote: > > Requested-by: wm4 ([FFmpeg-devel] [PATCH] avutil/opt: Support max > INT64_MAX in write_number() with AV_OPT_TYPE_INT64) > > Requested-by: ronald ([FFmpeg-devel] [PATCH] avutil/opt: Support max > INT64_MAX in write_number() with AV_OPT_TYPE_INT64) > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavutil/opt.c | 29 +++++++++++++++++++++++++++++ > > libavutil/opt.h | 1 + > > 2 files changed, 30 insertions(+) > > > > diff --git a/libavutil/opt.c b/libavutil/opt.c > > index 6669356..b6b4d9f 100644 > > --- a/libavutil/opt.c > > +++ b/libavutil/opt.c > > @@ -74,6 +74,7 @@ static int read_number(const AVOption *o, const void *dst, double *num, int *den > > case AV_OPT_TYPE_CHANNEL_LAYOUT: > > case AV_OPT_TYPE_DURATION: > > case AV_OPT_TYPE_INT64: > > + case AV_OPT_TYPE_UINT64: > > *intnum = *(int64_t *)dst; > > return 0; > > case AV_OPT_TYPE_FLOAT: > > @@ -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; > > Is there a reason why this uses int64_t, > > > + } else if (o->max > INT64_MAX + 1ULL && d > INT64_MAX + 1ULL) { > > + *(uint64_t *)dst = (llrint(d - (INT64_MAX + 1ULL)) + (INT64_MAX + 1ULL))*intnum; > > but this uint64_t, > > > + } else { > > + *(int64_t *)dst = llrint(d) * intnum; > > and this again int64_t? these are left over from the year old patch this is based on fixed locally [...]
On 20.11.2016 21:52, Michael Niedermayer wrote: > On Sun, Nov 20, 2016 at 08:55:44PM +0100, Andreas Cadhalpun wrote: >> On 20.11.2016 12:57, Michael Niedermayer wrote: >>> + if (intnum == 1 && d == (double)UINT64_MAX) { >>> + *(int64_t *)dst = UINT64_MAX; >> >> Is there a reason why this uses int64_t, >> >>> + } else if (o->max > INT64_MAX + 1ULL && d > INT64_MAX + 1ULL) { >>> + *(uint64_t *)dst = (llrint(d - (INT64_MAX + 1ULL)) + (INT64_MAX + 1ULL))*intnum; >> >> but this uint64_t, >> >>> + } else { >>> + *(int64_t *)dst = llrint(d) * intnum; >> >> and this again int64_t? > > these are left over from the year old patch this is based on > fixed locally OK. Now I've another question. Why does the check involve o->max? Is the out-of-range check at the beginning of write_number not sufficient? Best regards, Andreas
On Tue, Nov 22, 2016 at 12:29:02AM +0100, Andreas Cadhalpun wrote: > On 20.11.2016 21:52, Michael Niedermayer wrote: > > On Sun, Nov 20, 2016 at 08:55:44PM +0100, Andreas Cadhalpun wrote: > >> On 20.11.2016 12:57, Michael Niedermayer wrote: > >>> + if (intnum == 1 && d == (double)UINT64_MAX) { > >>> + *(int64_t *)dst = UINT64_MAX; > >> > >> Is there a reason why this uses int64_t, > >> > >>> + } else if (o->max > INT64_MAX + 1ULL && d > INT64_MAX + 1ULL) { > >>> + *(uint64_t *)dst = (llrint(d - (INT64_MAX + 1ULL)) + (INT64_MAX + 1ULL))*intnum; > >> > >> but this uint64_t, > >> > >>> + } else { > >>> + *(int64_t *)dst = llrint(d) * intnum; > >> > >> and this again int64_t? > > > > these are left over from the year old patch this is based on > > fixed locally > > OK. Now I've another question. Why does the check involve o->max? > Is the out-of-range check at the beginning of write_number not > sufficient? removed, this too came from the old patch thx [...]
On 22.11.2016 12:06, Michael Niedermayer wrote: > On Tue, Nov 22, 2016 at 12:29:02AM +0100, Andreas Cadhalpun wrote: >> OK. Now I've another question. Why does the check involve o->max? >> Is the out-of-range check at the beginning of write_number not >> sufficient? > > removed, this too came from the old patch Thanks. The patch should be OK then. Best regards, Andreas
On Tue, Nov 22, 2016 at 11:06:49PM +0100, Andreas Cadhalpun wrote: > On 22.11.2016 12:06, Michael Niedermayer wrote: > > On Tue, Nov 22, 2016 at 12:29:02AM +0100, Andreas Cadhalpun wrote: > >> OK. Now I've another question. Why does the check involve o->max? > >> Is the out-of-range check at the beginning of write_number not > >> sufficient? > > > > removed, this too came from the old patch > > Thanks. The patch should be OK then. applied thx [...]
diff --git a/libavutil/opt.c b/libavutil/opt.c index 6669356..b6b4d9f 100644 --- a/libavutil/opt.c +++ b/libavutil/opt.c @@ -74,6 +74,7 @@ static int read_number(const AVOption *o, const void *dst, double *num, int *den case AV_OPT_TYPE_CHANNEL_LAYOUT: case AV_OPT_TYPE_DURATION: case AV_OPT_TYPE_INT64: + case AV_OPT_TYPE_UINT64: *intnum = *(int64_t *)dst; return 0; case AV_OPT_TYPE_FLOAT: @@ -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; @@ -202,6 +217,7 @@ static int set_string(void *obj, const AVOption *o, const char *val, uint8_t **d } #define DEFAULT_NUMVAL(opt) ((opt->type == AV_OPT_TYPE_INT64 || \ + opt->type == AV_OPT_TYPE_UINT64 || \ opt->type == AV_OPT_TYPE_CONST || \ opt->type == AV_OPT_TYPE_FLAGS || \ opt->type == AV_OPT_TYPE_INT) \ @@ -458,6 +474,7 @@ int av_opt_set(void *obj, const char *name, const char *val, int search_flags) case AV_OPT_TYPE_FLAGS: case AV_OPT_TYPE_INT: case AV_OPT_TYPE_INT64: + case AV_OPT_TYPE_UINT64: case AV_OPT_TYPE_FLOAT: case AV_OPT_TYPE_DOUBLE: case AV_OPT_TYPE_RATIONAL: @@ -758,6 +775,9 @@ int av_opt_get(void *obj, const char *name, int search_flags, uint8_t **out_val) case AV_OPT_TYPE_INT64: ret = snprintf(buf, sizeof(buf), "%"PRId64, *(int64_t *)dst); break; + case AV_OPT_TYPE_UINT64: + ret = snprintf(buf, sizeof(buf), "%"PRIu64, *(uint64_t *)dst); + break; case AV_OPT_TYPE_FLOAT: ret = snprintf(buf, sizeof(buf), "%f", *(float *)dst); break; @@ -1106,6 +1126,9 @@ static void opt_list(void *obj, void *av_log_obj, const char *unit, case AV_OPT_TYPE_INT64: av_log(av_log_obj, AV_LOG_INFO, "%-12s ", "<int64>"); break; + case AV_OPT_TYPE_UINT64: + av_log(av_log_obj, AV_LOG_INFO, "%-12s ", "<uint64>"); + break; case AV_OPT_TYPE_DOUBLE: av_log(av_log_obj, AV_LOG_INFO, "%-12s ", "<double>"); break; @@ -1166,6 +1189,7 @@ static void opt_list(void *obj, void *av_log_obj, const char *unit, switch (opt->type) { case AV_OPT_TYPE_INT: case AV_OPT_TYPE_INT64: + case AV_OPT_TYPE_UINT64: case AV_OPT_TYPE_DOUBLE: case AV_OPT_TYPE_FLOAT: case AV_OPT_TYPE_RATIONAL: @@ -1210,6 +1234,7 @@ static void opt_list(void *obj, void *av_log_obj, const char *unit, break; } case AV_OPT_TYPE_INT: + case AV_OPT_TYPE_UINT64: case AV_OPT_TYPE_INT64: { const char *def_const = get_opt_const_name(obj, opt->unit, opt->default_val.i64); if (def_const) @@ -1288,6 +1313,7 @@ void av_opt_set_defaults2(void *s, int mask, int flags) case AV_OPT_TYPE_FLAGS: case AV_OPT_TYPE_INT: case AV_OPT_TYPE_INT64: + case AV_OPT_TYPE_UINT64: case AV_OPT_TYPE_DURATION: case AV_OPT_TYPE_CHANNEL_LAYOUT: case AV_OPT_TYPE_PIXEL_FMT: @@ -1648,6 +1674,7 @@ static int opt_size(enum AVOptionType type) case AV_OPT_TYPE_DURATION: case AV_OPT_TYPE_CHANNEL_LAYOUT: case AV_OPT_TYPE_INT64: + case AV_OPT_TYPE_UINT64: return sizeof(int64_t); case AV_OPT_TYPE_DOUBLE: return sizeof(double); @@ -1777,6 +1804,7 @@ int av_opt_query_ranges_default(AVOptionRanges **ranges_arg, void *obj, const ch case AV_OPT_TYPE_BOOL: case AV_OPT_TYPE_INT: case AV_OPT_TYPE_INT64: + case AV_OPT_TYPE_UINT64: case AV_OPT_TYPE_PIXEL_FMT: case AV_OPT_TYPE_SAMPLE_FMT: case AV_OPT_TYPE_FLOAT: @@ -1866,6 +1894,7 @@ int av_opt_is_set_to_default(void *obj, const AVOption *o) case AV_OPT_TYPE_CHANNEL_LAYOUT: case AV_OPT_TYPE_DURATION: case AV_OPT_TYPE_INT64: + case AV_OPT_TYPE_UINT64: read_number(o, dst, NULL, NULL, &i64); return o->default_val.i64 == i64; case AV_OPT_TYPE_STRING: diff --git a/libavutil/opt.h b/libavutil/opt.h index 9430b98..0d89379 100644 --- a/libavutil/opt.h +++ b/libavutil/opt.h @@ -228,6 +228,7 @@ enum AVOptionType{ AV_OPT_TYPE_RATIONAL, AV_OPT_TYPE_BINARY, ///< offset must point to a pointer immediately followed by an int for the length AV_OPT_TYPE_DICT, + AV_OPT_TYPE_UINT64, AV_OPT_TYPE_CONST = 128, AV_OPT_TYPE_IMAGE_SIZE = MKBETAG('S','I','Z','E'), ///< offset must point to two consecutive integers AV_OPT_TYPE_PIXEL_FMT = MKBETAG('P','F','M','T'),
Requested-by: wm4 ([FFmpeg-devel] [PATCH] avutil/opt: Support max > INT64_MAX in write_number() with AV_OPT_TYPE_INT64) Requested-by: ronald ([FFmpeg-devel] [PATCH] avutil/opt: Support max > INT64_MAX in write_number() with AV_OPT_TYPE_INT64) Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavutil/opt.c | 29 +++++++++++++++++++++++++++++ libavutil/opt.h | 1 + 2 files changed, 30 insertions(+)