diff mbox

[FFmpeg-devel] avfilter/vf_tile: add queue option

Message ID 20171117203437.24145-1-onemda@gmail.com
State New
Headers show

Commit Message

Paul B Mahol Nov. 17, 2017, 8:34 p.m. UTC
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(-)

Comments

Nicolas George Nov. 19, 2017, 4:19 p.m. UTC | #1
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,
Paul B Mahol Nov. 19, 2017, 5:30 p.m. UTC | #2
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.
Nicolas George Nov. 19, 2017, 5:38 p.m. UTC | #3
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,
Paul B Mahol Nov. 19, 2017, 7:17 p.m. UTC | #4
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?
Nicolas George Nov. 19, 2017, 7:19 p.m. UTC | #5
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,
Dave Rice Nov. 19, 2017, 8:20 p.m. UTC | #6
> 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
Nicolas George Nov. 19, 2017, 8:22 p.m. UTC | #7
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,
Thilo Borgmann Nov. 19, 2017, 10:06 p.m. UTC | #8
> 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
Nicolas George Nov. 19, 2017, 10:25 p.m. UTC | #9
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,
Thilo Borgmann Nov. 19, 2017, 11:34 p.m. UTC | #10
> 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
Gyan Nov. 20, 2017, 5:25 a.m. UTC | #11
On 11/20/2017 5:04 AM, Thilo Borgmann wrote:

> min_start
> n_begin

start_delay ?

Regards,
Gyan
Thilo Borgmann Nov. 20, 2017, 9:19 a.m. UTC | #12
> 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
Paul B Mahol Nov. 21, 2017, 5:29 p.m. UTC | #13
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.
Paul B Mahol Nov. 24, 2017, 3:44 p.m. UTC | #14
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?
Nicolas George Nov. 24, 2017, 6:07 p.m. UTC | #15
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,
Paul B Mahol Nov. 24, 2017, 6:17 p.m. UTC | #16
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?
Nicolas George Nov. 24, 2017, 6:20 p.m. UTC | #17
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,
Paul B Mahol Nov. 24, 2017, 6:42 p.m. UTC | #18
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?
Nicolas George Nov. 24, 2017, 8:46 p.m. UTC | #19
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,
Paul B Mahol Nov. 24, 2017, 8:49 p.m. UTC | #20
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.
Nicolas George Nov. 24, 2017, 8:56 p.m. UTC | #21
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,
Paul B Mahol Nov. 24, 2017, 9:13 p.m. UTC | #22
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 mbox

Patch

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,