[FFmpeg-devel] [PATCH] lavfi: add dejudder filter to remove judder produced by partially telecined material.

[Nicholas Robbins]

> On Saturday, February 8, 2014 5:56 PM, Stefano Sabatini <stefasab at gmail.com> wrote:

> > Subject nit:
> lavfi: add dejudder filter to remove judder ...

Fixed.

>>  + at section dejudder
>>  +
>>  +Remove judder. Judder can be introduced, for instance, by 
> @ref{pullup} filter.
> 
> Remove judder produced by partially interlaced telecined content.
> 
> Judder can be introduced ...
> 
> Note: I'm always suprised by the amont of cr at p relared to interlaced
> content (it seems most our filters are used to fix one problem or
> another related to it...).

Fixed. Well, less and less material is interlaced, so hopefully this problem will fade.
 
>>  +This filter accepts the following option:
> 
> options:

Fixed.
 
>>  +typedef struct {
>>  +    const AVClass *class;
> 
>>  +    int64_t *ringbuff;
>>  +    int a, b, c, d;
> 
> comments/doxy are welcome here, also "a, b, c, d" are not very
> descriptive variable names.

a--d are indexes to a ring buffer. I could name them "previous, second-previous, ultimate, penultimate" but that seemed cumbersome. Would a comment here about what they are be helpful?

>>  +    {"cycle", "length of the cycle to use for 
> dejuddering",
> 
> nit: set length ...
Fixed.

>>  +    outlink->time_base = av_mul_q(inlink->time_base, av_make_q(1, 2 
> * dj->cycle));
> 
>>  +    outlink->frame_rate = av_mul_q(inlink->frame_rate, av_make_q(2 * 
> dj->cycle, 1));
> 
> What happens if frame_rate is not defined? Maybe you should abort in
> that case.

If frame_rate is not defined isn't it 1/0? in this case, I want to leave it at 1/0 which my code does. 

> nit++: remove duplicated empty lines, here and below

Fixed.

>>  +    dj->ringbuff = av_mallocz(sizeof(int64_t) * (dj->cycle+2));
> 
> nit: sizeof(*dj->ringbuff) * ...)

Fixed.

>>  +static int filter_frame(AVFilterLink *inlink, AVFrame *frame)
>>  +{
>>  +    int i;
>>  +    AVFilterContext *ctx  = inlink->dst;
>>  +    AVFilterLink *outlink = ctx->outputs[0];
>>  +    DejudderContext *dj   = ctx->priv;
> 
>>  +    int64_t *judbuff      = dj->ringbuff;
> 
> you can probably remove the indirection and leave optims to the
> compiler

If I understand you, you are suggesting I replace all my judbuff's in the code with "inlink->dst->priv->ringbuff". This seems a little cumbersome. Is that what you mean? Other filters seem to do what I've done here.

>>  +    int64_t next_pts      = frame->pts;
>>  +    int64_t offset;
>>  +
>>  +
> 
>>  +    if (dj->start_count) {
>>  +        dj->start_count--;
>>  +        dj->new_pts = next_pts * 2 * dj->cycle;
> 
> what happens in case pts == AV_NOPTS_VALUE?

I didn't consider that. I've added code to just pass the frame then. When might a frame have AV_NOPTS_VALUE?

>>  +            for (i = 0; i < dj->cycle + 2; i++) judbuff[i] += 
> offset;
> 
> style: judbuff[i] += offset; in a separate line

Fixed.

>>  +AVFilter ff_vf_dejudder = {
>>  +    .name        = "dejudder",
> 
>>  +    .description = NULL_IF_CONFIG_SMALL("Remove judder produced by 
> pullup"),
> 
> missing ending point

Fixed.

Thanks for all your comments,

--
Nicholas Robbins