Message ID | 20240304130657.30631-5-anton@khirnov.net |
---|---|
State | Accepted |
Commit | fc706276c051c425538d1476a7be05442d06dd0f |
Headers | show |
Series | [FFmpeg-devel,01/29] lavu/opt: factor per-type dispatch out of av_opt_get() | 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 |
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
Quoting Marton Balint (2024-03-04 23:39:19) > 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. I see this more as establishing/clarifying the status of those fields rather than changing it. While the idea that everything not explicitly forbidden is allowed is nice in theory, I don't think it's viable for us, because so many (especially older) APIs are barely documented, if at all. If we adhered to it strictly, almost any extension of existing APIs could be considered a break. E.g. consider that it is not publicly documented what C type does any AVOption type map to, or even that we can add new ones. > > * @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. This is precisely what the callers have no business doing IMO, and what the AVOption API is supposed to abstract away. AVOptions are not supposed to be especially efficient currently, but if some use case requires that we should add new API to make access more efficient - e.g. that accepts AVOptions rather than string option names, and flags that make setters take ownership rather than copy. > Also there is av_opt_ptr() which implicitly returns this anyway. Or > you want to fully disallow pointer based member access? Yes I do, at least in this generic type-unsafe form. av_opt_ptr() has a single caller in our codebase that I'd like to get rid of, and then deprecate+remove the function itself. Also note that the fact av_opt_ptr() exists at all supports my interpretation that option users are not supposed to access offset. > 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. All of these can and should be solved with better getter/setter API. > > > */ > > @@ -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. I consistently forbid users to read the default value directly, they have to read it from the object. The only inconsistency is that default_val stores far more than just the default value for array options, but that follows from the constraints we are operating under. Recall the previous version of this patch where I put those values elsewhere and you objected to it. > In general, we should avoid having public API fields which are in fact > private. We have existing techniques to really hide those. For instance? The problem is those fields are not purely private, they are public in some contexts (what I call 'native access'), and private in others. That means they have to appear in the public header. > 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. I would much prefer not to be forced to add gazillions of option types for every minor semantics difference we might want to introduce.
On Mon, Mar 4, 2024 at 11:39 PM Marton Balint <cus@passwd.hu> wrote: > 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. > > > */ > > @@ -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. > There is no helper function for getting the default value of an option. If you disallow reading this field directly (as in one of your other patches), please add such a helper function(s), since library users need it. It should also work without instantiating the object, but directly on the class definition
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. + * * @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. */ @@ -308,6 +320,8 @@ typedef struct AVOption { enum AVOptionType type; /** + * Native access only. + * * the default value for scalar options */ union {