| Message ID | 20190519193557.25877-1-cus@passwd.hu |
|---|---|
| State | New |
| Headers | show |
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,
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
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,
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?
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
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".
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
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
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".
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
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
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
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".
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
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;
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(-)