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

[Michael Niedermayer]

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

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

Those who are best at talking, realize last or never when they are wrong.
-------------- 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/20200802/8355cfca/attachment.sig>