Hi! On 2019-05-20 20:51 +0200, Marton Balint wrote: > > On Mon, 20 May 2019, Gyan wrote: > > > On 20-05-2019 02:18 AM, Marton Balint wrote: > > > > > > On Sun, 19 May 2019, Paul B Mahol wrote: > > > > > > > On 5/19/19, Marton Balint <[email protected]> wrote: > > > > > > > > > > On Sun, 19 May 2019, Paul B Mahol wrote: > > > > > > > > > > > On 5/19/19, Marton Balint <[email protected]> wrote: > > > > > > > Fixes infinte loop with -vf loop=loop=1. > > > > > > > > > > > > > > Possible regression since > > > > > > > ef1aadffc785b48ed62c45d954289e754f43ef46. > > > > > > > > > > > > > > Signed-off-by: Marton Balint <[email protected]> > > > > > > > --- > > > > > > > libavfilter/f_loop.c | 2 +- > > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > > > diff --git a/libavfilter/f_loop.c b/libavfilter/f_loop.c > > > > > > > index d9d55f9837..3da753dd1e 100644 > > > > > > > --- a/libavfilter/f_loop.c > > > > > > > +++ b/libavfilter/f_loop.c > > > > > > > @@ -343,7 +343,7 @@ static int activate(AVFilterContext *ctx) > > > > > > > > > > > > > > FF_FILTER_FORWARD_STATUS_BACK(outlink, inlink); > > > > > > > > > > > > > > - if (!s->eof && (s->nb_frames < s->size || !s->loop)) { > > > > > > > + if (!s->eof && (s->nb_frames < s->size || > > > > > > > !s->loop || !s->size)) { > > > > > > > ret = ff_inlink_consume_frame(inlink, &frame); > > > > > > > if (ret < 0) > > > > > > > return ret; > > > > > > > -- > > > > > > > > > > > > I think better fix is to change default and minimal > > > > > > allowed loop size to > > > > > > 1. > > > > > > Does that sounds ok to you? > > > > > > > > > > Well, looping the whole length of the input would be more > > > > > intuitive to me > > > > > as the default. > > > > > > > > That would require infinite memory. > > > > > > So as the reverse filter. As long as it is properly documented that > > > the looped stuff is kept in memory so the user should not use this > > > for long clips, then I think it is fine. > > > > I disagree. Yes, for loop with only loop specified, it would be > > intuitive to loop the whole stream, but relying on users to exercise due > > diligence can't be counted upon. We're talking about a scenario where > > the user hasn't bothered to specify the size variable because they don't > > know or don't care or are sloppy. They won't take heed of the > > documentation until the command fails. The defaults should be robust > > against lax use. > > Fair enough, although I never liked the idea that we make the tool less > handy because we target unexperienced users.
FWIW, I guess the default behaviour of looping the complete input is much better from a user perspective. The typical users that have a need to loop a small clip will probably not want to spefify a size in frames and will probably not really understand why they need to specify one. The typical users that want to loop a particular number of frames, potentially at given offset into the specified input will probably read the manual and in turn quickly find and use the size and/or start options. > Anyway, I don't have strong feelings about this, maybe my patch has the > benefit of keeping existing behaviour (which is similar to how aloop works) > in contrast to what paul suggested, but I don't really mind Paul's or Bela's > solution either. I have no strong feelings either, but it seems the behaviour implemented by your patch seems ato fit more into the overall situation too. Alexander _______________________________________________ ffmpeg-devel mailing list [email protected] https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email [email protected] with subject "unsubscribe".
