Re: [FFmpeg-devel] [PATCH 05/29] lavu/opt: distinguish between native and foreign access for AVOption fields

[Marton Balint <cus@passwd.hu>]

On Mon, 4 Mar 2024, Anton Khirnov wrote:

> Native access is from the code that declared the options, foreign access
> is from code that is using the options. Forbid foreign access to
> AVOption.offset/default_val, for which there is no good reason, and
> which should allow us more freedom in extending their semantics in a
> compatible way.
> ---
> libavutil/opt.h | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/libavutil/opt.h b/libavutil/opt.h
> index e34b8506f8..e402f6a0a0 100644
> --- a/libavutil/opt.h
> +++ b/libavutil/opt.h
> @@ -43,6 +43,16 @@
>  * ("objects"). An option can have a help text, a type and a range of possible
>  * values. Options may then be enumerated, read and written to.
>  *
> + * There are two modes of access to members of AVOption and its child structs.
> + * One is called 'native access', and refers to access from the code that
> + * declares the AVOption in question.  The other is 'foreign access', and refers
> + * to access from other code.
> + *
> + * Certain struct members in this header are documented as 'native access only'
> + * or similar - it means that only the code that declared the AVOption in
> + * question is allowed to access the field. This allows us to extend the
> + * semantics of those fields without breaking API compatibility.
> + *

Changing private/public status of existing fields retrospecitvely can be 
considered an API break.

>  * @section avoptions_implement Implementing AVOptions
>  * This section describes how to add AVOptions capabilities to a struct.
>  *
> @@ -301,6 +311,8 @@ typedef struct AVOption {
>     const char *help;
>
>     /**
> +     * Native access only.
> +     *
>      * The offset relative to the context structure where the option
>      * value is stored. It should be 0 for named constants.

I don't quite see the reason hiding this. I think it is pretty safe to 
say that its semantics won't change, and getting the offset, adding it to 
the object pointer and directly reading or writing the value is the most 
efficient way to get or set the values. Also there is av_opt_ptr() which 
implicitly returns this anyway. Or you want to fully disallow pointer 
based member access? Some issues would be:
1) for a dictionary type you don't have a getter which does not copy
2) some types simply don't have a native typed getter/setter function
3) if you have multiple object of the same class, when using getters 
there will always be a linear search for the attribute name each time you 
get/set.

>      */
> @@ -308,6 +320,8 @@ typedef struct AVOption {
>     enum AVOptionType type;
>
>     /**
> +     * Native access only.
> +     *
>      * the default value for scalar options
>      */

One could argue that it will be more difficult to get the default value of 
an option (you'd have to create an object, call av_opt_set_defaults() and 
finally do av_opt_get), but what I find more problematic is the 
inconsistency. You are not allowed to access default_val, unless it is an 
array type, in which case you might access it to get array settings, but - 
oh well - not the default value.

In general, we should avoid having public API fields which are in fact 
private. We have existing techniques to really hide those. Considering we 
can always add new option types to define new semantics, I don't really 
think that keeping the existing public API fields public is such a blow.

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".