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

[Alexander Strasser]

Am 9. August 2020 12:56:53 MESZ schrieb Michael Niedermayer <michael at 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 at 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

[...]