diff mbox series

[FFmpeg-devel] avutil/opt: add AV_OPT_FLAG_FORCE_CONST

Message ID 20240414195720.21646-1-timo@rothenpieler.org
State New
Headers show
Series [FFmpeg-devel] avutil/opt: add AV_OPT_FLAG_FORCE_CONST | expand

Checks

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

Commit Message

Timo Rothenpieler April 14, 2024, 7:57 p.m. UTC 
---
 doc/APIchanges      |  3 +++
 libavutil/opt.c     | 14 ++++++++++++++
 libavutil/opt.h     |  5 +++++
 libavutil/version.h |  2 +-
 4 files changed, 23 insertions(+), 1 deletion(-)

Comments

Marton Balint April 14, 2024, 8:30 p.m. UTC | #1 
On Sun, 14 Apr 2024, Timo Rothenpieler wrote:

> ---
> doc/APIchanges      |  3 +++
> libavutil/opt.c     | 14 ++++++++++++++
> libavutil/opt.h     |  5 +++++
> libavutil/version.h |  2 +-
> 4 files changed, 23 insertions(+), 1 deletion(-)

Where do you intend to use this flag? So some justification or description 
of your plans is missing from the commit message.

>
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 63e7a47126..da2c87909b 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -2,6 +2,9 @@ The last version increases of all libraries were on 2024-03-07
>
> API changes, most recent first:
>
> +2024-04-xx - xxxxxxxxxx - lavu 59.15.101 - opt.h
> +  Add AV_OPT_FLAG_FORCE_CONST.
> +
> 2024-04-11 - xxxxxxxxxx - lavc 61.5.102 - avcodec.h
>   AVCodecContext.decoded_side_data may now be set by libavcodec after
>   calling avcodec_open2().
> diff --git a/libavutil/opt.c b/libavutil/opt.c
> index d11e9d2ac5..9e51fa47f9 100644
> --- a/libavutil/opt.c
> +++ b/libavutil/opt.c
> @@ -365,6 +365,20 @@ static int set_string_number(void *obj, void *target_obj, const AVOption *o, con
>                 if (o_named->flags & AV_OPT_FLAG_DEPRECATED)
>                     av_log(obj, AV_LOG_WARNING, "The \"%s\" option is deprecated: %s\n",
>                            o_named->name, o_named->help);
> +            } else if (o->flags & AV_OPT_FLAG_FORCE_CONST) {
> +                av_log(obj, AV_LOG_ERROR, "The \"%s\" option only accepts one of its pre-defined constant values",
> +                       o->name);
> +                buf[0] = ':'; buf[1] = ' '; buf[2] = 0;
> +                for (o_named = NULL; o_named = av_opt_next(target_obj, o_named); ) {
> +                    if (o_named->type == AV_OPT_TYPE_CONST &&
> +                        o_named->unit && o->unit &&
> +                        !strcmp(o_named->unit, o->unit)) {
> +                        av_log(obj, AV_LOG_ERROR, "%s%s", buf, o_named->name);
> +                        buf[0] = ',';
> +                    }
> +                }
> +                av_log(obj, AV_LOG_ERROR, "\n");
> +                return AVERROR(EINVAL);
>             } else {
>                 if (o->unit) {
>                     for (o_named = NULL; o_named = av_opt_next(target_obj, o_named); ) {
> diff --git a/libavutil/opt.h b/libavutil/opt.h
> index e6013662f6..67e2b687b2 100644
> --- a/libavutil/opt.h
> +++ b/libavutil/opt.h
> @@ -305,6 +305,11 @@ enum AVOptionType{
>  * Set if option constants can also reside in child objects.
>  */
> #define AV_OPT_FLAG_CHILD_CONSTS    (1 << 18)
> +/**
> + * The option will only accept AV_OPT_TYPE_CONST values.
> + * Any other user supplied values will be rejected.
> + */

What about built-in named constants, such as min/max/default?

> +#define AV_OPT_FLAG_FORCE_CONST     (1 << 19)
>
> /**
>  * May be set as default_val for AV_OPT_TYPE_FLAG_ARRAY options.
> diff --git a/libavutil/version.h b/libavutil/version.h
> index 1f2bddc022..5de2d92146 100644
> --- a/libavutil/version.h
> +++ b/libavutil/version.h
> @@ -80,7 +80,7 @@
>
> #define LIBAVUTIL_VERSION_MAJOR  59
> #define LIBAVUTIL_VERSION_MINOR  15
> -#define LIBAVUTIL_VERSION_MICRO 100
> +#define LIBAVUTIL_VERSION_MICRO 101
>
> #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
>                                                LIBAVUTIL_VERSION_MINOR, \
> -- 
> 2.34.1

Regards,
Marton
Timo Rothenpieler April 14, 2024, 8:35 p.m. UTC | #2 
On 14.04.2024 22:30, Marton Balint wrote:
> 
> 
> On Sun, 14 Apr 2024, Timo Rothenpieler wrote:
> 
>> ---
>> doc/APIchanges      |  3 +++
>> libavutil/opt.c     | 14 ++++++++++++++
>> libavutil/opt.h     |  5 +++++
>> libavutil/version.h |  2 +-
>> 4 files changed, 23 insertions(+), 1 deletion(-)
> 
> Where do you intend to use this flag? So some justification or 
> description of your plans is missing from the commit message.

Some options in nvenc could be greatly simplified with it, where right 
now there's multiple if/switch-trees in the code, just to weed out 
invalid values.

Due to it technically being an API break, just turning it on for old 
options is not that easy, but it's something for a future major update.

>>
>> diff --git a/doc/APIchanges b/doc/APIchanges
>> index 63e7a47126..da2c87909b 100644
>> --- a/doc/APIchanges
>> +++ b/doc/APIchanges
>> @@ -2,6 +2,9 @@ The last version increases of all libraries were on 
>> 2024-03-07
>>
>> API changes, most recent first:
>>
>> +2024-04-xx - xxxxxxxxxx - lavu 59.15.101 - opt.h
>> +  Add AV_OPT_FLAG_FORCE_CONST.
>> +
>> 2024-04-11 - xxxxxxxxxx - lavc 61.5.102 - avcodec.h
>>   AVCodecContext.decoded_side_data may now be set by libavcodec after
>>   calling avcodec_open2().
>> diff --git a/libavutil/opt.c b/libavutil/opt.c
>> index d11e9d2ac5..9e51fa47f9 100644
>> --- a/libavutil/opt.c
>> +++ b/libavutil/opt.c
>> @@ -365,6 +365,20 @@ static int set_string_number(void *obj, void 
>> *target_obj, const AVOption *o, con
>>                 if (o_named->flags & AV_OPT_FLAG_DEPRECATED)
>>                     av_log(obj, AV_LOG_WARNING, "The \"%s\" option is 
>> deprecated: %s\n",
>>                            o_named->name, o_named->help);
>> +            } else if (o->flags & AV_OPT_FLAG_FORCE_CONST) {
>> +                av_log(obj, AV_LOG_ERROR, "The \"%s\" option only 
>> accepts one of its pre-defined constant values",
>> +                       o->name);
>> +                buf[0] = ':'; buf[1] = ' '; buf[2] = 0;
>> +                for (o_named = NULL; o_named = 
>> av_opt_next(target_obj, o_named); ) {
>> +                    if (o_named->type == AV_OPT_TYPE_CONST &&
>> +                        o_named->unit && o->unit &&
>> +                        !strcmp(o_named->unit, o->unit)) {
>> +                        av_log(obj, AV_LOG_ERROR, "%s%s", buf, 
>> o_named->name);
>> +                        buf[0] = ',';
>> +                    }
>> +                }
>> +                av_log(obj, AV_LOG_ERROR, "\n");
>> +                return AVERROR(EINVAL);
>>             } else {
>>                 if (o->unit) {
>>                     for (o_named = NULL; o_named = 
>> av_opt_next(target_obj, o_named); ) {
>> diff --git a/libavutil/opt.h b/libavutil/opt.h
>> index e6013662f6..67e2b687b2 100644
>> --- a/libavutil/opt.h
>> +++ b/libavutil/opt.h
>> @@ -305,6 +305,11 @@ enum AVOptionType{
>>  * Set if option constants can also reside in child objects.
>>  */
>> #define AV_OPT_FLAG_CHILD_CONSTS    (1 << 18)
>> +/**
>> + * The option will only accept AV_OPT_TYPE_CONST values.
>> + * Any other user supplied values will be rejected.
>> + */
> 
> What about built-in named constants, such as min/max/default?
> 
>> +#define AV_OPT_FLAG_FORCE_CONST     (1 << 19)
>>
>> /**
>>  * May be set as default_val for AV_OPT_TYPE_FLAG_ARRAY options.
>> diff --git a/libavutil/version.h b/libavutil/version.h
>> index 1f2bddc022..5de2d92146 100644
>> --- a/libavutil/version.h
>> +++ b/libavutil/version.h
>> @@ -80,7 +80,7 @@
>>
>> #define LIBAVUTIL_VERSION_MAJOR  59
>> #define LIBAVUTIL_VERSION_MINOR  15
>> -#define LIBAVUTIL_VERSION_MICRO 100
>> +#define LIBAVUTIL_VERSION_MICRO 101
>>
>> #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
>>                                                LIBAVUTIL_VERSION_MINOR, \
>> -- 
>> 2.34.1
> 
> Regards,
> Marton
> _______________________________________________
> 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".
Michael Niedermayer April 16, 2024, 12:07 a.m. UTC | #3 
On Sun, Apr 14, 2024 at 10:35:57PM +0200, Timo Rothenpieler wrote:
> On 14.04.2024 22:30, Marton Balint wrote:
> > 
> > 
> > On Sun, 14 Apr 2024, Timo Rothenpieler wrote:
> > 
> > > ---
> > > doc/APIchanges      |  3 +++
> > > libavutil/opt.c     | 14 ++++++++++++++
> > > libavutil/opt.h     |  5 +++++
> > > libavutil/version.h |  2 +-
> > > 4 files changed, 23 insertions(+), 1 deletion(-)
> > 
> > Where do you intend to use this flag? So some justification or
> > description of your plans is missing from the commit message.
> 
> Some options in nvenc could be greatly simplified with it, where right now
> there's multiple if/switch-trees in the code, just to weed out invalid
> values.

This doesnt feel right.

The code would need to validate the actual value the same way it
checks min/max at least

avoptions allows the user to obtain a parameters address and set
it directly as well as set it without any AVOption

iam still not sure thats a good idea.
Why are the values not in a continous sequence of integers that can
be checked with min/max ?

thx

[...]
Timo Rothenpieler April 16, 2024, 8:30 a.m. UTC | #4 
On 16/04/2024 02:07, Michael Niedermayer wrote:
> On Sun, Apr 14, 2024 at 10:35:57PM +0200, Timo Rothenpieler wrote:
>> On 14.04.2024 22:30, Marton Balint wrote:
>>>
>>>
>>> On Sun, 14 Apr 2024, Timo Rothenpieler wrote:
>>>
>>>> ---
>>>> doc/APIchanges      |  3 +++
>>>> libavutil/opt.c     | 14 ++++++++++++++
>>>> libavutil/opt.h     |  5 +++++
>>>> libavutil/version.h |  2 +-
>>>> 4 files changed, 23 insertions(+), 1 deletion(-)
>>>
>>> Where do you intend to use this flag? So some justification or
>>> description of your plans is missing from the commit message.
>>
>> Some options in nvenc could be greatly simplified with it, where right now
>> there's multiple if/switch-trees in the code, just to weed out invalid
>> values.
> 
> This doesnt feel right.
> 
> The code would need to validate the actual value the same way it
> checks min/max at least
> 
> avoptions allows the user to obtain a parameters address and set
> it directly as well as set it without any AVOption
> 
> iam still not sure thats a good idea.
> Why are the values not in a continous sequence of integers that can
> be checked with min/max ?

Cause they're simply not, gotta ask Nvidia for details.
They started having enums with holes in them all over the place.

This flag obviously cannot be used for pre-existing options where users 
got accustomed to using various values.
And there also needs to be validation of valid values nevertheless, due 
to the possibility of API clients setting the values directly.

But this behaviour of only accepting the named constants from users, and 
no arbitrary values, is exactly what I'd like to achieve.
diff mbox series

Patch

diff --git a/doc/APIchanges b/doc/APIchanges
index 63e7a47126..da2c87909b 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -2,6 +2,9 @@  The last version increases of all libraries were on 2024-03-07
 
 API changes, most recent first:
 
+2024-04-xx - xxxxxxxxxx - lavu 59.15.101 - opt.h
+  Add AV_OPT_FLAG_FORCE_CONST.
+
 2024-04-11 - xxxxxxxxxx - lavc 61.5.102 - avcodec.h
   AVCodecContext.decoded_side_data may now be set by libavcodec after
   calling avcodec_open2().
diff --git a/libavutil/opt.c b/libavutil/opt.c
index d11e9d2ac5..9e51fa47f9 100644
--- a/libavutil/opt.c
+++ b/libavutil/opt.c
@@ -365,6 +365,20 @@  static int set_string_number(void *obj, void *target_obj, const AVOption *o, con
                 if (o_named->flags & AV_OPT_FLAG_DEPRECATED)
                     av_log(obj, AV_LOG_WARNING, "The \"%s\" option is deprecated: %s\n",
                            o_named->name, o_named->help);
+            } else if (o->flags & AV_OPT_FLAG_FORCE_CONST) {
+                av_log(obj, AV_LOG_ERROR, "The \"%s\" option only accepts one of its pre-defined constant values",
+                       o->name);
+                buf[0] = ':'; buf[1] = ' '; buf[2] = 0;
+                for (o_named = NULL; o_named = av_opt_next(target_obj, o_named); ) {
+                    if (o_named->type == AV_OPT_TYPE_CONST &&
+                        o_named->unit && o->unit &&
+                        !strcmp(o_named->unit, o->unit)) {
+                        av_log(obj, AV_LOG_ERROR, "%s%s", buf, o_named->name);
+                        buf[0] = ',';
+                    }
+                }
+                av_log(obj, AV_LOG_ERROR, "\n");
+                return AVERROR(EINVAL);
             } else {
                 if (o->unit) {
                     for (o_named = NULL; o_named = av_opt_next(target_obj, o_named); ) {
diff --git a/libavutil/opt.h b/libavutil/opt.h
index e6013662f6..67e2b687b2 100644
--- a/libavutil/opt.h
+++ b/libavutil/opt.h
@@ -305,6 +305,11 @@  enum AVOptionType{
  * Set if option constants can also reside in child objects.
  */
 #define AV_OPT_FLAG_CHILD_CONSTS    (1 << 18)
+/**
+ * The option will only accept AV_OPT_TYPE_CONST values.
+ * Any other user supplied values will be rejected.
+ */
+#define AV_OPT_FLAG_FORCE_CONST     (1 << 19)
 
 /**
  * May be set as default_val for AV_OPT_TYPE_FLAG_ARRAY options.
diff --git a/libavutil/version.h b/libavutil/version.h
index 1f2bddc022..5de2d92146 100644
--- a/libavutil/version.h
+++ b/libavutil/version.h
@@ -80,7 +80,7 @@ 
 
 #define LIBAVUTIL_VERSION_MAJOR  59
 #define LIBAVUTIL_VERSION_MINOR  15
-#define LIBAVUTIL_VERSION_MICRO 100
+#define LIBAVUTIL_VERSION_MICRO 101
 
 #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
                                                LIBAVUTIL_VERSION_MINOR, \