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

[Michael Niedermayer]

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

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.
This is even more important once it becomes obvious that some of these
unrelated changes are considered messy by some other developer


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

Freedom in capitalist society always remains about the same as it was in
ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090601/604f8860/attachment.pgp>