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

[Alexander Strasser]

On 2020-08-02 20:49 +0200, Michael Niedermayer wrote:
> On Sat, Aug 01, 2020 at 11:23:36PM +0200, Alexander Strasser wrote:
> > Hi Michael!
> >
> > On 2020-07-31 19:54 +0200, Michael Niedermayer wrote:
> > > On Thu, Jul 30, 2020 at 02:42:30PM +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 references the rc_eq string from the context too.
> > > > ---
> > > >  libavcodec/mpegvideo.h   | 2 +-
> > > >  libavcodec/ratecontrol.c | 3 +--
> > > >  2 files changed, 2 insertions(+), 3 deletions(-)
> > >
> > > crashes with:
> > > ./ffmpeg -i matrixbench_mpeg2.mpg -vcodec snow  -y file.avi
> > > ==22043== Invalid read of size 1
> > > ==22043==    at 0x4C32CF2: strlen (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> > > ==22043==    by 0x10A83AD: av_expr_parse (in ffmpeg-git/ffmpeg/ffmpeg_g)
> > > ==22043==    by 0x2858ED: ff_rate_control_init (in ffmpeg-git/ffmpeg/ffmpeg_g)
> > > ==22043==    by 0x28A668: encode_init (in ffmpeg-git/ffmpeg/ffmpeg_g)
> > > ==22043==    by 0xA7E2D0: avcodec_open2 (in ffmpeg-git/ffmpeg/ffmpeg_g)
> > > ==22043==    by 0x2F196A: init_output_stream.constprop.24 (in ffmpeg-git/ffmpeg/ffmpeg_g)
> > > ==22043==    by 0x2F383A: reap_filters (in ffmpeg-git/ffmpeg/ffmpeg_g)
> > > ==22043==    by 0x2F78D7: transcode (in ffmpeg-git/ffmpeg/ffmpeg_g)
> > > ==22043==    by 0x2D093B: main (in ffmpeg-git/ffmpeg/ffmpeg_g)
> > > ==22043==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
> > >
> > > if you cannot reproduce, say so and ill make a more detailed backtrace
> >
> > Good catch.
> >
> > I can reproduce all the time. I looked into it and the snow encoder
> > uses the rate control, that is elsewhere only used by the codecs that
> > use the MpegEncContext with initialization and common options. So in
> > case of snow, the rc_eq expression string is always initialized to
> > the default with the current code and with my patch it isn't
> > initialized any more.
> >
> > What should I aim for here?
> >
> > * Always initializing the rc_eq with the default expression
> > * Exposing the rc_eq option
>
> The user should definitly be able to set rc_eq as (s)he prefers for snow too
>
> thx

Thanks for the comment. New patch set finally sent.


  Alexander