[FFmpeg-devel,5/8] avformat/mux: add proper support for full N:M bitstream filtering

Previously only 1:1 bitstream filters were supported, the end of the stream was
not signalled to the bitstream filters and time base changes were ignored.

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavformat/internal.h |   1 +
 libavformat/mux.c      | 128 ++++++++++++++++++++++++++++++++++---------------
 2 files changed, 91 insertions(+), 38 deletions(-)

Marton Balint:
> Previously only 1:1 bitstream filters were supported, the end of the stream was
> not signalled to the bitstream filters and time base changes were ignored.
> 
> Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
>  libavformat/internal.h |   1 +
>  libavformat/mux.c      | 128 ++++++++++++++++++++++++++++++++++---------------
>  2 files changed, 91 insertions(+), 38 deletions(-)
> 
> diff --git a/libavformat/internal.h b/libavformat/internal.h
> index 332477a532..45aeef717a 100644
> --- a/libavformat/internal.h
> +++ b/libavformat/internal.h
> @@ -158,6 +158,7 @@ struct AVStreamInternal {
>       */
>      AVBSFContext **bsfcs;
>      int nb_bsfcs;
> +    int bsfcs_idx;
>  
>      /**
>       * Whether or not check_bitstream should still be run on each packet
> diff --git a/libavformat/mux.c b/libavformat/mux.c
> index 8c2d6a8060..3054ab8644 100644
> --- a/libavformat/mux.c
> +++ b/libavformat/mux.c
> @@ -840,14 +840,48 @@ static int prepare_input_packet(AVFormatContext *s, AVPacket *pkt)
>      return 0;
>  }
>  
> -static int do_packet_auto_bsf(AVFormatContext *s, AVPacket *pkt) {
> -    AVStream *st = s->streams[pkt->stream_index];
> -    int i, ret;
> +static int auto_bsf_receive_packet(AVFormatContext *s, AVStream *st, AVPacket *pkt)
> +{
> +    AVStreamInternal *sti = st->internal;
> +    int ret = AVERROR(EAGAIN);
> +    int eof = 0;
> +
> +    while (sti->bsfcs_idx) {
> +        /* get a packet from the previous filter up the chain */
> +        ret = av_bsf_receive_packet(sti->bsfcs[sti->bsfcs_idx - 1], pkt);
> +        if (ret == AVERROR(EAGAIN)) {
> +            sti->bsfcs_idx--;
> +            continue;
> +        } else if (ret == AVERROR_EOF) {
> +            eof = 1;
> +        } else if (ret < 0)
> +            break;
> +
> +        /* send it to the next filter down the chain */
> +        if (sti->bsfcs_idx < sti->nb_bsfcs) {
> +            ret = av_bsf_send_packet(sti->bsfcs[sti->bsfcs_idx], eof ? NULL : pkt);
> +            av_assert2(ret != AVERROR(EAGAIN));
> +            if (ret < 0)
> +                break;
> +            sti->bsfcs_idx++;
> +            eof = 0;
> +        } else if (eof) {
> +            break;
> +        } else {
> +            return 0;
> +        }
> +    }

Would it actually be possible to simplify this by using the
av_bsf_list-API (right now code like this exists here and in ffmpeg.c
and probably at even more places)? The problem I see with this is that
one cannot send a packet for filtering to an unfinished bsf list whereas
the current code has this capability (although AFAIK no one uses it; if
I am not mistaken, all bsf chains that are automatically inserted
consist of exactly one bsf).

(This actually brings up a question: If check_bitstream returns that
more packets from this stream need to be checked, the current packet is
currently sent to the bsf-list as-is; it would not be sent to any bsf
that get added to the list later. Makes me wonder whether this is
actually a problem.)

- Andreas

Marton Balint:
> Previously only 1:1 bitstream filters were supported, the end of the stream was
> not signalled to the bitstream filters and time base changes were ignored.
> 
> Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
>  libavformat/internal.h |   1 +
>  libavformat/mux.c      | 128 ++++++++++++++++++++++++++++++++++---------------
>  2 files changed, 91 insertions(+), 38 deletions(-)
> 
> diff --git a/libavformat/internal.h b/libavformat/internal.h
> index 332477a532..45aeef717a 100644
> --- a/libavformat/internal.h
> +++ b/libavformat/internal.h
> @@ -158,6 +158,7 @@ struct AVStreamInternal {
>       */
>      AVBSFContext **bsfcs;
>      int nb_bsfcs;
> +    int bsfcs_idx;
>  
>      /**
>       * Whether or not check_bitstream should still be run on each packet
> diff --git a/libavformat/mux.c b/libavformat/mux.c
> index 8c2d6a8060..3054ab8644 100644
> --- a/libavformat/mux.c
> +++ b/libavformat/mux.c
> @@ -840,14 +840,48 @@ static int prepare_input_packet(AVFormatContext *s, AVPacket *pkt)
>      return 0;
>  }
>  
> -static int do_packet_auto_bsf(AVFormatContext *s, AVPacket *pkt) {
> -    AVStream *st = s->streams[pkt->stream_index];
> -    int i, ret;
> +static int auto_bsf_receive_packet(AVFormatContext *s, AVStream *st, AVPacket *pkt)
> +{
> +    AVStreamInternal *sti = st->internal;
> +    int ret = AVERROR(EAGAIN);
> +    int eof = 0;
> +
> +    while (sti->bsfcs_idx) {
> +        /* get a packet from the previous filter up the chain */
> +        ret = av_bsf_receive_packet(sti->bsfcs[sti->bsfcs_idx - 1], pkt);
> +        if (ret == AVERROR(EAGAIN)) {
> +            sti->bsfcs_idx--;
> +            continue;
> +        } else if (ret == AVERROR_EOF) {
> +            eof = 1;
> +        } else if (ret < 0)
> +            break;
> +
> +        /* send it to the next filter down the chain */
> +        if (sti->bsfcs_idx < sti->nb_bsfcs) {
> +            ret = av_bsf_send_packet(sti->bsfcs[sti->bsfcs_idx], eof ? NULL : pkt);
> +            av_assert2(ret != AVERROR(EAGAIN));
> +            if (ret < 0)
> +                break;
> +            sti->bsfcs_idx++;
> +            eof = 0;
> +        } else if (eof) {
> +            break;
> +        } else {
> +            return 0;
> +        }
> +    }
> +
> +    return ret;
> +}
> +
> +static int need_auto_bsf(AVFormatContext *s, AVStream *st, AVPacket *pkt) {

{ belongs into a line of its own.

> +    int ret;
>  
>      if (!(s->flags & AVFMT_FLAG_AUTO_BSF))
> -        return 1;
> +        return 0;
>  
> -    if (s->oformat->check_bitstream) {
> +    if (pkt && s->oformat->check_bitstream) {
>          if (!st->internal->bitstream_checked) {
>              if ((ret = s->oformat->check_bitstream(s, pkt)) < 0)
>                  return ret;
> @@ -856,31 +890,7 @@ 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];
> -        // 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) {
> -            av_log(ctx, AV_LOG_ERROR,
> -                    "Failed to send packet to filter %s for stream %d\n",
> -                    ctx->filter->name, pkt->stream_index);
> -            return ret;
> -        }
> -        // TODO: when any automatically-added bitstream filter is generating multiple
> -        // output packets for a single input one, we'll need to call this in a loop
> -        // and write each output packet.
> -        if ((ret = av_bsf_receive_packet(ctx, pkt)) < 0) {
> -            if (ret == AVERROR(EAGAIN) || ret == AVERROR_EOF)
> -                return 0;
> -            av_log(ctx, AV_LOG_ERROR,
> -                    "Failed to receive packet from filter %s for stream %d\n",
> -                    ctx->filter->name, pkt->stream_index);
> -            if (s->error_recognition & AV_EF_EXPLODE)
> -                return ret;
> -            return 0;
> -        }
> -    }
> -    return 1;
> +    return st->internal->nb_bsfcs;
>  }
>  
>  static int interleaved_write_packet(AVFormatContext *s, AVPacket *pkt, int flush);
> @@ -909,19 +919,54 @@ static int write_packet_common(AVFormatContext *s, AVStream *st, AVPacket *pkt,
>  
>  static int write_packets_common(AVFormatContext *s, AVStream *st, AVPacket *pkt, int interleaved)
>  {
> -    int ret = do_packet_auto_bsf(s, pkt);
> -    if (ret == 0)
> -        return 0;
> -    else if (ret < 0)
> +    AVStreamInternal *sti = st->internal;
> +    int ret;
> +
> +    ret = need_auto_bsf(s, st, pkt);
> +    if (ret < 0)
>          goto fail;
>  
> -    ret = write_packet_common(s, st, pkt, interleaved);
> +    if (ret) {
> +        AVPacket opkt = {0};
> +        int consumed_packet = 0;
> +
> +        do {
> +            ret = auto_bsf_receive_packet(s, st, &opkt);
> +            if (ret < 0) {
> +                if (ret == AVERROR(EAGAIN) && !consumed_packet) {
> +                    av_assert2(sti->bsfcs_idx == 0);
> +                    ret = av_bsf_send_packet(sti->bsfcs[0], pkt);
> +                    av_assert2(ret != AVERROR(EAGAIN));
> +                    if (ret >= 0) {
> +                        consumed_packet = 1;
> +                        sti->bsfcs_idx = 1;
> +                        continue;
> +                    }
> +                }
> +                if (ret == AVERROR(EAGAIN) || ret == AVERROR_EOF)
> +                    ret = 0;
> +                if (ret < 0) {
> +                    av_log(s, AV_LOG_ERROR, "Error applying bitstream filters to an output "
> +                           "packet for stream #%d: %s\n", st->index, av_err2str(ret));
> +                    if (!(s->error_recognition & AV_EF_EXPLODE))
> +                        ret = 0;
> +                }
> +                goto fail;
> +            }
> +            av_packet_rescale_ts(&opkt, sti->bsfcs[sti->nb_bsfcs-1]->time_base_out, st->time_base);
> +            ret = write_packet_common(s, st, &opkt, interleaved);
> +            if (!interleaved)
> +                av_packet_unref(&opkt);
> +        } while (ret >= 0);
> +    } else {
> +        ret = pkt ? write_packet_common(s, st, pkt, interleaved) : 0;
> +    }
>      if (ret < 0)
>          goto fail;
>      return ret;
>  
>  fail:
> -    if (interleaved)
> +    if (interleaved && pkt)
>          av_packet_unref(pkt);
>      return ret;
>  }
> @@ -1264,9 +1309,16 @@ fail:
>  
>  int av_write_trailer(AVFormatContext *s)
>  {
> -    int ret, i;
> +    int i, ret1, ret = 0;
>  
> -    ret = interleaved_write_packet(s, NULL, 1);
> +    for (i = 0; i < s->nb_streams; i++) {
> +        ret1 = write_packets_common(s, s->streams[i], NULL, 1/*interleaved*/);
> +        if (ret >= 0)
> +            ret = ret1;
> +    }
> +    ret1 = interleaved_write_packet(s, NULL, 1);
> +    if (ret >= 0)
> +        ret = ret1;
>  
>      if (s->oformat->write_trailer) {
>          if (!(s->oformat->flags & AVFMT_NOFILE) && s->pb)
> 
I'm not fully done yet, but I already suggest the following changes:

1. Factor out the bitstream filter stuff (I mean what is after the if
(ret) in write_packets_common()) into a separate function.
2. Don't call write_packets_common() from av_write_trailer(); instead
simply do if (s->streams[i]->internal->nb_bsfs) ret1 = the function from 1.

This will already make sure that write_packets_common() is only called
when there is a packet, so that all of these checks for whether there is
a packet can be removed. Furthermore, this allows to include
prepare_input_packet() in write_packets_common().

- Andreas

On Sat, 28 Mar 2020, Andreas Rheinhardt wrote:

> Marton Balint:
>> Previously only 1:1 bitstream filters were supported, the end of the stream was
>> not signalled to the bitstream filters and time base changes were ignored.
>> 
>> Signed-off-by: Marton Balint <cus@passwd.hu>
>> ---
>>  libavformat/internal.h |   1 +
>>  libavformat/mux.c      | 128 ++++++++++++++++++++++++++++++++++---------------
>>  2 files changed, 91 insertions(+), 38 deletions(-)
>> 
>> diff --git a/libavformat/internal.h b/libavformat/internal.h
>> index 332477a532..45aeef717a 100644
>> --- a/libavformat/internal.h
>> +++ b/libavformat/internal.h
>> @@ -158,6 +158,7 @@ struct AVStreamInternal {
>>       */
>>      AVBSFContext **bsfcs;
>>      int nb_bsfcs;
>> +    int bsfcs_idx;
>>
>>      /**
>>       * Whether or not check_bitstream should still be run on each packet
>> diff --git a/libavformat/mux.c b/libavformat/mux.c
>> index 8c2d6a8060..3054ab8644 100644
>> --- a/libavformat/mux.c
>> +++ b/libavformat/mux.c
>> @@ -840,14 +840,48 @@ static int prepare_input_packet(AVFormatContext *s, AVPacket *pkt)
>>      return 0;
>>  }
>> 
>> -static int do_packet_auto_bsf(AVFormatContext *s, AVPacket *pkt) {
>> -    AVStream *st = s->streams[pkt->stream_index];
>> -    int i, ret;
>> +static int auto_bsf_receive_packet(AVFormatContext *s, AVStream *st, AVPacket *pkt)
>> +{
>> +    AVStreamInternal *sti = st->internal;
>> +    int ret = AVERROR(EAGAIN);
>> +    int eof = 0;
>> +
>> +    while (sti->bsfcs_idx) {
>> +        /* get a packet from the previous filter up the chain */
>> +        ret = av_bsf_receive_packet(sti->bsfcs[sti->bsfcs_idx - 1], pkt);
>> +        if (ret == AVERROR(EAGAIN)) {
>> +            sti->bsfcs_idx--;
>> +            continue;
>> +        } else if (ret == AVERROR_EOF) {
>> +            eof = 1;
>> +        } else if (ret < 0)
>> +            break;
>> +
>> +        /* send it to the next filter down the chain */
>> +        if (sti->bsfcs_idx < sti->nb_bsfcs) {
>> +            ret = av_bsf_send_packet(sti->bsfcs[sti->bsfcs_idx], eof ? NULL : pkt);
>> +            av_assert2(ret != AVERROR(EAGAIN));
>> +            if (ret < 0)
>> +                break;
>> +            sti->bsfcs_idx++;
>> +            eof = 0;
>> +        } else if (eof) {
>> +            break;
>> +        } else {
>> +            return 0;
>> +        }
>> +    }
>
> Would it actually be possible to simplify this by using the
> av_bsf_list-API (right now code like this exists here and in ffmpeg.c
> and probably at even more places)? The problem I see with this is that
> one cannot send a packet for filtering to an unfinished bsf list whereas
> the current code has this capability (although AFAIK no one uses it; if
> I am not mistaken, all bsf chains that are automatically inserted
> consist of exactly one bsf).

I am not aware of practical uses of more than one bsf, but it *was* 
supported for a long time.

Yes, the main issue with the list_bsf-API is that it does not currently 
supports adding filters to already initialized chains. I wrapped up some 
new API for it which can be used to do exactly that:

+/**
+ * Join a new bitstream filter to an already operating one.
+ *
+ * @param[in,out] out_bsf *out_bsf contains the existing, already initialized bitstream
+ *                        filter, the new filter will be joined to the output of this.
+ *                        It can be NULL, in which case an empty filter is assumed.
+ *                        Upon successful return *out_bsf will contain the new, combined
+ *                        bitstream filter which will be initialized.
+ * @param[in]     bsf     the bitstream filter to be joined to the output of *out_bsf.
+ *                        This filter must be uninitialized but it must be ready to be
+ *                        initalized, so input codec parameters and time base must be
+ *                        set if *out_bsf is NULL, otherwise the function will set these
+ *                        automatically based on the output parameters of *out_bsf.
+ *                        Upon successful return the bsf will be initialized.
+ *
+ * @return 0 on success, negative on error.
+ */
+int av_bsf_join(AVBSFContext **out_bsf, AVBSFContext *bsf);
+

So this API basically creates a list BSF transparenly if somebody wants to 
concatenate two BSF-s. It is a bit fraglie because it is using list bfs 
internals directly, instead of calling the init() method of the list bsf, 
but it works.

If people prefer this option, I can continue on this path and simply 
provide patches on top of the existing series, it is quite 
straightforward to do the conversion.

>
> (This actually brings up a question: If check_bitstream returns that
> more packets from this stream need to be checked, the current packet is
> currently sent to the bsf-list as-is; it would not be sent to any bsf
> that get added to the list later. Makes me wonder whether this is
> actually a problem.)

I always thought that is how it is supposed to work by design. But since 
there are no use cases for that at the moment, we don't know if that is 
satisfactory...

Regards,
Marton