| Message ID | 20240622131533.1313814-1-ffmpeg-devel@pileofstuff.org |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avfilter/af_afade: fix opt_type for nb_samples/ns | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Andrew Sayers: > The actual value is an int64_t, and is accessed elsewhere as AV_OPT_TYPE_INT64. > > Accessing it as INT will likely cause bugs on some 32-bit architectures. Whether this works or not will depend upon endianness, not on whether the architecture is 32-bit (as long as int is 32bits, which is mostly true for 64-bit architectures). > --- > libavfilter/af_afade.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/libavfilter/af_afade.c b/libavfilter/af_afade.c > index 3a45873460..c79271ec92 100644 > --- a/libavfilter/af_afade.c > +++ b/libavfilter/af_afade.c > @@ -452,8 +452,8 @@ const AVFilter ff_af_afade = { > #if CONFIG_ACROSSFADE_FILTER > > static const AVOption acrossfade_options[] = { > - { "nb_samples", "set number of samples for cross fade duration", OFFSET(nb_samples), AV_OPT_TYPE_INT, {.i64 = 44100}, 1, INT32_MAX/10, FLAGS }, > - { "ns", "set number of samples for cross fade duration", OFFSET(nb_samples), AV_OPT_TYPE_INT, {.i64 = 44100}, 1, INT32_MAX/10, FLAGS }, > + { "nb_samples", "set number of samples for cross fade duration", OFFSET(nb_samples), AV_OPT_TYPE_INT64, {.i64 = 44100}, 1, INT32_MAX/10, FLAGS }, > + { "ns", "set number of samples for cross fade duration", OFFSET(nb_samples), AV_OPT_TYPE_INT64, {.i64 = 44100}, 1, INT32_MAX/10, FLAGS }, > { "duration", "set cross fade duration", OFFSET(duration), AV_OPT_TYPE_DURATION, {.i64 = 0 }, 0, 60000000, FLAGS }, > { "d", "set cross fade duration", OFFSET(duration), AV_OPT_TYPE_DURATION, {.i64 = 0 }, 0, 60000000, FLAGS }, > { "overlap", "overlap 1st stream end with 2nd stream start", OFFSET(overlap), AV_OPT_TYPE_BOOL, {.i64 = 1 }, 0, 1, FLAGS }, LGTM. How did you find this? - Andreas
On Sat, Jun 22, 2024 at 03:20:52PM +0200, Andreas Rheinhardt wrote: > Andrew Sayers: > > The actual value is an int64_t, and is accessed elsewhere as AV_OPT_TYPE_INT64. > > > > Accessing it as INT will likely cause bugs on some 32-bit architectures. > > Whether this works or not will depend upon endianness, not on whether > the architecture is 32-bit (as long as int is 32bits, which is mostly > true for 64-bit architectures). > > > --- > > libavfilter/af_afade.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/libavfilter/af_afade.c b/libavfilter/af_afade.c > > index 3a45873460..c79271ec92 100644 > > --- a/libavfilter/af_afade.c > > +++ b/libavfilter/af_afade.c > > @@ -452,8 +452,8 @@ const AVFilter ff_af_afade = { > > #if CONFIG_ACROSSFADE_FILTER > > > > static const AVOption acrossfade_options[] = { > > - { "nb_samples", "set number of samples for cross fade duration", OFFSET(nb_samples), AV_OPT_TYPE_INT, {.i64 = 44100}, 1, INT32_MAX/10, FLAGS }, > > - { "ns", "set number of samples for cross fade duration", OFFSET(nb_samples), AV_OPT_TYPE_INT, {.i64 = 44100}, 1, INT32_MAX/10, FLAGS }, > > + { "nb_samples", "set number of samples for cross fade duration", OFFSET(nb_samples), AV_OPT_TYPE_INT64, {.i64 = 44100}, 1, INT32_MAX/10, FLAGS }, > > + { "ns", "set number of samples for cross fade duration", OFFSET(nb_samples), AV_OPT_TYPE_INT64, {.i64 = 44100}, 1, INT32_MAX/10, FLAGS }, > > { "duration", "set cross fade duration", OFFSET(duration), AV_OPT_TYPE_DURATION, {.i64 = 0 }, 0, 60000000, FLAGS }, > > { "d", "set cross fade duration", OFFSET(duration), AV_OPT_TYPE_DURATION, {.i64 = 0 }, 0, 60000000, FLAGS }, > > { "overlap", "overlap 1st stream end with 2nd stream start", OFFSET(overlap), AV_OPT_TYPE_BOOL, {.i64 = 1 }, 0, 1, FLAGS }, > > LGTM. How did you find this? It was a side-effect of yet another attempt to understand the codebase - looking for struct members with more than one associated AVOptionType. FWIW, the other oddities uncovered were: * libavformat/rtsp.c treats stimeout as both INT64 and DURATION (should probably have been caught at the time, but fixing it would likely break scripts that expect the current CLI behaviour) * libavdevice/v4l2.c treats list_format as both INT and CONST (not a bug, but a comment explaining this clever trick would have been nice) * libavfilter/buffersrc.c lets you set w/h separately or as video_size (not a bug, and I assume there's a reason for doing it this way)
On date Saturday 2024-06-22 15:20:52 +0200, Andreas Rheinhardt wrote: > Andrew Sayers: > > The actual value is an int64_t, and is accessed elsewhere as AV_OPT_TYPE_INT64. > > > > Accessing it as INT will likely cause bugs on some 32-bit architectures. > > Whether this works or not will depend upon endianness, not on whether > the architecture is 32-bit (as long as int is 32bits, which is mostly > true for 64-bit architectures). > > > --- > > libavfilter/af_afade.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/libavfilter/af_afade.c b/libavfilter/af_afade.c > > index 3a45873460..c79271ec92 100644 > > --- a/libavfilter/af_afade.c > > +++ b/libavfilter/af_afade.c > > @@ -452,8 +452,8 @@ const AVFilter ff_af_afade = { > > #if CONFIG_ACROSSFADE_FILTER > > > > static const AVOption acrossfade_options[] = { > > - { "nb_samples", "set number of samples for cross fade duration", OFFSET(nb_samples), AV_OPT_TYPE_INT, {.i64 = 44100}, 1, INT32_MAX/10, FLAGS }, > > - { "ns", "set number of samples for cross fade duration", OFFSET(nb_samples), AV_OPT_TYPE_INT, {.i64 = 44100}, 1, INT32_MAX/10, FLAGS }, > > + { "nb_samples", "set number of samples for cross fade duration", OFFSET(nb_samples), AV_OPT_TYPE_INT64, {.i64 = 44100}, 1, INT32_MAX/10, FLAGS }, > > + { "ns", "set number of samples for cross fade duration", OFFSET(nb_samples), AV_OPT_TYPE_INT64, {.i64 = 44100}, 1, INT32_MAX/10, FLAGS }, > > { "duration", "set cross fade duration", OFFSET(duration), AV_OPT_TYPE_DURATION, {.i64 = 0 }, 0, 60000000, FLAGS }, > > { "d", "set cross fade duration", OFFSET(duration), AV_OPT_TYPE_DURATION, {.i64 = 0 }, 0, 60000000, FLAGS }, > > { "overlap", "overlap 1st stream end with 2nd stream start", OFFSET(overlap), AV_OPT_TYPE_BOOL, {.i64 = 1 }, 0, 1, FLAGS }, > > LGTM. How did you find this? LGTM as well, will apply after dropping the second sentence in the commit log.
diff --git a/libavfilter/af_afade.c b/libavfilter/af_afade.c index 3a45873460..c79271ec92 100644 --- a/libavfilter/af_afade.c +++ b/libavfilter/af_afade.c @@ -452,8 +452,8 @@ const AVFilter ff_af_afade = { #if CONFIG_ACROSSFADE_FILTER static const AVOption acrossfade_options[] = { - { "nb_samples", "set number of samples for cross fade duration", OFFSET(nb_samples), AV_OPT_TYPE_INT, {.i64 = 44100}, 1, INT32_MAX/10, FLAGS }, - { "ns", "set number of samples for cross fade duration", OFFSET(nb_samples), AV_OPT_TYPE_INT, {.i64 = 44100}, 1, INT32_MAX/10, FLAGS }, + { "nb_samples", "set number of samples for cross fade duration", OFFSET(nb_samples), AV_OPT_TYPE_INT64, {.i64 = 44100}, 1, INT32_MAX/10, FLAGS }, + { "ns", "set number of samples for cross fade duration", OFFSET(nb_samples), AV_OPT_TYPE_INT64, {.i64 = 44100}, 1, INT32_MAX/10, FLAGS }, { "duration", "set cross fade duration", OFFSET(duration), AV_OPT_TYPE_DURATION, {.i64 = 0 }, 0, 60000000, FLAGS }, { "d", "set cross fade duration", OFFSET(duration), AV_OPT_TYPE_DURATION, {.i64 = 0 }, 0, 60000000, FLAGS }, { "overlap", "overlap 1st stream end with 2nd stream start", OFFSET(overlap), AV_OPT_TYPE_BOOL, {.i64 = 1 }, 0, 1, FLAGS },