Message ID | 20230320233408.134255-3-jeebjp@gmail.com |
---|---|
State | New |
Headers | show |
Series | encoder AVCodecContext configuration side data | expand |
Context | Check | Description |
---|---|---|
andriy/make_x86 | success | Make finished |
andriy/make_fate_x86 | success | Make fate finished |
Quoting Jan Ekström (2023-03-21 00:33:56) > This allows for private values to be stored before the {de,en}coder > has been opened and initialized. > > Add a new unsigned boolean entry to specifically note that a > context has been opened instead of just depending on the internal > pointer. > --- > libavcodec/avcodec.c | 18 +++++++++++------- > libavcodec/frame_thread_encoder.c | 7 ++++--- > libavcodec/internal.h | 5 +++++ > libavcodec/options.c | 20 +++++++++++++++++++- > 4 files changed, 39 insertions(+), 11 deletions(-) > > diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c > index fb1362290f..c110b19e08 100644 > --- a/libavcodec/avcodec.c > +++ b/libavcodec/avcodec.c > @@ -115,7 +115,7 @@ static int64_t get_bit_rate(AVCodecContext *ctx) > int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *codec, AVDictionary **options) > { > int ret = 0; > - AVCodecInternal *avci; > + AVCodecInternal *avci = NULL; Just initialize it to avctx->avci. > const FFCodec *codec2; > > if (avcodec_is_open(avctx)) > @@ -147,12 +147,13 @@ int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *code > if (avctx->extradata_size < 0 || avctx->extradata_size >= FF_MAX_EXTRADATA_SIZE) > return AVERROR(EINVAL); > > - avci = av_mallocz(sizeof(*avci)); > + avci = avctx->internal; > if (!avci) { > - ret = AVERROR(ENOMEM); > - goto end; > + av_log(avctx, AV_LOG_ERROR, > + "This AVCodecContext was not properly allocated! Please utilize " > + "avcodec_alloc_context3!\n"); > + return AVERROR(EINVAL); This should be an assert. > @@ -147,7 +149,21 @@ FF_ENABLE_DEPRECATION_WARNINGS > d++; > } > } > + > + avci = av_mallocz(sizeof(*avci)); > + if (!avci) > + goto alloc_fail; > + > + s->internal = avci; > + > return 0; > + > +alloc_fail: > + av_freep(&s->internal); > + > + av_freep(&s->priv_data); > + > + return AVERROR(ENOMEM); A bit overdoing it with empty lines. Otherwise looks ok.
Jan Ekström: > This allows for private values to be stored before the {de,en}coder > has been opened and initialized. > > Add a new unsigned boolean entry to specifically note that a > context has been opened instead of just depending on the internal > pointer. > --- > libavcodec/avcodec.c | 18 +++++++++++------- > libavcodec/frame_thread_encoder.c | 7 ++++--- > libavcodec/internal.h | 5 +++++ > libavcodec/options.c | 20 +++++++++++++++++++- > 4 files changed, 39 insertions(+), 11 deletions(-) > > diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c > index fb1362290f..c110b19e08 100644 > --- a/libavcodec/avcodec.c > +++ b/libavcodec/avcodec.c > @@ -115,7 +115,7 @@ static int64_t get_bit_rate(AVCodecContext *ctx) > int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *codec, AVDictionary **options) > { > int ret = 0; > - AVCodecInternal *avci; > + AVCodecInternal *avci = NULL; > const FFCodec *codec2; > > if (avcodec_is_open(avctx)) > @@ -147,12 +147,13 @@ int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *code > if (avctx->extradata_size < 0 || avctx->extradata_size >= FF_MAX_EXTRADATA_SIZE) > return AVERROR(EINVAL); > > - avci = av_mallocz(sizeof(*avci)); > + avci = avctx->internal; > if (!avci) { > - ret = AVERROR(ENOMEM); > - goto end; > + av_log(avctx, AV_LOG_ERROR, > + "This AVCodecContext was not properly allocated! Please utilize " > + "avcodec_alloc_context3!\n"); > + return AVERROR(EINVAL); > } > - avctx->internal = avci; > > avci->buffer_frame = av_frame_alloc(); > avci->buffer_pkt = av_packet_alloc(); > @@ -360,6 +361,9 @@ FF_ENABLE_DEPRECATION_WARNINGS > > end: > > + if (ret >= 0 && avci) > + avci->ctx_opened = 1; This should be moved before end: in order to save the checks. > + > return ret; > free_and_end: > avcodec_close(avctx); > @@ -470,7 +474,7 @@ av_cold int avcodec_close(AVCodecContext *avctx) > ff_icc_context_uninit(&avci->icc); > #endif > > - av_freep(&avctx->internal); > + avci->ctx_opened = 0; > } > > for (i = 0; i < avctx->nb_coded_side_data; i++) > @@ -703,7 +707,7 @@ void avcodec_string(char *buf, int buf_size, AVCodecContext *enc, int encode) > > int avcodec_is_open(AVCodecContext *s) > { > - return !!s->internal; > + return s->internal && s->internal->ctx_opened; > } > > int attribute_align_arg avcodec_receive_frame(AVCodecContext *avctx, AVFrame *frame) > diff --git a/libavcodec/frame_thread_encoder.c b/libavcodec/frame_thread_encoder.c > index 62d9580ad4..683ba52608 100644 > --- a/libavcodec/frame_thread_encoder.c > +++ b/libavcodec/frame_thread_encoder.c > @@ -110,8 +110,7 @@ static void * attribute_align_arg worker(void *v){ > pthread_mutex_unlock(&c->finished_task_mutex); > } > end: > - avcodec_close(avctx); > - av_freep(&avctx); > + avcodec_free_context(&avctx); This will also free the other stuff in avcodec_free_context() and might therefore cause double-frees. > return NULL; > } > > @@ -195,15 +194,17 @@ av_cold int ff_frame_thread_encoder_init(AVCodecContext *avctx) > > for(i=0; i<avctx->thread_count ; i++){ > void *tmpv; > + AVCodecInternal *avci; > thread_avctx = avcodec_alloc_context3(avctx->codec); > if (!thread_avctx) { > ret = AVERROR(ENOMEM); > goto fail; > } > tmpv = thread_avctx->priv_data; > + avci = thread_avctx->internal; > *thread_avctx = *avctx; > thread_avctx->priv_data = tmpv; > - thread_avctx->internal = NULL; > + thread_avctx->internal = avci; > thread_avctx->hw_frames_ctx = NULL; > ret = av_opt_copy(thread_avctx, avctx); > if (ret < 0) > diff --git a/libavcodec/internal.h b/libavcodec/internal.h > index a283c52e01..f21101752d 100644 > --- a/libavcodec/internal.h > +++ b/libavcodec/internal.h > @@ -163,6 +163,11 @@ typedef struct AVCodecInternal { > #if CONFIG_LCMS2 > FFIccContext icc; /* used to read and write embedded ICC profiles */ > #endif > + > + /** > + * a boolean to describe whether context is opened or not. > + */ > + unsigned int ctx_opened; > } AVCodecInternal; > > /** > diff --git a/libavcodec/options.c b/libavcodec/options.c > index a9b35ee1c3..f8fab164fb 100644 > --- a/libavcodec/options.c > +++ b/libavcodec/options.c > @@ -28,6 +28,7 @@ > > #include "avcodec.h" > #include "codec_internal.h" > +#include "internal.h" > #include "libavutil/avassert.h" > #include "libavutil/internal.h" > #include "libavutil/mem.h" > @@ -89,6 +90,7 @@ static const AVClass av_codec_context_class = { > static int init_context_defaults(AVCodecContext *s, const AVCodec *codec) > { > const FFCodec *const codec2 = ffcodec(codec); > + AVCodecInternal *avci = NULL; > int flags=0; > memset(s, 0, sizeof(AVCodecContext)); > > @@ -132,7 +134,7 @@ FF_ENABLE_DEPRECATION_WARNINGS > if(codec && codec2->priv_data_size){ > s->priv_data = av_mallocz(codec2->priv_data_size); > if (!s->priv_data) > - return AVERROR(ENOMEM); > + goto alloc_fail; > if(codec->priv_class){ > *(const AVClass**)s->priv_data = codec->priv_class; > av_opt_set_defaults(s->priv_data); > @@ -147,7 +149,21 @@ FF_ENABLE_DEPRECATION_WARNINGS > d++; > } > } > + > + avci = av_mallocz(sizeof(*avci)); > + if (!avci) > + goto alloc_fail; > + > + s->internal = avci; > + > return 0; > + > +alloc_fail: > + av_freep(&s->internal); > + > + av_freep(&s->priv_data); > + > + return AVERROR(ENOMEM); > } > > AVCodecContext *avcodec_alloc_context3(const AVCodec *codec) > @@ -174,6 +190,8 @@ void avcodec_free_context(AVCodecContext **pavctx) > > avcodec_close(avctx); > > + av_freep(&avctx->internal); Moving this to avcodec_free_context() creates a leak when using avcodec_close()+av_free(). This can be fixed by allocating the AVCodecContext and the AVCodecInternal jointly. > + > av_freep(&avctx->extradata); > av_freep(&avctx->subtitle_header); > av_freep(&avctx->intra_matrix);
On 3/24/2023 9:07 AM, Andreas Rheinhardt wrote: >> @@ -174,6 +190,8 @@ void avcodec_free_context(AVCodecContext **pavctx) >> >> avcodec_close(avctx); >> >> + av_freep(&avctx->internal); > Moving this to avcodec_free_context() creates a leak when using > avcodec_close()+av_free(). This can be fixed by allocating the > AVCodecContext and the AVCodecInternal jointly. Can't we just declare that doing av_free() on a AVCodecContext is not a valid API usage? Every other struct with an specific free function is very clear about it being the only way to free them. > >> + >> av_freep(&avctx->extradata); >> av_freep(&avctx->subtitle_header); >> av_freep(&avctx->intra_matrix);
Quoting James Almer (2023-03-24 14:02:40) > On 3/24/2023 9:07 AM, Andreas Rheinhardt wrote: > >> @@ -174,6 +190,8 @@ void avcodec_free_context(AVCodecContext **pavctx) > >> > >> avcodec_close(avctx); > >> > >> + av_freep(&avctx->internal); > > Moving this to avcodec_free_context() creates a leak when using > > avcodec_close()+av_free(). This can be fixed by allocating the > > AVCodecContext and the AVCodecInternal jointly. > > Can't we just declare that doing av_free() on a AVCodecContext is not a > valid API usage? Every other struct with an specific free function is > very clear about it being the only way to free them. Sadly I expect many callers still do this, even though avcodec_free_context() has existed since 2014. The proper solution is to deprecated avcodec_close(), but that needs a new parser API.
On Tue, Mar 21, 2023 at 01:33:56AM +0200, Jan Ekström wrote: > This allows for private values to be stored before the {de,en}coder > has been opened and initialized. > > Add a new unsigned boolean entry to specifically note that a > context has been opened instead of just depending on the internal > pointer. > --- > libavcodec/avcodec.c | 18 +++++++++++------- > libavcodec/frame_thread_encoder.c | 7 ++++--- > libavcodec/internal.h | 5 +++++ > libavcodec/options.c | 20 +++++++++++++++++++- > 4 files changed, 39 insertions(+), 11 deletions(-) this causes memory corruption with mjpeg ./ffmpeg_g -i lena.pnm -qscale 4 -intra_matrix 10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10 -chroma_intra_matrix 400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400 -bitexact file-custommatrix10,400.jpg i see video:19kB audio:0kB subtitle:0kB other streams:0kB global headers:0kB muxing overhead: unknown corrupted size vs. prev_size Aborted (core dumped) i can provide more details if it isnt reproduceable thx [...]
Michael Niedermayer: > On Tue, Mar 21, 2023 at 01:33:56AM +0200, Jan Ekström wrote: >> This allows for private values to be stored before the {de,en}coder >> has been opened and initialized. >> >> Add a new unsigned boolean entry to specifically note that a >> context has been opened instead of just depending on the internal >> pointer. >> --- >> libavcodec/avcodec.c | 18 +++++++++++------- >> libavcodec/frame_thread_encoder.c | 7 ++++--- >> libavcodec/internal.h | 5 +++++ >> libavcodec/options.c | 20 +++++++++++++++++++- >> 4 files changed, 39 insertions(+), 11 deletions(-) > > this causes memory corruption with mjpeg > ./ffmpeg_g -i lena.pnm -qscale 4 -intra_matrix 10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10 -chroma_intra_matrix 400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400 -bitexact file-custommatrix10,400.jpg > > i see > > video:19kB audio:0kB subtitle:0kB other streams:0kB global headers:0kB muxing overhead: unknown > corrupted size vs. prev_size > Aborted (core dumped) > > i can provide more details if it isnt reproduceable > This is an example of the double-frees due to using avcodec_free_context() to free the worker threads that I mentioned in my reply. - Andreas
diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c index fb1362290f..c110b19e08 100644 --- a/libavcodec/avcodec.c +++ b/libavcodec/avcodec.c @@ -115,7 +115,7 @@ static int64_t get_bit_rate(AVCodecContext *ctx) int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *codec, AVDictionary **options) { int ret = 0; - AVCodecInternal *avci; + AVCodecInternal *avci = NULL; const FFCodec *codec2; if (avcodec_is_open(avctx)) @@ -147,12 +147,13 @@ int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *code if (avctx->extradata_size < 0 || avctx->extradata_size >= FF_MAX_EXTRADATA_SIZE) return AVERROR(EINVAL); - avci = av_mallocz(sizeof(*avci)); + avci = avctx->internal; if (!avci) { - ret = AVERROR(ENOMEM); - goto end; + av_log(avctx, AV_LOG_ERROR, + "This AVCodecContext was not properly allocated! Please utilize " + "avcodec_alloc_context3!\n"); + return AVERROR(EINVAL); } - avctx->internal = avci; avci->buffer_frame = av_frame_alloc(); avci->buffer_pkt = av_packet_alloc(); @@ -360,6 +361,9 @@ FF_ENABLE_DEPRECATION_WARNINGS end: + if (ret >= 0 && avci) + avci->ctx_opened = 1; + return ret; free_and_end: avcodec_close(avctx); @@ -470,7 +474,7 @@ av_cold int avcodec_close(AVCodecContext *avctx) ff_icc_context_uninit(&avci->icc); #endif - av_freep(&avctx->internal); + avci->ctx_opened = 0; } for (i = 0; i < avctx->nb_coded_side_data; i++) @@ -703,7 +707,7 @@ void avcodec_string(char *buf, int buf_size, AVCodecContext *enc, int encode) int avcodec_is_open(AVCodecContext *s) { - return !!s->internal; + return s->internal && s->internal->ctx_opened; } int attribute_align_arg avcodec_receive_frame(AVCodecContext *avctx, AVFrame *frame) diff --git a/libavcodec/frame_thread_encoder.c b/libavcodec/frame_thread_encoder.c index 62d9580ad4..683ba52608 100644 --- a/libavcodec/frame_thread_encoder.c +++ b/libavcodec/frame_thread_encoder.c @@ -110,8 +110,7 @@ static void * attribute_align_arg worker(void *v){ pthread_mutex_unlock(&c->finished_task_mutex); } end: - avcodec_close(avctx); - av_freep(&avctx); + avcodec_free_context(&avctx); return NULL; } @@ -195,15 +194,17 @@ av_cold int ff_frame_thread_encoder_init(AVCodecContext *avctx) for(i=0; i<avctx->thread_count ; i++){ void *tmpv; + AVCodecInternal *avci; thread_avctx = avcodec_alloc_context3(avctx->codec); if (!thread_avctx) { ret = AVERROR(ENOMEM); goto fail; } tmpv = thread_avctx->priv_data; + avci = thread_avctx->internal; *thread_avctx = *avctx; thread_avctx->priv_data = tmpv; - thread_avctx->internal = NULL; + thread_avctx->internal = avci; thread_avctx->hw_frames_ctx = NULL; ret = av_opt_copy(thread_avctx, avctx); if (ret < 0) diff --git a/libavcodec/internal.h b/libavcodec/internal.h index a283c52e01..f21101752d 100644 --- a/libavcodec/internal.h +++ b/libavcodec/internal.h @@ -163,6 +163,11 @@ typedef struct AVCodecInternal { #if CONFIG_LCMS2 FFIccContext icc; /* used to read and write embedded ICC profiles */ #endif + + /** + * a boolean to describe whether context is opened or not. + */ + unsigned int ctx_opened; } AVCodecInternal; /** diff --git a/libavcodec/options.c b/libavcodec/options.c index a9b35ee1c3..f8fab164fb 100644 --- a/libavcodec/options.c +++ b/libavcodec/options.c @@ -28,6 +28,7 @@ #include "avcodec.h" #include "codec_internal.h" +#include "internal.h" #include "libavutil/avassert.h" #include "libavutil/internal.h" #include "libavutil/mem.h" @@ -89,6 +90,7 @@ static const AVClass av_codec_context_class = { static int init_context_defaults(AVCodecContext *s, const AVCodec *codec) { const FFCodec *const codec2 = ffcodec(codec); + AVCodecInternal *avci = NULL; int flags=0; memset(s, 0, sizeof(AVCodecContext)); @@ -132,7 +134,7 @@ FF_ENABLE_DEPRECATION_WARNINGS if(codec && codec2->priv_data_size){ s->priv_data = av_mallocz(codec2->priv_data_size); if (!s->priv_data) - return AVERROR(ENOMEM); + goto alloc_fail; if(codec->priv_class){ *(const AVClass**)s->priv_data = codec->priv_class; av_opt_set_defaults(s->priv_data); @@ -147,7 +149,21 @@ FF_ENABLE_DEPRECATION_WARNINGS d++; } } + + avci = av_mallocz(sizeof(*avci)); + if (!avci) + goto alloc_fail; + + s->internal = avci; + return 0; + +alloc_fail: + av_freep(&s->internal); + + av_freep(&s->priv_data); + + return AVERROR(ENOMEM); } AVCodecContext *avcodec_alloc_context3(const AVCodec *codec) @@ -174,6 +190,8 @@ void avcodec_free_context(AVCodecContext **pavctx) avcodec_close(avctx); + av_freep(&avctx->internal); + av_freep(&avctx->extradata); av_freep(&avctx->subtitle_header); av_freep(&avctx->intra_matrix);