| Message ID | 20170731120227.31047-7-george@nsup.org |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, Jul 31, 2017 at 02:02:20PM +0200, Nicolas George wrote: > Signed-off-by: Nicolas George <george@nsup.org> > --- > libavfilter/vf_overlay.c | 45 ++++----------------------------------------- > 1 file changed, 4 insertions(+), 41 deletions(-) > > diff --git a/libavfilter/vf_overlay.c b/libavfilter/vf_overlay.c > index c1d2a21c05..512df87630 100644 > --- a/libavfilter/vf_overlay.c > +++ b/libavfilter/vf_overlay.c > @@ -70,16 +70,6 @@ enum var_name { > VAR_VARS_NB > }; > > -enum EOFAction { > - EOF_ACTION_REPEAT, > - EOF_ACTION_ENDALL, > - EOF_ACTION_PASS > -}; > - > -static const char * const eof_action_str[] = { > - "repeat", "endall", "pass" > -}; > - > #define MAIN 0 > #define OVERLAY 1 > > @@ -131,10 +121,6 @@ typedef struct OverlayContext { > double var_values[VAR_VARS_NB]; > char *x_expr, *y_expr; > > - int eof_action; ///< action to take on EOF from source > - int opt_shortest; > - int opt_repeatlast; > - > AVExpr *x_pexpr, *y_pexpr; > > void (*blend_image)(AVFilterContext *ctx, AVFrame *dst, const AVFrame *src, int x, int y); > @@ -377,12 +363,11 @@ static int config_input_overlay(AVFilterLink *inlink) > } > > av_log(ctx, AV_LOG_VERBOSE, > - "main w:%d h:%d fmt:%s overlay w:%d h:%d fmt:%s eof_action:%s\n", > + "main w:%d h:%d fmt:%s overlay w:%d h:%d fmt:%s\n", > ctx->inputs[MAIN]->w, ctx->inputs[MAIN]->h, > av_get_pix_fmt_name(ctx->inputs[MAIN]->format), > ctx->inputs[OVERLAY]->w, ctx->inputs[OVERLAY]->h, > - av_get_pix_fmt_name(ctx->inputs[OVERLAY]->format), > - eof_action_str[s->eof_action]); > + av_get_pix_fmt_name(ctx->inputs[OVERLAY]->format)); > return 0; > } > > @@ -394,12 +379,6 @@ static int config_output(AVFilterLink *outlink) > > if ((ret = ff_framesync2_init_dualinput(&s->fs, ctx)) < 0) > return ret; > - if (s->opt_shortest) > - s->fs.in[0].after = s->fs.in[1].after = EXT_STOP; > - if (!s->opt_repeatlast) { > - s->fs.in[1].after = EXT_NULL; > - s->fs.in[1].sync = 0; > - } > > outlink->w = ctx->inputs[MAIN]->w; > outlink->h = ctx->inputs[MAIN]->h; > @@ -818,15 +797,6 @@ static av_cold int init(AVFilterContext *ctx) > { > OverlayContext *s = ctx->priv; > > - if (!s->opt_repeatlast || s->eof_action == EOF_ACTION_PASS) { > - s->opt_repeatlast = 0; > - s->eof_action = EOF_ACTION_PASS; > - } > - if (s->opt_shortest || s->eof_action == EOF_ACTION_ENDALL) { > - s->opt_shortest = 1; > - s->eof_action = EOF_ACTION_ENDALL; > - } > - > s->fs.on_event = do_blend; > return 0; > } > @@ -843,16 +813,9 @@ static int activate(AVFilterContext *ctx) > static const AVOption overlay_options[] = { > { "x", "set the x expression", OFFSET(x_expr), AV_OPT_TYPE_STRING, {.str = "0"}, CHAR_MIN, CHAR_MAX, FLAGS }, > { "y", "set the y expression", OFFSET(y_expr), AV_OPT_TYPE_STRING, {.str = "0"}, CHAR_MIN, CHAR_MAX, FLAGS }, > - { "eof_action", "Action to take when encountering EOF from secondary input ", > - OFFSET(eof_action), AV_OPT_TYPE_INT, { .i64 = EOF_ACTION_REPEAT }, > - EOF_ACTION_REPEAT, EOF_ACTION_PASS, .flags = FLAGS, "eof_action" }, > - { "repeat", "Repeat the previous frame.", 0, AV_OPT_TYPE_CONST, { .i64 = EOF_ACTION_REPEAT }, .flags = FLAGS, "eof_action" }, > - { "endall", "End both streams.", 0, AV_OPT_TYPE_CONST, { .i64 = EOF_ACTION_ENDALL }, .flags = FLAGS, "eof_action" }, > - { "pass", "Pass through the main input.", 0, AV_OPT_TYPE_CONST, { .i64 = EOF_ACTION_PASS }, .flags = FLAGS, "eof_action" }, > { "eval", "specify when to evaluate expressions", OFFSET(eval_mode), AV_OPT_TYPE_INT, {.i64 = EVAL_MODE_FRAME}, 0, EVAL_MODE_NB-1, FLAGS, "eval" }, > { "init", "eval expressions once during initialization", 0, AV_OPT_TYPE_CONST, {.i64=EVAL_MODE_INIT}, .flags = FLAGS, .unit = "eval" }, > { "frame", "eval expressions per-frame", 0, AV_OPT_TYPE_CONST, {.i64=EVAL_MODE_FRAME}, .flags = FLAGS, .unit = "eval" }, > - { "shortest", "force termination when the shortest input terminates", OFFSET(opt_shortest), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, FLAGS }, > { "format", "set output format", OFFSET(format), AV_OPT_TYPE_INT, {.i64=OVERLAY_FORMAT_YUV420}, 0, OVERLAY_FORMAT_NB-1, FLAGS, "format" }, > { "yuv420", "", 0, AV_OPT_TYPE_CONST, {.i64=OVERLAY_FORMAT_YUV420}, .flags = FLAGS, .unit = "format" }, > { "yuv422", "", 0, AV_OPT_TYPE_CONST, {.i64=OVERLAY_FORMAT_YUV422}, .flags = FLAGS, .unit = "format" }, > @@ -860,11 +823,10 @@ static const AVOption overlay_options[] = { > { "rgb", "", 0, AV_OPT_TYPE_CONST, {.i64=OVERLAY_FORMAT_RGB}, .flags = FLAGS, .unit = "format" }, > { "gbrp", "", 0, AV_OPT_TYPE_CONST, {.i64=OVERLAY_FORMAT_GBRP}, .flags = FLAGS, .unit = "format" }, > { "auto", "", 0, AV_OPT_TYPE_CONST, {.i64=OVERLAY_FORMAT_AUTO}, .flags = FLAGS, .unit = "format" }, > - { "repeatlast", "repeat overlay of the last overlay frame", OFFSET(opt_repeatlast), AV_OPT_TYPE_BOOL, {.i64=1}, 0, 1, FLAGS }, > { NULL } > }; removing these elements causes command lines using them to fail for example: ./ffmpeg -i matrixbench_mpeg2.mpg -i lena.pnm -filter_complex 'overlay=10:main_h-overlay_h-10:pass' -qscale 2 -t 1 file.avi [overlay @ 0x3662fa0] [Eval @ 0x7ffd8a759260] Undefined constant or missing '(' in 'pass' [overlay @ 0x3662fa0] Unable to parse option value "pass" [overlay @ 0x3662fa0] [Eval @ 0x7ffd8a7592e0] Undefined constant or missing '(' in 'pass' [overlay @ 0x3662fa0] Unable to parse option value "pass" [overlay @ 0x3662fa0] Error setting option eval to value pass. [Parsed_overlay_0 @ 0x3662ea0] Error applying options to the filter. [AVFilterGraph @ 0x364f120] Error initializing filter 'overlay' with args '10:main_h-overlay_h-10:pass' Error initializing complex filters. Invalid argument [...]
Le quintidi 15 thermidor, an CCXXV, Michael Niedermayer a écrit : > removing these elements causes command lines using them to fail > for example: > > ./ffmpeg -i matrixbench_mpeg2.mpg -i lena.pnm -filter_complex 'overlay=10:main_h-overlay_h-10:pass' -qscale 2 -t 1 file.avi Oh, the shorthand notation. The order of the non-essential options has changed in the past, including recently, and does not match the documentation, I have always considered it undocumented and as such unspecified. Moreover, with a separate object like that, keeping the order will be impossible: options added to the filter in the future will always come before the options in the framesync object. Therefore, I consider this minor break of compatibility acceptable. I should have mentioned it, but I forgot, sorry. What do other think? Regards,
On 8/2/17, Nicolas George <george@nsup.org> wrote: > Le quintidi 15 thermidor, an CCXXV, Michael Niedermayer a écrit : >> removing these elements causes command lines using them to fail >> for example: >> >> ./ffmpeg -i matrixbench_mpeg2.mpg -i lena.pnm -filter_complex >> 'overlay=10:main_h-overlay_h-10:pass' -qscale 2 -t 1 file.avi > > Oh, the shorthand notation. The order of the non-essential options has > changed in the past, including recently, and does not match the > documentation, I have always considered it undocumented and as such > unspecified. > > Moreover, with a separate object like that, keeping the order will be > impossible: options added to the filter in the future will always come > before the options in the framesync object. > > Therefore, I consider this minor break of compatibility acceptable. I > should have mentioned it, but I forgot, sorry. > > What do other think? > We should move forward, no need to keep shorthand compatibility for those options.
On 8/4/17, Paul B Mahol <onemda@gmail.com> wrote: > On 8/2/17, Nicolas George <george@nsup.org> wrote: >> Le quintidi 15 thermidor, an CCXXV, Michael Niedermayer a écrit : >>> removing these elements causes command lines using them to fail >>> for example: >>> >>> ./ffmpeg -i matrixbench_mpeg2.mpg -i lena.pnm -filter_complex >>> 'overlay=10:main_h-overlay_h-10:pass' -qscale 2 -t 1 file.avi >> >> Oh, the shorthand notation. The order of the non-essential options has >> changed in the past, including recently, and does not match the >> documentation, I have always considered it undocumented and as such >> unspecified. >> >> Moreover, with a separate object like that, keeping the order will be >> impossible: options added to the filter in the future will always come >> before the options in the framesync object. >> >> Therefore, I consider this minor break of compatibility acceptable. I >> should have mentioned it, but I forgot, sorry. >> >> What do other think? >> > > We should move forward, no need to keep shorthand compatibility for > those options. > Also what about to porting one of simple filter(just filter_frame call) to activate API, i can not do it myself unless someone show me how.
On Wed, Aug 02, 2017 at 08:06:37AM +0200, Nicolas George wrote: > Le quintidi 15 thermidor, an CCXXV, Michael Niedermayer a écrit : > > removing these elements causes command lines using them to fail > > for example: > > > > ./ffmpeg -i matrixbench_mpeg2.mpg -i lena.pnm -filter_complex 'overlay=10:main_h-overlay_h-10:pass' -qscale 2 -t 1 file.avi > > Oh, the shorthand notation. The order of the non-essential options has > changed in the past, including recently, and does not match the > documentation, The documentation says @section overlay [...] @table @option @item x @item y [...] @item eof_action Thats the order used in the example Also either way A user should be able to write a command line with filters from the documentation and know if it will be supported in the future before the future occurs and she finds out it stopped working. > I have always considered it undocumented and as such > unspecified. > Moreover, with a separate object like that, keeping the order will be > impossible: options added to the filter in the future will always come > before the options in the framesync object. > > Therefore, I consider this minor break of compatibility acceptable. I > should have mentioned it, but I forgot, sorry. > > What do other think? > > Regards, > > -- > Nicolas George > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Le septidi 17 thermidor, an CCXXV, Paul B Mahol a écrit : > Also what about to porting one of simple filter(just filter_frame > call) to activate API, > i can not do it myself unless someone show me how. Now that the new design has proved to work, rewriting doc/filter_design.txt is near the top of my list of priorities. But a filter that has only a filter_frame call does not benefit much from porting to activate design. It is only when there is a queue of frames involved or when something special is needed at EOF (which often come together) that it matters. I have a WIP branch for rewriting vf_fps. Regards,
L'octidi 18 thermidor, an CCXXV, Michael Niedermayer a écrit : > The documentation says > @section overlay > [...] > @table @option > @item x > @item y > [...] > @item eof_action > > Thats the order used in the example Yes. But it does not work like that for all the options. So basically, you guessed that you could go to the third option, and you were lucky, but it is not guaranteed. > Also either way > A user should be able to write a command line with filters from the > documentation and know if it will be supported in the future before > the future occurs and she finds out it stopped working. Yes, that would be ideal. But it is not presently true. We should make an effort to document the options that can be used in shorthand notation. And you are being evasive: do you consider this issue blocking? Regards,
diff --git a/libavfilter/vf_overlay.c b/libavfilter/vf_overlay.c index c1d2a21c05..512df87630 100644 --- a/libavfilter/vf_overlay.c +++ b/libavfilter/vf_overlay.c @@ -70,16 +70,6 @@ enum var_name { VAR_VARS_NB }; -enum EOFAction { - EOF_ACTION_REPEAT, - EOF_ACTION_ENDALL, - EOF_ACTION_PASS -}; - -static const char * const eof_action_str[] = { - "repeat", "endall", "pass" -}; - #define MAIN 0 #define OVERLAY 1 @@ -131,10 +121,6 @@ typedef struct OverlayContext { double var_values[VAR_VARS_NB]; char *x_expr, *y_expr; - int eof_action; ///< action to take on EOF from source - int opt_shortest; - int opt_repeatlast; - AVExpr *x_pexpr, *y_pexpr; void (*blend_image)(AVFilterContext *ctx, AVFrame *dst, const AVFrame *src, int x, int y); @@ -377,12 +363,11 @@ static int config_input_overlay(AVFilterLink *inlink) } av_log(ctx, AV_LOG_VERBOSE, - "main w:%d h:%d fmt:%s overlay w:%d h:%d fmt:%s eof_action:%s\n", + "main w:%d h:%d fmt:%s overlay w:%d h:%d fmt:%s\n", ctx->inputs[MAIN]->w, ctx->inputs[MAIN]->h, av_get_pix_fmt_name(ctx->inputs[MAIN]->format), ctx->inputs[OVERLAY]->w, ctx->inputs[OVERLAY]->h, - av_get_pix_fmt_name(ctx->inputs[OVERLAY]->format), - eof_action_str[s->eof_action]); + av_get_pix_fmt_name(ctx->inputs[OVERLAY]->format)); return 0; } @@ -394,12 +379,6 @@ static int config_output(AVFilterLink *outlink) if ((ret = ff_framesync2_init_dualinput(&s->fs, ctx)) < 0) return ret; - if (s->opt_shortest) - s->fs.in[0].after = s->fs.in[1].after = EXT_STOP; - if (!s->opt_repeatlast) { - s->fs.in[1].after = EXT_NULL; - s->fs.in[1].sync = 0; - } outlink->w = ctx->inputs[MAIN]->w; outlink->h = ctx->inputs[MAIN]->h; @@ -818,15 +797,6 @@ static av_cold int init(AVFilterContext *ctx) { OverlayContext *s = ctx->priv; - if (!s->opt_repeatlast || s->eof_action == EOF_ACTION_PASS) { - s->opt_repeatlast = 0; - s->eof_action = EOF_ACTION_PASS; - } - if (s->opt_shortest || s->eof_action == EOF_ACTION_ENDALL) { - s->opt_shortest = 1; - s->eof_action = EOF_ACTION_ENDALL; - } - s->fs.on_event = do_blend; return 0; } @@ -843,16 +813,9 @@ static int activate(AVFilterContext *ctx) static const AVOption overlay_options[] = { { "x", "set the x expression", OFFSET(x_expr), AV_OPT_TYPE_STRING, {.str = "0"}, CHAR_MIN, CHAR_MAX, FLAGS }, { "y", "set the y expression", OFFSET(y_expr), AV_OPT_TYPE_STRING, {.str = "0"}, CHAR_MIN, CHAR_MAX, FLAGS }, - { "eof_action", "Action to take when encountering EOF from secondary input ", - OFFSET(eof_action), AV_OPT_TYPE_INT, { .i64 = EOF_ACTION_REPEAT }, - EOF_ACTION_REPEAT, EOF_ACTION_PASS, .flags = FLAGS, "eof_action" }, - { "repeat", "Repeat the previous frame.", 0, AV_OPT_TYPE_CONST, { .i64 = EOF_ACTION_REPEAT }, .flags = FLAGS, "eof_action" }, - { "endall", "End both streams.", 0, AV_OPT_TYPE_CONST, { .i64 = EOF_ACTION_ENDALL }, .flags = FLAGS, "eof_action" }, - { "pass", "Pass through the main input.", 0, AV_OPT_TYPE_CONST, { .i64 = EOF_ACTION_PASS }, .flags = FLAGS, "eof_action" }, { "eval", "specify when to evaluate expressions", OFFSET(eval_mode), AV_OPT_TYPE_INT, {.i64 = EVAL_MODE_FRAME}, 0, EVAL_MODE_NB-1, FLAGS, "eval" }, { "init", "eval expressions once during initialization", 0, AV_OPT_TYPE_CONST, {.i64=EVAL_MODE_INIT}, .flags = FLAGS, .unit = "eval" }, { "frame", "eval expressions per-frame", 0, AV_OPT_TYPE_CONST, {.i64=EVAL_MODE_FRAME}, .flags = FLAGS, .unit = "eval" }, - { "shortest", "force termination when the shortest input terminates", OFFSET(opt_shortest), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, FLAGS }, { "format", "set output format", OFFSET(format), AV_OPT_TYPE_INT, {.i64=OVERLAY_FORMAT_YUV420}, 0, OVERLAY_FORMAT_NB-1, FLAGS, "format" }, { "yuv420", "", 0, AV_OPT_TYPE_CONST, {.i64=OVERLAY_FORMAT_YUV420}, .flags = FLAGS, .unit = "format" }, { "yuv422", "", 0, AV_OPT_TYPE_CONST, {.i64=OVERLAY_FORMAT_YUV422}, .flags = FLAGS, .unit = "format" }, @@ -860,11 +823,10 @@ static const AVOption overlay_options[] = { { "rgb", "", 0, AV_OPT_TYPE_CONST, {.i64=OVERLAY_FORMAT_RGB}, .flags = FLAGS, .unit = "format" }, { "gbrp", "", 0, AV_OPT_TYPE_CONST, {.i64=OVERLAY_FORMAT_GBRP}, .flags = FLAGS, .unit = "format" }, { "auto", "", 0, AV_OPT_TYPE_CONST, {.i64=OVERLAY_FORMAT_AUTO}, .flags = FLAGS, .unit = "format" }, - { "repeatlast", "repeat overlay of the last overlay frame", OFFSET(opt_repeatlast), AV_OPT_TYPE_BOOL, {.i64=1}, 0, 1, FLAGS }, { NULL } }; -AVFILTER_DEFINE_CLASS(overlay); +FRAMESYNC_DEFINE_CLASS(overlay, OverlayContext, fs); static const AVFilterPad avfilter_vf_overlay_inputs[] = { { @@ -893,6 +855,7 @@ static const AVFilterPad avfilter_vf_overlay_outputs[] = { AVFilter ff_vf_overlay = { .name = "overlay", .description = NULL_IF_CONFIG_SMALL("Overlay a video source on top of the input."), + .preinit = overlay_framesync_preinit, .init = init, .uninit = uninit, .priv_size = sizeof(OverlayContext),
Signed-off-by: Nicolas George <george@nsup.org> --- libavfilter/vf_overlay.c | 45 ++++----------------------------------------- 1 file changed, 4 insertions(+), 41 deletions(-)