| Message ID | 20220320231809.40398-1-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/avcodec: don't free AVOption settable fields in avcodec_close() | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_aarch64_jetson | success | Make finished |
| andriy/make_fate_aarch64_jetson | success | Make fate finished |
| andriy/make_armv7_RPi4 | success | Make finished |
| andriy/make_fate_armv7_RPi4 | success | Make fate finished |
James Almer: > It can uninitialize fields that may still be used after the context was closed, > so do it instead in avcodec_free_context(). > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavcodec/avcodec.c | 1 - > libavcodec/options.c | 2 +- > 2 files changed, 1 insertion(+), 2 deletions(-) > > diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c > index 38bdaad4fa..122d09b63a 100644 > --- a/libavcodec/avcodec.c > +++ b/libavcodec/avcodec.c > @@ -524,7 +524,6 @@ av_cold int avcodec_close(AVCodecContext *avctx) > > if (avctx->priv_data && avctx->codec && avctx->codec->priv_class) > av_opt_free(avctx->priv_data); > - av_opt_free(avctx); > av_freep(&avctx->priv_data); > if (av_codec_is_encoder(avctx->codec)) { > av_freep(&avctx->extradata); > diff --git a/libavcodec/options.c b/libavcodec/options.c > index 33f11480a7..91335415c1 100644 > --- a/libavcodec/options.c > +++ b/libavcodec/options.c > @@ -172,7 +172,7 @@ void avcodec_free_context(AVCodecContext **pavctx) > av_freep(&avctx->intra_matrix); > av_freep(&avctx->inter_matrix); > av_freep(&avctx->rc_override); > - av_channel_layout_uninit(&avctx->ch_layout); > + av_opt_free(avctx); > > av_freep(pavctx); > } This will lead to memleaks for users that use avcodec_close(avctx) + av_free(avctx) to free an AVCodecContext (e.g. our frame-threaded encoders do this). Notice that avcodec_free_context() violates the documentation of AVCodecContext.extradata (documented to not be freed for decoders) and AVCodecContext.subtitle_header and AVCodecContext.rc_override (documented to not be freed by lavc for encoders), so there is a reason for using it instead of avcodec_free_context() (even when not reusing the context). - Andreas
On 3/20/2022 8:26 PM, Andreas Rheinhardt wrote: > James Almer: >> It can uninitialize fields that may still be used after the context was closed, >> so do it instead in avcodec_free_context(). >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavcodec/avcodec.c | 1 - >> libavcodec/options.c | 2 +- >> 2 files changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c >> index 38bdaad4fa..122d09b63a 100644 >> --- a/libavcodec/avcodec.c >> +++ b/libavcodec/avcodec.c >> @@ -524,7 +524,6 @@ av_cold int avcodec_close(AVCodecContext *avctx) >> >> if (avctx->priv_data && avctx->codec && avctx->codec->priv_class) >> av_opt_free(avctx->priv_data); >> - av_opt_free(avctx); >> av_freep(&avctx->priv_data); >> if (av_codec_is_encoder(avctx->codec)) { >> av_freep(&avctx->extradata); >> diff --git a/libavcodec/options.c b/libavcodec/options.c >> index 33f11480a7..91335415c1 100644 >> --- a/libavcodec/options.c >> +++ b/libavcodec/options.c >> @@ -172,7 +172,7 @@ void avcodec_free_context(AVCodecContext **pavctx) >> av_freep(&avctx->intra_matrix); >> av_freep(&avctx->inter_matrix); >> av_freep(&avctx->rc_override); >> - av_channel_layout_uninit(&avctx->ch_layout); >> + av_opt_free(avctx); >> >> av_freep(pavctx); >> } > > This will lead to memleaks for users that use avcodec_close(avctx) + > av_free(avctx) to free an AVCodecContext (e.g. our frame-threaded > encoders do this). Notice that avcodec_free_context() violates the > documentation of AVCodecContext.extradata (documented to not be freed > for decoders) and AVCodecContext.subtitle_header and > AVCodecContext.rc_override (documented to not be freed by lavc for > encoders), so there is a reason for using it instead of > avcodec_free_context() (even when not reusing the context). That's an absolute mess of a situation. av_free(avctx) should not be an allowed or supported scenario when avcodec_free_context() exists. And why is the latter violating its own documentation? > > - Andreas > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
James Almer: > > > On 3/20/2022 8:26 PM, Andreas Rheinhardt wrote: >> James Almer: >>> It can uninitialize fields that may still be used after the context >>> was closed, >>> so do it instead in avcodec_free_context(). >>> >>> Signed-off-by: James Almer <jamrial@gmail.com> >>> --- >>> libavcodec/avcodec.c | 1 - >>> libavcodec/options.c | 2 +- >>> 2 files changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c >>> index 38bdaad4fa..122d09b63a 100644 >>> --- a/libavcodec/avcodec.c >>> +++ b/libavcodec/avcodec.c >>> @@ -524,7 +524,6 @@ av_cold int avcodec_close(AVCodecContext *avctx) >>> if (avctx->priv_data && avctx->codec && >>> avctx->codec->priv_class) >>> av_opt_free(avctx->priv_data); >>> - av_opt_free(avctx); >>> av_freep(&avctx->priv_data); >>> if (av_codec_is_encoder(avctx->codec)) { >>> av_freep(&avctx->extradata); >>> diff --git a/libavcodec/options.c b/libavcodec/options.c >>> index 33f11480a7..91335415c1 100644 >>> --- a/libavcodec/options.c >>> +++ b/libavcodec/options.c >>> @@ -172,7 +172,7 @@ void avcodec_free_context(AVCodecContext **pavctx) >>> av_freep(&avctx->intra_matrix); >>> av_freep(&avctx->inter_matrix); >>> av_freep(&avctx->rc_override); >>> - av_channel_layout_uninit(&avctx->ch_layout); >>> + av_opt_free(avctx); >>> av_freep(pavctx); >>> } >> >> This will lead to memleaks for users that use avcodec_close(avctx) + >> av_free(avctx) to free an AVCodecContext (e.g. our frame-threaded >> encoders do this). Notice that avcodec_free_context() violates the >> documentation of AVCodecContext.extradata (documented to not be freed >> for decoders) and AVCodecContext.subtitle_header and >> AVCodecContext.rc_override (documented to not be freed by lavc for >> encoders), so there is a reason for using it instead of >> avcodec_free_context() (even when not reusing the context). > > That's an absolute mess of a situation. av_free(avctx) should not be an > allowed or supported scenario when avcodec_free_context() exists. And > why is the latter violating its own documentation? > It is not violating its own documentation, but the documentation of the relevant AVCodecContext fields. IIRC Anton wanted a function that just frees the whole context, even if this meant that fields which are documented as being owned by the user are freed. Even documenting the current state of affairs in avcodec.h doesn't change the fact that there is a valid reason to use avcodec_close()+av_free(), so we can't pretend it doesn't happen. - Andreas
On 3/20/2022 8:34 PM, Andreas Rheinhardt wrote: > James Almer: >> >> >> On 3/20/2022 8:26 PM, Andreas Rheinhardt wrote: >>> James Almer: >>>> It can uninitialize fields that may still be used after the context >>>> was closed, >>>> so do it instead in avcodec_free_context(). >>>> >>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>> --- >>>> libavcodec/avcodec.c | 1 - >>>> libavcodec/options.c | 2 +- >>>> 2 files changed, 1 insertion(+), 2 deletions(-) >>>> >>>> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c >>>> index 38bdaad4fa..122d09b63a 100644 >>>> --- a/libavcodec/avcodec.c >>>> +++ b/libavcodec/avcodec.c >>>> @@ -524,7 +524,6 @@ av_cold int avcodec_close(AVCodecContext *avctx) >>>> if (avctx->priv_data && avctx->codec && >>>> avctx->codec->priv_class) >>>> av_opt_free(avctx->priv_data); >>>> - av_opt_free(avctx); >>>> av_freep(&avctx->priv_data); >>>> if (av_codec_is_encoder(avctx->codec)) { >>>> av_freep(&avctx->extradata); >>>> diff --git a/libavcodec/options.c b/libavcodec/options.c >>>> index 33f11480a7..91335415c1 100644 >>>> --- a/libavcodec/options.c >>>> +++ b/libavcodec/options.c >>>> @@ -172,7 +172,7 @@ void avcodec_free_context(AVCodecContext **pavctx) >>>> av_freep(&avctx->intra_matrix); >>>> av_freep(&avctx->inter_matrix); >>>> av_freep(&avctx->rc_override); >>>> - av_channel_layout_uninit(&avctx->ch_layout); >>>> + av_opt_free(avctx); >>>> av_freep(pavctx); >>>> } >>> >>> This will lead to memleaks for users that use avcodec_close(avctx) + >>> av_free(avctx) to free an AVCodecContext (e.g. our frame-threaded >>> encoders do this). Notice that avcodec_free_context() violates the >>> documentation of AVCodecContext.extradata (documented to not be freed >>> for decoders) and AVCodecContext.subtitle_header and >>> AVCodecContext.rc_override (documented to not be freed by lavc for >>> encoders), so there is a reason for using it instead of >>> avcodec_free_context() (even when not reusing the context). >> >> That's an absolute mess of a situation. av_free(avctx) should not be an >> allowed or supported scenario when avcodec_free_context() exists. And >> why is the latter violating its own documentation? >> > > It is not violating its own documentation, but the documentation of the > relevant AVCodecContext fields. IIRC Anton wanted a function that just > frees the whole context, even if this meant that fields which are > documented as being owned by the user are freed. Even documenting the > current state of affairs in avcodec.h doesn't change the fact that there > is a valid reason to use avcodec_close()+av_free(), so we can't pretend > it doesn't happen. > > - Andreas Ok, do i add a codecpar copy like i suggested in http://ffmpeg.org/pipermail/ffmpeg-devel/2022-March/294312.html, then? It works, but it feels really weird doing that in what's the cleanup portion of the function. Alternatively, add the dance from https://patchwork.ffmpeg.org/project/ffmpeg/patch/20220319030407.45503-1-jamrial@gmail.com/ which should have the same effect and never fail, unlike param copy.
On Sun, 20 Mar 2022, James Almer wrote: > On 3/20/2022 8:34 PM, Andreas Rheinhardt wrote: >> James Almer: >>> >>> >>> On 3/20/2022 8:26 PM, Andreas Rheinhardt wrote: >>>> James Almer: >>>>> It can uninitialize fields that may still be used after the context >>>>> was closed, >>>>> so do it instead in avcodec_free_context(). >>>>> >>>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>>> --- >>>>> libavcodec/avcodec.c | 1 - >>>>> libavcodec/options.c | 2 +- >>>>> 2 files changed, 1 insertion(+), 2 deletions(-) >>>>> >>>>> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c >>>>> index 38bdaad4fa..122d09b63a 100644 >>>>> --- a/libavcodec/avcodec.c >>>>> +++ b/libavcodec/avcodec.c >>>>> @@ -524,7 +524,6 @@ av_cold int avcodec_close(AVCodecContext *avctx) >>>>> if (avctx->priv_data && avctx->codec && >>>>> avctx->codec->priv_class) >>>>> av_opt_free(avctx->priv_data); >>>>> - av_opt_free(avctx); >>>>> av_freep(&avctx->priv_data); >>>>> if (av_codec_is_encoder(avctx->codec)) { >>>>> av_freep(&avctx->extradata); >>>>> diff --git a/libavcodec/options.c b/libavcodec/options.c >>>>> index 33f11480a7..91335415c1 100644 >>>>> --- a/libavcodec/options.c >>>>> +++ b/libavcodec/options.c >>>>> @@ -172,7 +172,7 @@ void avcodec_free_context(AVCodecContext **pavctx) >>>>> av_freep(&avctx->intra_matrix); >>>>> av_freep(&avctx->inter_matrix); >>>>> av_freep(&avctx->rc_override); >>>>> - av_channel_layout_uninit(&avctx->ch_layout); >>>>> + av_opt_free(avctx); >>>>> av_freep(pavctx); >>>>> } >>>> >>>> This will lead to memleaks for users that use avcodec_close(avctx) + >>>> av_free(avctx) to free an AVCodecContext (e.g. our frame-threaded >>>> encoders do this). Notice that avcodec_free_context() violates the >>>> documentation of AVCodecContext.extradata (documented to not be freed >>>> for decoders) and AVCodecContext.subtitle_header and >>>> AVCodecContext.rc_override (documented to not be freed by lavc for >>>> encoders), so there is a reason for using it instead of >>>> avcodec_free_context() (even when not reusing the context). >>> >>> That's an absolute mess of a situation. av_free(avctx) should not be an >>> allowed or supported scenario when avcodec_free_context() exists. And >>> why is the latter violating its own documentation? >>> >> >> It is not violating its own documentation, but the documentation of the >> relevant AVCodecContext fields. IIRC Anton wanted a function that just >> frees the whole context, even if this meant that fields which are >> documented as being owned by the user are freed. Even documenting the >> current state of affairs in avcodec.h doesn't change the fact that there >> is a valid reason to use avcodec_close()+av_free(), so we can't pretend >> it doesn't happen. >> >> - Andreas > > Ok, do i add a codecpar copy like i suggested in > http://ffmpeg.org/pipermail/ffmpeg-devel/2022-March/294312.html, then? It > works, but it feels really weird doing that in what's the cleanup portion of > the function. > Alternatively, add the dance from > https://patchwork.ffmpeg.org/project/ffmpeg/patch/20220319030407.45503-1-jamrial@gmail.com/ > which should have the same effect and never fail, unlike param copy. The latter would also leak memory on avcodec_close()+av_freep(). Regards, Marton
On Mon, 21 Mar 2022, Marton Balint wrote: > > > On Sun, 20 Mar 2022, James Almer wrote: > >> On 3/20/2022 8:34 PM, Andreas Rheinhardt wrote: >>> James Almer: >>>> >>>> >>>> On 3/20/2022 8:26 PM, Andreas Rheinhardt wrote: >>>>> James Almer: >>>>>> It can uninitialize fields that may still be used after the context >>>>>> was closed, >>>>>> so do it instead in avcodec_free_context(). >>>>>> >>>>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>>>> --- >>>>>> libavcodec/avcodec.c | 1 - >>>>>> libavcodec/options.c | 2 +- >>>>>> 2 files changed, 1 insertion(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c >>>>>> index 38bdaad4fa..122d09b63a 100644 >>>>>> --- a/libavcodec/avcodec.c >>>>>> +++ b/libavcodec/avcodec.c >>>>>> @@ -524,7 +524,6 @@ av_cold int avcodec_close(AVCodecContext *avctx) >>>>>> if (avctx->priv_data && avctx->codec && >>>>>> avctx->codec->priv_class) >>>>>> av_opt_free(avctx->priv_data); >>>>>> - av_opt_free(avctx); >>>>>> av_freep(&avctx->priv_data); >>>>>> if (av_codec_is_encoder(avctx->codec)) { >>>>>> av_freep(&avctx->extradata); >>>>>> diff --git a/libavcodec/options.c b/libavcodec/options.c >>>>>> index 33f11480a7..91335415c1 100644 >>>>>> --- a/libavcodec/options.c >>>>>> +++ b/libavcodec/options.c >>>>>> @@ -172,7 +172,7 @@ void avcodec_free_context(AVCodecContext >>>>>> **pavctx) >>>>>> av_freep(&avctx->intra_matrix); >>>>>> av_freep(&avctx->inter_matrix); >>>>>> av_freep(&avctx->rc_override); >>>>>> - av_channel_layout_uninit(&avctx->ch_layout); >>>>>> + av_opt_free(avctx); >>>>>> av_freep(pavctx); >>>>>> } >>>>> >>>>> This will lead to memleaks for users that use avcodec_close(avctx) + >>>>> av_free(avctx) to free an AVCodecContext (e.g. our frame-threaded >>>>> encoders do this). Notice that avcodec_free_context() violates the >>>>> documentation of AVCodecContext.extradata (documented to not be freed >>>>> for decoders) and AVCodecContext.subtitle_header and >>>>> AVCodecContext.rc_override (documented to not be freed by lavc for >>>>> encoders), so there is a reason for using it instead of >>>>> avcodec_free_context() (even when not reusing the context). >>>> >>>> That's an absolute mess of a situation. av_free(avctx) should not be an >>>> allowed or supported scenario when avcodec_free_context() exists. And >>>> why is the latter violating its own documentation? >>>> >>> >>> It is not violating its own documentation, but the documentation of the >>> relevant AVCodecContext fields. IIRC Anton wanted a function that just >>> frees the whole context, even if this meant that fields which are >>> documented as being owned by the user are freed. Even documenting the >>> current state of affairs in avcodec.h doesn't change the fact that there >>> is a valid reason to use avcodec_close()+av_free(), so we can't pretend >>> it doesn't happen. >>> >>> - Andreas >> >> Ok, do i add a codecpar copy like i suggested in >> http://ffmpeg.org/pipermail/ffmpeg-devel/2022-March/294312.html, then? It >> works, but it feels really weird doing that in what's the cleanup portion >> of the function. >> Alternatively, add the dance from >> https://patchwork.ffmpeg.org/project/ffmpeg/patch/20220319030407.45503-1-jamrial@gmail.com/ >> which should have the same effect and never fail, unlike param copy. > > The latter would also leak memory on avcodec_close()+av_freep(). Sorry, I meant the first one.
On 3/20/2022 9:05 PM, Marton Balint wrote: > > > On Mon, 21 Mar 2022, Marton Balint wrote: > >> >> >> On Sun, 20 Mar 2022, James Almer wrote: >> >>> On 3/20/2022 8:34 PM, Andreas Rheinhardt wrote: >>>> James Almer: >>>>> >>>>> >>>>> On 3/20/2022 8:26 PM, Andreas Rheinhardt wrote: >>>>>> James Almer: >>>>>>> It can uninitialize fields that may still be used after the >>>>>>> context >>>>>>> was closed, >>>>>>> so do it instead in avcodec_free_context(). >>>>>>> >>>>>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>>>>> --- >>>>>>> libavcodec/avcodec.c | 1 - >>>>>>> libavcodec/options.c | 2 +- >>>>>>> 2 files changed, 1 insertion(+), 2 deletions(-) >>>>>>> >>>>>>> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c >>>>>>> index 38bdaad4fa..122d09b63a 100644 >>>>>>> --- a/libavcodec/avcodec.c >>>>>>> +++ b/libavcodec/avcodec.c >>>>>>> @@ -524,7 +524,6 @@ av_cold int avcodec_close(AVCodecContext >>>>>>> *avctx) >>>>>>> if (avctx->priv_data && avctx->codec && >>>>>>> avctx->codec->priv_class) >>>>>>> av_opt_free(avctx->priv_data); >>>>>>> - av_opt_free(avctx); >>>>>>> av_freep(&avctx->priv_data); >>>>>>> if (av_codec_is_encoder(avctx->codec)) { >>>>>>> av_freep(&avctx->extradata); >>>>>>> diff --git a/libavcodec/options.c b/libavcodec/options.c >>>>>>> index 33f11480a7..91335415c1 100644 >>>>>>> --- a/libavcodec/options.c >>>>>>> +++ b/libavcodec/options.c >>>>>>> @@ -172,7 +172,7 @@ void avcodec_free_context(AVCodecContext >>>>>>> **pavctx) >>>>>>> av_freep(&avctx->intra_matrix); >>>>>>> av_freep(&avctx->inter_matrix); >>>>>>> av_freep(&avctx->rc_override); >>>>>>> - av_channel_layout_uninit(&avctx->ch_layout); >>>>>>> + av_opt_free(avctx); >>>>>>> av_freep(pavctx); >>>>>>> } >>>>>> >>>>>> This will lead to memleaks for users that use >>>>>> avcodec_close(avctx) + >>>>>> av_free(avctx) to free an AVCodecContext (e.g. our frame-threaded >>>>>> encoders do this). Notice that avcodec_free_context() violates the >>>>>> documentation of AVCodecContext.extradata (documented to not be >>>>>> freed >>>>>> for decoders) and AVCodecContext.subtitle_header and >>>>>> AVCodecContext.rc_override (documented to not be freed by lavc for >>>>>> encoders), so there is a reason for using it instead of >>>>>> avcodec_free_context() (even when not reusing the context). >>>>> >>>>> That's an absolute mess of a situation. av_free(avctx) should not >>>>> be an >>>>> allowed or supported scenario when avcodec_free_context() exists. >>>>> And >>>>> why is the latter violating its own documentation? >>>>> >>>> >>>> It is not violating its own documentation, but the documentation >>>> of the >>>> relevant AVCodecContext fields. IIRC Anton wanted a function that >>>> just >>>> frees the whole context, even if this meant that fields which are >>>> documented as being owned by the user are freed. Even documenting the >>>> current state of affairs in avcodec.h doesn't change the fact that >>>> there >>>> is a valid reason to use avcodec_close()+av_free(), so we can't >>>> pretend >>>> it doesn't happen. >>>> >>>> - Andreas >>> >>> Ok, do i add a codecpar copy like i suggested in >>> http://ffmpeg.org/pipermail/ffmpeg-devel/2022-March/294312.html, >>> then? It >>> works, but it feels really weird doing that in what's the cleanup >>> portion >>> of the function. >>> Alternatively, add the dance from >>> https://patchwork.ffmpeg.org/project/ffmpeg/patch/20220319030407.45503-1-jamrial@gmail.com/ >>> >>> which should have the same effect and never fail, unlike param copy. >> >> The latter would also leak memory on avcodec_close()+av_freep(). > > Sorry, I meant the first one. avformat_free_context() calls avcodec_free_context() on the relevant AVCodecContexts. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Marton Balint: > > > On Mon, 21 Mar 2022, Marton Balint wrote: > >> >> >> On Sun, 20 Mar 2022, James Almer wrote: >> >>> On 3/20/2022 8:34 PM, Andreas Rheinhardt wrote: >>>> James Almer: >>>>> >>>>> >>>>> On 3/20/2022 8:26 PM, Andreas Rheinhardt wrote: >>>>>> James Almer: >>>>>>> It can uninitialize fields that may still be used after the >>>>>>> context >>>>>>> was closed, >>>>>>> so do it instead in avcodec_free_context(). >>>>>>> >>>>>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>>>>> --- >>>>>>> libavcodec/avcodec.c | 1 - >>>>>>> libavcodec/options.c | 2 +- >>>>>>> 2 files changed, 1 insertion(+), 2 deletions(-) >>>>>>> >>>>>>> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c >>>>>>> index 38bdaad4fa..122d09b63a 100644 >>>>>>> --- a/libavcodec/avcodec.c >>>>>>> +++ b/libavcodec/avcodec.c >>>>>>> @@ -524,7 +524,6 @@ av_cold int avcodec_close(AVCodecContext >>>>>>> *avctx) >>>>>>> if (avctx->priv_data && avctx->codec && >>>>>>> avctx->codec->priv_class) >>>>>>> av_opt_free(avctx->priv_data); >>>>>>> - av_opt_free(avctx); >>>>>>> av_freep(&avctx->priv_data); >>>>>>> if (av_codec_is_encoder(avctx->codec)) { >>>>>>> av_freep(&avctx->extradata); >>>>>>> diff --git a/libavcodec/options.c b/libavcodec/options.c >>>>>>> index 33f11480a7..91335415c1 100644 >>>>>>> --- a/libavcodec/options.c >>>>>>> +++ b/libavcodec/options.c >>>>>>> @@ -172,7 +172,7 @@ void avcodec_free_context(AVCodecContext >>>>>>> **pavctx) >>>>>>> av_freep(&avctx->intra_matrix); >>>>>>> av_freep(&avctx->inter_matrix); >>>>>>> av_freep(&avctx->rc_override); >>>>>>> - av_channel_layout_uninit(&avctx->ch_layout); >>>>>>> + av_opt_free(avctx); >>>>>>> av_freep(pavctx); >>>>>>> } >>>>>> >>>>>> This will lead to memleaks for users that use >>>>>> avcodec_close(avctx) + >>>>>> av_free(avctx) to free an AVCodecContext (e.g. our frame-threaded >>>>>> encoders do this). Notice that avcodec_free_context() violates the >>>>>> documentation of AVCodecContext.extradata (documented to not be >>>>>> freed >>>>>> for decoders) and AVCodecContext.subtitle_header and >>>>>> AVCodecContext.rc_override (documented to not be freed by lavc for >>>>>> encoders), so there is a reason for using it instead of >>>>>> avcodec_free_context() (even when not reusing the context). >>>>> >>>>> That's an absolute mess of a situation. av_free(avctx) should not >>>>> be an >>>>> allowed or supported scenario when avcodec_free_context() exists. >>>>> And >>>>> why is the latter violating its own documentation? >>>>> >>>> >>>> It is not violating its own documentation, but the documentation >>>> of the >>>> relevant AVCodecContext fields. IIRC Anton wanted a function that >>>> just >>>> frees the whole context, even if this meant that fields which are >>>> documented as being owned by the user are freed. Even documenting the >>>> current state of affairs in avcodec.h doesn't change the fact that >>>> there >>>> is a valid reason to use avcodec_close()+av_free(), so we can't >>>> pretend >>>> it doesn't happen. >>>> >>>> - Andreas >>> >>> Ok, do i add a codecpar copy like i suggested in >>> http://ffmpeg.org/pipermail/ffmpeg-devel/2022-March/294312.html, >>> then? It >>> works, but it feels really weird doing that in what's the cleanup >>> portion >>> of the function. >>> Alternatively, add the dance from >>> https://patchwork.ffmpeg.org/project/ffmpeg/patch/20220319030407.45503-1-jamrial@gmail.com/ >>> >>> which should have the same effect and never fail, unlike param copy. >> >> The latter would also leak memory on avcodec_close()+av_freep(). > > Sorry, I meant the first one. This AVCodecContext is ultimately freed with avcodec_free_context(), so there would be no memleak. - Andreas
On Sun, 20 Mar 2022, James Almer wrote: > On 3/20/2022 9:05 PM, Marton Balint wrote: >> >> >> On Mon, 21 Mar 2022, Marton Balint wrote: >> >>> >>> >>> On Sun, 20 Mar 2022, James Almer wrote: >>> >>>> On 3/20/2022 8:34 PM, Andreas Rheinhardt wrote: >>>>> James Almer: >>>>>> >>>>>> >>>>>> On 3/20/2022 8:26 PM, Andreas Rheinhardt wrote: >>>>>>> James Almer: >>>>>>>> It can uninitialize fields that may still be used after the >>>>>>>> context >>>>>>>> was closed, >>>>>>>> so do it instead in avcodec_free_context(). >>>>>>>> >>>>>>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>>>>>> --- >>>>>>>> libavcodec/avcodec.c | 1 - >>>>>>>> libavcodec/options.c | 2 +- >>>>>>>> 2 files changed, 1 insertion(+), 2 deletions(-) >>>>>>>> >>>>>>>> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c >>>>>>>> index 38bdaad4fa..122d09b63a 100644 >>>>>>>> --- a/libavcodec/avcodec.c >>>>>>>> +++ b/libavcodec/avcodec.c >>>>>>>> @@ -524,7 +524,6 @@ av_cold int avcodec_close(AVCodecContext >>>>>>>> *avctx) >>>>>>>> if (avctx->priv_data && avctx->codec && >>>>>>>> avctx->codec->priv_class) >>>>>>>> av_opt_free(avctx->priv_data); >>>>>>>> - av_opt_free(avctx); >>>>>>>> av_freep(&avctx->priv_data); >>>>>>>> if (av_codec_is_encoder(avctx->codec)) { >>>>>>>> av_freep(&avctx->extradata); >>>>>>>> diff --git a/libavcodec/options.c b/libavcodec/options.c >>>>>>>> index 33f11480a7..91335415c1 100644 >>>>>>>> --- a/libavcodec/options.c >>>>>>>> +++ b/libavcodec/options.c >>>>>>>> @@ -172,7 +172,7 @@ void avcodec_free_context(AVCodecContext >>>>>>>> **pavctx) >>>>>>>> av_freep(&avctx->intra_matrix); >>>>>>>> av_freep(&avctx->inter_matrix); >>>>>>>> av_freep(&avctx->rc_override); >>>>>>>> - av_channel_layout_uninit(&avctx->ch_layout); >>>>>>>> + av_opt_free(avctx); >>>>>>>> av_freep(pavctx); >>>>>>>> } >>>>>>> >>>>>>> This will lead to memleaks for users that use avcodec_close(avctx) >>>>>>> + >>>>>>> av_free(avctx) to free an AVCodecContext (e.g. our frame-threaded >>>>>>> encoders do this). Notice that avcodec_free_context() violates the >>>>>>> documentation of AVCodecContext.extradata (documented to not be >>>>>>> freed >>>>>>> for decoders) and AVCodecContext.subtitle_header and >>>>>>> AVCodecContext.rc_override (documented to not be freed by lavc for >>>>>>> encoders), so there is a reason for using it instead of >>>>>>> avcodec_free_context() (even when not reusing the context). >>>>>> >>>>>> That's an absolute mess of a situation. av_free(avctx) should not be >>>>>> an >>>>>> allowed or supported scenario when avcodec_free_context() exists. >>>>>> And >>>>>> why is the latter violating its own documentation? >>>>>> >>>>> >>>>> It is not violating its own documentation, but the documentation of >>>>> the >>>>> relevant AVCodecContext fields. IIRC Anton wanted a function that >>>>> just >>>>> frees the whole context, even if this meant that fields which are >>>>> documented as being owned by the user are freed. Even documenting the >>>>> current state of affairs in avcodec.h doesn't change the fact that >>>>> there >>>>> is a valid reason to use avcodec_close()+av_free(), so we can't >>>>> pretend >>>>> it doesn't happen. >>>>> >>>>> - Andreas >>>> >>>> Ok, do i add a codecpar copy like i suggested in >>>> http://ffmpeg.org/pipermail/ffmpeg-devel/2022-March/294312.html, then? >>>> It >>>> works, but it feels really weird doing that in what's the cleanup >>>> portion >>>> of the function. >>>> Alternatively, add the dance from >>>> https://patchwork.ffmpeg.org/project/ffmpeg/patch/20220319030407.45503-1-jamrial@gmail.com/ >>>> >>>> which should have the same effect and never fail, unlike param copy. >>> >>> The latter would also leak memory on avcodec_close()+av_freep(). >> >> Sorry, I meant the first one. > > avformat_free_context() calls avcodec_free_context() on the relevant > AVCodecContexts. Sorry, I meant the second case, if you do the dance with AVCodecContext->ch_layout in avcodec_close() then avcodec_close()+av_freep(avctx) will leak ch_layout.map on custom layouts. I hope I finally make sense :) Regards, Marton
On 3/20/2022 9:21 PM, Marton Balint wrote: > > > On Sun, 20 Mar 2022, James Almer wrote: > >> On 3/20/2022 9:05 PM, Marton Balint wrote: >>> >>> >>> On Mon, 21 Mar 2022, Marton Balint wrote: >>> >>>> >>>> >>>> On Sun, 20 Mar 2022, James Almer wrote: >>>> >>>>> On 3/20/2022 8:34 PM, Andreas Rheinhardt wrote: >>>>>> James Almer: >>>>>>> >>>>>>> >>>>>>> On 3/20/2022 8:26 PM, Andreas Rheinhardt wrote: >>>>>>>> James Almer: >>>>>>>>> It can uninitialize fields that may still be used after the >>>>>>>>> context >>>>>>>>> was closed, >>>>>>>>> so do it instead in avcodec_free_context(). >>>>>>>>> >>>>>>>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>>>>>>> --- >>>>>>>>> libavcodec/avcodec.c | 1 - >>>>>>>>> libavcodec/options.c | 2 +- >>>>>>>>> 2 files changed, 1 insertion(+), 2 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c >>>>>>>>> index 38bdaad4fa..122d09b63a 100644 >>>>>>>>> --- a/libavcodec/avcodec.c >>>>>>>>> +++ b/libavcodec/avcodec.c >>>>>>>>> @@ -524,7 +524,6 @@ av_cold int avcodec_close(AVCodecContext >>>>>>>>> *avctx) >>>>>>>>> if (avctx->priv_data && avctx->codec && >>>>>>>>> avctx->codec->priv_class) >>>>>>>>> av_opt_free(avctx->priv_data); >>>>>>>>> - av_opt_free(avctx); >>>>>>>>> av_freep(&avctx->priv_data); >>>>>>>>> if (av_codec_is_encoder(avctx->codec)) { >>>>>>>>> av_freep(&avctx->extradata); >>>>>>>>> diff --git a/libavcodec/options.c b/libavcodec/options.c >>>>>>>>> index 33f11480a7..91335415c1 100644 >>>>>>>>> --- a/libavcodec/options.c >>>>>>>>> +++ b/libavcodec/options.c >>>>>>>>> @@ -172,7 +172,7 @@ void avcodec_free_context(AVCodecContext >>>>>>>>> **pavctx) >>>>>>>>> av_freep(&avctx->intra_matrix); >>>>>>>>> av_freep(&avctx->inter_matrix); >>>>>>>>> av_freep(&avctx->rc_override); >>>>>>>>> - av_channel_layout_uninit(&avctx->ch_layout); >>>>>>>>> + av_opt_free(avctx); >>>>>>>>> av_freep(pavctx); >>>>>>>>> } >>>>>>>> >>>>>>>> This will lead to memleaks for users that use >>>>>>>> avcodec_close(avctx) >>>>>>>> + >>>>>>>> av_free(avctx) to free an AVCodecContext (e.g. our >>>>>>>> frame-threaded >>>>>>>> encoders do this). Notice that avcodec_free_context() >>>>>>>> violates the >>>>>>>> documentation of AVCodecContext.extradata (documented to not be >>>>>>>> freed >>>>>>>> for decoders) and AVCodecContext.subtitle_header and >>>>>>>> AVCodecContext.rc_override (documented to not be freed by >>>>>>>> lavc for >>>>>>>> encoders), so there is a reason for using it instead of >>>>>>>> avcodec_free_context() (even when not reusing the context). >>>>>>> >>>>>>> That's an absolute mess of a situation. av_free(avctx) should >>>>>>> not be >>>>>>> an >>>>>>> allowed or supported scenario when avcodec_free_context() exists. >>>>>>> And >>>>>>> why is the latter violating its own documentation? >>>>>>> >>>>>> >>>>>> It is not violating its own documentation, but the >>>>>> documentation of >>>>>> the >>>>>> relevant AVCodecContext fields. IIRC Anton wanted a function that >>>>>> just >>>>>> frees the whole context, even if this meant that fields which are >>>>>> documented as being owned by the user are freed. Even >>>>>> documenting the >>>>>> current state of affairs in avcodec.h doesn't change the fact that >>>>>> there >>>>>> is a valid reason to use avcodec_close()+av_free(), so we can't >>>>>> pretend >>>>>> it doesn't happen. >>>>>> >>>>>> - Andreas >>>>> >>>>> Ok, do i add a codecpar copy like i suggested in >>>>> http://ffmpeg.org/pipermail/ffmpeg-devel/2022-March/294312.html, >>>>> then? >>>>> It >>>>> works, but it feels really weird doing that in what's the cleanup >>>>> portion >>>>> of the function. >>>>> Alternatively, add the dance from >>>>> https://patchwork.ffmpeg.org/project/ffmpeg/patch/20220319030407.45503-1-jamrial@gmail.com/ >>>>> >>>>> which should have the same effect and never fail, unlike param copy. >>>> >>>> The latter would also leak memory on avcodec_close()+av_freep(). >>> >>> Sorry, I meant the first one. >> >> avformat_free_context() calls avcodec_free_context() on the relevant >> AVCodecContexts. > > Sorry, I meant the second case, if you do the dance with > AVCodecContext->ch_layout in avcodec_close() then > avcodec_close()+av_freep(avctx) will leak ch_layout.map on custom layouts. > > I hope I finally make sense :) Oh, i meant to say doing that dance in lavf, in the same place the first option called param copy, instead of said param copy call. Sorry i wasn't explicit. > > Regards, > Marton > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
James Almer: > On 3/20/2022 8:34 PM, Andreas Rheinhardt wrote: >> James Almer: >>> >>> >>> On 3/20/2022 8:26 PM, Andreas Rheinhardt wrote: >>>> James Almer: >>>>> It can uninitialize fields that may still be used after the context >>>>> was closed, >>>>> so do it instead in avcodec_free_context(). >>>>> >>>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>>> --- >>>>> libavcodec/avcodec.c | 1 - >>>>> libavcodec/options.c | 2 +- >>>>> 2 files changed, 1 insertion(+), 2 deletions(-) >>>>> >>>>> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c >>>>> index 38bdaad4fa..122d09b63a 100644 >>>>> --- a/libavcodec/avcodec.c >>>>> +++ b/libavcodec/avcodec.c >>>>> @@ -524,7 +524,6 @@ av_cold int avcodec_close(AVCodecContext *avctx) >>>>> if (avctx->priv_data && avctx->codec && >>>>> avctx->codec->priv_class) >>>>> av_opt_free(avctx->priv_data); >>>>> - av_opt_free(avctx); >>>>> av_freep(&avctx->priv_data); >>>>> if (av_codec_is_encoder(avctx->codec)) { >>>>> av_freep(&avctx->extradata); >>>>> diff --git a/libavcodec/options.c b/libavcodec/options.c >>>>> index 33f11480a7..91335415c1 100644 >>>>> --- a/libavcodec/options.c >>>>> +++ b/libavcodec/options.c >>>>> @@ -172,7 +172,7 @@ void avcodec_free_context(AVCodecContext **pavctx) >>>>> av_freep(&avctx->intra_matrix); >>>>> av_freep(&avctx->inter_matrix); >>>>> av_freep(&avctx->rc_override); >>>>> - av_channel_layout_uninit(&avctx->ch_layout); >>>>> + av_opt_free(avctx); >>>>> av_freep(pavctx); >>>>> } >>>> >>>> This will lead to memleaks for users that use avcodec_close(avctx) + >>>> av_free(avctx) to free an AVCodecContext (e.g. our frame-threaded >>>> encoders do this). Notice that avcodec_free_context() violates the >>>> documentation of AVCodecContext.extradata (documented to not be freed >>>> for decoders) and AVCodecContext.subtitle_header and >>>> AVCodecContext.rc_override (documented to not be freed by lavc for >>>> encoders), so there is a reason for using it instead of >>>> avcodec_free_context() (even when not reusing the context). >>> >>> That's an absolute mess of a situation. av_free(avctx) should not be an >>> allowed or supported scenario when avcodec_free_context() exists. And >>> why is the latter violating its own documentation? >>> >> >> It is not violating its own documentation, but the documentation of the >> relevant AVCodecContext fields. IIRC Anton wanted a function that just >> frees the whole context, even if this meant that fields which are >> documented as being owned by the user are freed. Even documenting the >> current state of affairs in avcodec.h doesn't change the fact that there >> is a valid reason to use avcodec_close()+av_free(), so we can't pretend >> it doesn't happen. >> >> - Andreas > > Ok, do i add a codecpar copy like i suggested in > http://ffmpeg.org/pipermail/ffmpeg-devel/2022-March/294312.html, then? > It works, but it feels really weird doing that in what's the cleanup > portion of the function. > Alternatively, add the dance from > https://patchwork.ffmpeg.org/project/ffmpeg/patch/20220319030407.45503-1-jamrial@gmail.com/ > which should have the same effect and never fail, unlike param copy. The other place where the internal context is reinitialized (in read_frame_internal()) also calls avcodec_parameters_to_context(); yet this code is reached when the demuxer updates the AVCodecParameters and sets need_context_update accordingly whereas the other call to avcodec_close() happens when no such updates were triggered. So I can live with both. (Is it actually intended for AVChannelLayout to be movable? If so, it should be documented.) - Andreas
On 3/20/2022 9:46 PM, Andreas Rheinhardt wrote: > James Almer: >> On 3/20/2022 8:34 PM, Andreas Rheinhardt wrote: >>> James Almer: >>>> >>>> >>>> On 3/20/2022 8:26 PM, Andreas Rheinhardt wrote: >>>>> James Almer: >>>>>> It can uninitialize fields that may still be used after the context >>>>>> was closed, >>>>>> so do it instead in avcodec_free_context(). >>>>>> >>>>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>>>> --- >>>>>> libavcodec/avcodec.c | 1 - >>>>>> libavcodec/options.c | 2 +- >>>>>> 2 files changed, 1 insertion(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c >>>>>> index 38bdaad4fa..122d09b63a 100644 >>>>>> --- a/libavcodec/avcodec.c >>>>>> +++ b/libavcodec/avcodec.c >>>>>> @@ -524,7 +524,6 @@ av_cold int avcodec_close(AVCodecContext *avctx) >>>>>> if (avctx->priv_data && avctx->codec && >>>>>> avctx->codec->priv_class) >>>>>> av_opt_free(avctx->priv_data); >>>>>> - av_opt_free(avctx); >>>>>> av_freep(&avctx->priv_data); >>>>>> if (av_codec_is_encoder(avctx->codec)) { >>>>>> av_freep(&avctx->extradata); >>>>>> diff --git a/libavcodec/options.c b/libavcodec/options.c >>>>>> index 33f11480a7..91335415c1 100644 >>>>>> --- a/libavcodec/options.c >>>>>> +++ b/libavcodec/options.c >>>>>> @@ -172,7 +172,7 @@ void avcodec_free_context(AVCodecContext **pavctx) >>>>>> av_freep(&avctx->intra_matrix); >>>>>> av_freep(&avctx->inter_matrix); >>>>>> av_freep(&avctx->rc_override); >>>>>> - av_channel_layout_uninit(&avctx->ch_layout); >>>>>> + av_opt_free(avctx); >>>>>> av_freep(pavctx); >>>>>> } >>>>> >>>>> This will lead to memleaks for users that use avcodec_close(avctx) + >>>>> av_free(avctx) to free an AVCodecContext (e.g. our frame-threaded >>>>> encoders do this). Notice that avcodec_free_context() violates the >>>>> documentation of AVCodecContext.extradata (documented to not be freed >>>>> for decoders) and AVCodecContext.subtitle_header and >>>>> AVCodecContext.rc_override (documented to not be freed by lavc for >>>>> encoders), so there is a reason for using it instead of >>>>> avcodec_free_context() (even when not reusing the context). >>>> >>>> That's an absolute mess of a situation. av_free(avctx) should not be an >>>> allowed or supported scenario when avcodec_free_context() exists. And >>>> why is the latter violating its own documentation? >>>> >>> >>> It is not violating its own documentation, but the documentation of the >>> relevant AVCodecContext fields. IIRC Anton wanted a function that just >>> frees the whole context, even if this meant that fields which are >>> documented as being owned by the user are freed. Even documenting the >>> current state of affairs in avcodec.h doesn't change the fact that there >>> is a valid reason to use avcodec_close()+av_free(), so we can't pretend >>> it doesn't happen. >>> >>> - Andreas >> >> Ok, do i add a codecpar copy like i suggested in >> http://ffmpeg.org/pipermail/ffmpeg-devel/2022-March/294312.html, then? >> It works, but it feels really weird doing that in what's the cleanup >> portion of the function. >> Alternatively, add the dance from >> https://patchwork.ffmpeg.org/project/ffmpeg/patch/20220319030407.45503-1-jamrial@gmail.com/ >> which should have the same effect and never fail, unlike param copy. > > The other place where the internal context is reinitialized (in > read_frame_internal()) also calls avcodec_parameters_to_context(); yet > this code is reached when the demuxer updates the AVCodecParameters and > sets need_context_update accordingly whereas the other call to > avcodec_close() happens when no such updates were triggered. So I can > live with both. I'll push the codecpar copy one since it's cleaner looking, then, and add a comment about why it's there so we can remove it once avcodec_close() stops freeing ch_layout, if it happens before the former is removed. > (Is it actually intended for AVChannelLayout to be movable? If so, it > should be documented.) > > - Andreas > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c index 38bdaad4fa..122d09b63a 100644 --- a/libavcodec/avcodec.c +++ b/libavcodec/avcodec.c @@ -524,7 +524,6 @@ av_cold int avcodec_close(AVCodecContext *avctx) if (avctx->priv_data && avctx->codec && avctx->codec->priv_class) av_opt_free(avctx->priv_data); - av_opt_free(avctx); av_freep(&avctx->priv_data); if (av_codec_is_encoder(avctx->codec)) { av_freep(&avctx->extradata); diff --git a/libavcodec/options.c b/libavcodec/options.c index 33f11480a7..91335415c1 100644 --- a/libavcodec/options.c +++ b/libavcodec/options.c @@ -172,7 +172,7 @@ void avcodec_free_context(AVCodecContext **pavctx) av_freep(&avctx->intra_matrix); av_freep(&avctx->inter_matrix); av_freep(&avctx->rc_override); - av_channel_layout_uninit(&avctx->ch_layout); + av_opt_free(avctx); av_freep(pavctx); }
It can uninitialize fields that may still be used after the context was closed, so do it instead in avcodec_free_context(). Signed-off-by: James Almer <jamrial@gmail.com> --- libavcodec/avcodec.c | 1 - libavcodec/options.c | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-)