[FFmpeg-devel] [PATCH v2 1/6] avformat: only allow a single bitstream filter when muxing

Marton Balint cus at passwd.hu
Sat Apr 25 11:14:04 EEST 2020



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 at 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


More information about the ffmpeg-devel mailing list