[FFmpeg-devel] [PATCH] libavfilter-soc?: implement fish filter

[Stefano Sabatini]

On date Monday 2009-06-01 17:42:11 +0200, Michael Niedermayer encoded:
> On Mon, Jun 01, 2009 at 04:11:02PM +0200, Stefano Sabatini wrote:
> > On date Monday 2009-06-01 14:04:05 +0200, Stefano Sabatini encoded:
> > > On date Monday 2009-06-01 13:56:14 +0200, Stefano Sabatini encoded:
> > > > On date Monday 2009-06-01 11:51:56 +0200, Michael Niedermayer encoded:
> > > > > On Sun, May 31, 2009 at 05:12:56PM +0200, Stefano Sabatini wrote:
> > > > > > On date Saturday 2009-05-30 22:04:52 +0200, Michael Niedermayer encoded:
> > > > > [...]
> > > > > > > > +
> > > > > > > > +typedef struct {
> > > > > > > > +    const AVClass *class;
> > > > > > > > +
> > > > > > > > +    int w, h;
> > > > > > > 
> > > > > > > > +    char *h_range, *s_range, *v_range;
> > > > > > > 
> > > > > > > They are ultimately not strings
> > > > > > 
> > > > > > Yes, but then how should I put them in the context when using
> > > > > > av_set_options_string()? (BTW this is the same "trick" used to set an
> > > > > > expression).
> > > > > 
> > > > > the ranges can be set to expressions that get evaluated for each frame?
> > > > 
> > > > I believe so and that would be cool, that could be:
> > > > expr='between(h, 100, 200) * between(v, 100, 255)'
> > > > or
> > > > expr='between(h, mod(N), N+100) * between(v, 100, 255)'
> > > > 
> > > > I'm supposing speed is not an issue for this filter, right?
> > > 
> > > After one minute of thinking, having h_first, h_second, v_first,
> > > v_second, ...
> > > 
> > > would require just a per-frame evaluation, that's better than to
> > > evaluate an expression for each pixel.
> > 
> > Patch updated, I'm not yet satisfied with how the S/V ranges are
> > dealt, what about to normalize S/V ranges this way:
> > 100:70  -> 70:70
> > -5:5    -> 0:5
> > 250:260 -> 250:255
> > ?
> [...]
> > +typedef struct {
> > +    const AVClass *class;
> > +
> > +    int w, h;
> > +
> > +    double  var_values[VARS_NB];
> > +
> > +    char *h_first_expr, *h_second_expr,
> > +         *s_min_expr, *s_max_expr,
> > +         *v_min_expr, *v_max_expr;
> > +
> 
> > +    AVEvalExpr *h_first_evalexpr, *h_second_evalexpr,
> > +               *s_min_evalexpr, *s_max_evalexpr,
> > +               *v_min_evalexpr, *v_max_evalexpr;
> 
> doesnt need to be in the context
> 
> 
> 
> > +
> > +    int hsub, vsub;
> > +
> 
> > +    char *zap_color_str;    ///< if specified, not matched pixels will be set to zap_color
> 
> same issue as ive mentioned in the previous review
> 
> to evaluate an expression per frame, the corresponding char* expression
> OR the AVEvalExpr must be kept but NOT both
> 
> to parse a single int [4] once you do not need the string

So I have to repeat again the question:

then how should I put them in the context when using
av_set_options_string()?

There's no way to set a color directly using av_set_string(), same for
an expression, that's why I need to store the string in the context
and *then* convert it to the target struct, I can't see how to avoid
that but extending opt.c.
 
> your code is a mess ...
> 
> iam not sure what to do here.
> The original fish filter did not do any of that, also i think i should
> remind you that porting code and any other change unrelated to porting must
> be in seperate patches.

That would be more work for small gain, I'm not sure I'm willing to do
that.

> This is even more important once it becomes obvious that some of these
> unrelated changes are considered messy by some other developer

Regards.
-- 
FFmpeg = Freak & Fundamental Mere Peaceful Erotic Geek