| Message ID | 20171117203437.24145-1-onemda@gmail.com |
|---|---|
| State | New |
| Headers | show |
Le septidi 27 brumaire, an CCXXVI, Paul B Mahol a écrit : > Signed-off-by: Paul B Mahol <onemda@gmail.com> > --- > doc/filters.texi | 4 ++++ > libavfilter/vf_tile.c | 18 +++++++++++++++++- > 2 files changed, 21 insertions(+), 1 deletion(-) > > diff --git a/doc/filters.texi b/doc/filters.texi > index 5d99437871..7eeeafab8e 100644 > --- a/doc/filters.texi > +++ b/doc/filters.texi > @@ -14461,6 +14461,10 @@ is "black". > @item overlap > Set the number of frames to overlap when tiling several successive frames together. > The value must be between @code{0} and @var{nb_frames - 1}. > + > +@item queue > +Set the number of frames to initially queue when displaying first frame. > +The value must be between @code{0} and @var{nb_frames}. > @end table Queue where? To do what later, and when? I cannot determine what this option is supposed to do, even with a quick glance at the code, so I guess a normal user would be completely baffled. Regards,
On 11/19/17, Nicolas George <george@nsup.org> wrote: > Le septidi 27 brumaire, an CCXXVI, Paul B Mahol a ecrit : >> Signed-off-by: Paul B Mahol <onemda@gmail.com> >> --- >> doc/filters.texi | 4 ++++ >> libavfilter/vf_tile.c | 18 +++++++++++++++++- >> 2 files changed, 21 insertions(+), 1 deletion(-) >> >> diff --git a/doc/filters.texi b/doc/filters.texi >> index 5d99437871..7eeeafab8e 100644 >> --- a/doc/filters.texi >> +++ b/doc/filters.texi >> @@ -14461,6 +14461,10 @@ is "black". >> @item overlap >> Set the number of frames to overlap when tiling several successive frames >> together. >> The value must be between @code{0} and @var{nb_frames - 1}. >> + >> +@item queue >> +Set the number of frames to initially queue when displaying first frame. >> +The value must be between @code{0} and @var{nb_frames}. >> @end table > > Queue where? To do what later, and when? > > I cannot determine what this option is supposed to do, even with a quick > glance at the code, so I guess a normal user would be completely > baffled. It allows user to start outputing video frames immediately with first input frame. Instead of after nb_frames.
Le nonidi 29 brumaire, an CCXXVI, Paul B Mahol a écrit : > It allows user to start outputing video frames immediately with first > input frame. > Instead of after nb_frames. I think I understand what it means, but only because I have read the code, and even like that I am not entirely sure. This needs to be made clearer, enough for somebody who does not know the code, and go to the documentation part of the patch. Regards,
On 11/19/17, Nicolas George <george@nsup.org> wrote: > Le nonidi 29 brumaire, an CCXXVI, Paul B Mahol a ecrit : >> It allows user to start outputing video frames immediately with first >> input frame. >> Instead of after nb_frames. > > I think I understand what it means, but only because I have read the > code, and even like that I am not entirely sure. This needs to be made > clearer, enough for somebody who does not know the code, and go to the > documentation part of the patch. So what you propose?
Paul B Mahol (2017-11-19):
> So what you propose?
I do not know. It is your patch, and I am not even sure I understood
your explanation correctly.
Regards,
> On Nov 19, 2017, at 2:19 PM, Nicolas George <george@nsup.org> wrote: > > Paul B Mahol (2017-11-19): >> So what you propose? > > I do not know. It is your patch, and I am not even sure I understood > your explanation correctly. IMHO, ‘queue' is a good name for what it does. Perhaps an additional example would help demonstrate the option better. For example `ffplay -f lavfi -i testsrc=r=1 -vf tile=1x8:overlap=7:queue=1` will display 8 adjoining frames of the testsrc. Without `queue=1` in this example, the output will be stalled for 8 seconds before displaying, whereas with `queue=1` there will be an output frame for each input frame (the first frame of the output simply showing the first frame of the input plus seven empty spaces). Suggestion: @item Produce a filmstrip animation from frame @code{n-7} to @code{n}: @example ffmpeg -i file.avi -vf 'tile=1x8:overlap=7:queue=1' filmstrip.mkv @end example Dave Rice
Dave Rice (2017-11-19):
> IMHO, ‘queue' is a good name for what it does.
Based on your explanation, I would say not at all. It rather seems to
say the opposite. I would suggest something like "init_padding".
Regards,
> Am 19.11.2017 um 21:22 schrieb Nicolas George <george@nsup.org>: > > Dave Rice (2017-11-19): >> IMHO, ‘queue' is a good name for what it does. > > Based on your explanation, I would say not at all. It rather seems to > say the opposite. I would suggest something like "init_padding". Based on Dave's example I'd say the queue parameter defines the minimum of available tiles before output is generated. 'queue' -> 'min' 'queue' -> 'min_tiles' 'queue' -> 'min_available' I'd suggest something like that. -Thilo
Thilo Borgmann (2017-11-19): > Based on Dave's example I'd say the queue parameter defines the > minimum of available tiles before output is generated. > > 'queue' -> 'min' > 'queue' -> 'min_tiles' > 'queue' -> 'min_available' > > I'd suggest something like that. "min" would imply a global minimum, the names you suggest do not contain anything that suggests it applies only at the beginning, and neither does "queue". I stick with "init_padding". Regards,
> Am 19.11.2017 um 23:25 schrieb Nicolas George <george@nsup.org>: > > Thilo Borgmann (2017-11-19): >> Based on Dave's example I'd say the queue parameter defines the >> minimum of available tiles before output is generated. >> >> 'queue' -> 'min' >> 'queue' -> 'min_tiles' >> 'queue' -> 'min_available' >> >> I'd suggest something like that. > > "min" would imply a global minimum, the names you suggest do not contain > anything that suggests it applies only at the beginning, and neither > does "queue". I stick with "init_padding". min_start n_begin My 2 cents. Not going to fight this, I find both 'queue' and 'init_padding' all but intuitive from the users pov. -Thilo
On 11/20/2017 5:04 AM, Thilo Borgmann wrote: > min_start > n_begin start_delay ? Regards, Gyan
> Am 20.11.2017 um 06:25 schrieb Gyan Doshi <gyandoshi@gmail.com>: > > >> On 11/20/2017 5:04 AM, Thilo Borgmann wrote: >> >> min_start >> n_begin > > start_delay ? Just to complete my thoughts... init_padding would be good and for code-viewing people it is way better than queue. My thoughts are about the name of the option exposed to the user, only. -Thilo
On 11/19/17, Nicolas George <george@nsup.org> wrote: > Thilo Borgmann (2017-11-19): >> Based on Dave's example I'd say the queue parameter defines the >> minimum of available tiles before output is generated. >> >> 'queue' -> 'min' >> 'queue' -> 'min_tiles' >> 'queue' -> 'min_available' >> >> I'd suggest something like that. > > "min" would imply a global minimum, the names you suggest do not contain > anything that suggests it applies only at the beginning, and neither > does "queue". I stick with "init_padding". This "clash" with padding option, and may confuse users.
On 11/21/17, Paul B Mahol <onemda@gmail.com> wrote: > On 11/19/17, Nicolas George <george@nsup.org> wrote: >> Thilo Borgmann (2017-11-19): >>> Based on Dave's example I'd say the queue parameter defines the >>> minimum of available tiles before output is generated. >>> >>> 'queue' -> 'min' >>> 'queue' -> 'min_tiles' >>> 'queue' -> 'min_available' >>> >>> I'd suggest something like that. >> >> "min" would imply a global minimum, the names you suggest do not contain >> anything that suggests it applies only at the beginning, and neither >> does "queue". I stick with "init_padding". > > This "clash" with padding option, and may confuse users. > Because there are no more comments can I assume pushing this patch as is OK?
Paul B Mahol (2017-11-24):
> Because there are no more comments can I assume pushing this patch as is OK?
Depends on how you amended it after the discussion.
Regards,
On 11/24/17, Nicolas George <george@nsup.org> wrote: > Paul B Mahol (2017-11-24): >> Because there are no more comments can I assume pushing this patch as is >> OK? > > Depends on how you amended it after the discussion. What you want to change?
Paul B Mahol (2017-11-24):
> What you want to change?
The name of the option and its documentation. The documentation was
unclear, and if there was a consensus about the name of the option, it
was not for "queue".
Regards,
On 11/24/17, Nicolas George <george@nsup.org> wrote: > Paul B Mahol (2017-11-24): >> What you want to change? > > The name of the option and its documentation. The documentation was > unclear, and if there was a consensus about the name of the option, it > was not for "queue". But for what option instead was consensus?
Paul B Mahol (2017-11-24):
> But for what option instead was consensus?
I do not remember. That is your patch, therefore I think it is your
responsibility to re-read the discussion, make a choice that you think
will suit everybody, and re-submit the patch for final approval.
By the way, since the documentation was unclear, I was not able to
soundly review the code changes themselves. Please do not push without
re-submitting.
Regards,
On 11/24/17, Nicolas George <george@nsup.org> wrote: > Paul B Mahol (2017-11-24): >> But for what option instead was consensus? > > I do not remember. That is your patch, therefore I think it is your > responsibility to re-read the discussion, make a choice that you think > will suit everybody, and re-submit the patch for final approval. > > By the way, since the documentation was unclear, I was not able to > soundly review the code changes themselves. Please do not push without > re-submitting. WTF you are very evil persona.
Paul B Mahol (2017-11-24):
> WTF you are very evil persona.
I do not know what you mean, and please stop ad-hominem attacks.
I only want time to review the code while knowing what it is supposed to
do. So please re-submit your patch.
Regards, and EOT for me tonight,
On 11/24/17, Nicolas George <george@nsup.org> wrote: > Paul B Mahol (2017-11-24): >> WTF you are very evil persona. > > I do not know what you mean, and please stop ad-hominem attacks. > > I only want time to review the code while knowing what it is supposed to > do. So please re-submit your patch. > > Regards, and EOT for me tonight, You have very high opinion obout yourself and yours opinions.
diff --git a/doc/filters.texi b/doc/filters.texi index 5d99437871..7eeeafab8e 100644 --- a/doc/filters.texi +++ b/doc/filters.texi @@ -14461,6 +14461,10 @@ is "black". @item overlap Set the number of frames to overlap when tiling several successive frames together. The value must be between @code{0} and @var{nb_frames - 1}. + +@item queue +Set the number of frames to initially queue when displaying first frame. +The value must be between @code{0} and @var{nb_frames}. @end table @subsection Examples diff --git a/libavfilter/vf_tile.c b/libavfilter/vf_tile.c index 7717ce12e7..dfc9377569 100644 --- a/libavfilter/vf_tile.c +++ b/libavfilter/vf_tile.c @@ -38,6 +38,7 @@ typedef struct TileContext { unsigned margin; unsigned padding; unsigned overlap; + unsigned queue; unsigned current; unsigned nb_frames; FFDrawContext draw; @@ -62,6 +63,8 @@ static const AVOption tile_options[] = { { "color", "set the color of the unused area", OFFSET(rgba_color), AV_OPT_TYPE_COLOR, {.str = "black"}, .flags = FLAGS }, { "overlap", "set how many frames to overlap for each render", OFFSET(overlap), AV_OPT_TYPE_INT, {.i64 = 0}, 0, INT_MAX, FLAGS }, + { "queue", " set how many frames to initially queue", OFFSET(queue), + AV_OPT_TYPE_INT, {.i64 = 0}, 0, INT_MAX, FLAGS }, { NULL } }; @@ -99,6 +102,19 @@ static av_cold int init(AVFilterContext *ctx) tile->overlap = tile->nb_frames - 1; } + if (!tile->queue) { + tile->queue = tile->nb_frames; + tile->current = 0; + } else { + if (tile->queue > tile->nb_frames) { + av_log(ctx, AV_LOG_WARNING, "queue must be less than or equal to %d\n", tile->nb_frames); + tile->queue = tile->nb_frames; + tile->current = 0; + } else { + tile->current = tile->nb_frames - tile->queue; + } + } + return 0; } @@ -201,7 +217,7 @@ static int filter_frame(AVFilterLink *inlink, AVFrame *picref) tile->out_ref->height = outlink->h; /* fill surface once for margin/padding */ - if (tile->margin || tile->padding) + if (tile->margin || tile->padding || tile->queue != tile->nb_frames) ff_fill_rectangle(&tile->draw, &tile->blank, tile->out_ref->data, tile->out_ref->linesize,
Signed-off-by: Paul B Mahol <onemda@gmail.com> --- doc/filters.texi | 4 ++++ libavfilter/vf_tile.c | 18 +++++++++++++++++- 2 files changed, 21 insertions(+), 1 deletion(-)