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

[Marton Balint]

On Fri, 17 Apr 2020, Marton Balint wrote:

>
>
> On Fri, 10 Apr 2020, Andreas Rheinhardt wrote:
>
>> 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.
>
> I plan to apply this patch soon.

Pushed.

Regards,
Marton