[FFmpeg-devel] [Patch] New filter -- dejudder

[Nicholas Robbins]

> On Wednesday, January 29, 2014 3:03 PM, Lukasz Marek <lukasz.m.luki at gmail.com> wrote:

> > On 29.01.2014 18:52, Nicholas Robbins wrote:
> 
>>  This filter removes the judder introduced by -vf pullup into videos that 
> were partially telescened.
> Missing doc and change log update
added.

> You are inconsistent with spaces around =+-*/ personally I don't care 
> much about it, but inconsistency in commit should be fixed.
> Also spaces between if/for and ( are missing.

Fixed. 

>>  +typedef struct {
>>  +    const AVClass *class;
>>  +    int64_t *ringbuff;
>>  +    int a,b,c,d;
> 
> Can they be more meaningful?

They are indices to a circular buffer. I wanted them to be short. If it is preferable I could change them to two_previous, previous, ultimate, penultimate. It will make the "new_pts +=..." line in filter_frame longer.


>>  +    int64_t new,next;
> 
> I know this is C, but I try to not use reserved words for C++. new can 
> be replaced anyway with something more specific, like new_pts?
> And also, new can be moved to local variable in filter_frame, and next 
> variable seems to be not used at all.

new changed to new_pts, it is carried over from one frame to the next, so can't be local to filter_frame. next removed.


>>  +    int startcount;
> 
> maybe start_count?
Done.

> I'm not native speaker so I may be wrong, but "length of the 
> cycle" 
> seems more natural to me.
Done.

>>  +    dj->ringbuff=(int64_t *)av_mallocz(sizeof(int64_t)*(dj->cycle+2));
> 
> A cast seems to be unneeded.
Removed.

 
>>  +    if (!dj->ringbuff) return AVERROR(ENOMEM);
> 
> Moving return to new line makes it a bit more readable.
Done.

>>  +    av_freep(&(dj->ringbuff));
>>  +}
>>  +
>>  +
>>  +
> 
> no need to make 3 lines break.

Done.

>>  +    frame->pts = dj->new;
>>  +
>>  +    for(i=0;i<dj->cycle+2;i++) av_log(ctx, AV_LOG_DEBUG, 
> "%"PRId64"\t", judbuff[i]);
> 
> move av_log to new line.
Done.

> I don't know filters API nor algorithm so can review at this point of view.
> 
> 
> -- 
> Best Regards,
> Lukasz Marek

Thank you for the review. As I said before, this is my first time contributing to a project like ffmpeg, so please pardon any rookie mistakes. I'm happy to learn how you all do things. I've attached the new patch.


Nicholas Robbins
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Adding-dejudder-filter-to-remove-judder-produced-by-.patch
Type: text/x-patch
Size: 6397 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140129/675e1fcc/attachment.bin>