| Message ID | 20161229143403.2851-6-george@nsup.org |
|---|---|
| State | New |
| Headers | show |
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 ? [...]
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,
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?
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,
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" [...]
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,
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.
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.