| Message ID | 20240201082938.16687-3-anton@khirnov.net |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/4] lavf/mpegts: drop a cargo-culted check | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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(&sti->avctx); > + sti->avctx = avctx_new; > + > + avctx_new = NULL; > + > +fail: > + avcodec_free_context(&avctx_new); > + avcodec_parameters_free(&par_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(&sti->info->duration_error); > av_freep(&sti->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(&sti->avctx->ch_layout, &st->codecpar->ch_layout); > + codec_close(sti); > av_bsf_free(&sti->extract_extradata.bsf); > } > if (ic->pb) {
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(&sti->avctx); > + sti->avctx = avctx_new; > + > + avctx_new = NULL; > + > +fail: > + avcodec_free_context(&avctx_new); > + avcodec_parameters_free(&par_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(&sti->info->duration_error); > av_freep(&sti->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(&sti->avctx->ch_layout, &st->codecpar->ch_layout); > + codec_close(sti); > av_bsf_free(&sti->extract_extradata.bsf); > } > if (ic->pb) {
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.
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(&sti->avctx); + sti->avctx = avctx_new; + + avctx_new = NULL; + +fail: + avcodec_free_context(&avctx_new); + avcodec_parameters_free(&par_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(&sti->info->duration_error); av_freep(&sti->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(&sti->avctx->ch_layout, &st->codecpar->ch_layout); + codec_close(sti); av_bsf_free(&sti->extract_extradata.bsf); } if (ic->pb) {