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

[Michael Niedermayer]

On Sun, Aug 16, 2020 at 10:57:26PM +0200, Alexander Strasser wrote:
> 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

Are there cases where NULL is needed ?
If so, a flag that specifies that the value may be NULL could be
a possibility too to seperate the fields that do not allow or
support NULL vs the ones where it is needed and supported

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Republics decline into democracies and democracies degenerate into
despotisms. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200817/8fc9a48a/attachment.sig>