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 |
Context | Check | Description |
---|---|---|
andriy/default | pending | |
andriy/make | success | Make finished |
andriy/make_fate | success | Make fate finished |
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 [...]
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
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 [...]
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 --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) {