| Message ID | 20240501190156.36095-2-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/3] avcodec/avcodec: prevent ch_layout from being uninitialized in ff_codec_close() | expand |
James Almer: > It's a user-set parameter shared with AVCodecParameters, so it should only > be freed by avcodec_free_context(). > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavcodec/avcodec.c | 11 ++++++----- > libavcodec/options.c | 4 ++++ > 2 files changed, 10 insertions(+), 5 deletions(-) > > diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c > index fc8a40e4db..e560efff6a 100644 > --- a/libavcodec/avcodec.c > +++ b/libavcodec/avcodec.c > @@ -458,11 +458,12 @@ av_cold void ff_codec_close(AVCodecContext *avctx) > > av_freep(&avctx->internal); > } > - > - for (i = 0; i < avctx->nb_coded_side_data; i++) > - av_freep(&avctx->coded_side_data[i].data); > - av_freep(&avctx->coded_side_data); > - avctx->nb_coded_side_data = 0; > + if (av_codec_is_encoder(avctx->codec)) { > + for (i = 0; i < avctx->nb_coded_side_data; i++) > + av_freep(&avctx->coded_side_data[i].data); > + av_freep(&avctx->coded_side_data); > + avctx->nb_coded_side_data = 0; > + } > > av_buffer_unref(&avctx->hw_frames_ctx); > av_buffer_unref(&avctx->hw_device_ctx); > diff --git a/libavcodec/options.c b/libavcodec/options.c > index 0c3b40a186..7c32a71275 100644 > --- a/libavcodec/options.c > +++ b/libavcodec/options.c > @@ -177,6 +177,10 @@ void avcodec_free_context(AVCodecContext **pavctx) > av_freep(&avctx->inter_matrix); > av_freep(&avctx->rc_override); > av_channel_layout_uninit(&avctx->ch_layout); > + for (int i = 0; i < avctx->nb_coded_side_data; i++) > + av_freep(&avctx->coded_side_data[i].data); > + av_freep(&avctx->coded_side_data); > + avctx->nb_coded_side_data = 0; > av_frame_side_data_free( > &avctx->decoded_side_data, &avctx->nb_decoded_side_data); > 1. ff_codec_close() already has an "if is_encoder" branch, it does not need another one. 2. The code in ff_codec_close() will be redundant as soon as FF_API_AVCODEC_CLOSE is no more, so it should be inside FF_API_AVCODEC_CLOSE. 3. The documentation of this field does not mention ownership, but given that this field existed for a long time lavc's previous behaviour established the implicit contract that lavc will free this in avcodec_close(). You are breaking this implicit contract for no good reason apart from this new principle that user-set stuff shared with AVCodecParameters should not be freed in avcodec_close(). Which is crazy given that the relevant AVCodecParameters field has only been added recently. - Andreas
On 5/1/2024 5:40 PM, Andreas Rheinhardt wrote: > James Almer: >> It's a user-set parameter shared with AVCodecParameters, so it should only >> be freed by avcodec_free_context(). >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavcodec/avcodec.c | 11 ++++++----- >> libavcodec/options.c | 4 ++++ >> 2 files changed, 10 insertions(+), 5 deletions(-) >> >> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c >> index fc8a40e4db..e560efff6a 100644 >> --- a/libavcodec/avcodec.c >> +++ b/libavcodec/avcodec.c >> @@ -458,11 +458,12 @@ av_cold void ff_codec_close(AVCodecContext *avctx) >> >> av_freep(&avctx->internal); >> } >> - >> - for (i = 0; i < avctx->nb_coded_side_data; i++) >> - av_freep(&avctx->coded_side_data[i].data); >> - av_freep(&avctx->coded_side_data); >> - avctx->nb_coded_side_data = 0; >> + if (av_codec_is_encoder(avctx->codec)) { >> + for (i = 0; i < avctx->nb_coded_side_data; i++) >> + av_freep(&avctx->coded_side_data[i].data); >> + av_freep(&avctx->coded_side_data); >> + avctx->nb_coded_side_data = 0; >> + } >> >> av_buffer_unref(&avctx->hw_frames_ctx); >> av_buffer_unref(&avctx->hw_device_ctx); >> diff --git a/libavcodec/options.c b/libavcodec/options.c >> index 0c3b40a186..7c32a71275 100644 >> --- a/libavcodec/options.c >> +++ b/libavcodec/options.c >> @@ -177,6 +177,10 @@ void avcodec_free_context(AVCodecContext **pavctx) >> av_freep(&avctx->inter_matrix); >> av_freep(&avctx->rc_override); >> av_channel_layout_uninit(&avctx->ch_layout); >> + for (int i = 0; i < avctx->nb_coded_side_data; i++) >> + av_freep(&avctx->coded_side_data[i].data); >> + av_freep(&avctx->coded_side_data); >> + avctx->nb_coded_side_data = 0; >> av_frame_side_data_free( >> &avctx->decoded_side_data, &avctx->nb_decoded_side_data); >> > > 1. ff_codec_close() already has an "if is_encoder" branch, it does not > need another one. > 2. The code in ff_codec_close() will be redundant as soon as > FF_API_AVCODEC_CLOSE is no more, so it should be inside > FF_API_AVCODEC_CLOSE. > 3. The documentation of this field does not mention ownership, but given > that this field existed for a long time lavc's previous behaviour > established the implicit contract that lavc will free this in > avcodec_close(). You are breaking this implicit contract for no good Yes, I wrote this set forgetting avcodec_close() was still a thing. > reason apart from this new principle that user-set stuff shared with > AVCodecParameters should not be freed in avcodec_close(). Which is crazy > given that the relevant AVCodecParameters field has only been added > recently.
diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c index fc8a40e4db..e560efff6a 100644 --- a/libavcodec/avcodec.c +++ b/libavcodec/avcodec.c @@ -458,11 +458,12 @@ av_cold void ff_codec_close(AVCodecContext *avctx) av_freep(&avctx->internal); } - - for (i = 0; i < avctx->nb_coded_side_data; i++) - av_freep(&avctx->coded_side_data[i].data); - av_freep(&avctx->coded_side_data); - avctx->nb_coded_side_data = 0; + if (av_codec_is_encoder(avctx->codec)) { + for (i = 0; i < avctx->nb_coded_side_data; i++) + av_freep(&avctx->coded_side_data[i].data); + av_freep(&avctx->coded_side_data); + avctx->nb_coded_side_data = 0; + } av_buffer_unref(&avctx->hw_frames_ctx); av_buffer_unref(&avctx->hw_device_ctx); diff --git a/libavcodec/options.c b/libavcodec/options.c index 0c3b40a186..7c32a71275 100644 --- a/libavcodec/options.c +++ b/libavcodec/options.c @@ -177,6 +177,10 @@ void avcodec_free_context(AVCodecContext **pavctx) av_freep(&avctx->inter_matrix); av_freep(&avctx->rc_override); av_channel_layout_uninit(&avctx->ch_layout); + for (int i = 0; i < avctx->nb_coded_side_data; i++) + av_freep(&avctx->coded_side_data[i].data); + av_freep(&avctx->coded_side_data); + avctx->nb_coded_side_data = 0; av_frame_side_data_free( &avctx->decoded_side_data, &avctx->nb_decoded_side_data);
It's a user-set parameter shared with AVCodecParameters, so it should only be freed by avcodec_free_context(). Signed-off-by: James Almer <jamrial@gmail.com> --- libavcodec/avcodec.c | 11 ++++++----- libavcodec/options.c | 4 ++++ 2 files changed, 10 insertions(+), 5 deletions(-)