[FFmpeg-devel] [PATCH] avcodec/avcodec: don't free AVOption settable fields in avcodec_close()

[FFmpeg-devel] [PATCH] avcodec/avcodec: don't free AVOption settable fields in avcodec_close()

[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);
 }
-- 
2.35.1

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avcodec/avcodec: don't free AVOption settable fields in avcodec_close()

[Andreas Rheinhardt]

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
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avcodec/avcodec: don't free AVOption settable fields in avcodec_close()

[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?

> 
> - 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".
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avcodec/avcodec: don't free AVOption settable fields in avcodec_close()

[Andreas Rheinhardt]

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
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avcodec/avcodec: don't free AVOption settable fields in avcodec_close()

[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.
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avcodec/avcodec: don't free AVOption settable fields in avcodec_close()

[Marton Balint]

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
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avcodec/avcodec: don't free AVOption settable fields in avcodec_close()

[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.
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avcodec/avcodec: don't free AVOption settable fields in avcodec_close()

[James Almer]

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".
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avcodec/avcodec: don't free AVOption settable fields in avcodec_close()

[Andreas Rheinhardt]

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
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avcodec/avcodec: don't free AVOption settable fields in avcodec_close()

[Marton Balint]

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
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avcodec/avcodec: don't free AVOption settable fields in avcodec_close()

[James Almer]

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".
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avcodec/avcodec: don't free AVOption settable fields in avcodec_close()

[Andreas Rheinhardt]

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
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avcodec/avcodec: don't free AVOption settable fields in avcodec_close()

[James Almer]

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".
_______________________________________________
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".