[FFmpeg-devel] [PATCH 1/2] avcodec/bsf: use simplified algorithm for bsf_list chained filtering
Marton Balint
cus at passwd.hu
Fri Apr 17 01:19:46 EEST 2020
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.
Regards,
Marton
>
> [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)
>>>>
>>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
More information about the ffmpeg-devel
mailing list