[FFmpeg-devel] [PATCH 2/2] lavc/ratecontrol: Fix error logging for parsing and evaluation of rc_eq

[Alexander Strasser]

On 2020-08-13 22:32 +0200, Michael Niedermayer wrote:
> On Wed, Aug 12, 2020 at 08:58:30PM +0200, Alexander Strasser wrote:
> > On 2020-08-09 14:56 +0200, Michael Niedermayer wrote:
[...]
> > >
> > > lib user
> > >
> > > > If I'm not mistaken, a library user should not set it to NULL. Is it even possible while respecting public API?
> > >
> > > if its not possible then the patch should be fine. I was just asking becasue
> > > thats the thing that would change if it can be set
> >
> > The rc_eq field was removed from AVCodecContext with the merge
> > commit bfab430856 and lives only inside private contexts these
> > days.
> >
> > Now I'm not 100% sure if there is a way to use av_opt API to set a
> > string type option's value to NULL. Though it would worry me if that
> > is possible, even for options that have a default string.
> >
> > If you know about that, I would be grateful to hear. Will see
> > if I can it try out myself too.
>
> One can probably do it (maybe with av_opt_ptr()) how realistic an
> issue this is, i dont know.
>
> For the future we should ensure though its clearly documented if
> NULL is valid for AV_OPT_TYPE_STRING because part of the issue
> here is i think its not clearly documented if this is allowed

Unfortunately I think ATM it is always possible to set (unset?) a
string option to NULL with a simple av_set_opt call.

I think in particular in combination with defaults this is problematic.

I'm sure many places in our code don't expect a NULL for an
AV_OPT_TYPE_STRING field that has a default.

What do you and others think about the matter?
How should we deal with this?

As I see the situation, we could either decide

1. to fix a lot of places to handle NULL?
2. to document for a lot of options that NULL is invalid
3. to disallow
    a) setting options to NULL
    b) setting options to NULL if they have a non-NULL default
4. to reset options to their defaults if they are set to NULL


This may be a bit over-simplified because of the av_opt APIs and
different ways the AV_OPT_TYPE_ opts are treated. So this should
probably be looked into a bit closer.


[...]

  Alexander