Re: [FFmpeg-devel] [PATCH 3/4] lavf/demux: stop calling avcodec_close()

2024-02-05 Thread Anton Khirnov 
Quoting Leo Izen (2024-02-03 03:35:45)
> On 2/1/24 03:29, Anton Khirnov wrote:
> > Replace it with recreating the codec context.
> > 
> > This is the last remaining blocker for deprecating avcodec_close().
> > ---
> >   libavformat/demux.c | 53 -
> >   1 file changed, 48 insertions(+), 5 deletions(-)
> > 
> > diff --git a/libavformat/demux.c b/libavformat/demux.c
> > index 6f640b92b1..c1640c459c 100644
> > --- a/libavformat/demux.c
> > +++ b/libavformat/demux.c
> > @@ -1250,6 +1250,52 @@ static int64_t ts_to_samples(AVStream *st, int64_t 
> > ts)
> >   return av_rescale(ts, st->time_base.num * st->codecpar->sample_rate, 
> > st->time_base.den);
> >   }
> >   
> > +static int codec_close(FFStream *sti)
> > +{
> > +AVCodecContext *avctx_new = NULL;
> > +AVCodecParameters *par_tmp = NULL;
> > +int ret = 0;
> > +
> 
> I believe we can drop the initialization from avctx_new and from ret, 
> because avctx_new is assigned immediately, and ret is assigned before 
> each goto before it's assigned properly.

It's safer wrt future modifications of the code, e.g. if someone would
insert something that could fail before allocating avctx.

-- 
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-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 3/4] lavf/demux: stop calling avcodec_close()

2024-02-02 Thread James Almer 

On 2/1/2024 5:29 AM, Anton Khirnov wrote:

Replace it with recreating the codec context.

This is the last remaining blocker for deprecating avcodec_close().
---
  libavformat/demux.c | 53 -
  1 file changed, 48 insertions(+), 5 deletions(-)

diff --git a/libavformat/demux.c b/libavformat/demux.c
index 6f640b92b1..c1640c459c 100644
--- a/libavformat/demux.c
+++ b/libavformat/demux.c
@@ -1250,6 +1250,52 @@ static int64_t ts_to_samples(AVStream *st, int64_t ts)
  return av_rescale(ts, st->time_base.num * st->codecpar->sample_rate, 
st->time_base.den);
  }
  
+static int codec_close(FFStream *sti)


This returns an error code but you're not checking it.


+{
+AVCodecContext *avctx_new = NULL;
+AVCodecParameters *par_tmp = NULL;
+int ret = 0;
+
+avctx_new = avcodec_alloc_context3(sti->avctx->codec);
+if (!avctx_new) {
+ret = AVERROR(ENOMEM);
+goto fail;
+}
+
+par_tmp = avcodec_parameters_alloc();
+if (!par_tmp) {
+ret = AVERROR(ENOMEM);
+goto fail;
+}
+
+ret = avcodec_parameters_from_context(par_tmp, sti->avctx);
+if (ret < 0)
+goto fail;
+
+ret = avcodec_parameters_to_context(avctx_new, par_tmp);
+if (ret < 0)
+goto fail;
+
+avctx_new->pkt_timebase = sti->avctx->pkt_timebase;
+
+#if FF_API_TICKS_PER_FRAME
+FF_DISABLE_DEPRECATION_WARNINGS
+avctx_new->ticks_per_frame = sti->avctx->ticks_per_frame;
+FF_ENABLE_DEPRECATION_WARNINGS
+#endif
+
+avcodec_free_context(>avctx);
+sti->avctx = avctx_new;
+
+avctx_new = NULL;
+
+fail:
+avcodec_free_context(_new);
+avcodec_parameters_free(_tmp);
+
+return ret;


For success scenarios, this will return whatever was last set on ret, 
which in this case is the return value of 
avcodec_parameters_to_context() that just happens to be 0. But something 
else could be added later that also sets ret after it, so the proper 
thing to do is to set ret to 0 immediately above the fail label.



+}
+
  static int read_frame_internal(AVFormatContext *s, AVPacket *pkt)
  {
  FFFormatContext *const si = ffformatcontext(s);
@@ -1286,7 +1332,7 @@ static int read_frame_internal(AVFormatContext *s, 
AVPacket *pkt)
  if (sti->need_context_update) {
  if (avcodec_is_open(sti->avctx)) {
  av_log(s, AV_LOG_DEBUG, "Demuxer context update while decoder is 
open, closing and trying to re-open\n");
-avcodec_close(sti->avctx);
+codec_close(sti);
  sti->info->found_decoder = 0;
  }
  
@@ -3017,10 +3063,7 @@ find_stream_info_err:

  av_freep(>info->duration_error);
  av_freep(>info);
  }
-avcodec_close(sti->avctx);
-// FIXME: avcodec_close() frees AVOption settable fields which 
includes ch_layout,
-//so we need to restore it.
-av_channel_layout_copy(>avctx->ch_layout, 
>codecpar->ch_layout);
+codec_close(sti);
  av_bsf_free(>extract_extradata.bsf);
  }
  if (ic->pb) {

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 3/4] lavf/demux: stop calling avcodec_close()

2024-02-02 Thread Leo Izen 

On 2/1/24 03:29, Anton Khirnov wrote:

Replace it with recreating the codec context.

This is the last remaining blocker for deprecating avcodec_close().
---
  libavformat/demux.c | 53 -
  1 file changed, 48 insertions(+), 5 deletions(-)

diff --git a/libavformat/demux.c b/libavformat/demux.c
index 6f640b92b1..c1640c459c 100644
--- a/libavformat/demux.c
+++ b/libavformat/demux.c
@@ -1250,6 +1250,52 @@ static int64_t ts_to_samples(AVStream *st, int64_t ts)
  return av_rescale(ts, st->time_base.num * st->codecpar->sample_rate, 
st->time_base.den);
  }
  
+static int codec_close(FFStream *sti)

+{
+AVCodecContext *avctx_new = NULL;
+AVCodecParameters *par_tmp = NULL;
+int ret = 0;
+


I believe we can drop the initialization from avctx_new and from ret, 
because avctx_new is assigned immediately, and ret is assigned before 
each goto before it's assigned properly.




+avctx_new = avcodec_alloc_context3(sti->avctx->codec);
+if (!avctx_new) {
+ret = AVERROR(ENOMEM);
+goto fail;
+}
+
+par_tmp = avcodec_parameters_alloc();
+if (!par_tmp) {
+ret = AVERROR(ENOMEM);
+goto fail;
+}
+
+ret = avcodec_parameters_from_context(par_tmp, sti->avctx);
+if (ret < 0)
+goto fail;
+
+ret = avcodec_parameters_to_context(avctx_new, par_tmp);
+if (ret < 0)
+goto fail;
+
+avctx_new->pkt_timebase = sti->avctx->pkt_timebase;
+
+#if FF_API_TICKS_PER_FRAME
+FF_DISABLE_DEPRECATION_WARNINGS
+avctx_new->ticks_per_frame = sti->avctx->ticks_per_frame;
+FF_ENABLE_DEPRECATION_WARNINGS
+#endif
+
+avcodec_free_context(>avctx);
+sti->avctx = avctx_new;
+
+avctx_new = NULL;
+
+fail:
+avcodec_free_context(_new);
+avcodec_parameters_free(_tmp);
+
+return ret;
+}
+
  static int read_frame_internal(AVFormatContext *s, AVPacket *pkt)
  {
  FFFormatContext *const si = ffformatcontext(s);
@@ -1286,7 +1332,7 @@ static int read_frame_internal(AVFormatContext *s, 
AVPacket *pkt)
  if (sti->need_context_update) {
  if (avcodec_is_open(sti->avctx)) {
  av_log(s, AV_LOG_DEBUG, "Demuxer context update while decoder is 
open, closing and trying to re-open\n");
-avcodec_close(sti->avctx);
+codec_close(sti);
  sti->info->found_decoder = 0;
  }
  
@@ -3017,10 +3063,7 @@ find_stream_info_err:

  av_freep(>info->duration_error);
  av_freep(>info);
  }
-avcodec_close(sti->avctx);
-// FIXME: avcodec_close() frees AVOption settable fields which 
includes ch_layout,
-//so we need to restore it.
-av_channel_layout_copy(>avctx->ch_layout, 
>codecpar->ch_layout);
+codec_close(sti);
  av_bsf_free(>extract_extradata.bsf);
  }
  if (ic->pb) {


OpenPGP_signature.asc
Description: OpenPGP digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 3/4] lavf/demux: stop calling avcodec_close()

2024-02-01 Thread Anton Khirnov 
Replace it with recreating the codec context.

This is the last remaining blocker for deprecating avcodec_close().
---
 libavformat/demux.c | 53 -
 1 file changed, 48 insertions(+), 5 deletions(-)

diff --git a/libavformat/demux.c b/libavformat/demux.c
index 6f640b92b1..c1640c459c 100644
--- a/libavformat/demux.c
+++ b/libavformat/demux.c
@@ -1250,6 +1250,52 @@ static int64_t ts_to_samples(AVStream *st, int64_t ts)
 return av_rescale(ts, st->time_base.num * st->codecpar->sample_rate, 
st->time_base.den);
 }
 
+static int codec_close(FFStream *sti)
+{
+AVCodecContext *avctx_new = NULL;
+AVCodecParameters *par_tmp = NULL;
+int ret = 0;
+
+avctx_new = avcodec_alloc_context3(sti->avctx->codec);
+if (!avctx_new) {
+ret = AVERROR(ENOMEM);
+goto fail;
+}
+
+par_tmp = avcodec_parameters_alloc();
+if (!par_tmp) {
+ret = AVERROR(ENOMEM);
+goto fail;
+}
+
+ret = avcodec_parameters_from_context(par_tmp, sti->avctx);
+if (ret < 0)
+goto fail;
+
+ret = avcodec_parameters_to_context(avctx_new, par_tmp);
+if (ret < 0)
+goto fail;
+
+avctx_new->pkt_timebase = sti->avctx->pkt_timebase;
+
+#if FF_API_TICKS_PER_FRAME
+FF_DISABLE_DEPRECATION_WARNINGS
+avctx_new->ticks_per_frame = sti->avctx->ticks_per_frame;
+FF_ENABLE_DEPRECATION_WARNINGS
+#endif
+
+avcodec_free_context(>avctx);
+sti->avctx = avctx_new;
+
+avctx_new = NULL;
+
+fail:
+avcodec_free_context(_new);
+avcodec_parameters_free(_tmp);
+
+return ret;
+}
+
 static int read_frame_internal(AVFormatContext *s, AVPacket *pkt)
 {
 FFFormatContext *const si = ffformatcontext(s);
@@ -1286,7 +1332,7 @@ static int read_frame_internal(AVFormatContext *s, 
AVPacket *pkt)
 if (sti->need_context_update) {
 if (avcodec_is_open(sti->avctx)) {
 av_log(s, AV_LOG_DEBUG, "Demuxer context update while decoder 
is open, closing and trying to re-open\n");
-avcodec_close(sti->avctx);
+codec_close(sti);
 sti->info->found_decoder = 0;
 }
 
@@ -3017,10 +3063,7 @@ find_stream_info_err:
 av_freep(>info->duration_error);
 av_freep(>info);
 }
-avcodec_close(sti->avctx);
-// FIXME: avcodec_close() frees AVOption settable fields which 
includes ch_layout,
-//so we need to restore it.
-av_channel_layout_copy(>avctx->ch_layout, 
>codecpar->ch_layout);
+codec_close(sti);
 av_bsf_free(>extract_extradata.bsf);
 }
 if (ic->pb) {
-- 
2.42.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-requ...@ffmpeg.org with subject "unsubscribe".