[FFmpeg-devel] [PATCH 1/2] avcodec/bsf: use simplified algorithm for bsf_list chained filtering

[Andreas Rheinhardt]

Marton Balint:
> Based on the one in ffmpeg.c and it is not using an extra flush_idx variable.
> 
> Signed-off-by: Marton Balint <cus at passwd.hu>
> ---
>  libavcodec/bsf.c | 64 ++++++++++++++++++++++----------------------------------
>  1 file changed, 25 insertions(+), 39 deletions(-)
> 
> diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c
> index 7b96183e64..b9fc771a88 100644
> --- a/libavcodec/bsf.c
> +++ b/libavcodec/bsf.c
> @@ -18,6 +18,7 @@
>  
>  #include <string.h>
>  
> +#include "libavutil/avassert.h"
>  #include "libavutil/log.h"
>  #include "libavutil/mem.h"
>  #include "libavutil/opt.h"
> @@ -266,7 +267,6 @@ typedef struct BSFListContext {
>      int nb_bsfs;
>  
>      unsigned idx;           // index of currently processed BSF
> -    unsigned flushed_idx;   // index of BSF being flushed
>  
>      char * item_name;
>  } BSFListContext;
> @@ -304,57 +304,43 @@ fail:
>  static int bsf_list_filter(AVBSFContext *bsf, AVPacket *out)
>  {
>      BSFListContext *lst = bsf->priv_data;
> -    int ret;
> +    int ret, eof = 0;
>  
>      if (!lst->nb_bsfs)
>          return ff_bsf_get_packet_ref(bsf, out);
>  
>      while (1) {
> -        if (lst->idx > lst->flushed_idx) {
> +        /* get a packet from the previous filter up the chain */
> +        if (lst->idx)
>              ret = av_bsf_receive_packet(lst->bsfs[lst->idx-1], out);
> -            if (ret == AVERROR(EAGAIN)) {
> -                /* no more packets from idx-1, try with previous */
> -                lst->idx--;
> -                continue;
> -            } else if (ret == AVERROR_EOF) {
> -                /* filter idx-1 is done, continue with idx...nb_bsfs */
> -                lst->flushed_idx = lst->idx;
> -                continue;

Is it just me or did this continue make no sense at all? It claims to
continue with idx...nb_bsf, yet it actually tried to get a new packet
via ff_bsf_get_packet_ref().

> -            }else if (ret < 0) {
> -                /* filtering error */
> -                break;
> -            }
> -        } else {
> +        else
>              ret = ff_bsf_get_packet_ref(bsf, out);
> -            if (ret == AVERROR_EOF) {
> -                lst->idx = lst->flushed_idx;
> -            } else if (ret < 0)
> -                break;
> -        }
> +        if (ret == AVERROR(EAGAIN)) {
> +            if (!lst->idx)
> +                return ret;
> +            lst->idx--;
> +            continue;
> +        } else if (ret == AVERROR_EOF) {
> +            eof = 1;
> +        } else if (ret < 0)
> +            return ret;

If you return here, the chain may not be completely drained (e.g. bsf 0
may still have packets ready to be output even if bsf 1 returned an
error while filtering a packet obtained from bsf 0) and despite this
error, the caller is responsible (according to the doc of
av_bsf_send/receive_packet) to drain the chain completely before sending
new packets.
In particular, if you use this in mux.c, you will have to keep calling
av_bsf_receive_packet() until the chain is drained even when the bsf
returns an error or when a write error occurs and you have to do this
even on AV_EF_EXPLODE. (Otherwise there might be a situation where the
next packet to be written can't even be sent to the chain because it is
not drained.)

(The code in ffmpeg.c is buggy in this regard.)

>  
> +        /* send it to the next filter down the chain */
>          if (lst->idx < lst->nb_bsfs) {
> -            AVPacket *pkt;
> -            if (ret == AVERROR_EOF && lst->idx == lst->flushed_idx) {
> -                /* ff_bsf_get_packet_ref returned EOF and idx is first
> -                 * filter of yet not flushed filter chain */
> -                pkt = NULL;
> -            } else {
> -                pkt = out;
> +            ret = av_bsf_send_packet(lst->bsfs[lst->idx], eof ? NULL : out);

The BSF API treats a blank packet (no data, no side_data) as indicating
eof, so you can simply always send out. (More on this later.)

- Andreas

> +            av_assert1(ret != AVERROR(EAGAIN));
> +            if (ret < 0) {
> +                av_packet_unref(out);
> +                return ret;
>              }
> -            ret = av_bsf_send_packet(lst->bsfs[lst->idx], pkt);
> -            if (ret < 0)
> -                break;
>              lst->idx++;
> +            eof = 0;
> +        } else if (eof) {
> +            return ret;
>          } else {
> -            /* The end of filter chain, break to return result */
> -            break;
> +            return 0;
>          }
>      }
> -
> -    if (ret < 0)
> -        av_packet_unref(out);
> -
> -    return ret;
>  }
>  
>  static void bsf_list_flush(AVBSFContext *bsf)
> @@ -363,7 +349,7 @@ static void bsf_list_flush(AVBSFContext *bsf)
>  
>      for (int i = 0; i < lst->nb_bsfs; i++)
>          av_bsf_flush(lst->bsfs[i]);
> -    lst->idx = lst->flushed_idx = 0;
> +    lst->idx = 0;
>  }
>  
>  static void bsf_list_close(AVBSFContext *bsf)
>