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

[Nicolas George]

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,

-- 
  Nicolas George


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Michael Niedermayer]

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"

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No great genius has ever existed without some touch of madness. -- Aristotle


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Nicolas George]

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,

-- 
  Nicolas George


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Clément Bœsch]

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?

-- 
Clément B.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Nicolas George]

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,

-- 
  Nicolas George


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Michael Niedermayer]

On Thu, Dec 29, 2016 at 03:33:52PM +0100, Nicolas George wrote:
> Signed-off-by: Nicolas George 
> ---
>  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;
> @@ -,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 _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
?

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel