[FFmpeg-devel] lavu/opt: Mention that AVOptions is not reentrant

An external API developer might think they can use AVOptions to modify values
during playback (e.g. putting a playback quality slider next to the volume
slider).  Make it clear that behaviour is not recommended.

Include the warning in the group description and the text for every function
that sets options, but leave it implicit in functions that get options.
This reflects the fact that *modifying* options is likely to cause weird bugs,
while *reading* options isn't guaranteed but is likely to be safe.
---
 libavutil/opt.h | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

On Wed, Jun 5, 2024 at 3:18 PM Andrew Sayers <ffmpeg-devel@pileofstuff.org>
wrote:

> An external API developer might think they can use AVOptions to modify
> values
> during playback (e.g. putting a playback quality slider next to the volume
> slider).  Make it clear that behaviour is not recommended.
>

There are options that can be changed at runtime,
And it works just fine.


>
> Include the warning in the group description and the text for every
> function
> that sets options, but leave it implicit in functions that get options.
> This reflects the fact that *modifying* options is likely to cause weird
> bugs,
> while *reading* options isn't guaranteed but is likely to be safe.
> ---
>  libavutil/opt.h | 30 ++++++++++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/libavutil/opt.h b/libavutil/opt.h
> index 07e27a9208..13292c6473 100644
> --- a/libavutil/opt.h
> +++ b/libavutil/opt.h
> @@ -53,11 +53,16 @@
>   * question is allowed to access the field. This allows us to extend the
>   * semantics of those fields without breaking API compatibility.
>   *
> + * Note that AVOptions functions are not reentrant, and options may be
> accessed
> + * from internal FFmpeg threads.  Unless otherwise noted, it is best to
> avoid
> + * modifying options once a struct has been initialized.
> + *
>   * @section avoptions_scope Scope of AVOptions
>   *
>   * AVOptions is designed to support any set of multimedia configuration
> options
> - * that can be defined at compile-time.  Although it is mainly used to
> expose
> - * FFmpeg options, you are welcome to adapt it to your own use case.
> + * that can be defined at compile-time and set at object creation time.
> Although
> + * it is mainly used to expose FFmpeg options, you are welcome to adapt it
> + * to your own use case.
>   *
>   * No single approach can ever fully solve the problem of configuration,
>   * but please submit a patch if you believe you have found a problem
> @@ -483,6 +488,9 @@ typedef struct AVOptionRanges {
>  /**
>   * Set the values of all AVOption fields to their default values.
>   *
> + * Note: like all AVOptions functions, this is not reentrant.  Unless
> + * otherwise noted, it should only be used before initializing the struct.
> + *
>   * @param s an AVOption-enabled struct (its first member must be a
> pointer to AVClass)
>   */
>  void av_opt_set_defaults(void *s);
> @@ -492,6 +500,9 @@ void av_opt_set_defaults(void *s);
>   * AVOption fields for which (opt->flags & mask) == flags will have their
>   * default applied to s.
>   *
> + * Note: like all AVOptions functions, this is not reentrant.  Unless
> + * otherwise noted, it should only be used before initializing the struct.
> + *
>   * @param s an AVOption-enabled struct (its first member must be a
> pointer to AVClass)
>   * @param mask combination of AV_OPT_FLAG_*
>   * @param flags combination of AV_OPT_FLAG_*
> @@ -661,6 +672,9 @@ enum {
>   * key. ctx must be an AVClass context, storing is done using
>   * AVOptions.
>   *
> + * Note: like all AVOptions functions, this is not reentrant.  Unless
> + * otherwise noted, it should only be used before initializing the struct.
> + *
>   * @param opts options string to parse, may be NULL
>   * @param key_val_sep a 0-terminated list of characters used to
>   * separate key from value
> @@ -679,6 +693,9 @@ int av_set_options_string(void *ctx, const char *opts,
>   * Parse the key-value pairs list in opts. For each key=value pair found,
>   * set the value of the corresponding option in ctx.
>   *
> + * Note: like all AVOptions functions, this is not reentrant.  Unless
> + * otherwise noted, it should only be used before initializing the struct.
> + *
>   * @param ctx          the AVClass object to set options on
>   * @param opts         the options string, key-value pairs separated by a
>   *                     delimiter
> @@ -709,6 +726,9 @@ int av_opt_set_from_string(void *ctx, const char *opts,
>  /**
>   * Set all the options from a given dictionary on an object.
>   *
> + * Note: like all AVOptions functions, this is not reentrant.  Unless
> + * otherwise noted, it should only be used before initializing the struct.
> + *
>   * @param obj a struct whose first element is a pointer to AVClass
>   * @param options options to process. This dictionary will be freed and
> replaced
>   *                by a new one containing all options not found in obj.
> @@ -726,6 +746,9 @@ int av_opt_set_dict(void *obj, struct AVDictionary
> **options);
>  /**
>   * Set all the options from a given dictionary on an object.
>   *
> + * Note: like all AVOptions functions, this is not reentrant.  Unless
> + * otherwise noted, it should only be used before initializing the struct.
> + *
>   * @param obj a struct whose first element is a pointer to AVClass
>   * @param options options to process. This dictionary will be freed and
> replaced
>   *                by a new one containing all options not found in obj.
> @@ -764,6 +787,9 @@ int av_opt_copy(void *dest, const void *src);
>   * @{
>   * Those functions set the field of obj with the given name to value.
>   *
> + * Note: like all AVOptions functions, these are not reentrant.  Unless
> + * otherwise noted, they should only be used before initializing the
> struct.
> + *
>   * @param[in] obj A struct whose first element is a pointer to an AVClass.
>   * @param[in] name the name of the field to set
>   * @param[in] val The value to set. In case of av_opt_set() if the field
> is not
> --
> 2.45.1
>
> _______________________________________________
> 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".
>

On Wed, Jun 05, 2024 at 03:34:50PM +0200, Paul B Mahol wrote:
> On Wed, Jun 5, 2024 at 3:18 PM Andrew Sayers <ffmpeg-devel@pileofstuff.org>
> wrote:
> 
> > An external API developer might think they can use AVOptions to modify
> > values
> > during playback (e.g. putting a playback quality slider next to the volume
> > slider).  Make it clear that behaviour is not recommended.
> >
> 
> There are options that can be changed at runtime,
> And it works just fine.

How would an external developer know which options can be safely changed
(preferably including in future versions of FFmpeg) vs. ones where their tests
got lucky and happened not to trigger a read during a non-atomic write?

Hi,

On Wed, Jun 5, 2024 at 9:44 AM Andrew Sayers <ffmpeg-devel@pileofstuff.org>
wrote:

> On Wed, Jun 05, 2024 at 03:34:50PM +0200, Paul B Mahol wrote:
> > On Wed, Jun 5, 2024 at 3:18 PM Andrew Sayers <
> ffmpeg-devel@pileofstuff.org>
> > wrote:
> >
> > > An external API developer might think they can use AVOptions to modify
> > > values
> > > during playback (e.g. putting a playback quality slider next to the
> volume
> > > slider).  Make it clear that behaviour is not recommended.
> > >
> >
> > There are options that can be changed at runtime,
> > And it works just fine.
>
> How would an external developer know which options can be safely changed
> (preferably including in future versions of FFmpeg) vs. ones where their
> tests
> got lucky and happened not to trigger a read during a non-atomic write?
>

If you see that happening, it would be good to submit a bug report. Right
now it's very abstract.

Ronald

On Wed, Jun 5, 2024 at 3:44 PM Andrew Sayers <ffmpeg-devel@pileofstuff.org>
wrote:

> On Wed, Jun 05, 2024 at 03:34:50PM +0200, Paul B Mahol wrote:
> > On Wed, Jun 5, 2024 at 3:18 PM Andrew Sayers <
> ffmpeg-devel@pileofstuff.org>
> > wrote:
> >
> > > An external API developer might think they can use AVOptions to modify
> > > values
> > > during playback (e.g. putting a playback quality slider next to the
> volume
> > > slider).  Make it clear that behaviour is not recommended.
> > >
> >
> > There are options that can be changed at runtime,
> > And it works just fine.
>
> How would an external developer know which options can be safely changed
> (preferably including in future versions of FFmpeg) vs. ones where their
> tests
> got lucky and happened not to trigger a read during a non-atomic write?
>

See this flag:

#define AV_OPT_FLAG_RUNTIME_PARAM   (1 << 15)

What is the point/gain in changing options from multiple threads
concurrently?



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

On Wed, Jun 05, 2024 at 09:46:16AM -0400, Ronald S. Bultje wrote:
> Hi,
> 
> On Wed, Jun 5, 2024 at 9:44 AM Andrew Sayers <ffmpeg-devel@pileofstuff.org>
> wrote:
> 
> > On Wed, Jun 05, 2024 at 03:34:50PM +0200, Paul B Mahol wrote:
> > > On Wed, Jun 5, 2024 at 3:18 PM Andrew Sayers <
> > ffmpeg-devel@pileofstuff.org>
> > > wrote:
> > >
> > > > An external API developer might think they can use AVOptions to modify
> > > > values
> > > > during playback (e.g. putting a playback quality slider next to the
> > volume
> > > > slider).  Make it clear that behaviour is not recommended.
> > > >
> > >
> > > There are options that can be changed at runtime,
> > > And it works just fine.
> >
> > How would an external developer know which options can be safely changed
> > (preferably including in future versions of FFmpeg) vs. ones where their
> > tests
> > got lucky and happened not to trigger a read during a non-atomic write?
> >
> 
> If you see that happening, it would be good to submit a bug report. Right
> now it's very abstract.

I think we might be talking past each other - here's a concrete example:

The private struct "SetTSContext" includes an AVOptions-accessible member
"time_base", currently implemented as an AVRational (i.e. a pair of ints).
write_number() in libavutil/opt.c sets options of type AV_OPT_TYPE_RATIONAL
in such a way that a poorly-timed read could see the new numerator
and old denominator (or the other way around).

If I wrote a program that let users dynamically change the time base,
and someone switched their timebase from 1/30 to 100/3000, one unlucky user
might have a few frames encoded with a timebase of 100/30.  Is that something
the AVOptions API is supposed to support?  If yes, the bug is that
AVOptions access isn't guarded by a mutex.  If no, there's no bug, just an
edge case worth mentioning in the docs.

On Wed, Jun 05, 2024 at 03:22:55PM +0100, Andrew Sayers wrote:
> On Wed, Jun 05, 2024 at 09:46:16AM -0400, Ronald S. Bultje wrote:
> > Hi,
> > 
> > On Wed, Jun 5, 2024 at 9:44 AM Andrew Sayers <ffmpeg-devel@pileofstuff.org>
> > wrote:
> > 
> > > On Wed, Jun 05, 2024 at 03:34:50PM +0200, Paul B Mahol wrote:
> > > > On Wed, Jun 5, 2024 at 3:18 PM Andrew Sayers <
> > > ffmpeg-devel@pileofstuff.org>
> > > > wrote:
> > > >
> > > > > An external API developer might think they can use AVOptions to modify
> > > > > values
> > > > > during playback (e.g. putting a playback quality slider next to the
> > > volume
> > > > > slider).  Make it clear that behaviour is not recommended.
> > > > >
> > > >
> > > > There are options that can be changed at runtime,
> > > > And it works just fine.
> > >
> > > How would an external developer know which options can be safely changed
> > > (preferably including in future versions of FFmpeg) vs. ones where their
> > > tests
> > > got lucky and happened not to trigger a read during a non-atomic write?
> > >
> > 
> > If you see that happening, it would be good to submit a bug report. Right
> > now it's very abstract.
> 
> I think we might be talking past each other - here's a concrete example:
> 
> The private struct "SetTSContext" includes an AVOptions-accessible member
> "time_base", currently implemented as an AVRational (i.e. a pair of ints).
> write_number() in libavutil/opt.c sets options of type AV_OPT_TYPE_RATIONAL
> in such a way that a poorly-timed read could see the new numerator
> and old denominator (or the other way around).
> 
> If I wrote a program that let users dynamically change the time base,
> and someone switched their timebase from 1/30 to 100/3000, one unlucky user
> might have a few frames encoded with a timebase of 100/30.  Is that something
> the AVOptions API is supposed to support?  If yes, the bug is that
> AVOptions access isn't guarded by a mutex.  If no, there's no bug, just an
> edge case worth mentioning in the docs.

AVOption simply provides light weight access to the struct fields.
Calling AVOption non re-entrant in modifying a field you arent even allowed
to modify from 2 threads is confusing

If you want to modify a field from 2 threads that field could be some sort
of atomic type. This can then easily be added to AVOption

thx

[...]

On Thu, Jun 06, 2024 at 01:17:48AM +0200, Michael Niedermayer wrote:
[...]
> AVOption simply provides light weight access to the struct fields.
> Calling AVOption non re-entrant in modifying a field you arent even allowed
> to modify from 2 threads is confusing

I think you're saying there's already a rule about modifying AVOptions from
2 threads.  Could you explain that in more detail?

> If you want to modify a field from 2 threads that field could be some sort
> of atomic type. This can then easily be added to AVOption

Doing that for a single option would involve publicly guaranteeing its
representation for at least one major version.  At that point, you might as well
just tell people to access it as a member of a public struct.

To be clear - this isn't a programming problem, it's a design problem.
The interface currently allows external developers to assume something will
always work when it's actually just something that could be supported some day.
Writing up the current behaviour as a guarantee lets them avoid writing code
that will generate hard-to-reproduce bugs, at the cost of making it slightly
harder for us to do something we've never needed to do in the past.

On Thu, Jun 06, 2024 at 09:29:24AM +0100, Andrew Sayers wrote:
> On Thu, Jun 06, 2024 at 01:17:48AM +0200, Michael Niedermayer wrote:
> [...]
> > AVOption simply provides light weight access to the struct fields.
> > Calling AVOption non re-entrant in modifying a field you arent even allowed
> > to modify from 2 threads is confusing
> 
> I think you're saying there's already a rule about modifying AVOptions from
> 2 threads.  Could you explain that in more detail?

Well, one way this can be argued is this:
Latest C draft: (but i doubt this is different) ISO/IEC 9899:2017   C17 ballot N2176

"Two expression evaluations conflict if one of them modifies a memory location and the other one
 reads or modifies the same memory location"

so if you have 2 threads and one writes into a int and another reads it at the
same time, the code is broken.
The code doing said act through some API doesnt become less broken

Calling AVOption non re-rentrant because of that is false thats as if one executed
strtok_r(a,b,c) with the VERY same a,b,c from 2 threads and then said
its not thread safe

strtok_r() is thread safe and reentrant if its used correctly, so is AVOption


> 
> > If you want to modify a field from 2 threads that field could be some sort
> > of atomic type. This can then easily be added to AVOption
> 
> Doing that for a single option would involve publicly guaranteeing its
> representation for at least one major version.

A feature can be added at any time without a major version bump
adding an option that uses a atomic type can thus also be done at any time

It can be possible to transparenntly change the underlaying representation
under an AVOption without ABI break but that requires some thought and care

thx

[...]

On Thu, Jun 06, 2024 at 04:24:11PM +0200, Michael Niedermayer wrote:
> On Thu, Jun 06, 2024 at 09:29:24AM +0100, Andrew Sayers wrote:
> > On Thu, Jun 06, 2024 at 01:17:48AM +0200, Michael Niedermayer wrote:
> > [...]
> > > AVOption simply provides light weight access to the struct fields.
> > > Calling AVOption non re-entrant in modifying a field you arent even allowed
> > > to modify from 2 threads is confusing
> > 
> > I think you're saying there's already a rule about modifying AVOptions from
> > 2 threads.  Could you explain that in more detail?
> 
> Well, one way this can be argued is this:
> Latest C draft: (but i doubt this is different) ISO/IEC 9899:2017   C17 ballot N2176
> 
> "Two expression evaluations conflict if one of them modifies a memory location and the other one
>  reads or modifies the same memory location"
> 
> so if you have 2 threads and one writes into a int and another reads it at the
> same time, the code is broken.
> The code doing said act through some API doesnt become less broken
> 
> Calling AVOption non re-rentrant because of that is false thats as if one executed
> strtok_r(a,b,c) with the VERY same a,b,c from 2 threads and then said
> its not thread safe
> 
> strtok_r() is thread safe and reentrant if its used correctly, so is AVOption
[...]

Ok, how about if the patch avoided the word "reentrant" and just said:

+ * Note: AVOptions values should not be modified after a struct is initialized.

Andrew Sayers:
> On Thu, Jun 06, 2024 at 04:24:11PM +0200, Michael Niedermayer wrote:
>> On Thu, Jun 06, 2024 at 09:29:24AM +0100, Andrew Sayers wrote:
>>> On Thu, Jun 06, 2024 at 01:17:48AM +0200, Michael Niedermayer wrote:
>>> [...]
>>>> AVOption simply provides light weight access to the struct fields.
>>>> Calling AVOption non re-entrant in modifying a field you arent even allowed
>>>> to modify from 2 threads is confusing
>>>
>>> I think you're saying there's already a rule about modifying AVOptions from
>>> 2 threads.  Could you explain that in more detail?
>>
>> Well, one way this can be argued is this:
>> Latest C draft: (but i doubt this is different) ISO/IEC 9899:2017   C17 ballot N2176
>>
>> "Two expression evaluations conflict if one of them modifies a memory location and the other one
>>  reads or modifies the same memory location"
>>
>> so if you have 2 threads and one writes into a int and another reads it at the
>> same time, the code is broken.
>> The code doing said act through some API doesnt become less broken
>>
>> Calling AVOption non re-rentrant because of that is false thats as if one executed
>> strtok_r(a,b,c) with the VERY same a,b,c from 2 threads and then said
>> its not thread safe
>>
>> strtok_r() is thread safe and reentrant if its used correctly, so is AVOption
> [...]
> 
> Ok, how about if the patch avoided the word "reentrant" and just said:
> 
> + * Note: AVOptions values should not be modified after a struct is initialized.

This is wrong either. As Paul has already pointed out to you, some
options are allowed to be modified lateron.

- Andreas

On Thu, Jun 06, 2024 at 05:21:00PM +0200, Andreas Rheinhardt wrote:
> Andrew Sayers:
> > On Thu, Jun 06, 2024 at 04:24:11PM +0200, Michael Niedermayer wrote:
> >> On Thu, Jun 06, 2024 at 09:29:24AM +0100, Andrew Sayers wrote:
> >>> On Thu, Jun 06, 2024 at 01:17:48AM +0200, Michael Niedermayer wrote:
> >>> [...]
> >>>> AVOption simply provides light weight access to the struct fields.
> >>>> Calling AVOption non re-entrant in modifying a field you arent even allowed
> >>>> to modify from 2 threads is confusing
> >>>
> >>> I think you're saying there's already a rule about modifying AVOptions from
> >>> 2 threads.  Could you explain that in more detail?
> >>
> >> Well, one way this can be argued is this:
> >> Latest C draft: (but i doubt this is different) ISO/IEC 9899:2017   C17 ballot N2176
> >>
> >> "Two expression evaluations conflict if one of them modifies a memory location and the other one
> >>  reads or modifies the same memory location"
> >>
> >> so if you have 2 threads and one writes into a int and another reads it at the
> >> same time, the code is broken.
> >> The code doing said act through some API doesnt become less broken
> >>
> >> Calling AVOption non re-rentrant because of that is false thats as if one executed
> >> strtok_r(a,b,c) with the VERY same a,b,c from 2 threads and then said
> >> its not thread safe
> >>
> >> strtok_r() is thread safe and reentrant if its used correctly, so is AVOption
> > [...]
> > 
> > Ok, how about if the patch avoided the word "reentrant" and just said:
> > 
> > + * Note: AVOptions values should not be modified after a struct is initialized.
> 
> This is wrong either. As Paul has already pointed out to you, some
> options are allowed to be modified lateron.

Ah, I'd interpreted "runtime" to be the opposite of "compile-time", not
"initialization-time".  I'll propose a new patch that should be clearer.