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

[Andreas Rheinhardt]

Marton Balint:
> 
> 
> On Thu, 9 Apr 2020, Andreas Rheinhardt wrote:
> 
>> 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().
> 
> Agreed, seems like a bug of the old code.
> 
>>
>>> -            }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.
> 
> I don't see an issue, bsf 0 will be drained on a subsequent call to
> bsf_list_filter.
> 
>> 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.)
> 
> How else could it work? You call av_bsf_receive_packet() until you get
> AVERROR(EAGAIN) or AVERROR_EOF, as the doc of av_bsf_receive_packet()
> says. If you get an other error, it is up to you to decice, either
> ignore it and retry draining or report it to the user (it seems we only
> do this if AV_EF_EXPLODE is set).

I wanted to stress this point because your earlier code [1] was
different than what I have just sketched. It did not completely drain
the bsf list on errors and therefore had the possibility to completely
ignore a packet: Consider the following scenario: One has a list of two
bsfs and the first packet can create multiple output packets out of the
first input packets. Imagine the second bsf returns an error when it is
fed the first of these. Then write_packets_common() would exit without
having drained the first filter (if it returns an error or not depends
upon AV_EF_EXPLODE). When the caller sends the next packet,
auto_bsf_receive_packet() would first try to drain the first bsf. If the
second bsf now errors out again, write_packets_common() returns without
having tried to send the new packet.

> 
>>
>> (The code in ffmpeg.c is buggy in this regard.)
> 
> I see one problem only, that you can't differentiate between a permanent
> and a temporary av_bsf_receive_packet() error, so a badly written bsf
> can get you and infinite loop by its filter call always returning an
> error, if you decide to ignore BSF errors.
> 
Yes. Hopefully we don't have such rogue bitstream filters.

- Andreas

PS: I forgot to mention something in the first answer: Good job at
simplifying this function.

[1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2020-March/259204.html

>>
>>>
>>> +        /* 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.)
> 
> out might be a packet coming from the user calling
> av_bsf_receive_packet(), and yes, according to the docs it *should* be a
> clean packet (what does *should* mean here anyway?), but if it is not,
> then all hell breaks loose. So it feels safer to me to simply pass NULL,
> instead of a supposadely clean packet.
> 
> Regards,
> Marton
> 
>>
>> - 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)
>>>
>>