[FFmpeg-devel] [PATCH 1/3] avcodec/avcodec: prevent ch_layout from being uninitialized in ff_codec_close()

[FFmpeg-devel] [PATCH 1/3] avcodec/avcodec: prevent ch_layout from being uninitialized in ff_codec_close()

[James Almer]

It's a user-set parameter shared with AVCodecParameters, so it should only
be freed by avcodec_free_context().

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/avcodec.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
index 888dd76228..fc8a40e4db 100644
--- a/libavcodec/avcodec.c
+++ b/libavcodec/avcodec.c
@@ -414,6 +414,7 @@ void avsubtitle_free(AVSubtitle *sub)
 
 av_cold void ff_codec_close(AVCodecContext *avctx)
 {
+    AVChannelLayout ch_layout;
     int i;
 
     if (!avctx)
@@ -468,7 +469,13 @@ av_cold void ff_codec_close(AVCodecContext *avctx)
 
     if (avctx->priv_data && avctx->codec && avctx->codec->priv_class)
         av_opt_free(avctx->priv_data);
+
+    // Work around av_opt_free() unsetting ch_layout
+    ch_layout = avctx->ch_layout;
+    memset(&avctx->ch_layout, 0, sizeof(avctx->ch_layout));
     av_opt_free(avctx);
+    avctx->ch_layout = ch_layout;
+
     av_freep(&avctx->priv_data);
     if (av_codec_is_encoder(avctx->codec)) {
         av_freep(&avctx->extradata);
-- 
2.44.0

_______________________________________________
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] [PATCH 2/3] avcodec/avcodec: don't free coded_side_data in ff_codec_close() when decoding

[James Almer]

It's a user-set parameter shared with AVCodecParameters, so it should only
be freed by avcodec_free_context().

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/avcodec.c | 11 ++++++-----
 libavcodec/options.c |  4 ++++
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
index fc8a40e4db..e560efff6a 100644
--- a/libavcodec/avcodec.c
+++ b/libavcodec/avcodec.c
@@ -458,11 +458,12 @@ av_cold void ff_codec_close(AVCodecContext *avctx)
 
         av_freep(&avctx->internal);
     }
-
-    for (i = 0; i < avctx->nb_coded_side_data; i++)
-        av_freep(&avctx->coded_side_data[i].data);
-    av_freep(&avctx->coded_side_data);
-    avctx->nb_coded_side_data = 0;
+    if (av_codec_is_encoder(avctx->codec)) {
+        for (i = 0; i < avctx->nb_coded_side_data; i++)
+            av_freep(&avctx->coded_side_data[i].data);
+        av_freep(&avctx->coded_side_data);
+        avctx->nb_coded_side_data = 0;
+    }
 
     av_buffer_unref(&avctx->hw_frames_ctx);
     av_buffer_unref(&avctx->hw_device_ctx);
diff --git a/libavcodec/options.c b/libavcodec/options.c
index 0c3b40a186..7c32a71275 100644
--- a/libavcodec/options.c
+++ b/libavcodec/options.c
@@ -177,6 +177,10 @@ void avcodec_free_context(AVCodecContext **pavctx)
     av_freep(&avctx->inter_matrix);
     av_freep(&avctx->rc_override);
     av_channel_layout_uninit(&avctx->ch_layout);
+    for (int i = 0; i < avctx->nb_coded_side_data; i++)
+        av_freep(&avctx->coded_side_data[i].data);
+    av_freep(&avctx->coded_side_data);
+    avctx->nb_coded_side_data = 0;
     av_frame_side_data_free(
         &avctx->decoded_side_data, &avctx->nb_decoded_side_data);
 
-- 
2.44.0

_______________________________________________
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 2/3] avcodec/avcodec: don't free coded_side_data in ff_codec_close() when decoding

[Andreas Rheinhardt]

James Almer:
> It's a user-set parameter shared with AVCodecParameters, so it should only
> be freed by avcodec_free_context().
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavcodec/avcodec.c | 11 ++++++-----
>  libavcodec/options.c |  4 ++++
>  2 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
> index fc8a40e4db..e560efff6a 100644
> --- a/libavcodec/avcodec.c
> +++ b/libavcodec/avcodec.c
> @@ -458,11 +458,12 @@ av_cold void ff_codec_close(AVCodecContext *avctx)
>  
>          av_freep(&avctx->internal);
>      }
> -
> -    for (i = 0; i < avctx->nb_coded_side_data; i++)
> -        av_freep(&avctx->coded_side_data[i].data);
> -    av_freep(&avctx->coded_side_data);
> -    avctx->nb_coded_side_data = 0;
> +    if (av_codec_is_encoder(avctx->codec)) {
> +        for (i = 0; i < avctx->nb_coded_side_data; i++)
> +            av_freep(&avctx->coded_side_data[i].data);
> +        av_freep(&avctx->coded_side_data);
> +        avctx->nb_coded_side_data = 0;
> +    }
>  
>      av_buffer_unref(&avctx->hw_frames_ctx);
>      av_buffer_unref(&avctx->hw_device_ctx);
> diff --git a/libavcodec/options.c b/libavcodec/options.c
> index 0c3b40a186..7c32a71275 100644
> --- a/libavcodec/options.c
> +++ b/libavcodec/options.c
> @@ -177,6 +177,10 @@ void avcodec_free_context(AVCodecContext **pavctx)
>      av_freep(&avctx->inter_matrix);
>      av_freep(&avctx->rc_override);
>      av_channel_layout_uninit(&avctx->ch_layout);
> +    for (int i = 0; i < avctx->nb_coded_side_data; i++)
> +        av_freep(&avctx->coded_side_data[i].data);
> +    av_freep(&avctx->coded_side_data);
> +    avctx->nb_coded_side_data = 0;
>      av_frame_side_data_free(
>          &avctx->decoded_side_data, &avctx->nb_decoded_side_data);
>  

1. ff_codec_close() already has an "if is_encoder" branch, it does not
need another one.
2. The code in ff_codec_close() will be redundant as soon as
FF_API_AVCODEC_CLOSE is no more, so it should be inside
FF_API_AVCODEC_CLOSE.
3. The documentation of this field does not mention ownership, but given
that this field existed for a long time lavc's previous behaviour
established the implicit contract that lavc will free this in
avcodec_close(). You are breaking this implicit contract for no good
reason apart from this new principle that user-set stuff shared with
AVCodecParameters should not be freed in avcodec_close(). Which is crazy
given that the relevant AVCodecParameters field has only been added
recently.

- Andreas

_______________________________________________
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 2/3] avcodec/avcodec: don't free coded_side_data in ff_codec_close() when decoding

[James Almer]

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

Yes, I wrote this set forgetting avcodec_close() was still a thing.

> reason apart from this new principle that user-set stuff shared with
> AVCodecParameters should not be freed in avcodec_close(). Which is crazy
> given that the relevant AVCodecParameters field has only been added
> recently.


_______________________________________________
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] [PATCH 3/3] avcodec/avcodec: free decoded_side_data in ff_codec_close() when decoding

[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);
-- 
2.44.0

_______________________________________________
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 3/3] avcodec/avcodec: free decoded_side_data in ff_codec_close() when decoding

[Andreas Rheinhardt]

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

_______________________________________________
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 3/3] avcodec/avcodec: free decoded_side_data in ff_codec_close() when decoding

[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?
_______________________________________________
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 3/3] avcodec/avcodec: free decoded_side_data in ff_codec_close() when decoding

[Andreas Rheinhardt]

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

_______________________________________________
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 1/3] avcodec/avcodec: prevent ch_layout from being uninitialized in ff_codec_close()

[Andreas Rheinhardt]

James Almer:
> It's a user-set parameter shared with AVCodecParameters, so it should only
> be freed by avcodec_free_context().
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavcodec/avcodec.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
> index 888dd76228..fc8a40e4db 100644
> --- a/libavcodec/avcodec.c
> +++ b/libavcodec/avcodec.c
> @@ -414,6 +414,7 @@ void avsubtitle_free(AVSubtitle *sub)
>  
>  av_cold void ff_codec_close(AVCodecContext *avctx)
>  {
> +    AVChannelLayout ch_layout;
>      int i;
>  
>      if (!avctx)
> @@ -468,7 +469,13 @@ av_cold void ff_codec_close(AVCodecContext *avctx)
>  
>      if (avctx->priv_data && avctx->codec && avctx->codec->priv_class)
>          av_opt_free(avctx->priv_data);
> +
> +    // Work around av_opt_free() unsetting ch_layout
> +    ch_layout = avctx->ch_layout;
> +    memset(&avctx->ch_layout, 0, sizeof(avctx->ch_layout));
>      av_opt_free(avctx);
> +    avctx->ch_layout = ch_layout;
> +
>      av_freep(&avctx->priv_data);
>      if (av_codec_is_encoder(avctx->codec)) {
>          av_freep(&avctx->extradata);

This and the other patches will cause memleaks for users that use
allocated channel layouts and avcodec_close()+av_free() (this is
deprecated, not forbidden).

Furthermore, where does the rule "user-set parameters shared with
AVCodecParameters should only be freed by avcodec_free_context()" come
from? It is news to me.

- 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 1/3] avcodec/avcodec: prevent ch_layout from being uninitialized in ff_codec_close()

[James Almer]

On 5/1/2024 5:26 PM, Andreas Rheinhardt wrote:
> James Almer:
>> It's a user-set parameter shared with AVCodecParameters, so it should only
>> be freed by avcodec_free_context().
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>   libavcodec/avcodec.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
>> index 888dd76228..fc8a40e4db 100644
>> --- a/libavcodec/avcodec.c
>> +++ b/libavcodec/avcodec.c
>> @@ -414,6 +414,7 @@ void avsubtitle_free(AVSubtitle *sub)
>>   
>>   av_cold void ff_codec_close(AVCodecContext *avctx)
>>   {
>> +    AVChannelLayout ch_layout;
>>       int i;
>>   
>>       if (!avctx)
>> @@ -468,7 +469,13 @@ av_cold void ff_codec_close(AVCodecContext *avctx)
>>   
>>       if (avctx->priv_data && avctx->codec && avctx->codec->priv_class)
>>           av_opt_free(avctx->priv_data);
>> +
>> +    // Work around av_opt_free() unsetting ch_layout
>> +    ch_layout = avctx->ch_layout;
>> +    memset(&avctx->ch_layout, 0, sizeof(avctx->ch_layout));
>>       av_opt_free(avctx);
>> +    avctx->ch_layout = ch_layout;
>> +
>>       av_freep(&avctx->priv_data);
>>       if (av_codec_is_encoder(avctx->codec)) {
>>           av_freep(&avctx->extradata);
> 
> This and the other patches will cause memleaks for users that use
> allocated channel layouts and avcodec_close()+av_free() (this is
> deprecated, not forbidden).

That's awful, but guess it needs to be supported until avcodec_close() 
is gone, so I'm withdrawing this patch.

> 
> Furthermore, where does the rule "user-set parameters shared with
> AVCodecParameters should only be freed by avcodec_free_context()" come
> from? It is news to me.

It's not a rule, it's the ideal/expected behavior seeing the crash 
Michael found, where the only shared field cleared during 
avcodec_close() was ch_layout because it may contain allocated data and 
can be set through an AVOption.
If you're copying params between codecpar and avctx, the latter should 
not have only one of the relevant fields nuked on an internal failure.
_______________________________________________
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".