| Message ID | 20200408224231.23126-1-cus@passwd.hu |
|---|---|
| State | Accepted |
| Commit | 7d8944547333dd158b8d5e76036422502215a332 |
| Headers | show |
| Series | [FFmpeg-devel,1/2] avcodec/bsf: use simplified algorithm for bsf_list chained filtering | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
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@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) >
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@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@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
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@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) >>> >>
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@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@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
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@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
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; - }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; + /* 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); + 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)
Based on the one in ffmpeg.c and it is not using an extra flush_idx variable. Signed-off-by: Marton Balint <cus@passwd.hu> --- libavcodec/bsf.c | 64 ++++++++++++++++++++++---------------------------------- 1 file changed, 25 insertions(+), 39 deletions(-)