[FFmpeg-devel,06/17] lavfi: add ff_inlink_process_timeline().

Submitted by Nicolas George on Dec. 29, 2016, 2:33 p.m.

Details

Message ID 20161229143403.2851-6-george@nsup.org
State New
Headers show

Commit Message

Nicolas George Dec. 29, 2016, 2:33 p.m.
Signed-off-by: Nicolas George <george@nsup.org>
---
 libavfilter/avfilter.c | 34 +++++++++++++++++++++-------------
 libavfilter/filters.h  |  6 ++++++
 2 files changed, 27 insertions(+), 13 deletions(-)


Changes in this commit: rename ff_link -> ff_inlink and move to filters.h.
Already LGTM by Michael.

Comments

Michael Niedermayer Dec. 29, 2016, 8:17 p.m.
On Thu, Dec 29, 2016 at 03:33:52PM +0100, Nicolas George wrote:
> Signed-off-by: Nicolas George <george@nsup.org>
> ---
>  libavfilter/avfilter.c | 34 +++++++++++++++++++++-------------
>  libavfilter/filters.h  |  6 ++++++
>  2 files changed, 27 insertions(+), 13 deletions(-)
> 
> 
> Changes in this commit: rename ff_link -> ff_inlink and move to filters.h.
> Already LGTM by Michael.
> 
> 
> diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
> index 2fe8b980e0..d1ba7d9bad 100644
> --- a/libavfilter/avfilter.c
> +++ b/libavfilter/avfilter.c
> @@ -1099,7 +1099,6 @@ static int ff_filter_frame_framed(AVFilterLink *link, AVFrame *frame)
>      AVFilterContext *dstctx = link->dst;
>      AVFilterPad *dst = link->dstpad;
>      int ret;
> -    int64_t pts;
>  
>      if (!(filter_frame = dst->filter_frame))
>          filter_frame = default_filter_frame;
> @@ -1111,24 +1110,15 @@ static int ff_filter_frame_framed(AVFilterLink *link, AVFrame *frame)
>      }
>  
>      ff_inlink_process_commands(link, frame);
> +    ff_inlink_process_timeline(link, frame);
>  
> -    pts = frame->pts;
> -    if (dstctx->enable_str) {
> -        int64_t pos = av_frame_get_pkt_pos(frame);
> -        dstctx->var_values[VAR_N] = link->frame_count_out;
> -        dstctx->var_values[VAR_T] = pts == AV_NOPTS_VALUE ? NAN : pts * av_q2d(link->time_base);
> -        dstctx->var_values[VAR_W] = link->w;
> -        dstctx->var_values[VAR_H] = link->h;
> -        dstctx->var_values[VAR_POS] = pos == -1 ? NAN : pos;
> -
> -        dstctx->is_disabled = fabs(av_expr_eval(dstctx->enable, dstctx->var_values, NULL)) < 0.5;
> +        /* TODO reindent */
>          if (dstctx->is_disabled &&
>              (dstctx->filter->flags & AVFILTER_FLAG_SUPPORT_TIMELINE_GENERIC))
>              filter_frame = default_filter_frame;
> -    }
>      ret = filter_frame(link, frame);
>      link->frame_count_out++;
> -    ff_update_link_current_pts(link, pts);
> +    ff_update_link_current_pts(link, frame->pts);
>      return ret;
>  
>  fail:
> @@ -1571,6 +1561,24 @@ void ff_inlink_process_commands(AVFilterLink *link, const AVFrame *frame)
>      }
>  }
>  
> +void ff_inlink_process_timeline(AVFilterLink *link, const AVFrame *frame)
> +{
> +    AVFilterContext *dstctx = link->dst;
> +    int64_t pts = frame->pts;
> +    int64_t pos = av_frame_get_pkt_pos(frame);
> +
> +    if (!dstctx->enable_str)
> +        return;
> +
> +    dstctx->var_values[VAR_N] = link->frame_count_out;
> +    dstctx->var_values[VAR_T] = pts == AV_NOPTS_VALUE ? NAN : pts * av_q2d(link->time_base);
> +    dstctx->var_values[VAR_W] = link->w;
> +    dstctx->var_values[VAR_H] = link->h;
> +    dstctx->var_values[VAR_POS] = pos == -1 ? NAN : pos;
> +
> +    dstctx->is_disabled = fabs(av_expr_eval(dstctx->enable, dstctx->var_values, NULL)) < 0.5;
> +}
> +
>  const AVClass *avfilter_get_class(void)
>  {
>      return &avfilter_class;
> diff --git a/libavfilter/filters.h b/libavfilter/filters.h
> index efbef2918d..69a29c640f 100644
> --- a/libavfilter/filters.h
> +++ b/libavfilter/filters.h
> @@ -39,6 +39,12 @@ void ff_filter_set_ready(AVFilterContext *filter, unsigned priority);
>  void ff_inlink_process_commands(AVFilterLink *link, const AVFrame *frame);
>  

>  /**
> + * Process the timeline expression of the link for the time of the frame.
> + * It will update link->is_disabled.
> + */
> +void ff_inlink_process_timeline(AVFilterLink *link, const AVFrame *frame);

maybe the enabledness value should be returned by the function (too)

how does this work with multiple inlinks ?
(dstctx / dstctx->enable / dstctx->is_disabled would be the same)

Also whats your oppinion about calling this
ff_inlink_evaluate_timeline_at_frame
or
ff_inlink_evaluate_timeline_at
?

[...]
Nicolas George Dec. 30, 2016, 11:37 a.m.
Le nonidi 9 nivôse, an CCXXV, Michael Niedermayer a écrit :
> maybe the enabledness value should be returned by the function (too)

It does not seem very useful at first glance, but I may be missing
something.

> how does this work with multiple inlinks ?
> (dstctx / dstctx->enable / dstctx->is_disabled would be the same)

Currently, if I read the code correctly, only filters with a single
input support timeline enable/disable.

In fact, I do not see how timeline could make sense for a filter with
several inputs. "Disabling" a filter means replacing it by the
pass-through filter "null", but it has a single input and output.

> Also whats your oppinion about calling this
> ff_inlink_evaluate_timeline_at_frame
> or
> ff_inlink_evaluate_timeline_at
> ?

I have no strong feeling about it one way or the other, I just find the
symmetry ff_inlink_process_timeline() / ff_inlink_process_commands()
nice.

I have addressed your remarks about the documentation in my work tree. I
do not think it is worth sending the whole series again just for that,
but here are the docs in case they make understanding the rest easier.
It repeats a bit what is explained in the new doxy for activate().

Note that I also changed ff_inlink_acknowledge_status() to return the
link's timestamp, currently ignored but will be useful soon (vf_fps
typically), and it avoids filters accessing the link directly (better
for threading later).

Also note that they are referring to the activate callback before it is
introduced. I can move the "lavfi: add AVFilter.activate" before if it
is a concern.

/**
 * Mark a filter ready and schedule it for activation.
 *
 * This is automatically done when something happens to the filter (queued
 * frame, status change, request on output).
 * Filters implementing the activate callback can call it directly to
 * perform one more round of processing later.
 * It is also useful for filters reacting to external or asynchronous
 * events.
 */
void ff_filter_set_ready(AVFilterContext *filter, unsigned priority);

/**
 * Test and acknowledge the change of status on the link.
 *
 * Status means EOF or an error condition; a change from the normal (0)
 * status to a non-zero status can be queued in a filter's input link, it
 * becomes relevant after the frames queued in the link's FIFO are
 * processed. This function tests if frames are still queued and if a queued
 * status change has not yet been processed. In that case it performs basic
 * treatment (updating the link's timestamp) and returns a positive value to
 * let the filter do its own treatments (flushing...).
 *
 * Filters implementing the activate callback should call this function when
 * they think it might succeed (usually after checking unsuccessfully for a
 * queued frame).
 * Filters implementing the filter_frame and request_frame callbacks do not
 * need to call that since the same treatment happens in ff_filter_frame().
 *
 * @param[out] rstatus  new or current status
 * @param[out] rpts     current timestamp of the link in link time base
 * @return  >0 if status changed, <0 if status already acked, 0 otherwise
 */
int ff_inlink_acknowledge_status(AVFilterLink *link, int *rstatus, int64_t *rpts);

Regards,
Clément Bœsch Dec. 30, 2016, 11:47 a.m.
On Fri, Dec 30, 2016 at 12:37:41PM +0100, Nicolas George wrote:
[...]
> In fact, I do not see how timeline could make sense for a filter with
> several inputs. "Disabling" a filter means replacing it by the
> pass-through filter "null", but it has a single input and output.
> 

You want a watermark to be effective only in a timerange, the second input
(the watermark) would get ignored outside that range, and the first one
pass through. This would be filter specific (FLAG_SUPPORT_TIMELINE_INTERNAL).

That's how overlay and dualinput actually works with a given outlink,
doesn't it?
Nicolas George Dec. 30, 2016, 12:19 p.m.
Le decadi 10 nivôse, an CCXXV, Clement Boesch a écrit :
> You want a watermark to be effective only in a timerange, the second input
> (the watermark) would get ignored outside that range, and the first one
> pass through. This would be filter specific (FLAG_SUPPORT_TIMELINE_INTERNAL).
> 
> That's how overlay and dualinput actually works with a given outlink,
> doesn't it?

You are right, I missed that when replying to Michael. It works because
the inputs are clearly defined as primary and secondary. I believe I did
not break it with the current series; if there is a FATE test then I am
sure I did not.

Still, the current code is fragile: since the enable expression is
evaluated for both inputs at the time the frames arrive and are queued
in the dualinput/framesync context, it may cause slightly incorrect
results: the timestamp for enable/disable would be taken from the
incoming frame that allows to flush the current frame.

Still, I think fixing that is orthogonal to the current patch series.

Regards,
Michael Niedermayer Dec. 30, 2016, 9:42 p.m.
On Fri, Dec 30, 2016 at 12:37:41PM +0100, Nicolas George wrote:
> Le nonidi 9 nivôse, an CCXXV, Michael Niedermayer a écrit :
> > maybe the enabledness value should be returned by the function (too)
> 
> It does not seem very useful at first glance, but I may be missing
> something.

// do some statistics, whatever
...
if (ff_inlink_evaluate_timeline_at_frame()) {
    process frame
}
pass output on


If we imagine a filter that processes a series of frames, lets say
for motion estimation or deinterlacing then the point at which it
becomes enabled may need some processing (or storage) to have occured
on past frames

i find it cleaner to have the effect of a function be its return
value instead of it primarly writing a value into some structure field



> 
> > how does this work with multiple inlinks ?
> > (dstctx / dstctx->enable / dstctx->is_disabled would be the same)
> 
> Currently, if I read the code correctly, only filters with a single
> input support timeline enable/disable.
> 
> In fact, I do not see how timeline could make sense for a filter with
> several inputs. "Disabling" a filter means replacing it by the
> pass-through filter "null", but it has a single input and output.
> 

> > Also whats your oppinion about calling this
> > ff_inlink_evaluate_timeline_at_frame
> > or
> > ff_inlink_evaluate_timeline_at
> > ?
> 
> I have no strong feeling about it one way or the other, I just find the
> symmetry ff_inlink_process_timeline() / ff_inlink_process_commands()
> nice.

i think i didnt explain why i suggested this.
to me "process" sounds a bit unspecified and unclear. as in
what does processing a timeline mean ? processing the whole timeline?
what would that do ?
while "evaluate_timeline_at" avoids this ambiguity.
Iam not attached to the specific words, maybe theres a better term

ff_inlink_is_enabled_at() maybe


> 
> I have addressed your remarks about the documentation in my work tree. I
> do not think it is worth sending the whole series again just for that,
> but here are the docs in case they make understanding the rest easier.
> It repeats a bit what is explained in the new doxy for activate().
> 
> Note that I also changed ff_inlink_acknowledge_status() to return the
> link's timestamp, currently ignored but will be useful soon (vf_fps
> typically), and it avoids filters accessing the link directly (better
> for threading later).
> 
> Also note that they are referring to the activate callback before it is
> introduced. I can move the "lavfi: add AVFilter.activate" before if it
> is a concern.
> 
> /**
>  * Mark a filter ready and schedule it for activation.
>  *
>  * This is automatically done when something happens to the filter (queued
>  * frame, status change, request on output).
>  * Filters implementing the activate callback can call it directly to
>  * perform one more round of processing later.
>  * It is also useful for filters reacting to external or asynchronous
>  * events.
>  */
> void ff_filter_set_ready(AVFilterContext *filter, unsigned priority);

if the only use of this (from a filter) is to call it to get activate()
re-called, just an idea but
what would be your oppinion on using a return code from activate() ?
E"iam not done, call me again later"

[...]
Nicolas George Dec. 31, 2016, 10:41 a.m.
Le decadi 10 nivôse, an CCXXV, Michael Niedermayer a écrit :
> // do some statistics, whatever
> ...
> if (ff_inlink_evaluate_timeline_at_frame()) {
>     process frame
> }
> pass output on
> 
> 
> If we imagine a filter that processes a series of frames, lets say
> for motion estimation or deinterlacing then the point at which it
> becomes enabled may need some processing (or storage) to have occured
> on past frames
> 
> i find it cleaner to have the effect of a function be its return
> value instead of it primarly writing a value into some structure field

That makes sense. Changed. The new code looks like this:

    dstctx->is_disabled = !ff_inlink_evaluate_timeline_at_frame(link, frame);

And the doxy:

/**
 * Evaluate the timeline expression of the link for the time and properties
 * of the frame.
 * @return  >0 if enabled, 0 if disabled
 * @note  It does not update link->dst->is_disabled.
 */
int ff_inlink_evaluate_timeline_at_frame(AVFilterLink *link, const AVFrame *frame);

> > /**
> >  * Mark a filter ready and schedule it for activation.
> >  *
> >  * This is automatically done when something happens to the filter (queued
> >  * frame, status change, request on output).
> >  * Filters implementing the activate callback can call it directly to
> >  * perform one more round of processing later.
> >  * It is also useful for filters reacting to external or asynchronous
> >  * events.
> >  */
> > void ff_filter_set_ready(AVFilterContext *filter, unsigned priority);

> if the only use of this (from a filter) is to call it to get activate()
> re-called, just an idea but
> what would be your oppinion on using a return code from activate() ?
> E"iam not done, call me again later"

I considered this, but I decided against it for the combination of
several minor reasons:

- a return code is more easily lost in a refactoring;

- the function is still needed for the framework (note that it already
  exists, this patch only makes it visible outside avfilter.c);

- the function will be needed for filters that react to external or
  asynchronous events;

- having several mechanisms to do approximatively the same thing is not
  the best idea;

- it does not allow to set a priority (I have not yet documented the
  priorities, but the idea is that processing a frame is more urgent
  than forwarding a request).

> iam tempted to suggest to call this
> ff_inlink_request_frame()
> 
> it would be the better name if ff_request_frame() didnt exist.

I tried to make ff_filter_frame() work for both cases, but it proved too
inconvenient. I renamed it ff_inlink_request_frame(), I do not think the
confusion can cause actual problems. At worst, someone will write
ff_request_frame(), get an assert failure when testing and fix it, the
same could happen as well with the other name.

The code changes are still minor, I will wait a bit before sending the
new series in case there are remarks about patches 12, 13, 14, 16, 17 or
more about the others.

Regards,

Patch hide | download patch | download mbox

diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
index 2fe8b980e0..d1ba7d9bad 100644
--- a/libavfilter/avfilter.c
+++ b/libavfilter/avfilter.c
@@ -1099,7 +1099,6 @@  static int ff_filter_frame_framed(AVFilterLink *link, AVFrame *frame)
     AVFilterContext *dstctx = link->dst;
     AVFilterPad *dst = link->dstpad;
     int ret;
-    int64_t pts;
 
     if (!(filter_frame = dst->filter_frame))
         filter_frame = default_filter_frame;
@@ -1111,24 +1110,15 @@  static int ff_filter_frame_framed(AVFilterLink *link, AVFrame *frame)
     }
 
     ff_inlink_process_commands(link, frame);
+    ff_inlink_process_timeline(link, frame);
 
-    pts = frame->pts;
-    if (dstctx->enable_str) {
-        int64_t pos = av_frame_get_pkt_pos(frame);
-        dstctx->var_values[VAR_N] = link->frame_count_out;
-        dstctx->var_values[VAR_T] = pts == AV_NOPTS_VALUE ? NAN : pts * av_q2d(link->time_base);
-        dstctx->var_values[VAR_W] = link->w;
-        dstctx->var_values[VAR_H] = link->h;
-        dstctx->var_values[VAR_POS] = pos == -1 ? NAN : pos;
-
-        dstctx->is_disabled = fabs(av_expr_eval(dstctx->enable, dstctx->var_values, NULL)) < 0.5;
+        /* TODO reindent */
         if (dstctx->is_disabled &&
             (dstctx->filter->flags & AVFILTER_FLAG_SUPPORT_TIMELINE_GENERIC))
             filter_frame = default_filter_frame;
-    }
     ret = filter_frame(link, frame);
     link->frame_count_out++;
-    ff_update_link_current_pts(link, pts);
+    ff_update_link_current_pts(link, frame->pts);
     return ret;
 
 fail:
@@ -1571,6 +1561,24 @@  void ff_inlink_process_commands(AVFilterLink *link, const AVFrame *frame)
     }
 }
 
+void ff_inlink_process_timeline(AVFilterLink *link, const AVFrame *frame)
+{
+    AVFilterContext *dstctx = link->dst;
+    int64_t pts = frame->pts;
+    int64_t pos = av_frame_get_pkt_pos(frame);
+
+    if (!dstctx->enable_str)
+        return;
+
+    dstctx->var_values[VAR_N] = link->frame_count_out;
+    dstctx->var_values[VAR_T] = pts == AV_NOPTS_VALUE ? NAN : pts * av_q2d(link->time_base);
+    dstctx->var_values[VAR_W] = link->w;
+    dstctx->var_values[VAR_H] = link->h;
+    dstctx->var_values[VAR_POS] = pos == -1 ? NAN : pos;
+
+    dstctx->is_disabled = fabs(av_expr_eval(dstctx->enable, dstctx->var_values, NULL)) < 0.5;
+}
+
 const AVClass *avfilter_get_class(void)
 {
     return &avfilter_class;
diff --git a/libavfilter/filters.h b/libavfilter/filters.h
index efbef2918d..69a29c640f 100644
--- a/libavfilter/filters.h
+++ b/libavfilter/filters.h
@@ -39,6 +39,12 @@  void ff_filter_set_ready(AVFilterContext *filter, unsigned priority);
 void ff_inlink_process_commands(AVFilterLink *link, const AVFrame *frame);
 
 /**
+ * Process the timeline expression of the link for the time of the frame.
+ * It will update link->is_disabled.
+ */
+void ff_inlink_process_timeline(AVFilterLink *link, const AVFrame *frame);
+
+/**
  * Make sure a frame is writable.
  * This is similar to av_frame_make_writable() except it uses the link's
  * buffer allocation callback, and therefore allows direct rendering.