| Message ID | 20240501190156.36095-3-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 set by the library, so it should be freed when closing the context. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavcodec/avcodec.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c > index e560efff6a..189a0a2193 100644 > --- a/libavcodec/avcodec.c > +++ b/libavcodec/avcodec.c > @@ -463,7 +463,9 @@ av_cold void ff_codec_close(AVCodecContext *avctx) > av_freep(&avctx->coded_side_data[i].data); > av_freep(&avctx->coded_side_data); > avctx->nb_coded_side_data = 0; > - } > + } else if (av_codec_is_decoder(avctx->codec)) > + av_frame_side_data_free(&avctx->decoded_side_data, > + &avctx->nb_decoded_side_data); > > av_buffer_unref(&avctx->hw_frames_ctx); > av_buffer_unref(&avctx->hw_device_ctx); The documentation actually states that it is "owned and freed by the encoder" for encoding, so your restriction to decoders is wrong. And without it, the corresponding code in avcodec_free_context() is redundant. - Andreas
On 5/1/2024 5:43 PM, Andreas Rheinhardt wrote: > James Almer: >> It's set by the library, so it should be freed when closing the context. >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavcodec/avcodec.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c >> index e560efff6a..189a0a2193 100644 >> --- a/libavcodec/avcodec.c >> +++ b/libavcodec/avcodec.c >> @@ -463,7 +463,9 @@ av_cold void ff_codec_close(AVCodecContext *avctx) >> av_freep(&avctx->coded_side_data[i].data); >> av_freep(&avctx->coded_side_data); >> avctx->nb_coded_side_data = 0; >> - } >> + } else if (av_codec_is_decoder(avctx->codec)) >> + av_frame_side_data_free(&avctx->decoded_side_data, >> + &avctx->nb_decoded_side_data); >> >> av_buffer_unref(&avctx->hw_frames_ctx); >> av_buffer_unref(&avctx->hw_device_ctx); > > The documentation actually states that it is "owned and freed by the > encoder" for encoding, so your restriction to decoders is wrong. And > without it, the corresponding code in avcodec_free_context() is redundant. > > - Andreas Do you suggest freeing it in ff_codec_close() for both scenarios then?
James Almer: > On 5/1/2024 5:43 PM, Andreas Rheinhardt wrote: >> James Almer: >>> It's set by the library, so it should be freed when closing the context. >>> >>> Signed-off-by: James Almer <jamrial@gmail.com> >>> --- >>> libavcodec/avcodec.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c >>> index e560efff6a..189a0a2193 100644 >>> --- a/libavcodec/avcodec.c >>> +++ b/libavcodec/avcodec.c >>> @@ -463,7 +463,9 @@ av_cold void ff_codec_close(AVCodecContext *avctx) >>> av_freep(&avctx->coded_side_data[i].data); >>> av_freep(&avctx->coded_side_data); >>> avctx->nb_coded_side_data = 0; >>> - } >>> + } else if (av_codec_is_decoder(avctx->codec)) >>> + av_frame_side_data_free(&avctx->decoded_side_data, >>> + &avctx->nb_decoded_side_data); >>> av_buffer_unref(&avctx->hw_frames_ctx); >>> av_buffer_unref(&avctx->hw_device_ctx); >> >> The documentation actually states that it is "owned and freed by the >> encoder" for encoding, so your restriction to decoders is wrong. And >> without it, the corresponding code in avcodec_free_context() is >> redundant. >> >> - Andreas > > Do you suggest freeing it in ff_codec_close() for both scenarios then? Yes. - Andreas
diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c index e560efff6a..189a0a2193 100644 --- a/libavcodec/avcodec.c +++ b/libavcodec/avcodec.c @@ -463,7 +463,9 @@ av_cold void ff_codec_close(AVCodecContext *avctx) av_freep(&avctx->coded_side_data[i].data); av_freep(&avctx->coded_side_data); avctx->nb_coded_side_data = 0; - } + } else if (av_codec_is_decoder(avctx->codec)) + av_frame_side_data_free(&avctx->decoded_side_data, + &avctx->nb_decoded_side_data); av_buffer_unref(&avctx->hw_frames_ctx); av_buffer_unref(&avctx->hw_device_ctx);
It's set by the library, so it should be freed when closing the context. Signed-off-by: James Almer <jamrial@gmail.com> --- libavcodec/avcodec.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)