diff mbox series

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

Message ID a7451140937532e426c960a441ee67a280520ba7.1596835157.git.eclipse7@gmx.net
State New
Headers show
Series [FFmpeg-devel,1/2] lavc/snowenc: Expose an option to set the rc_eq expression
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Alexander Strasser Aug. 7, 2020, 9:26 p.m. UTC
Don't pass a potential NULL pointer to av_log, instead use a default
in the rc_eq AVOption definitions. Now the context variable always
holds a string; even if it's the default expression.

Note this also fixes the evaluation error path, where a similar
av_log call references the rc_eq string from the context too.

Signed-off-by: Alexander Strasser <eclipse7@gmx.net>
---
 libavcodec/mpegvideo.h   | 2 +-
 libavcodec/ratecontrol.c | 3 +--
 libavcodec/snowenc.c     | 2 +-
 3 files changed, 3 insertions(+), 4 deletions(-)

--

Comments

Michael Niedermayer Aug. 9, 2020, 10:56 a.m. UTC | #1
On Fri, Aug 07, 2020 at 11:26:58PM +0200, Alexander Strasser wrote:
> Don't pass a potential NULL pointer to av_log, instead use a default
> in the rc_eq AVOption definitions. Now the context variable always
> holds a string; even if it's the default expression.
> 
> Note this also fixes the evaluation error path, where a similar
> av_log call references the rc_eq string from the context too.
> 
> Signed-off-by: Alexander Strasser <eclipse7@gmx.net>
> ---
>  libavcodec/mpegvideo.h   | 2 +-
>  libavcodec/ratecontrol.c | 3 +--
>  libavcodec/snowenc.c     | 2 +-
>  3 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/libavcodec/mpegvideo.h b/libavcodec/mpegvideo.h
> index 29e692f245..d2515a3bbf 100644
> --- a/libavcodec/mpegvideo.h
> +++ b/libavcodec/mpegvideo.h
> @@ -637,7 +637,7 @@ FF_MPV_OPT_CMP_FUNC, \
>            "defined in the section 'Expression Evaluation', the following functions are available: "                                                                             \
>            "bits2qp(bits), qp2bits(qp). Also the following constants are available: iTex pTex tex mv "                                                                           \
>            "fCode iCount mcVar var isI isP isB avgQP qComp avgIITex avgPITex avgPPTex avgBPTex avgTex.",                                                                         \
> -                                                                    FF_MPV_OFFSET(rc_eq), AV_OPT_TYPE_STRING,                           .flags = FF_MPV_OPT_FLAGS },            \
> +                                                                    FF_MPV_OFFSET(rc_eq), AV_OPT_TYPE_STRING, {.str = "tex^qComp" },    .flags = FF_MPV_OPT_FLAGS },            \
>  {"rc_init_cplx", "initial complexity for 1-pass encoding",          FF_MPV_OFFSET(rc_initial_cplx), AV_OPT_TYPE_FLOAT, {.dbl = 0 }, -FLT_MAX, FLT_MAX, FF_MPV_OPT_FLAGS},       \
>  {"rc_buf_aggressivity", "currently useless",                        FF_MPV_OFFSET(rc_buffer_aggressivity), AV_OPT_TYPE_FLOAT, {.dbl = 1.0 }, -FLT_MAX, FLT_MAX, FF_MPV_OPT_FLAGS}, \
>  {"border_mask", "increase the quantizer for macroblocks close to borders", FF_MPV_OFFSET(border_masking), AV_OPT_TYPE_FLOAT, {.dbl = 0 }, -FLT_MAX, FLT_MAX, FF_MPV_OPT_FLAGS},    \
> diff --git a/libavcodec/ratecontrol.c b/libavcodec/ratecontrol.c
> index 6b77ccd006..03513177f7 100644
> --- a/libavcodec/ratecontrol.c
> +++ b/libavcodec/ratecontrol.c
> @@ -515,8 +515,7 @@ av_cold int ff_rate_control_init(MpegEncContext *s)
>              s->avctx->rc_max_available_vbv_use = 1.0;
>      }
> 
> -    res = av_expr_parse(&rcc->rc_eq_eval,
> -                        s->rc_eq ? s->rc_eq : "tex^qComp",
> +    res = av_expr_parse(&rcc->rc_eq_eval, s->rc_eq,
>                          const_names, func1_names, func1,
>                          NULL, NULL, 0, s->avctx);

what happens if the user sets rc_eq explicitly to NULL ?

thx

[...]
Alexander Strasser Aug. 9, 2020, 12:24 p.m. UTC | #2
Am 9. August 2020 12:56:53 MESZ schrieb Michael Niedermayer <michael@niedermayer.cc>:
>On Fri, Aug 07, 2020 at 11:26:58PM +0200, Alexander Strasser wrote:
>> Don't pass a potential NULL pointer to av_log, instead use a default
>> in the rc_eq AVOption definitions. Now the context variable always
>> holds a string; even if it's the default expression.
>> 
>> Note this also fixes the evaluation error path, where a similar
>> av_log call references the rc_eq string from the context too.
>> 
>> Signed-off-by: Alexander Strasser <eclipse7@gmx.net>
>> ---
>>  libavcodec/mpegvideo.h   | 2 +-
>>  libavcodec/ratecontrol.c | 3 +--
>>  libavcodec/snowenc.c     | 2 +-
>>  3 files changed, 3 insertions(+), 4 deletions(-)
>> 
>> diff --git a/libavcodec/mpegvideo.h b/libavcodec/mpegvideo.h
>> index 29e692f245..d2515a3bbf 100644
>> --- a/libavcodec/mpegvideo.h
>> +++ b/libavcodec/mpegvideo.h
>> @@ -637,7 +637,7 @@ FF_MPV_OPT_CMP_FUNC, \
>>            "defined in the section 'Expression Evaluation', the
>following functions are available: "                                   
>                                         \
>>            "bits2qp(bits), qp2bits(qp). Also the following constants
>are available: iTex pTex tex mv "                                      
>                                    \
>>            "fCode iCount mcVar var isI isP isB avgQP qComp avgIITex
>avgPITex avgPPTex avgBPTex avgTex.",                                   
>                                     \
>> -                                                                   
>FF_MPV_OFFSET(rc_eq), AV_OPT_TYPE_STRING,                          
>.flags = FF_MPV_OPT_FLAGS },            \
>> +                                                                   
>FF_MPV_OFFSET(rc_eq), AV_OPT_TYPE_STRING, {.str = "tex^qComp" },   
>.flags = FF_MPV_OPT_FLAGS },            \
>>  {"rc_init_cplx", "initial complexity for 1-pass encoding",         
>FF_MPV_OFFSET(rc_initial_cplx), AV_OPT_TYPE_FLOAT, {.dbl = 0 },
>-FLT_MAX, FLT_MAX, FF_MPV_OPT_FLAGS},       \
>>  {"rc_buf_aggressivity", "currently useless",                       
>FF_MPV_OFFSET(rc_buffer_aggressivity), AV_OPT_TYPE_FLOAT, {.dbl = 1.0
>}, -FLT_MAX, FLT_MAX, FF_MPV_OPT_FLAGS}, \
>>  {"border_mask", "increase the quantizer for macroblocks close to
>borders", FF_MPV_OFFSET(border_masking), AV_OPT_TYPE_FLOAT, {.dbl = 0
>}, -FLT_MAX, FLT_MAX, FF_MPV_OPT_FLAGS},    \
>> diff --git a/libavcodec/ratecontrol.c b/libavcodec/ratecontrol.c
>> index 6b77ccd006..03513177f7 100644
>> --- a/libavcodec/ratecontrol.c
>> +++ b/libavcodec/ratecontrol.c
>> @@ -515,8 +515,7 @@ av_cold int ff_rate_control_init(MpegEncContext
>*s)
>>              s->avctx->rc_max_available_vbv_use = 1.0;
>>      }
>> 
>> -    res = av_expr_parse(&rcc->rc_eq_eval,
>> -                        s->rc_eq ? s->rc_eq : "tex^qComp",
>> +    res = av_expr_parse(&rcc->rc_eq_eval, s->rc_eq,
>>                          const_names, func1_names, func1,
>>                          NULL, NULL, 0, s->avctx);
>
>what happens if the user sets rc_eq explicitly to NULL ?

Not sure which user you mean.

If I'm not mistaken, a library user should not set it to NULL. Is it even possible while respecting public API?

If an internal user like an encoder sets rc_eq explicitly to NULL it will of course crash like in Snow without my previous patch

I guess I don't know what you prefer. Thus I'm waiting for a reply before attempting a new patch set if it's needed.


  Alexander

[...]
Michael Niedermayer Aug. 9, 2020, 12:56 p.m. UTC | #3
On Sun, Aug 09, 2020 at 02:24:34PM +0200, Alexander Strasser wrote:
> 
> 
> Am 9. August 2020 12:56:53 MESZ schrieb Michael Niedermayer <michael@niedermayer.cc>:
> >On Fri, Aug 07, 2020 at 11:26:58PM +0200, Alexander Strasser wrote:
> >> Don't pass a potential NULL pointer to av_log, instead use a default
> >> in the rc_eq AVOption definitions. Now the context variable always
> >> holds a string; even if it's the default expression.
> >> 
> >> Note this also fixes the evaluation error path, where a similar
> >> av_log call references the rc_eq string from the context too.
> >> 
> >> Signed-off-by: Alexander Strasser <eclipse7@gmx.net>
> >> ---
> >>  libavcodec/mpegvideo.h   | 2 +-
> >>  libavcodec/ratecontrol.c | 3 +--
> >>  libavcodec/snowenc.c     | 2 +-
> >>  3 files changed, 3 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/libavcodec/mpegvideo.h b/libavcodec/mpegvideo.h
> >> index 29e692f245..d2515a3bbf 100644
> >> --- a/libavcodec/mpegvideo.h
> >> +++ b/libavcodec/mpegvideo.h
> >> @@ -637,7 +637,7 @@ FF_MPV_OPT_CMP_FUNC, \
> >>            "defined in the section 'Expression Evaluation', the
> >following functions are available: "                                   
> >                                         \
> >>            "bits2qp(bits), qp2bits(qp). Also the following constants
> >are available: iTex pTex tex mv "                                      
> >                                    \
> >>            "fCode iCount mcVar var isI isP isB avgQP qComp avgIITex
> >avgPITex avgPPTex avgBPTex avgTex.",                                   
> >                                     \
> >> -                                                                   
> >FF_MPV_OFFSET(rc_eq), AV_OPT_TYPE_STRING,                          
> >.flags = FF_MPV_OPT_FLAGS },            \
> >> +                                                                   
> >FF_MPV_OFFSET(rc_eq), AV_OPT_TYPE_STRING, {.str = "tex^qComp" },   
> >.flags = FF_MPV_OPT_FLAGS },            \
> >>  {"rc_init_cplx", "initial complexity for 1-pass encoding",         
> >FF_MPV_OFFSET(rc_initial_cplx), AV_OPT_TYPE_FLOAT, {.dbl = 0 },
> >-FLT_MAX, FLT_MAX, FF_MPV_OPT_FLAGS},       \
> >>  {"rc_buf_aggressivity", "currently useless",                       
> >FF_MPV_OFFSET(rc_buffer_aggressivity), AV_OPT_TYPE_FLOAT, {.dbl = 1.0
> >}, -FLT_MAX, FLT_MAX, FF_MPV_OPT_FLAGS}, \
> >>  {"border_mask", "increase the quantizer for macroblocks close to
> >borders", FF_MPV_OFFSET(border_masking), AV_OPT_TYPE_FLOAT, {.dbl = 0
> >}, -FLT_MAX, FLT_MAX, FF_MPV_OPT_FLAGS},    \
> >> diff --git a/libavcodec/ratecontrol.c b/libavcodec/ratecontrol.c
> >> index 6b77ccd006..03513177f7 100644
> >> --- a/libavcodec/ratecontrol.c
> >> +++ b/libavcodec/ratecontrol.c
> >> @@ -515,8 +515,7 @@ av_cold int ff_rate_control_init(MpegEncContext
> >*s)
> >>              s->avctx->rc_max_available_vbv_use = 1.0;
> >>      }
> >> 
> >> -    res = av_expr_parse(&rcc->rc_eq_eval,
> >> -                        s->rc_eq ? s->rc_eq : "tex^qComp",
> >> +    res = av_expr_parse(&rcc->rc_eq_eval, s->rc_eq,
> >>                          const_names, func1_names, func1,
> >>                          NULL, NULL, 0, s->avctx);
> >
> >what happens if the user sets rc_eq explicitly to NULL ?
> 
> Not sure which user you mean.

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

thx

[...]
Alexander Strasser Aug. 12, 2020, 6:58 p.m. UTC | #4
On 2020-08-09 14:56 +0200, Michael Niedermayer wrote:
> On Sun, Aug 09, 2020 at 02:24:34PM +0200, Alexander Strasser wrote:
> >
> >
> > Am 9. August 2020 12:56:53 MESZ schrieb Michael Niedermayer <michael@niedermayer.cc>:
> > >On Fri, Aug 07, 2020 at 11:26:58PM +0200, Alexander Strasser wrote:
> > >> Don't pass a potential NULL pointer to av_log, instead use a default
> > >> in the rc_eq AVOption definitions. Now the context variable always
> > >> holds a string; even if it's the default expression.
> > >>
> > >> Note this also fixes the evaluation error path, where a similar
> > >> av_log call references the rc_eq string from the context too.
> > >>
> > >> Signed-off-by: Alexander Strasser <eclipse7@gmx.net>
> > >> ---
> > >>  libavcodec/mpegvideo.h   | 2 +-
> > >>  libavcodec/ratecontrol.c | 3 +--
> > >>  libavcodec/snowenc.c     | 2 +-
> > >>  3 files changed, 3 insertions(+), 4 deletions(-)
> > >>
> > >> diff --git a/libavcodec/mpegvideo.h b/libavcodec/mpegvideo.h
> > >> index 29e692f245..d2515a3bbf 100644
> > >> --- a/libavcodec/mpegvideo.h
> > >> +++ b/libavcodec/mpegvideo.h
> > >> @@ -637,7 +637,7 @@ FF_MPV_OPT_CMP_FUNC, \
> > >>            "defined in the section 'Expression Evaluation', the
> > >following functions are available: "
> > >                                         \
> > >>            "bits2qp(bits), qp2bits(qp). Also the following constants
> > >are available: iTex pTex tex mv "
> > >                                    \
> > >>            "fCode iCount mcVar var isI isP isB avgQP qComp avgIITex
> > >avgPITex avgPPTex avgBPTex avgTex.",
> > >                                     \
> > >> -
> > >FF_MPV_OFFSET(rc_eq), AV_OPT_TYPE_STRING,
> > >.flags = FF_MPV_OPT_FLAGS },            \
> > >> +
> > >FF_MPV_OFFSET(rc_eq), AV_OPT_TYPE_STRING, {.str = "tex^qComp" },
> > >.flags = FF_MPV_OPT_FLAGS },            \
> > >>  {"rc_init_cplx", "initial complexity for 1-pass encoding",
> > >FF_MPV_OFFSET(rc_initial_cplx), AV_OPT_TYPE_FLOAT, {.dbl = 0 },
> > >-FLT_MAX, FLT_MAX, FF_MPV_OPT_FLAGS},       \
> > >>  {"rc_buf_aggressivity", "currently useless",
> > >FF_MPV_OFFSET(rc_buffer_aggressivity), AV_OPT_TYPE_FLOAT, {.dbl = 1.0
> > >}, -FLT_MAX, FLT_MAX, FF_MPV_OPT_FLAGS}, \
> > >>  {"border_mask", "increase the quantizer for macroblocks close to
> > >borders", FF_MPV_OFFSET(border_masking), AV_OPT_TYPE_FLOAT, {.dbl = 0
> > >}, -FLT_MAX, FLT_MAX, FF_MPV_OPT_FLAGS},    \
> > >> diff --git a/libavcodec/ratecontrol.c b/libavcodec/ratecontrol.c
> > >> index 6b77ccd006..03513177f7 100644
> > >> --- a/libavcodec/ratecontrol.c
> > >> +++ b/libavcodec/ratecontrol.c
> > >> @@ -515,8 +515,7 @@ av_cold int ff_rate_control_init(MpegEncContext
> > >*s)
> > >>              s->avctx->rc_max_available_vbv_use = 1.0;
> > >>      }
> > >>
> > >> -    res = av_expr_parse(&rcc->rc_eq_eval,
> > >> -                        s->rc_eq ? s->rc_eq : "tex^qComp",
> > >> +    res = av_expr_parse(&rcc->rc_eq_eval, s->rc_eq,
> > >>                          const_names, func1_names, func1,
> > >>                          NULL, NULL, 0, s->avctx);
> > >
> > >what happens if the user sets rc_eq explicitly to NULL ?
> >
> > Not sure which user you mean.
>
> 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.


[...]

  Alexander
Michael Niedermayer Aug. 13, 2020, 8:32 p.m. UTC | #5
On Wed, Aug 12, 2020 at 08:58:30PM +0200, Alexander Strasser wrote:
> On 2020-08-09 14:56 +0200, Michael Niedermayer wrote:
> > On Sun, Aug 09, 2020 at 02:24:34PM +0200, Alexander Strasser wrote:
> > >
> > >
> > > Am 9. August 2020 12:56:53 MESZ schrieb Michael Niedermayer <michael@niedermayer.cc>:
> > > >On Fri, Aug 07, 2020 at 11:26:58PM +0200, Alexander Strasser wrote:
> > > >> Don't pass a potential NULL pointer to av_log, instead use a default
> > > >> in the rc_eq AVOption definitions. Now the context variable always
> > > >> holds a string; even if it's the default expression.
> > > >>
> > > >> Note this also fixes the evaluation error path, where a similar
> > > >> av_log call references the rc_eq string from the context too.
> > > >>
> > > >> Signed-off-by: Alexander Strasser <eclipse7@gmx.net>
> > > >> ---
> > > >>  libavcodec/mpegvideo.h   | 2 +-
> > > >>  libavcodec/ratecontrol.c | 3 +--
> > > >>  libavcodec/snowenc.c     | 2 +-
> > > >>  3 files changed, 3 insertions(+), 4 deletions(-)
> > > >>
> > > >> diff --git a/libavcodec/mpegvideo.h b/libavcodec/mpegvideo.h
> > > >> index 29e692f245..d2515a3bbf 100644
> > > >> --- a/libavcodec/mpegvideo.h
> > > >> +++ b/libavcodec/mpegvideo.h
> > > >> @@ -637,7 +637,7 @@ FF_MPV_OPT_CMP_FUNC, \
> > > >>            "defined in the section 'Expression Evaluation', the
> > > >following functions are available: "
> > > >                                         \
> > > >>            "bits2qp(bits), qp2bits(qp). Also the following constants
> > > >are available: iTex pTex tex mv "
> > > >                                    \
> > > >>            "fCode iCount mcVar var isI isP isB avgQP qComp avgIITex
> > > >avgPITex avgPPTex avgBPTex avgTex.",
> > > >                                     \
> > > >> -
> > > >FF_MPV_OFFSET(rc_eq), AV_OPT_TYPE_STRING,
> > > >.flags = FF_MPV_OPT_FLAGS },            \
> > > >> +
> > > >FF_MPV_OFFSET(rc_eq), AV_OPT_TYPE_STRING, {.str = "tex^qComp" },
> > > >.flags = FF_MPV_OPT_FLAGS },            \
> > > >>  {"rc_init_cplx", "initial complexity for 1-pass encoding",
> > > >FF_MPV_OFFSET(rc_initial_cplx), AV_OPT_TYPE_FLOAT, {.dbl = 0 },
> > > >-FLT_MAX, FLT_MAX, FF_MPV_OPT_FLAGS},       \
> > > >>  {"rc_buf_aggressivity", "currently useless",
> > > >FF_MPV_OFFSET(rc_buffer_aggressivity), AV_OPT_TYPE_FLOAT, {.dbl = 1.0
> > > >}, -FLT_MAX, FLT_MAX, FF_MPV_OPT_FLAGS}, \
> > > >>  {"border_mask", "increase the quantizer for macroblocks close to
> > > >borders", FF_MPV_OFFSET(border_masking), AV_OPT_TYPE_FLOAT, {.dbl = 0
> > > >}, -FLT_MAX, FLT_MAX, FF_MPV_OPT_FLAGS},    \
> > > >> diff --git a/libavcodec/ratecontrol.c b/libavcodec/ratecontrol.c
> > > >> index 6b77ccd006..03513177f7 100644
> > > >> --- a/libavcodec/ratecontrol.c
> > > >> +++ b/libavcodec/ratecontrol.c
> > > >> @@ -515,8 +515,7 @@ av_cold int ff_rate_control_init(MpegEncContext
> > > >*s)
> > > >>              s->avctx->rc_max_available_vbv_use = 1.0;
> > > >>      }
> > > >>
> > > >> -    res = av_expr_parse(&rcc->rc_eq_eval,
> > > >> -                        s->rc_eq ? s->rc_eq : "tex^qComp",
> > > >> +    res = av_expr_parse(&rcc->rc_eq_eval, s->rc_eq,
> > > >>                          const_names, func1_names, func1,
> > > >>                          NULL, NULL, 0, s->avctx);
> > > >
> > > >what happens if the user sets rc_eq explicitly to NULL ?
> > >
> > > Not sure which user you mean.
> >
> > 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

thx

[...]
Alexander Strasser Aug. 16, 2020, 8:57 p.m. UTC | #6
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
Michael Niedermayer Aug. 17, 2020, 9:28 p.m. UTC | #7
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

[...]
diff mbox series

Patch

diff --git a/libavcodec/mpegvideo.h b/libavcodec/mpegvideo.h
index 29e692f245..d2515a3bbf 100644
--- a/libavcodec/mpegvideo.h
+++ b/libavcodec/mpegvideo.h
@@ -637,7 +637,7 @@  FF_MPV_OPT_CMP_FUNC, \
           "defined in the section 'Expression Evaluation', the following functions are available: "                                                                             \
           "bits2qp(bits), qp2bits(qp). Also the following constants are available: iTex pTex tex mv "                                                                           \
           "fCode iCount mcVar var isI isP isB avgQP qComp avgIITex avgPITex avgPPTex avgBPTex avgTex.",                                                                         \
-                                                                    FF_MPV_OFFSET(rc_eq), AV_OPT_TYPE_STRING,                           .flags = FF_MPV_OPT_FLAGS },            \
+                                                                    FF_MPV_OFFSET(rc_eq), AV_OPT_TYPE_STRING, {.str = "tex^qComp" },    .flags = FF_MPV_OPT_FLAGS },            \
 {"rc_init_cplx", "initial complexity for 1-pass encoding",          FF_MPV_OFFSET(rc_initial_cplx), AV_OPT_TYPE_FLOAT, {.dbl = 0 }, -FLT_MAX, FLT_MAX, FF_MPV_OPT_FLAGS},       \
 {"rc_buf_aggressivity", "currently useless",                        FF_MPV_OFFSET(rc_buffer_aggressivity), AV_OPT_TYPE_FLOAT, {.dbl = 1.0 }, -FLT_MAX, FLT_MAX, FF_MPV_OPT_FLAGS}, \
 {"border_mask", "increase the quantizer for macroblocks close to borders", FF_MPV_OFFSET(border_masking), AV_OPT_TYPE_FLOAT, {.dbl = 0 }, -FLT_MAX, FLT_MAX, FF_MPV_OPT_FLAGS},    \
diff --git a/libavcodec/ratecontrol.c b/libavcodec/ratecontrol.c
index 6b77ccd006..03513177f7 100644
--- a/libavcodec/ratecontrol.c
+++ b/libavcodec/ratecontrol.c
@@ -515,8 +515,7 @@  av_cold int ff_rate_control_init(MpegEncContext *s)
             s->avctx->rc_max_available_vbv_use = 1.0;
     }

-    res = av_expr_parse(&rcc->rc_eq_eval,
-                        s->rc_eq ? s->rc_eq : "tex^qComp",
+    res = av_expr_parse(&rcc->rc_eq_eval, s->rc_eq,
                         const_names, func1_names, func1,
                         NULL, NULL, 0, s->avctx);
     if (res < 0) {
diff --git a/libavcodec/snowenc.c b/libavcodec/snowenc.c
index 16d2b7c302..71fb5654cd 100644
--- a/libavcodec/snowenc.c
+++ b/libavcodec/snowenc.c
@@ -1952,7 +1952,7 @@  static const AVOption options[] = {
      "defined in the section 'Expression Evaluation', the following functions are available: "
      "bits2qp(bits), qp2bits(qp). Also the following constants are available: iTex pTex tex mv "
      "fCode iCount mcVar var isI isP isB avgQP qComp avgIITex avgPITex avgPPTex avgBPTex avgTex.",
-                                                                                  OFFSET(m.rc_eq), AV_OPT_TYPE_STRING, { .str = NULL }, 0, 0, VE },
+                                                                                  OFFSET(m.rc_eq), AV_OPT_TYPE_STRING, { .str = "tex^qComp" }, 0, 0, VE },
     { NULL },
 };