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

Submitted by Marton Balint on May 19, 2019, 7:35 p.m.

Details

Message ID 20190519193557.25877-1-cus@passwd.hu
State New
Headers show

Commit Message

Marton Balint May 19, 2019, 7:35 p.m.
Fixes infinte loop with -vf loop=loop=1.

Possible regression since ef1aadffc785b48ed62c45d954289e754f43ef46.

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavfilter/f_loop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Nicolas George May 19, 2019, 7:37 p.m.
Marton Balint (12019-05-19):
> Fixes infinte loop with -vf loop=loop=1.

The subject line talks about loop=0, this line about loop=1. Typo
somewhere, or am I missing something?

Regards,
Marton Balint May 19, 2019, 7:51 p.m.
On Sun, 19 May 2019, Nicolas George wrote:

> Marton Balint (12019-05-19):
>> Fixes infinte loop with -vf loop=loop=1.
>
> The subject line talks about loop=0, this line about loop=1. Typo
> somewhere, or am I missing something?

loop=1 is the loop count (number of loops), not the loop size (number of 
frames to loop) which is also 0 by default. Issue is present with any 
nonzero loop count, 1 is just an example.

Regards,
Marton
Nicolas George May 19, 2019, 7:52 p.m.
Marton Balint (12019-05-19):
> loop=1 is the loop count (number of loops), not the loop size (number of
> frames to loop) which is also 0 by default. Issue is present with any
> nonzero loop count, 1 is just an example.

Ok, thanks for explaining.

Regards,
Paul B Mahol May 19, 2019, 7:54 p.m.
On 5/19/19, Marton Balint <cus@passwd.hu> wrote:
> Fixes infinte loop with -vf loop=loop=1.
>
> Possible regression since ef1aadffc785b48ed62c45d954289e754f43ef46.
>
> Signed-off-by: Marton Balint <cus@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;
> --
> 2.16.4
>
> _______________________________________________
> 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".

I think better fix is to change default and minimal allowed loop size to 1.
Does that sounds ok to you?
Marton Balint May 19, 2019, 8:09 p.m.
On Sun, 19 May 2019, Paul B Mahol wrote:

> On 5/19/19, Marton Balint <cus@passwd.hu> wrote:
>> Fixes infinte loop with -vf loop=loop=1.
>>
>> Possible regression since ef1aadffc785b48ed62c45d954289e754f43ef46.
>>
>> Signed-off-by: Marton Balint <cus@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;
>> --
>> 2.16.4
>>
>> _______________________________________________
>> 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".
>
> 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.

Regards,
Marton
Paul B Mahol May 19, 2019, 8:23 p.m.
On 5/19/19, Marton Balint <cus@passwd.hu> wrote:
>
>
> On Sun, 19 May 2019, Paul B Mahol wrote:
>
>> On 5/19/19, Marton Balint <cus@passwd.hu> wrote:
>>> Fixes infinte loop with -vf loop=loop=1.
>>>
>>> Possible regression since ef1aadffc785b48ed62c45d954289e754f43ef46.
>>>
>>> Signed-off-by: Marton Balint <cus@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;
>>> --
>>> 2.16.4
>>>
>>> _______________________________________________
>>> 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".
>>
>> 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.

>
> Regards,
> Marton
> _______________________________________________
> 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 May 19, 2019, 8:48 p.m.
On Sun, 19 May 2019, Paul B Mahol wrote:

> On 5/19/19, Marton Balint <cus@passwd.hu> wrote:
>>
>>
>> On Sun, 19 May 2019, Paul B Mahol wrote:
>>
>>> On 5/19/19, Marton Balint <cus@passwd.hu> wrote:
>>>> Fixes infinte loop with -vf loop=loop=1.
>>>>
>>>> Possible regression since ef1aadffc785b48ed62c45d954289e754f43ef46.
>>>>
>>>> Signed-off-by: Marton Balint <cus@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;
>>>> --
>>>> 2.16.4
>>>>
>>>> _______________________________________________
>>>> 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".
>>>
>>> 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.

Regards,
Marton
Gyan May 20, 2019, 5:02 a.m.
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@passwd.hu> wrote:
>>>
>>>
>>> On Sun, 19 May 2019, Paul B Mahol wrote:
>>>
>>>> On 5/19/19, Marton Balint <cus@passwd.hu> wrote:
>>>>> Fixes infinte loop with -vf loop=loop=1.
>>>>>
>>>>> Possible regression since ef1aadffc785b48ed62c45d954289e754f43ef46.
>>>>>
>>>>> Signed-off-by: Marton Balint <cus@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;
>>>>> -- 
>>>>> 2.16.4
>>>>>
>>>>> _______________________________________________
>>>>> 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".
>>>>
>>>> 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.

Gyan
Bodecs Bela May 20, 2019, 8:58 a.m.
2019.05.20. 7:02 keltezéssel, Gyan írta:
>
>
> 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@passwd.hu> wrote:
>>>>
>>>>
>>>> On Sun, 19 May 2019, Paul B Mahol wrote:
>>>>
>>>>> On 5/19/19, Marton Balint <cus@passwd.hu> wrote:
>>>>>> Fixes infinte loop with -vf loop=loop=1.
>>>>>>
>>>>>> Possible regression since ef1aadffc785b48ed62c45d954289e754f43ef46.
>>>>>>
>>>>>> Signed-off-by: Marton Balint <cus@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;
>>>>>> -- 
>>>>>> 2.16.4
>>>>>>
>>>>>> _______________________________________________
>>>>>> 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".
>>>>>
>>>>> 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.
>
just an idea. What about to have no-default value for loop length/size 
at all? I mean to let the size as mandatory parameter?

bb


> Gyan
> _______________________________________________
> 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 May 20, 2019, 6:51 p.m.
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@passwd.hu> wrote:
>>>>
>>>>
>>>> On Sun, 19 May 2019, Paul B Mahol wrote:
>>>>
>>>>> On 5/19/19, Marton Balint <cus@passwd.hu> wrote:
>>>>>> Fixes infinte loop with -vf loop=loop=1.
>>>>>>
>>>>>> Possible regression since ef1aadffc785b48ed62c45d954289e754f43ef46.
>>>>>>
>>>>>> Signed-off-by: Marton Balint <cus@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;
>>>>>> -- 
>>>>>> 2.16.4
>>>>>>
>>>>>> _______________________________________________
>>>>>> 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".
>>>>>
>>>>> 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.

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.

Regards,
Marton
Alexander Strasser May 22, 2019, 12:46 a.m.
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@passwd.hu> wrote:
> > > > >
> > > > > On Sun, 19 May 2019, Paul B Mahol wrote:
> > > > >
> > > > > > On 5/19/19, Marton Balint <cus@passwd.hu> wrote:
> > > > > > > Fixes infinte loop with -vf loop=loop=1.
> > > > > > >
> > > > > > > Possible regression since ef1aadffc785b48ed62c45d954289e754f43ef46.
> > > > > > >
> > > > > > > Signed-off-by: Marton Balint <cus@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.


  Alexander
Marton Balint May 23, 2019, 7:26 p.m.
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@passwd.hu> wrote:
>> > > > >
>> > > > > On Sun, 19 May 2019, Paul B Mahol wrote:
>> > > > >
>> > > > > > On 5/19/19, Marton Balint <cus@passwd.hu> wrote:
>> > > > > > > Fixes infinte loop with -vf loop=loop=1.
>> > > > > > >
>> > > > > > > Possible regression since ef1aadffc785b48ed62c45d954289e754f43ef46.
>> > > > > > >
>> > > > > > > Signed-off-by: Marton Balint <cus@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.

Thanks,
Marton
Paul B Mahol May 23, 2019, 7:39 p.m.
On 5/23/19, Marton Balint <cus@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@passwd.hu> wrote:
>>> > > > >
>>> > > > > On Sun, 19 May 2019, Paul B Mahol wrote:
>>> > > > >
>>> > > > > > On 5/19/19, Marton Balint <cus@passwd.hu> wrote:
>>> > > > > > > Fixes infinte loop with -vf loop=loop=1.
>>> > > > > > >
>>> > > > > > > Possible regression since
>>> > > > > > > ef1aadffc785b48ed62c45d954289e754f43ef46.
>>> > > > > > >
>>> > > > > > > Signed-off-by: Marton Balint <cus@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.

>
> Thanks,
> Marton
> _______________________________________________
> 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 May 23, 2019, 9:18 p.m.
On Thu, 23 May 2019, Paul B Mahol wrote:

> On 5/23/19, Marton Balint <cus@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@passwd.hu> wrote:
>>>> > > > >
>>>> > > > > On Sun, 19 May 2019, Paul B Mahol wrote:
>>>> > > > >
>>>> > > > > > On 5/19/19, Marton Balint <cus@passwd.hu> wrote:
>>>> > > > > > > Fixes infinte loop with -vf loop=loop=1.
>>>> > > > > > >
>>>> > > > > > > Possible regression since
>>>> > > > > > > ef1aadffc785b48ed62c45d954289e754f43ef46.
>>>> > > > > > >
>>>> > > > > > > Signed-off-by: Marton Balint <cus@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

Patch hide | download patch | download mbox

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;