[FFmpeg-devel] [PATCH] lavfi: port decimate libmpcodecs filter

[Stefano Sabatini]

On date Tuesday 2012-08-28 15:36:57 +0200, Nicolas George encoded:
> Le primidi 11 fructidor, an CCXX, Stefano Sabatini a écrit :
> > Are you suggesting to change the filter to accept an int rather than a
> > float? That would break compatibility with the previous filter.
> 
> It would not be practical for the user. I am just suggesting to immediately
> multiply it by 1<<20 and store it as an integer. Testing would work better.

I'll comment in another mail.
 
> > diff --git a/libavfilter/libmpcodecs/vf_decimate.c b/libavfilter/libmpcodecs/vf_decimate.c
> > index 1fd7bce..e9a03f1 100644
> > --- a/libavfilter/libmpcodecs/vf_decimate.c
> > +++ b/libavfilter/libmpcodecs/vf_decimate.c
> 
> This part looks strange. Are those changes made for testing that crept to
> the patch?

Yes, I'll remove that part before pushing (and I'm realizing more and
more how a diff/psnr filter would be useful for testing ports).
 
> > +typedef struct {
> > +    int lo, hi;                    ///< lower and higher threshold number of differences
> > +                                   ///< values for 8x8 blocks
> 
> Does it work with multiple lines like that?

It should according to Doxygen manual (no I didn't test, as I don't
believe that internal doxy rendering is so important).

> 
> > +/**
> > + * Return 1 in case the two frames are different, 0 otherwise.
> > + */
> 
> s/frames/planes/?
> s/in case/if/?
> 
> > +/**
> > + * Tell if the frame should be decimated, that is if it is no much
> > + * different with respect to the reference frame ref.
> > + */
> > +static int decimate_frame(AVFilterContext *ctx,
> > +                          AVFilterBufferRef *cur, AVFilterBufferRef *ref)
> 
> If I read things correctly, the return values of diff_planes and
> decimate_frame are in the opposite directions: diff_planes = 1 -> the planes
> are different, decimate_frame = 1 -> the frames are not different.

Yes, I kept the original filter logic, since the function takes into
account considerations about the frame drop/keep count, so
diff_frame() was going to be a slightly misleading name. I slightly
reworded the comment.

> These are very minor nits, feel free to push if no one else objects. Thanks
> for the great work!
-- 
FFmpeg = Fostering Forgiving Maxi Philosofic Educated Gnome