[FFmpeg-devel] [PATCH] avcodec/avcodec: don't uninitialize ch_layout in avcodec_close()

[FFmpeg-devel] [PATCH] avcodec/avcodec: don't uninitialize ch_layout in avcodec_close()

[James Almer]

The function is not meant to clear codec parameters, and the lavf demux code
relies on this behavior.
Regression since 327efa66331ebdc0087c6b656059a8df2f404019.

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

diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
index 38bdaad4fa..253c9f56cc 100644
--- a/libavcodec/avcodec.c
+++ b/libavcodec/avcodec.c
@@ -469,6 +469,7 @@ void avsubtitle_free(AVSubtitle *sub)
 
 av_cold int avcodec_close(AVCodecContext *avctx)
 {
+    AVChannelLayout ch_layout;
     int i;
 
     if (!avctx)
@@ -524,7 +525,12 @@ 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() will uninitialize avctx->ch_layout, but we want to keep it.
+       It will be uninitialized in avcodec_free_context() */
+    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.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 uninitialize ch_layout in avcodec_close()

[Hendrik Leppkes]

On Sat, Mar 19, 2022 at 4:04 AM James Almer <jamrial@gmail.com> wrote:
>
> The function is not meant to clear codec parameters, and the lavf demux code
> relies on this behavior.
> Regression since 327efa66331ebdc0087c6b656059a8df2f404019.
>
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavcodec/avcodec.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
> index 38bdaad4fa..253c9f56cc 100644
> --- a/libavcodec/avcodec.c
> +++ b/libavcodec/avcodec.c
> @@ -469,6 +469,7 @@ void avsubtitle_free(AVSubtitle *sub)
>
>  av_cold int avcodec_close(AVCodecContext *avctx)
>  {
> +    AVChannelLayout ch_layout;
>      int i;
>
>      if (!avctx)
> @@ -524,7 +525,12 @@ 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() will uninitialize avctx->ch_layout, but we want to keep it.
> +       It will be uninitialized in avcodec_free_context() */
> +    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 feels pretty ugly and still a bit risky that any call to
av_opt_free could invalidate data its not supposed to. Maybe we should
have a flag for AVOptions instead where av_opt_free won't touch an
entry, because its only there to set/get it, not manage its memory?

- Hendrik
_______________________________________________
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 uninitialize ch_layout in avcodec_close()

[Anton Khirnov]

Quoting James Almer (2022-03-19 04:04:07)
> The function is not meant to clear codec parameters, and the lavf demux code
> relies on this behavior.

Maybe it shouldn't?

Which code is it exactly?

-- 
Anton Khirnov
_______________________________________________
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 uninitialize ch_layout in avcodec_close()

[James Almer]

On 3/19/2022 10:47 AM, Anton Khirnov wrote:
> Quoting James Almer (2022-03-19 04:04:07)
>> The function is not meant to clear codec parameters, and the lavf demux code
>> relies on this behavior.
> 
> Maybe it shouldn't?
> 
> Which code is it exactly?

The parser included by demux.c. That file calls avcodec_close() in 
certain situations, but the avctx is still used when calling 
av_parser_parse2() as it's expected that all the parameters are left 
intact, and all were until 327efa6633, where avctx->ch_layout started 
being uninitialized, and it shouldn't.
_______________________________________________
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 uninitialize ch_layout in avcodec_close()

[James Almer]

On 3/19/2022 4:50 AM, Hendrik Leppkes wrote:
> On Sat, Mar 19, 2022 at 4:04 AM James Almer <jamrial@gmail.com> wrote:
>>
>> The function is not meant to clear codec parameters, and the lavf demux code
>> relies on this behavior.
>> Regression since 327efa66331ebdc0087c6b656059a8df2f404019.
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>   libavcodec/avcodec.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
>> index 38bdaad4fa..253c9f56cc 100644
>> --- a/libavcodec/avcodec.c
>> +++ b/libavcodec/avcodec.c
>> @@ -469,6 +469,7 @@ void avsubtitle_free(AVSubtitle *sub)
>>
>>   av_cold int avcodec_close(AVCodecContext *avctx)
>>   {
>> +    AVChannelLayout ch_layout;
>>       int i;
>>
>>       if (!avctx)
>> @@ -524,7 +525,12 @@ 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() will uninitialize avctx->ch_layout, but we want to keep it.
>> +       It will be uninitialized in avcodec_free_context() */
>> +    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 feels pretty ugly and still a bit risky that any call to
> av_opt_free could invalidate data its not supposed to. Maybe we should
> have a flag for AVOptions instead where av_opt_free won't touch an
> entry, because its only there to set/get it, not manage its memory?

Where would that flag be set? av_opt_free() takes none. And that 
function exists purely to free strings, dictionaries, and now 
uninitialize AVChannelLayout elements in a struct. If you don't want to 
free what av_opt_set() allocated, you shouldn't call av_opt_free() at all.

> 
> - Hendrik
> _______________________________________________
> 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 uninitialize ch_layout in avcodec_close()

[James Almer]

On 3/20/2022 2:03 PM, James Almer wrote:
> On 3/19/2022 10:47 AM, Anton Khirnov wrote:
>> Quoting James Almer (2022-03-19 04:04:07)
>>> The function is not meant to clear codec parameters, and the lavf 
>>> demux code
>>> relies on this behavior.
>>
>> Maybe it shouldn't?
>>
>> Which code is it exactly?
> 
> The parser included by demux.c. That file calls avcodec_close() in 
> certain situations, but the avctx is still used when calling 
> av_parser_parse2() as it's expected that all the parameters are left 
> intact, and all were until 327efa6633, where avctx->ch_layout started 
> being uninitialized, and it shouldn't.

An alternative is

> diff --git a/libavformat/demux.c b/libavformat/demux.c
> index c1c9422ac0..c14e44cb07 100644
> --- a/libavformat/demux.c
> +++ b/libavformat/demux.c
> @@ -2957,6 +2957,7 @@ find_stream_info_err:
>              av_freep(&sti->info);
>          }
>          avcodec_close(sti->avctx);
> +        avcodec_parameters_to_context(sti->avctx, st->codecpar);
>          av_bsf_free(&sti->extract_extradata.bsf);
>      }
>      if (ic->pb) {

Which works around this specific issue, but either way, 
avcodec_close(avctx) should not nuke avctx->ch_layout.
_______________________________________________
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 uninitialize ch_layout in avcodec_close()

[Hendrik Leppkes]

On Sun, Mar 20, 2022 at 7:52 PM James Almer <jamrial@gmail.com> wrote:
>
> On 3/19/2022 4:50 AM, Hendrik Leppkes wrote:
> > On Sat, Mar 19, 2022 at 4:04 AM James Almer <jamrial@gmail.com> wrote:
> >>
> >> The function is not meant to clear codec parameters, and the lavf demux code
> >> relies on this behavior.
> >> Regression since 327efa66331ebdc0087c6b656059a8df2f404019.
> >>
> >> Signed-off-by: James Almer <jamrial@gmail.com>
> >> ---
> >>   libavcodec/avcodec.c | 6 ++++++
> >>   1 file changed, 6 insertions(+)
> >>
> >> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
> >> index 38bdaad4fa..253c9f56cc 100644
> >> --- a/libavcodec/avcodec.c
> >> +++ b/libavcodec/avcodec.c
> >> @@ -469,6 +469,7 @@ void avsubtitle_free(AVSubtitle *sub)
> >>
> >>   av_cold int avcodec_close(AVCodecContext *avctx)
> >>   {
> >> +    AVChannelLayout ch_layout;
> >>       int i;
> >>
> >>       if (!avctx)
> >> @@ -524,7 +525,12 @@ 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() will uninitialize avctx->ch_layout, but we want to keep it.
> >> +       It will be uninitialized in avcodec_free_context() */
> >> +    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 feels pretty ugly and still a bit risky that any call to
> > av_opt_free could invalidate data its not supposed to. Maybe we should
> > have a flag for AVOptions instead where av_opt_free won't touch an
> > entry, because its only there to set/get it, not manage its memory?
>
> Where would that flag be set? av_opt_free() takes none. And that
> function exists purely to free strings, dictionaries, and now
> uninitialize AVChannelLayout elements in a struct. If you don't want to
> free what av_opt_set() allocated, you shouldn't call av_opt_free() at all.
>

On the AVOption element in the table, with the other AV_OPT_FLAG_*
_______________________________________________
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 uninitialize ch_layout in avcodec_close()

[James Almer]

On 3/20/2022 7:01 PM, Hendrik Leppkes wrote:
> On Sun, Mar 20, 2022 at 7:52 PM James Almer <jamrial@gmail.com> wrote:
>>
>> On 3/19/2022 4:50 AM, Hendrik Leppkes wrote:
>>> On Sat, Mar 19, 2022 at 4:04 AM James Almer <jamrial@gmail.com> wrote:
>>>>
>>>> The function is not meant to clear codec parameters, and the lavf demux code
>>>> relies on this behavior.
>>>> Regression since 327efa66331ebdc0087c6b656059a8df2f404019.
>>>>
>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>> ---
>>>>    libavcodec/avcodec.c | 6 ++++++
>>>>    1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
>>>> index 38bdaad4fa..253c9f56cc 100644
>>>> --- a/libavcodec/avcodec.c
>>>> +++ b/libavcodec/avcodec.c
>>>> @@ -469,6 +469,7 @@ void avsubtitle_free(AVSubtitle *sub)
>>>>
>>>>    av_cold int avcodec_close(AVCodecContext *avctx)
>>>>    {
>>>> +    AVChannelLayout ch_layout;
>>>>        int i;
>>>>
>>>>        if (!avctx)
>>>> @@ -524,7 +525,12 @@ 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() will uninitialize avctx->ch_layout, but we want to keep it.
>>>> +       It will be uninitialized in avcodec_free_context() */
>>>> +    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 feels pretty ugly and still a bit risky that any call to
>>> av_opt_free could invalidate data its not supposed to. Maybe we should
>>> have a flag for AVOptions instead where av_opt_free won't touch an
>>> entry, because its only there to set/get it, not manage its memory?
>>
>> Where would that flag be set? av_opt_free() takes none. And that
>> function exists purely to free strings, dictionaries, and now
>> uninitialize AVChannelLayout elements in a struct. If you don't want to
>> free what av_opt_set() allocated, you shouldn't call av_opt_free() at all.
>>
> 
> On the AVOption element in the table, with the other AV_OPT_FLAG_*

Oh, you meant flagging the actual AVOption. My bad, i for some reason 
thought you meant having the user flag which options they wanted to "own".

I'll try to implement this. Any suggestion for the flag name? 
AV_OPT_FLAG_NO_FREE?

> _______________________________________________
> 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 uninitialize ch_layout in avcodec_close()

[Marton Balint]

On Sun, 20 Mar 2022, James Almer wrote:

>
>
> On 3/20/2022 7:01 PM, Hendrik Leppkes wrote:
>>  On Sun, Mar 20, 2022 at 7:52 PM James Almer <jamrial@gmail.com> wrote:
>>>
>>>  On 3/19/2022 4:50 AM, Hendrik Leppkes wrote:
>>>>  On Sat, Mar 19, 2022 at 4:04 AM James Almer <jamrial@gmail.com> wrote:
>>>>>
>>>>>  The function is not meant to clear codec parameters, and the lavf demux
>>>>>  code
>>>>>  relies on this behavior.
>>>>>  Regression since 327efa66331ebdc0087c6b656059a8df2f404019.
>>>>>
>>>>>  Signed-off-by: James Almer <jamrial@gmail.com>
>>>>>  ---
>>>>>     libavcodec/avcodec.c | 6 ++++++
>>>>>     1 file changed, 6 insertions(+)
>>>>>
>>>>>  diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
>>>>>  index 38bdaad4fa..253c9f56cc 100644
>>>>>  --- a/libavcodec/avcodec.c
>>>>>  +++ b/libavcodec/avcodec.c
>>>>>  @@ -469,6 +469,7 @@ void avsubtitle_free(AVSubtitle *sub)
>>>>>
>>>>>     av_cold int avcodec_close(AVCodecContext *avctx)
>>>>>     {
>>>>>  +    AVChannelLayout ch_layout;
>>>>>         int i;
>>>>>
>>>>>         if (!avctx)
>>>>>  @@ -524,7 +525,12 @@ 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() will uninitialize avctx->ch_layout, but we want
>>>>>  to keep it.
>>>>>  +       It will be uninitialized in avcodec_free_context() */
>>>>>  +    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 feels pretty ugly and still a bit risky that any call to
>>>>  av_opt_free could invalidate data its not supposed to. Maybe we should
>>>>  have a flag for AVOptions instead where av_opt_free won't touch an
>>>>  entry, because its only there to set/get it, not manage its memory?
>>>
>>>  Where would that flag be set? av_opt_free() takes none. And that
>>>  function exists purely to free strings, dictionaries, and now
>>>  uninitialize AVChannelLayout elements in a struct. If you don't want to
>>>  free what av_opt_set() allocated, you shouldn't call av_opt_free() at
>>>  all.
>>>
>>
>>  On the AVOption element in the table, with the other AV_OPT_FLAG_*
>
> Oh, you meant flagging the actual AVOption. My bad, i for some reason thought 
> you meant having the user flag which options they wanted to "own".
>
> I'll try to implement this. Any suggestion for the flag name? 
> AV_OPT_FLAG_NO_FREE?

This also looks hackish to me. Isn't it a lot simpler to remove 
av_opt_free from avcodec_close()? It is deprecated for freeing data of an 
avcodec context since 2014. Actually maybe avcodec_close() should be 
deprecated as well.

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 uninitialize ch_layout in avcodec_close()

[James Almer]

On 3/20/2022 7:34 PM, Marton Balint wrote:
> 
> 
> On Sun, 20 Mar 2022, James Almer wrote:
> 
>>
>>
>> On 3/20/2022 7:01 PM, Hendrik Leppkes wrote:
>>>  On Sun, Mar 20, 2022 at 7:52 PM James Almer <jamrial@gmail.com> wrote:
>>>>
>>>>  On 3/19/2022 4:50 AM, Hendrik Leppkes wrote:
>>>>>  On Sat, Mar 19, 2022 at 4:04 AM James Almer <jamrial@gmail.com> 
>>>>> wrote:
>>>>>>
>>>>>>  The function is not meant to clear codec parameters, and the lavf 
>>>>>> demux
>>>>>>  code
>>>>>>  relies on this behavior.
>>>>>>  Regression since 327efa66331ebdc0087c6b656059a8df2f404019.
>>>>>>
>>>>>>  Signed-off-by: James Almer <jamrial@gmail.com>
>>>>>>  ---
>>>>>>     libavcodec/avcodec.c | 6 ++++++
>>>>>>     1 file changed, 6 insertions(+)
>>>>>>
>>>>>>  diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
>>>>>>  index 38bdaad4fa..253c9f56cc 100644
>>>>>>  --- a/libavcodec/avcodec.c
>>>>>>  +++ b/libavcodec/avcodec.c
>>>>>>  @@ -469,6 +469,7 @@ void avsubtitle_free(AVSubtitle *sub)
>>>>>>
>>>>>>     av_cold int avcodec_close(AVCodecContext *avctx)
>>>>>>     {
>>>>>>  +    AVChannelLayout ch_layout;
>>>>>>         int i;
>>>>>>
>>>>>>         if (!avctx)
>>>>>>  @@ -524,7 +525,12 @@ 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() will uninitialize avctx->ch_layout, but we 
>>>>>> want
>>>>>>  to keep it.
>>>>>>  +       It will be uninitialized in avcodec_free_context() */
>>>>>>  +    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 feels pretty ugly and still a bit risky that any call to
>>>>>  av_opt_free could invalidate data its not supposed to. Maybe we 
>>>>> should
>>>>>  have a flag for AVOptions instead where av_opt_free won't touch an
>>>>>  entry, because its only there to set/get it, not manage its memory?
>>>>
>>>>  Where would that flag be set? av_opt_free() takes none. And that
>>>>  function exists purely to free strings, dictionaries, and now
>>>>  uninitialize AVChannelLayout elements in a struct. If you don't 
>>>> want to
>>>>  free what av_opt_set() allocated, you shouldn't call av_opt_free() at
>>>>  all.
>>>>
>>>
>>>  On the AVOption element in the table, with the other AV_OPT_FLAG_*
>>
>> Oh, you meant flagging the actual AVOption. My bad, i for some reason 
>> thought you meant having the user flag which options they wanted to 
>> "own".
>>
>> I'll try to implement this. Any suggestion for the flag name? 
>> AV_OPT_FLAG_NO_FREE?
> 
> This also looks hackish to me. Isn't it a lot simpler to remove 
> av_opt_free from avcodec_close()? It is deprecated for freeing data of 
> an avcodec context since 2014. Actually maybe avcodec_close() should be 
> deprecated as well.

It frees a lot of things. Extradata if encoder, subtitle_header if 
decoder, hw_frames_ctx, hw_device_ctx, the entirety of AVCodecInternal, etc.

But yes, we could move the av_opt_free() call to avcodec_free_context(). 
The only options of type STRING are sub_charenc, dump_separator, and 
codec_whitelist, and afair you're not supposed to reuse an 
AVCodecContext after closing it, so it should be fine freeing those 
alongside the context.

> 
> 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 uninitialize ch_layout in avcodec_close()

[Marton Balint]

On Sun, 20 Mar 2022, James Almer wrote:

> On 3/20/2022 7:34 PM, Marton Balint wrote:
>>
>>
>>  On Sun, 20 Mar 2022, James Almer wrote:
>> 
>>> 
>>>
>>>  On 3/20/2022 7:01 PM, Hendrik Leppkes wrote:
>>>>   On Sun, Mar 20, 2022 at 7:52 PM James Almer <jamrial@gmail.com> wrote:
>>>>>
>>>>>   On 3/19/2022 4:50 AM, Hendrik Leppkes wrote:
>>>>>>   On Sat, Mar 19, 2022 at 4:04 AM James Almer <jamrial@gmail.com>
>>>>>>  wrote:
>>>>>>>
>>>>>>>   The function is not meant to clear codec parameters, and the lavf
>>>>>>>  demux
>>>>>>>   code
>>>>>>>   relies on this behavior.
>>>>>>>   Regression since 327efa66331ebdc0087c6b656059a8df2f404019.
>>>>>>>
>>>>>>>   Signed-off-by: James Almer <jamrial@gmail.com>
>>>>>>>   ---
>>>>>>>      libavcodec/avcodec.c | 6 ++++++
>>>>>>>      1 file changed, 6 insertions(+)
>>>>>>>
>>>>>>>   diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
>>>>>>>   index 38bdaad4fa..253c9f56cc 100644
>>>>>>>   --- a/libavcodec/avcodec.c
>>>>>>>   +++ b/libavcodec/avcodec.c
>>>>>>>   @@ -469,6 +469,7 @@ void avsubtitle_free(AVSubtitle *sub)
>>>>>>>
>>>>>>>      av_cold int avcodec_close(AVCodecContext *avctx)
>>>>>>>      {
>>>>>>>   +    AVChannelLayout ch_layout;
>>>>>>>          int i;
>>>>>>>
>>>>>>>          if (!avctx)
>>>>>>>   @@ -524,7 +525,12 @@ 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() will uninitialize avctx->ch_layout, but we
>>>>>>>  want
>>>>>>>   to keep it.
>>>>>>>   +       It will be uninitialized in avcodec_free_context() */
>>>>>>>   +    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 feels pretty ugly and still a bit risky that any call to
>>>>>>   av_opt_free could invalidate data its not supposed to. Maybe we
>>>>>>  should
>>>>>>   have a flag for AVOptions instead where av_opt_free won't touch an
>>>>>>   entry, because its only there to set/get it, not manage its memory?
>>>>>
>>>>>   Where would that flag be set? av_opt_free() takes none. And that
>>>>>   function exists purely to free strings, dictionaries, and now
>>>>>   uninitialize AVChannelLayout elements in a struct. If you don't want
>>>>>  to
>>>>>   free what av_opt_set() allocated, you shouldn't call av_opt_free() at
>>>>>   all.
>>>>> 
>>>>
>>>>   On the AVOption element in the table, with the other AV_OPT_FLAG_*
>>>
>>>  Oh, you meant flagging the actual AVOption. My bad, i for some reason
>>>  thought you meant having the user flag which options they wanted to
>>>  "own".
>>>
>>>  I'll try to implement this. Any suggestion for the flag name?
>>>  AV_OPT_FLAG_NO_FREE?
>>
>>  This also looks hackish to me. Isn't it a lot simpler to remove
>>  av_opt_free from avcodec_close()? It is deprecated for freeing data of an
>>  avcodec context since 2014. Actually maybe avcodec_close() should be
>>  deprecated as well.
>
> It frees a lot of things. Extradata if encoder, subtitle_header if decoder, 
> hw_frames_ctx, hw_device_ctx, the entirety of AVCodecInternal, etc.

Yeah, but it still kind of vague which fields are supposed to be freed or 
reset and which fields are not... So in the long run deprecation of 
avcodec_close() should be considered IMHO.

>
> But yes, we could move the av_opt_free() call to avcodec_free_context(). The 
> only options of type STRING are sub_charenc, dump_separator, and 
> codec_whitelist, and afair you're not supposed to reuse an AVCodecContext 
> after closing it, so it should be fine freeing those alongside the context.

I kind of prefer this. avcodec_close() documents that reusing a closed 
context is not supported anymore. (anymore meaning since 2016...)

Thanks,
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 uninitialize ch_layout in avcodec_close()

[Andreas Rheinhardt]

James Almer:
> The function is not meant to clear codec parameters, and the lavf demux code
> relies on this behavior.
> Regression since 327efa66331ebdc0087c6b656059a8df2f404019.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavcodec/avcodec.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
> index 38bdaad4fa..253c9f56cc 100644
> --- a/libavcodec/avcodec.c
> +++ b/libavcodec/avcodec.c
> @@ -469,6 +469,7 @@ void avsubtitle_free(AVSubtitle *sub)
>  
>  av_cold int avcodec_close(AVCodecContext *avctx)
>  {
> +    AVChannelLayout ch_layout;
>      int i;
>  
>      if (!avctx)
> @@ -524,7 +525,12 @@ 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() will uninitialize avctx->ch_layout, but we want to keep it.
> +       It will be uninitialized in avcodec_free_context() */
> +    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);

avcodec_close() has always* called av_opt_free() and therefore
uninitialized allocated options (the documentation of avcodec_close()
states that it "frees all the data associated with it"); the new channel
layout API is (potentially) based on allocations, so ch_layout belongs
to this group of elements.
If the demux code wants to preserve ch_layout, then the demux code
should do it itself; it should not life in avcodec_close() (where it
would be a hack).

- Andreas

*: or at least for a very long time -- I didn't do archeology for this
statement.
_______________________________________________
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 uninitialize ch_layout in avcodec_close()

[Hendrik Leppkes]

On Mon, Mar 21, 2022 at 12:38 AM Andreas Rheinhardt
<andreas.rheinhardt@outlook.com> wrote:
>
> James Almer:
> > The function is not meant to clear codec parameters, and the lavf demux code
> > relies on this behavior.
> > Regression since 327efa66331ebdc0087c6b656059a8df2f404019.
> >
> > Signed-off-by: James Almer <jamrial@gmail.com>
> > ---
> >  libavcodec/avcodec.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
> > index 38bdaad4fa..253c9f56cc 100644
> > --- a/libavcodec/avcodec.c
> > +++ b/libavcodec/avcodec.c
> > @@ -469,6 +469,7 @@ void avsubtitle_free(AVSubtitle *sub)
> >
> >  av_cold int avcodec_close(AVCodecContext *avctx)
> >  {
> > +    AVChannelLayout ch_layout;
> >      int i;
> >
> >      if (!avctx)
> > @@ -524,7 +525,12 @@ 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() will uninitialize avctx->ch_layout, but we want to keep it.
> > +       It will be uninitialized in avcodec_free_context() */
> > +    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);
>
> avcodec_close() has always* called av_opt_free() and therefore
> uninitialized allocated options (the documentation of avcodec_close()
> states that it "frees all the data associated with it"); the new channel
> layout API is (potentially) based on allocations, so ch_layout belongs
> to this group of elements.
> If the demux code wants to preserve ch_layout, then the demux code
> should do it itself; it should not life in avcodec_close() (where it
> would be a hack).
>

But is it expected that this is going to happen? It doesn't reset any
other codec properties, and if I set ch_layout without using avoptions
(you know, like 99% of all callers, if you use an codec context in
code then avoptions are rather clumsy), would I expect it to be reset?
ch_layout just happens to get reset because it potentially has
allocations, not because every codec property gets reset, its a
technical detail not a logical conclusion. I think there is such a
thing as being too strict about weird semantics here.

- Hendrik
_______________________________________________
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 uninitialize ch_layout in avcodec_close()

[Anton Khirnov]

Quoting Hendrik Leppkes (2022-03-21 08:37:43)
> On Mon, Mar 21, 2022 at 12:38 AM Andreas Rheinhardt
> <andreas.rheinhardt@outlook.com> wrote:
> >
> > James Almer:
> > > The function is not meant to clear codec parameters, and the lavf demux code
> > > relies on this behavior.
> > > Regression since 327efa66331ebdc0087c6b656059a8df2f404019.
> > >
> > > Signed-off-by: James Almer <jamrial@gmail.com>
> > > ---
> > >  libavcodec/avcodec.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
> > > index 38bdaad4fa..253c9f56cc 100644
> > > --- a/libavcodec/avcodec.c
> > > +++ b/libavcodec/avcodec.c
> > > @@ -469,6 +469,7 @@ void avsubtitle_free(AVSubtitle *sub)
> > >
> > >  av_cold int avcodec_close(AVCodecContext *avctx)
> > >  {
> > > +    AVChannelLayout ch_layout;
> > >      int i;
> > >
> > >      if (!avctx)
> > > @@ -524,7 +525,12 @@ 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() will uninitialize avctx->ch_layout, but we want to keep it.
> > > +       It will be uninitialized in avcodec_free_context() */
> > > +    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);
> >
> > avcodec_close() has always* called av_opt_free() and therefore
> > uninitialized allocated options (the documentation of avcodec_close()
> > states that it "frees all the data associated with it"); the new channel
> > layout API is (potentially) based on allocations, so ch_layout belongs
> > to this group of elements.
> > If the demux code wants to preserve ch_layout, then the demux code
> > should do it itself; it should not life in avcodec_close() (where it
> > would be a hack).
> >
> 
> But is it expected that this is going to happen? It doesn't reset any
> other codec properties, and if I set ch_layout without using avoptions
> (you know, like 99% of all callers, if you use an codec context in
> code then avoptions are rather clumsy), would I expect it to be reset?
> ch_layout just happens to get reset because it potentially has
> allocations, not because every codec property gets reset, its a
> technical detail not a logical conclusion. I think there is such a
> thing as being too strict about weird semantics here.

People should not use avcodec_close() anyway.

-- 
Anton Khirnov
_______________________________________________
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 uninitialize ch_layout in avcodec_close()

[James Almer]

On 3/21/2022 4:51 AM, Anton Khirnov wrote:
> Quoting Hendrik Leppkes (2022-03-21 08:37:43)
>> On Mon, Mar 21, 2022 at 12:38 AM Andreas Rheinhardt
>> <andreas.rheinhardt@outlook.com> wrote:
>>>
>>> James Almer:
>>>> The function is not meant to clear codec parameters, and the lavf demux code
>>>> relies on this behavior.
>>>> Regression since 327efa66331ebdc0087c6b656059a8df2f404019.
>>>>
>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>> ---
>>>>   libavcodec/avcodec.c | 6 ++++++
>>>>   1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
>>>> index 38bdaad4fa..253c9f56cc 100644
>>>> --- a/libavcodec/avcodec.c
>>>> +++ b/libavcodec/avcodec.c
>>>> @@ -469,6 +469,7 @@ void avsubtitle_free(AVSubtitle *sub)
>>>>
>>>>   av_cold int avcodec_close(AVCodecContext *avctx)
>>>>   {
>>>> +    AVChannelLayout ch_layout;
>>>>       int i;
>>>>
>>>>       if (!avctx)
>>>> @@ -524,7 +525,12 @@ 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() will uninitialize avctx->ch_layout, but we want to keep it.
>>>> +       It will be uninitialized in avcodec_free_context() */
>>>> +    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);
>>>
>>> avcodec_close() has always* called av_opt_free() and therefore
>>> uninitialized allocated options (the documentation of avcodec_close()
>>> states that it "frees all the data associated with it"); the new channel
>>> layout API is (potentially) based on allocations, so ch_layout belongs
>>> to this group of elements.
>>> If the demux code wants to preserve ch_layout, then the demux code
>>> should do it itself; it should not life in avcodec_close() (where it
>>> would be a hack).
>>>
>>
>> But is it expected that this is going to happen? It doesn't reset any
>> other codec properties, and if I set ch_layout without using avoptions
>> (you know, like 99% of all callers, if you use an codec context in
>> code then avoptions are rather clumsy), would I expect it to be reset?
>> ch_layout just happens to get reset because it potentially has
>> allocations, not because every codec property gets reset, its a
>> technical detail not a logical conclusion. I think there is such a
>> thing as being too strict about weird semantics here.
> 
> People should not use avcodec_close() anyway.

What's your opinion on the "don't free" AVOption flag Hendrik suggested?
_______________________________________________
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 uninitialize ch_layout in avcodec_close()

[Andreas Rheinhardt]

James Almer:
> On 3/21/2022 4:51 AM, Anton Khirnov wrote:
>> Quoting Hendrik Leppkes (2022-03-21 08:37:43)
>>> On Mon, Mar 21, 2022 at 12:38 AM Andreas Rheinhardt
>>> <andreas.rheinhardt@outlook.com> wrote:
>>>>
>>>> James Almer:
>>>>> The function is not meant to clear codec parameters, and the lavf
>>>>> demux code
>>>>> relies on this behavior.
>>>>> Regression since 327efa66331ebdc0087c6b656059a8df2f404019.
>>>>>
>>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>>> ---
>>>>>   libavcodec/avcodec.c | 6 ++++++
>>>>>   1 file changed, 6 insertions(+)
>>>>>
>>>>> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
>>>>> index 38bdaad4fa..253c9f56cc 100644
>>>>> --- a/libavcodec/avcodec.c
>>>>> +++ b/libavcodec/avcodec.c
>>>>> @@ -469,6 +469,7 @@ void avsubtitle_free(AVSubtitle *sub)
>>>>>
>>>>>   av_cold int avcodec_close(AVCodecContext *avctx)
>>>>>   {
>>>>> +    AVChannelLayout ch_layout;
>>>>>       int i;
>>>>>
>>>>>       if (!avctx)
>>>>> @@ -524,7 +525,12 @@ 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() will uninitialize avctx->ch_layout, but we
>>>>> want to keep it.
>>>>> +       It will be uninitialized in avcodec_free_context() */
>>>>> +    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);
>>>>
>>>> avcodec_close() has always* called av_opt_free() and therefore
>>>> uninitialized allocated options (the documentation of avcodec_close()
>>>> states that it "frees all the data associated with it"); the new
>>>> channel
>>>> layout API is (potentially) based on allocations, so ch_layout belongs
>>>> to this group of elements.
>>>> If the demux code wants to preserve ch_layout, then the demux code
>>>> should do it itself; it should not life in avcodec_close() (where it
>>>> would be a hack).
>>>>
>>>
>>> But is it expected that this is going to happen? It doesn't reset any
>>> other codec properties, and if I set ch_layout without using avoptions
>>> (you know, like 99% of all callers, if you use an codec context in
>>> code then avoptions are rather clumsy), would I expect it to be reset?
>>> ch_layout just happens to get reset because it potentially has
>>> allocations, not because every codec property gets reset, its a
>>> technical detail not a logical conclusion. I think there is such a
>>> thing as being too strict about weird semantics here.
>>
>> People should not use avcodec_close() anyway.
> 
> What's your opinion on the "don't free" AVOption flag Hendrik suggested?

If one uses this on AVCodecContext.ch_layout, avcodec_close+av_free
might leak. So I don't see a usecase for it.

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