[FFmpeg-devel] [PATCH] avfilter/f_loop: do not loop if loop size is 0

[Marton Balint]

On Thu, 23 May 2019, Paul B Mahol wrote:

> On 5/23/19, Marton Balint <cus at passwd.hu> wrote:
>>
>>
>> On Wed, 22 May 2019, Alexander Strasser wrote:
>>
>>> 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 <cus at passwd.hu> wrote:
>>>> > > > >
>>>> > > > > On Sun, 19 May 2019, Paul B Mahol wrote:
>>>> > > > >
>>>> > > > > > On 5/19/19, Marton Balint <cus at passwd.hu> wrote:
>>>> > > > > > > Fixes infinte loop with -vf loop=loop=1.
>>>> > > > > > >
>>>> > > > > > > Possible regression since
>>>> > > > > > > ef1aadffc785b48ed62c45d954289e754f43ef46.
>>>> > > > > > >
>>>> > > > > > > Signed-off-by: Marton Balint <cus at passwd.hu>
>>>> > > > > > > ---
>>>> > > > > > >  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.
>>>
>>
>> Paul, let me know what you prefer.
>
> Initial patch is fine, as additional patch you could print warning if size is 0.

Will send a new one because I noticed another regression when converting 
to activate...

Regards.
Marton