| Message ID | 20200418191847.25815-1-cus@passwd.hu |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel,v2,1/6] avformat: only allow a single bitstream filter when muxing | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
Marton Balint: > Current muxers only use a single bitstream filter, so there is no need to > maintain code which operates on a list of bitstream filters. When multiple > bitstream filters are needed muxers can simply use a list bitstream filter. > > If there is a use case in the future when different bitstream filters should be > added at subsequent packets then a new API possibly involving reconfiguring the > list bistream filter can be added knowing the exact requirements. > > Signed-off-by: Marton Balint <cus@passwd.hu> > --- > libavformat/dashenc.c | 6 ++---- > libavformat/internal.h | 5 ++--- > libavformat/mux.c | 6 +++--- > libavformat/segment.c | 6 ++---- > libavformat/utils.c | 27 +++++++++------------------ > 5 files changed, 18 insertions(+), 32 deletions(-) > > diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c > index 5a8cff4034..b977761a00 100644 > --- a/libavformat/dashenc.c > +++ b/libavformat/dashenc.c > @@ -2307,10 +2307,8 @@ static int dash_check_bitstream(struct AVFormatContext *s, const AVPacket *avpkt > if (ret == 1) { > AVStream *st = s->streams[avpkt->stream_index]; > AVStream *ost = oc->streams[0]; > - st->internal->bsfcs = ost->internal->bsfcs; > - st->internal->nb_bsfcs = ost->internal->nb_bsfcs; > - ost->internal->bsfcs = NULL; > - ost->internal->nb_bsfcs = 0; > + st->internal->bsfc = ost->internal->bsfc; > + ost->internal->bsfc = NULL; > } > return ret; > } > diff --git a/libavformat/internal.h b/libavformat/internal.h > index 7e4284b217..cafb4a9686 100644 > --- a/libavformat/internal.h > +++ b/libavformat/internal.h > @@ -152,12 +152,11 @@ struct AVStreamInternal { > int reorder; > > /** > - * bitstream filters to run on stream > + * bitstream filter to run on stream > * - encoding: Set by muxer using ff_stream_add_bitstream_filter > * - decoding: unused > */ > - AVBSFContext **bsfcs; > - int nb_bsfcs; > + AVBSFContext *bsfc; > > /** > * Whether or not check_bitstream should still be run on each packet > diff --git a/libavformat/mux.c b/libavformat/mux.c > index 3d63d59faf..5209c84f40 100644 > --- a/libavformat/mux.c > +++ b/libavformat/mux.c > @@ -824,7 +824,7 @@ static int prepare_input_packet(AVFormatContext *s, AVPacket *pkt) > > static int do_packet_auto_bsf(AVFormatContext *s, AVPacket *pkt) { > AVStream *st = s->streams[pkt->stream_index]; > - int i, ret; > + int ret; > > if (!(s->flags & AVFMT_FLAG_AUTO_BSF)) > return 1; > @@ -838,8 +838,8 @@ static int do_packet_auto_bsf(AVFormatContext *s, AVPacket *pkt) { > } > } > > - for (i = 0; i < st->internal->nb_bsfcs; i++) { > - AVBSFContext *ctx = st->internal->bsfcs[i]; > + if (st->internal->bsfc) { > + AVBSFContext *ctx = st->internal->bsfc; > // TODO: when any bitstream filter requires flushing at EOF, we'll need to > // flush each stream's BSF chain on write_trailer. > if ((ret = av_bsf_send_packet(ctx, pkt)) < 0) { > diff --git a/libavformat/segment.c b/libavformat/segment.c > index 60b72b7d15..32c09827eb 100644 > --- a/libavformat/segment.c > +++ b/libavformat/segment.c > @@ -1034,10 +1034,8 @@ static int seg_check_bitstream(struct AVFormatContext *s, const AVPacket *pkt) > if (ret == 1) { > AVStream *st = s->streams[pkt->stream_index]; > AVStream *ost = oc->streams[pkt->stream_index]; > - st->internal->bsfcs = ost->internal->bsfcs; > - st->internal->nb_bsfcs = ost->internal->nb_bsfcs; > - ost->internal->bsfcs = NULL; > - ost->internal->nb_bsfcs = 0; > + st->internal->bsfc = ost->internal->bsfc; > + ost->internal->bsfc = NULL; > } > return ret; > } > diff --git a/libavformat/utils.c b/libavformat/utils.c > index a58e47fabc..eff73252ec 100644 > --- a/libavformat/utils.c > +++ b/libavformat/utils.c > @@ -4410,10 +4410,7 @@ static void free_stream(AVStream **pst) > > if (st->internal) { > avcodec_free_context(&st->internal->avctx); > - for (i = 0; i < st->internal->nb_bsfcs; i++) { > - av_bsf_free(&st->internal->bsfcs[i]); > - av_freep(&st->internal->bsfcs); I can't believe it! This only works if there is only one bsf. So a real list has indeed never been tested. > - } > + av_bsf_free(&st->internal->bsfc); > av_freep(&st->internal->priv_pts); > av_bsf_free(&st->internal->extract_extradata.bsf); > av_packet_free(&st->internal->extract_extradata.pkt); > @@ -5574,7 +5571,11 @@ int ff_stream_add_bitstream_filter(AVStream *st, const char *name, const char *a > int ret; > const AVBitStreamFilter *bsf; > AVBSFContext *bsfc; > - AVCodecParameters *in_par; > + > + if (st->internal->bsfc) { > + av_log(NULL, AV_LOG_ERROR, "A bitstream filter is already specified for stream %d\n", st->index); > + return AVERROR(EINVAL); > + } Given that this is a situation which should not happen at all (until we really have a situation where one wants to add more than one bsf, in which case further changes to this function are required anyway), an assert seems more appropriate. > > if (!(bsf = av_bsf_get_by_name(name))) { > av_log(NULL, AV_LOG_ERROR, "Unknown bitstream filter '%s'\n", name); > @@ -5584,15 +5585,8 @@ int ff_stream_add_bitstream_filter(AVStream *st, const char *name, const char *a > if ((ret = av_bsf_alloc(bsf, &bsfc)) < 0) > return ret; > > - if (st->internal->nb_bsfcs) { > - in_par = st->internal->bsfcs[st->internal->nb_bsfcs - 1]->par_out; > - bsfc->time_base_in = st->internal->bsfcs[st->internal->nb_bsfcs - 1]->time_base_out; > - } else { > - in_par = st->codecpar; > - bsfc->time_base_in = st->time_base; > - } > - > - if ((ret = avcodec_parameters_copy(bsfc->par_in, in_par)) < 0) { > + bsfc->time_base_in = st->time_base; > + if ((ret = avcodec_parameters_copy(bsfc->par_in, st->codecpar)) < 0) { > av_bsf_free(&bsfc); > return ret; > } > @@ -5615,10 +5609,7 @@ int ff_stream_add_bitstream_filter(AVStream *st, const char *name, const char *a > return ret; > } > > - if ((ret = av_dynarray_add_nofree(&st->internal->bsfcs, &st->internal->nb_bsfcs, bsfc))) { > - av_bsf_free(&bsfc); > - return ret; > - } > + st->internal->bsfc = bsfc; > > av_log(NULL, AV_LOG_VERBOSE, > "Automatically inserted bitstream filter '%s'; args='%s'\n", > LGTM otherwise. - Andreas
On Sun, 19 Apr 2020, Andreas Rheinhardt wrote: > Marton Balint: >> Current muxers only use a single bitstream filter, so there is no need to >> maintain code which operates on a list of bitstream filters. When multiple >> bitstream filters are needed muxers can simply use a list bitstream filter. >> >> If there is a use case in the future when different bitstream filters should be >> added at subsequent packets then a new API possibly involving reconfiguring the >> list bistream filter can be added knowing the exact requirements. >> >> Signed-off-by: Marton Balint <cus@passwd.hu> >> --- >> libavformat/dashenc.c | 6 ++---- >> libavformat/internal.h | 5 ++--- >> libavformat/mux.c | 6 +++--- >> libavformat/segment.c | 6 ++---- >> libavformat/utils.c | 27 +++++++++------------------ >> 5 files changed, 18 insertions(+), 32 deletions(-) >> >> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c >> index 5a8cff4034..b977761a00 100644 >> --- a/libavformat/dashenc.c >> +++ b/libavformat/dashenc.c >> @@ -2307,10 +2307,8 @@ static int dash_check_bitstream(struct AVFormatContext *s, const AVPacket *avpkt >> if (ret == 1) { >> AVStream *st = s->streams[avpkt->stream_index]; >> AVStream *ost = oc->streams[0]; >> - st->internal->bsfcs = ost->internal->bsfcs; >> - st->internal->nb_bsfcs = ost->internal->nb_bsfcs; >> - ost->internal->bsfcs = NULL; >> - ost->internal->nb_bsfcs = 0; >> + st->internal->bsfc = ost->internal->bsfc; >> + ost->internal->bsfc = NULL; >> } >> return ret; >> } >> diff --git a/libavformat/internal.h b/libavformat/internal.h >> index 7e4284b217..cafb4a9686 100644 >> --- a/libavformat/internal.h >> +++ b/libavformat/internal.h >> @@ -152,12 +152,11 @@ struct AVStreamInternal { >> int reorder; >> >> /** >> - * bitstream filters to run on stream >> + * bitstream filter to run on stream >> * - encoding: Set by muxer using ff_stream_add_bitstream_filter >> * - decoding: unused >> */ >> - AVBSFContext **bsfcs; >> - int nb_bsfcs; >> + AVBSFContext *bsfc; >> >> /** >> * Whether or not check_bitstream should still be run on each packet >> diff --git a/libavformat/mux.c b/libavformat/mux.c >> index 3d63d59faf..5209c84f40 100644 >> --- a/libavformat/mux.c >> +++ b/libavformat/mux.c >> @@ -824,7 +824,7 @@ static int prepare_input_packet(AVFormatContext *s, AVPacket *pkt) >> >> static int do_packet_auto_bsf(AVFormatContext *s, AVPacket *pkt) { >> AVStream *st = s->streams[pkt->stream_index]; >> - int i, ret; >> + int ret; >> >> if (!(s->flags & AVFMT_FLAG_AUTO_BSF)) >> return 1; >> @@ -838,8 +838,8 @@ static int do_packet_auto_bsf(AVFormatContext *s, AVPacket *pkt) { >> } >> } >> >> - for (i = 0; i < st->internal->nb_bsfcs; i++) { >> - AVBSFContext *ctx = st->internal->bsfcs[i]; >> + if (st->internal->bsfc) { >> + AVBSFContext *ctx = st->internal->bsfc; >> // TODO: when any bitstream filter requires flushing at EOF, we'll need to >> // flush each stream's BSF chain on write_trailer. >> if ((ret = av_bsf_send_packet(ctx, pkt)) < 0) { >> diff --git a/libavformat/segment.c b/libavformat/segment.c >> index 60b72b7d15..32c09827eb 100644 >> --- a/libavformat/segment.c >> +++ b/libavformat/segment.c >> @@ -1034,10 +1034,8 @@ static int seg_check_bitstream(struct AVFormatContext *s, const AVPacket *pkt) >> if (ret == 1) { >> AVStream *st = s->streams[pkt->stream_index]; >> AVStream *ost = oc->streams[pkt->stream_index]; >> - st->internal->bsfcs = ost->internal->bsfcs; >> - st->internal->nb_bsfcs = ost->internal->nb_bsfcs; >> - ost->internal->bsfcs = NULL; >> - ost->internal->nb_bsfcs = 0; >> + st->internal->bsfc = ost->internal->bsfc; >> + ost->internal->bsfc = NULL; >> } >> return ret; >> } >> diff --git a/libavformat/utils.c b/libavformat/utils.c >> index a58e47fabc..eff73252ec 100644 >> --- a/libavformat/utils.c >> +++ b/libavformat/utils.c >> @@ -4410,10 +4410,7 @@ static void free_stream(AVStream **pst) >> >> if (st->internal) { >> avcodec_free_context(&st->internal->avctx); >> - for (i = 0; i < st->internal->nb_bsfcs; i++) { >> - av_bsf_free(&st->internal->bsfcs[i]); >> - av_freep(&st->internal->bsfcs); > > I can't believe it! This only works if there is only one bsf. So a real > list has indeed never been tested. > >> - } >> + av_bsf_free(&st->internal->bsfc); >> av_freep(&st->internal->priv_pts); >> av_bsf_free(&st->internal->extract_extradata.bsf); >> av_packet_free(&st->internal->extract_extradata.pkt); >> @@ -5574,7 +5571,11 @@ int ff_stream_add_bitstream_filter(AVStream *st, const char *name, const char *a >> int ret; >> const AVBitStreamFilter *bsf; >> AVBSFContext *bsfc; >> - AVCodecParameters *in_par; >> + >> + if (st->internal->bsfc) { >> + av_log(NULL, AV_LOG_ERROR, "A bitstream filter is already specified for stream %d\n", st->index); >> + return AVERROR(EINVAL); >> + } > > Given that this is a situation which should not happen at all (until we > really have a situation where one wants to add more than one bsf, in > which case further changes to this function are required anyway), an > assert seems more appropriate. OK, changed locally. > >> >> if (!(bsf = av_bsf_get_by_name(name))) { >> av_log(NULL, AV_LOG_ERROR, "Unknown bitstream filter '%s'\n", name); >> @@ -5584,15 +5585,8 @@ int ff_stream_add_bitstream_filter(AVStream *st, const char *name, const char *a >> if ((ret = av_bsf_alloc(bsf, &bsfc)) < 0) >> return ret; >> >> - if (st->internal->nb_bsfcs) { >> - in_par = st->internal->bsfcs[st->internal->nb_bsfcs - 1]->par_out; >> - bsfc->time_base_in = st->internal->bsfcs[st->internal->nb_bsfcs - 1]->time_base_out; >> - } else { >> - in_par = st->codecpar; >> - bsfc->time_base_in = st->time_base; >> - } >> - >> - if ((ret = avcodec_parameters_copy(bsfc->par_in, in_par)) < 0) { >> + bsfc->time_base_in = st->time_base; >> + if ((ret = avcodec_parameters_copy(bsfc->par_in, st->codecpar)) < 0) { >> av_bsf_free(&bsfc); >> return ret; >> } >> @@ -5615,10 +5609,7 @@ int ff_stream_add_bitstream_filter(AVStream *st, const char *name, const char *a >> return ret; >> } >> >> - if ((ret = av_dynarray_add_nofree(&st->internal->bsfcs, &st->internal->nb_bsfcs, bsfc))) { >> - av_bsf_free(&bsfc); >> - return ret; >> - } >> + st->internal->bsfc = bsfc; >> >> av_log(NULL, AV_LOG_VERBOSE, >> "Automatically inserted bitstream filter '%s'; args='%s'\n", >> > LGTM otherwise. Will apply soon. Also ping for other patches in the series. Thanks, Marton
On Sat, 25 Apr 2020, Marton Balint wrote: > > > On Sun, 19 Apr 2020, Andreas Rheinhardt wrote: > >> Marton Balint: >>> Current muxers only use a single bitstream filter, so there is no need to >>> maintain code which operates on a list of bitstream filters. When multiple >>> bitstream filters are needed muxers can simply use a list bitstream > filter. >>> >>> If there is a use case in the future when different bitstream filters > should be >>> added at subsequent packets then a new API possibly involving > reconfiguring the >>> list bistream filter can be added knowing the exact requirements. >>> >>> Signed-off-by: Marton Balint <cus@passwd.hu> >>> --- >>> libavformat/dashenc.c | 6 ++---- >>> libavformat/internal.h | 5 ++--- >>> libavformat/mux.c | 6 +++--- >>> libavformat/segment.c | 6 ++---- >>> libavformat/utils.c | 27 +++++++++------------------ >>> 5 files changed, 18 insertions(+), 32 deletions(-) >>> >>> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c >>> index 5a8cff4034..b977761a00 100644 >>> --- a/libavformat/dashenc.c >>> +++ b/libavformat/dashenc.c >>> @@ -2307,10 +2307,8 @@ static int dash_check_bitstream(struct > AVFormatContext *s, const AVPacket *avpkt >>> if (ret == 1) { >>> AVStream *st = s->streams[avpkt->stream_index]; >>> AVStream *ost = oc->streams[0]; >>> - st->internal->bsfcs = ost->internal->bsfcs; >>> - st->internal->nb_bsfcs = ost->internal->nb_bsfcs; >>> - ost->internal->bsfcs = NULL; >>> - ost->internal->nb_bsfcs = 0; >>> + st->internal->bsfc = ost->internal->bsfc; >>> + ost->internal->bsfc = NULL; >>> } >>> return ret; >>> } >>> diff --git a/libavformat/internal.h b/libavformat/internal.h >>> index 7e4284b217..cafb4a9686 100644 >>> --- a/libavformat/internal.h >>> +++ b/libavformat/internal.h >>> @@ -152,12 +152,11 @@ struct AVStreamInternal { >>> int reorder; >>> >>> /** >>> - * bitstream filters to run on stream >>> + * bitstream filter to run on stream >>> * - encoding: Set by muxer using ff_stream_add_bitstream_filter >>> * - decoding: unused >>> */ >>> - AVBSFContext **bsfcs; >>> - int nb_bsfcs; >>> + AVBSFContext *bsfc; >>> >>> /** >>> * Whether or not check_bitstream should still be run on each packet >>> diff --git a/libavformat/mux.c b/libavformat/mux.c >>> index 3d63d59faf..5209c84f40 100644 >>> --- a/libavformat/mux.c >>> +++ b/libavformat/mux.c >>> @@ -824,7 +824,7 @@ static int prepare_input_packet(AVFormatContext *s, > AVPacket *pkt) >>> >>> static int do_packet_auto_bsf(AVFormatContext *s, AVPacket *pkt) { >>> AVStream *st = s->streams[pkt->stream_index]; >>> - int i, ret; >>> + int ret; >>> >>> if (!(s->flags & AVFMT_FLAG_AUTO_BSF)) >>> return 1; >>> @@ -838,8 +838,8 @@ static int do_packet_auto_bsf(AVFormatContext *s, > AVPacket *pkt) { >>> } >>> } >>> >>> - for (i = 0; i < st->internal->nb_bsfcs; i++) { >>> - AVBSFContext *ctx = st->internal->bsfcs[i]; >>> + if (st->internal->bsfc) { >>> + AVBSFContext *ctx = st->internal->bsfc; >>> // TODO: when any bitstream filter requires flushing at EOF, > we'll need to >>> // flush each stream's BSF chain on write_trailer. >>> if ((ret = av_bsf_send_packet(ctx, pkt)) < 0) { >>> diff --git a/libavformat/segment.c b/libavformat/segment.c >>> index 60b72b7d15..32c09827eb 100644 >>> --- a/libavformat/segment.c >>> +++ b/libavformat/segment.c >>> @@ -1034,10 +1034,8 @@ static int seg_check_bitstream(struct > AVFormatContext *s, const AVPacket *pkt) >>> if (ret == 1) { >>> AVStream *st = s->streams[pkt->stream_index]; >>> AVStream *ost = oc->streams[pkt->stream_index]; >>> - st->internal->bsfcs = ost->internal->bsfcs; >>> - st->internal->nb_bsfcs = ost->internal->nb_bsfcs; >>> - ost->internal->bsfcs = NULL; >>> - ost->internal->nb_bsfcs = 0; >>> + st->internal->bsfc = ost->internal->bsfc; >>> + ost->internal->bsfc = NULL; >>> } >>> return ret; >>> } >>> diff --git a/libavformat/utils.c b/libavformat/utils.c >>> index a58e47fabc..eff73252ec 100644 >>> --- a/libavformat/utils.c >>> +++ b/libavformat/utils.c >>> @@ -4410,10 +4410,7 @@ static void free_stream(AVStream **pst) >>> >>> if (st->internal) { >>> avcodec_free_context(&st->internal->avctx); >>> - for (i = 0; i < st->internal->nb_bsfcs; i++) { >>> - av_bsf_free(&st->internal->bsfcs[i]); >>> - av_freep(&st->internal->bsfcs); >> >> I can't believe it! This only works if there is only one bsf. So a real >> list has indeed never been tested. >> >>> - } >>> + av_bsf_free(&st->internal->bsfc); >>> av_freep(&st->internal->priv_pts); >>> av_bsf_free(&st->internal->extract_extradata.bsf); >>> av_packet_free(&st->internal->extract_extradata.pkt); >>> @@ -5574,7 +5571,11 @@ int ff_stream_add_bitstream_filter(AVStream *st, > const char *name, const char *a >>> int ret; >>> const AVBitStreamFilter *bsf; >>> AVBSFContext *bsfc; >>> - AVCodecParameters *in_par; >>> + >>> + if (st->internal->bsfc) { >>> + av_log(NULL, AV_LOG_ERROR, "A bitstream filter is already > specified for stream %d\n", st->index); >>> + return AVERROR(EINVAL); >>> + } >> >> Given that this is a situation which should not happen at all (until we >> really have a situation where one wants to add more than one bsf, in >> which case further changes to this function are required anyway), an >> assert seems more appropriate. > > OK, changed locally. > >> >>> >>> if (!(bsf = av_bsf_get_by_name(name))) { >>> av_log(NULL, AV_LOG_ERROR, "Unknown bitstream filter '%s'\n", > name); >>> @@ -5584,15 +5585,8 @@ int ff_stream_add_bitstream_filter(AVStream *st, > const char *name, const char *a >>> if ((ret = av_bsf_alloc(bsf, &bsfc)) < 0) >>> return ret; >>> >>> - if (st->internal->nb_bsfcs) { >>> - in_par = st->internal->bsfcs[st->internal->nb_bsfcs - > 1]->par_out; >>> - bsfc->time_base_in = st->internal->bsfcs[st->internal->nb_bsfcs - > 1]->time_base_out; >>> - } else { >>> - in_par = st->codecpar; >>> - bsfc->time_base_in = st->time_base; >>> - } >>> - >>> - if ((ret = avcodec_parameters_copy(bsfc->par_in, in_par)) < 0) { >>> + bsfc->time_base_in = st->time_base; >>> + if ((ret = avcodec_parameters_copy(bsfc->par_in, st->codecpar)) < 0) > { >>> av_bsf_free(&bsfc); >>> return ret; >>> } >>> @@ -5615,10 +5609,7 @@ int ff_stream_add_bitstream_filter(AVStream *st, > const char *name, const char *a >>> return ret; >>> } >>> >>> - if ((ret = av_dynarray_add_nofree(&st->internal->bsfcs, > &st->internal->nb_bsfcs, bsfc))) { >>> - av_bsf_free(&bsfc); >>> - return ret; >>> - } >>> + st->internal->bsfc = bsfc; >>> >>> av_log(NULL, AV_LOG_VERBOSE, >>> "Automatically inserted bitstream filter '%s'; args='%s'\n", >>> >> LGTM otherwise. > > Will apply soon. Also ping for other patches in the series. Pushed. Regards, Marton
On 4/25/2020 5:14 AM, Marton Balint wrote: > > > On Sun, 19 Apr 2020, Andreas Rheinhardt wrote: > >> Marton Balint: >>> Current muxers only use a single bitstream filter, so there is no >>> need to >>> maintain code which operates on a list of bitstream filters. When >>> multiple >>> bitstream filters are needed muxers can simply use a list bitstream >>> filter. >>> >>> If there is a use case in the future when different bitstream filters >>> should be >>> added at subsequent packets then a new API possibly involving >>> reconfiguring the >>> list bistream filter can be added knowing the exact requirements. >>> >>> Signed-off-by: Marton Balint <cus@passwd.hu> >>> --- >>> libavformat/dashenc.c | 6 ++---- >>> libavformat/internal.h | 5 ++--- >>> libavformat/mux.c | 6 +++--- >>> libavformat/segment.c | 6 ++---- >>> libavformat/utils.c | 27 +++++++++------------------ >>> 5 files changed, 18 insertions(+), 32 deletions(-) >>> >>> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c >>> index 5a8cff4034..b977761a00 100644 >>> --- a/libavformat/dashenc.c >>> +++ b/libavformat/dashenc.c >>> @@ -2307,10 +2307,8 @@ static int dash_check_bitstream(struct >>> AVFormatContext *s, const AVPacket *avpkt >>> if (ret == 1) { >>> AVStream *st = s->streams[avpkt->stream_index]; >>> AVStream *ost = oc->streams[0]; >>> - st->internal->bsfcs = ost->internal->bsfcs; >>> - st->internal->nb_bsfcs = ost->internal->nb_bsfcs; >>> - ost->internal->bsfcs = NULL; >>> - ost->internal->nb_bsfcs = 0; >>> + st->internal->bsfc = ost->internal->bsfc; >>> + ost->internal->bsfc = NULL; >>> } >>> return ret; >>> } >>> diff --git a/libavformat/internal.h b/libavformat/internal.h >>> index 7e4284b217..cafb4a9686 100644 >>> --- a/libavformat/internal.h >>> +++ b/libavformat/internal.h >>> @@ -152,12 +152,11 @@ struct AVStreamInternal { >>> int reorder; >>> >>> /** >>> - * bitstream filters to run on stream >>> + * bitstream filter to run on stream >>> * - encoding: Set by muxer using ff_stream_add_bitstream_filter >>> * - decoding: unused >>> */ >>> - AVBSFContext **bsfcs; >>> - int nb_bsfcs; >>> + AVBSFContext *bsfc; >>> >>> /** >>> * Whether or not check_bitstream should still be run on each >>> packet >>> diff --git a/libavformat/mux.c b/libavformat/mux.c >>> index 3d63d59faf..5209c84f40 100644 >>> --- a/libavformat/mux.c >>> +++ b/libavformat/mux.c >>> @@ -824,7 +824,7 @@ static int prepare_input_packet(AVFormatContext >>> *s, AVPacket *pkt) >>> >>> static int do_packet_auto_bsf(AVFormatContext *s, AVPacket *pkt) { >>> AVStream *st = s->streams[pkt->stream_index]; >>> - int i, ret; >>> + int ret; >>> >>> if (!(s->flags & AVFMT_FLAG_AUTO_BSF)) >>> return 1; >>> @@ -838,8 +838,8 @@ static int do_packet_auto_bsf(AVFormatContext *s, >>> AVPacket *pkt) { >>> } >>> } >>> >>> - for (i = 0; i < st->internal->nb_bsfcs; i++) { >>> - AVBSFContext *ctx = st->internal->bsfcs[i]; >>> + if (st->internal->bsfc) { >>> + AVBSFContext *ctx = st->internal->bsfc; >>> // TODO: when any bitstream filter requires flushing at EOF, >>> we'll need to >>> // flush each stream's BSF chain on write_trailer. >>> if ((ret = av_bsf_send_packet(ctx, pkt)) < 0) { >>> diff --git a/libavformat/segment.c b/libavformat/segment.c >>> index 60b72b7d15..32c09827eb 100644 >>> --- a/libavformat/segment.c >>> +++ b/libavformat/segment.c >>> @@ -1034,10 +1034,8 @@ static int seg_check_bitstream(struct >>> AVFormatContext *s, const AVPacket *pkt) >>> if (ret == 1) { >>> AVStream *st = s->streams[pkt->stream_index]; >>> AVStream *ost = oc->streams[pkt->stream_index]; >>> - st->internal->bsfcs = ost->internal->bsfcs; >>> - st->internal->nb_bsfcs = ost->internal->nb_bsfcs; >>> - ost->internal->bsfcs = NULL; >>> - ost->internal->nb_bsfcs = 0; >>> + st->internal->bsfc = ost->internal->bsfc; >>> + ost->internal->bsfc = NULL; >>> } >>> return ret; >>> } >>> diff --git a/libavformat/utils.c b/libavformat/utils.c >>> index a58e47fabc..eff73252ec 100644 >>> --- a/libavformat/utils.c >>> +++ b/libavformat/utils.c >>> @@ -4410,10 +4410,7 @@ static void free_stream(AVStream **pst) >>> >>> if (st->internal) { >>> avcodec_free_context(&st->internal->avctx); >>> - for (i = 0; i < st->internal->nb_bsfcs; i++) { >>> - av_bsf_free(&st->internal->bsfcs[i]); >>> - av_freep(&st->internal->bsfcs); >> >> I can't believe it! This only works if there is only one bsf. So a real >> list has indeed never been tested. >> >>> - } >>> + av_bsf_free(&st->internal->bsfc); >>> av_freep(&st->internal->priv_pts); >>> av_bsf_free(&st->internal->extract_extradata.bsf); >>> av_packet_free(&st->internal->extract_extradata.pkt); >>> @@ -5574,7 +5571,11 @@ int ff_stream_add_bitstream_filter(AVStream >>> *st, const char *name, const char *a >>> int ret; >>> const AVBitStreamFilter *bsf; >>> AVBSFContext *bsfc; >>> - AVCodecParameters *in_par; >>> + >>> + if (st->internal->bsfc) { >>> + av_log(NULL, AV_LOG_ERROR, "A bitstream filter is already >>> specified for stream %d\n", st->index); >>> + return AVERROR(EINVAL); >>> + } >> >> Given that this is a situation which should not happen at all (until we >> really have a situation where one wants to add more than one bsf, in >> which case further changes to this function are required anyway), an >> assert seems more appropriate. > > OK, changed locally. > >> >>> >>> if (!(bsf = av_bsf_get_by_name(name))) { >>> av_log(NULL, AV_LOG_ERROR, "Unknown bitstream filter >>> '%s'\n", name); >>> @@ -5584,15 +5585,8 @@ int ff_stream_add_bitstream_filter(AVStream >>> *st, const char *name, const char *a >>> if ((ret = av_bsf_alloc(bsf, &bsfc)) < 0) >>> return ret; >>> >>> - if (st->internal->nb_bsfcs) { >>> - in_par = st->internal->bsfcs[st->internal->nb_bsfcs - >>> 1]->par_out; >>> - bsfc->time_base_in = >>> st->internal->bsfcs[st->internal->nb_bsfcs - 1]->time_base_out; >>> - } else { >>> - in_par = st->codecpar; >>> - bsfc->time_base_in = st->time_base; >>> - } >>> - >>> - if ((ret = avcodec_parameters_copy(bsfc->par_in, in_par)) < 0) { >>> + bsfc->time_base_in = st->time_base; >>> + if ((ret = avcodec_parameters_copy(bsfc->par_in, st->codecpar)) >>> < 0) { >>> av_bsf_free(&bsfc); >>> return ret; >>> } >>> @@ -5615,10 +5609,7 @@ int ff_stream_add_bitstream_filter(AVStream >>> *st, const char *name, const char *a >>> return ret; >>> } >>> >>> - if ((ret = av_dynarray_add_nofree(&st->internal->bsfcs, >>> &st->internal->nb_bsfcs, bsfc))) { >>> - av_bsf_free(&bsfc); >>> - return ret; >>> - } >>> + st->internal->bsfc = bsfc; >>> >>> av_log(NULL, AV_LOG_VERBOSE, >>> "Automatically inserted bitstream filter '%s'; args='%s'\n", >>> >> LGTM otherwise. > > Will apply soon. Also ping for other patches in the series. If you send a v3 of what's left of this set, please do it in a new thread and not as replies to this one.
diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c index 5a8cff4034..b977761a00 100644 --- a/libavformat/dashenc.c +++ b/libavformat/dashenc.c @@ -2307,10 +2307,8 @@ static int dash_check_bitstream(struct AVFormatContext *s, const AVPacket *avpkt if (ret == 1) { AVStream *st = s->streams[avpkt->stream_index]; AVStream *ost = oc->streams[0]; - st->internal->bsfcs = ost->internal->bsfcs; - st->internal->nb_bsfcs = ost->internal->nb_bsfcs; - ost->internal->bsfcs = NULL; - ost->internal->nb_bsfcs = 0; + st->internal->bsfc = ost->internal->bsfc; + ost->internal->bsfc = NULL; } return ret; } diff --git a/libavformat/internal.h b/libavformat/internal.h index 7e4284b217..cafb4a9686 100644 --- a/libavformat/internal.h +++ b/libavformat/internal.h @@ -152,12 +152,11 @@ struct AVStreamInternal { int reorder; /** - * bitstream filters to run on stream + * bitstream filter to run on stream * - encoding: Set by muxer using ff_stream_add_bitstream_filter * - decoding: unused */ - AVBSFContext **bsfcs; - int nb_bsfcs; + AVBSFContext *bsfc; /** * Whether or not check_bitstream should still be run on each packet diff --git a/libavformat/mux.c b/libavformat/mux.c index 3d63d59faf..5209c84f40 100644 --- a/libavformat/mux.c +++ b/libavformat/mux.c @@ -824,7 +824,7 @@ static int prepare_input_packet(AVFormatContext *s, AVPacket *pkt) static int do_packet_auto_bsf(AVFormatContext *s, AVPacket *pkt) { AVStream *st = s->streams[pkt->stream_index]; - int i, ret; + int ret; if (!(s->flags & AVFMT_FLAG_AUTO_BSF)) return 1; @@ -838,8 +838,8 @@ static int do_packet_auto_bsf(AVFormatContext *s, AVPacket *pkt) { } } - for (i = 0; i < st->internal->nb_bsfcs; i++) { - AVBSFContext *ctx = st->internal->bsfcs[i]; + if (st->internal->bsfc) { + AVBSFContext *ctx = st->internal->bsfc; // TODO: when any bitstream filter requires flushing at EOF, we'll need to // flush each stream's BSF chain on write_trailer. if ((ret = av_bsf_send_packet(ctx, pkt)) < 0) { diff --git a/libavformat/segment.c b/libavformat/segment.c index 60b72b7d15..32c09827eb 100644 --- a/libavformat/segment.c +++ b/libavformat/segment.c @@ -1034,10 +1034,8 @@ static int seg_check_bitstream(struct AVFormatContext *s, const AVPacket *pkt) if (ret == 1) { AVStream *st = s->streams[pkt->stream_index]; AVStream *ost = oc->streams[pkt->stream_index]; - st->internal->bsfcs = ost->internal->bsfcs; - st->internal->nb_bsfcs = ost->internal->nb_bsfcs; - ost->internal->bsfcs = NULL; - ost->internal->nb_bsfcs = 0; + st->internal->bsfc = ost->internal->bsfc; + ost->internal->bsfc = NULL; } return ret; } diff --git a/libavformat/utils.c b/libavformat/utils.c index a58e47fabc..eff73252ec 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -4410,10 +4410,7 @@ static void free_stream(AVStream **pst) if (st->internal) { avcodec_free_context(&st->internal->avctx); - for (i = 0; i < st->internal->nb_bsfcs; i++) { - av_bsf_free(&st->internal->bsfcs[i]); - av_freep(&st->internal->bsfcs); - } + av_bsf_free(&st->internal->bsfc); av_freep(&st->internal->priv_pts); av_bsf_free(&st->internal->extract_extradata.bsf); av_packet_free(&st->internal->extract_extradata.pkt); @@ -5574,7 +5571,11 @@ int ff_stream_add_bitstream_filter(AVStream *st, const char *name, const char *a int ret; const AVBitStreamFilter *bsf; AVBSFContext *bsfc; - AVCodecParameters *in_par; + + if (st->internal->bsfc) { + av_log(NULL, AV_LOG_ERROR, "A bitstream filter is already specified for stream %d\n", st->index); + return AVERROR(EINVAL); + } if (!(bsf = av_bsf_get_by_name(name))) { av_log(NULL, AV_LOG_ERROR, "Unknown bitstream filter '%s'\n", name); @@ -5584,15 +5585,8 @@ int ff_stream_add_bitstream_filter(AVStream *st, const char *name, const char *a if ((ret = av_bsf_alloc(bsf, &bsfc)) < 0) return ret; - if (st->internal->nb_bsfcs) { - in_par = st->internal->bsfcs[st->internal->nb_bsfcs - 1]->par_out; - bsfc->time_base_in = st->internal->bsfcs[st->internal->nb_bsfcs - 1]->time_base_out; - } else { - in_par = st->codecpar; - bsfc->time_base_in = st->time_base; - } - - if ((ret = avcodec_parameters_copy(bsfc->par_in, in_par)) < 0) { + bsfc->time_base_in = st->time_base; + if ((ret = avcodec_parameters_copy(bsfc->par_in, st->codecpar)) < 0) { av_bsf_free(&bsfc); return ret; } @@ -5615,10 +5609,7 @@ int ff_stream_add_bitstream_filter(AVStream *st, const char *name, const char *a return ret; } - if ((ret = av_dynarray_add_nofree(&st->internal->bsfcs, &st->internal->nb_bsfcs, bsfc))) { - av_bsf_free(&bsfc); - return ret; - } + st->internal->bsfc = bsfc; av_log(NULL, AV_LOG_VERBOSE, "Automatically inserted bitstream filter '%s'; args='%s'\n",
Current muxers only use a single bitstream filter, so there is no need to maintain code which operates on a list of bitstream filters. When multiple bitstream filters are needed muxers can simply use a list bitstream filter. If there is a use case in the future when different bitstream filters should be added at subsequent packets then a new API possibly involving reconfiguring the list bistream filter can be added knowing the exact requirements. Signed-off-by: Marton Balint <cus@passwd.hu> --- libavformat/dashenc.c | 6 ++---- libavformat/internal.h | 5 ++--- libavformat/mux.c | 6 +++--- libavformat/segment.c | 6 ++---- libavformat/utils.c | 27 +++++++++------------------ 5 files changed, 18 insertions(+), 32 deletions(-)