[FFmpeg-devel] [PATCH 1/2] avcodec/bsf: use simplified algorithm for bsf_list chained filtering
Marton Balint
cus at passwd.hu
Fri Apr 10 00:06:12 EEST 2020
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).
>
> (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.
>
>>
>> + /* 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