diff mbox series

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

Message ID 20200730124226.GA29814@akuma.local
State New
Headers show
Series [FFmpeg-devel] lavc/ratecontrol: Fix error logging for parsing and evaluation of rc_eq | expand

Checks

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

Commit Message

Alexander Strasser July 30, 2020, 12:42 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 references the rc_eq string from the context too.
---
 libavcodec/mpegvideo.h   | 2 +-
 libavcodec/ratecontrol.c | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

--

Comments

Michael Niedermayer July 31, 2020, 5:54 p.m. UTC | #1 
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

thx

[...]
Alexander Strasser Aug. 1, 2020, 9:23 p.m. UTC | #2 
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


[...]

  Alexander
Michael Niedermayer Aug. 2, 2020, 6:49 p.m. UTC | #3 
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

[...]
Alexander Strasser Aug. 7, 2020, 10:27 p.m. UTC | #4 
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
diff mbox series

Patch

diff --git a/libavcodec/mpegvideo.h b/libavcodec/mpegvideo.h
index 29e692f245..d3261e3218 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) {