On 8/20/15, Ganesh Ajjanagadde <gajja...@mit.edu> wrote:
> On Thu, Aug 20, 2015 at 11:24 AM, Paul B Mahol <one...@gmail.com> wrote:
>> This is shorter and consistent across filters.
>>
>> Signed-off-by: Paul B Mahol <one...@gmail.com>
>> ---
>> libavfilter/vf_dejudder.c | 66
>> +++++++++++++++++++++++------------------------
>> 1 file changed, 33 insertions(+), 33 deletions(-)
>>
>> diff --git a/libavfilter/vf_dejudder.c b/libavfilter/vf_dejudder.c
>> index ab525b6..4705cb6 100644
>> --- a/libavfilter/vf_dejudder.c
>> +++ b/libavfilter/vf_dejudder.c
>> @@ -55,7 +55,7 @@
>> #include "internal.h"
>> #include "video.h"
>>
>> -typedef struct {
>> +typedef struct DejudderContext {
>
> This should not be part of this patch.
> Doing a random scan through filters,
> it seems like there is a lack of consistency on this typedef:
> see e.g vf_decimate, vf_bbox (written by others),
> af_aphaser (written by you), and this one (in its current state).
>
> If you want consistency in this, perhaps a single patch going through
> all filters and unifying this would be useful.
>
> As an aside, I do not like typedef of structures at all;
> note that kernel coding style expressly forbids this:
> see e.g
> https://stackoverflow.com/questions/252780/why-should-we-typedef-a-struct-so-often-in-c
> (Jens and Jerry Hick's answer) and comments for that.
> However, this is rampant in FFmpeg,
> and will need further discussion.
> Notice that the developer documentation does not cover this.
>
>> const AVClass *class;
>> int64_t *ringbuff;
>> int i1, i2, i3, i4;
>> @@ -80,40 +80,40 @@ AVFILTER_DEFINE_CLASS(dejudder);
>> static int config_out_props(AVFilterLink *outlink)
>> {
>> AVFilterContext *ctx = outlink->src;
>> - DejudderContext *dj = ctx->priv;
>> + DejudderContext *s = ctx->priv;
>> AVFilterLink *inlink = outlink->src->inputs[0];
>>
>> - 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));
>> + outlink->time_base = av_mul_q(inlink->time_base, av_make_q(1, 2 *
>> s->cycle));
>> + outlink->frame_rate = av_mul_q(inlink->frame_rate, av_make_q(2 *
>> s->cycle, 1));
>>
>> - av_log(ctx, AV_LOG_VERBOSE, "cycle:%d\n", dj->cycle);
>> + av_log(ctx, AV_LOG_VERBOSE, "cycle:%d\n", s->cycle);
>>
>> return 0;
>> }
>>
>> static av_cold int dejudder_init(AVFilterContext *ctx)
>> {
>> - DejudderContext *dj = ctx->priv;
>> + DejudderContext *s = ctx->priv;
>>
>> - dj->ringbuff = av_mallocz_array(dj->cycle+2, sizeof(*dj->ringbuff));
>> - if (!dj->ringbuff)
>> + s->ringbuff = av_mallocz_array(s->cycle+2, sizeof(*s->ringbuff));
>> + if (!s->ringbuff)
>> return AVERROR(ENOMEM);
>>
>> - dj->new_pts = 0;
>> - dj->i1 = 0;
>> - dj->i2 = 1;
>> - dj->i3 = 2;
>> - dj->i4 = 3;
>> - dj->start_count = dj->cycle + 2;
>> + s->new_pts = 0;
>> + s->i1 = 0;
>> + s->i2 = 1;
>> + s->i3 = 2;
>> + s->i4 = 3;
>> + s->start_count = s->cycle + 2;
>>
>> return 0;
>> }
>>
>> static av_cold void dejudder_uninit(AVFilterContext *ctx)
>> {
>> - DejudderContext *dj = ctx->priv;
>> + DejudderContext *s = ctx->priv;
>>
>> - av_freep(&(dj->ringbuff));
>> + av_freep(&(s->ringbuff));
>> }
>>
>> static int filter_frame(AVFilterLink *inlink, AVFrame *frame)
>> @@ -121,36 +121,36 @@ static int filter_frame(AVFilterLink *inlink,
>> AVFrame *frame)
>> int k;
>> AVFilterContext *ctx = inlink->dst;
>> AVFilterLink *outlink = ctx->outputs[0];
>> - DejudderContext *dj = ctx->priv;
>> - int64_t *judbuff = dj->ringbuff;
>> + DejudderContext *s = ctx->priv;
>> + int64_t *judbuff = s->ringbuff;
>> int64_t next_pts = frame->pts;
>> int64_t offset;
>>
>> if (next_pts == AV_NOPTS_VALUE)
>> return ff_filter_frame(outlink, frame);
>>
>> - if (dj->start_count) {
>> - dj->start_count--;
>> - dj->new_pts = next_pts * 2 * dj->cycle;
>> + if (s->start_count) {
>> + s->start_count--;
>> + s->new_pts = next_pts * 2 * s->cycle;
>> } else {
>> - if (next_pts < judbuff[dj->i2]) {
>> - offset = next_pts + judbuff[dj->i3] - judbuff[dj->i4] -
>> judbuff[dj->i1];
>> - for (k = 0; k < dj->cycle + 2; k++)
>> + if (next_pts < judbuff[s->i2]) {
>> + offset = next_pts + judbuff[s->i3] - judbuff[s->i4] -
>> judbuff[s->i1];
>> + for (k = 0; k < s->cycle + 2; k++)
>> judbuff[k] += offset;
>> }
>> - dj->new_pts += (dj->cycle - 1) * (judbuff[dj->i3] -
>> judbuff[dj->i1])
>> - + (dj->cycle + 1) * (next_pts - judbuff[dj->i4]);
>> + s->new_pts += (s->cycle - 1) * (judbuff[s->i3] - judbuff[s->i1])
>> + + (s->cycle + 1) * (next_pts - judbuff[s->i4]);
>> }
>>
>> - judbuff[dj->i2] = next_pts;
>> - dj->i1 = dj->i2;
>> - dj->i2 = dj->i3;
>> - dj->i3 = dj->i4;
>> - dj->i4 = (dj->i4 + 1) % (dj->cycle + 2);
>> + judbuff[s->i2] = next_pts;
>> + s->i1 = s->i2;
>> + s->i2 = s->i3;
>> + s->i3 = s->i4;
>> + s->i4 = (s->i4 + 1) % (s->cycle + 2);
>>
>> - frame->pts = dj->new_pts;
>> + frame->pts = s->new_pts;
>>
>> - for (k = 0; k < dj->cycle + 2; k++)
>> + for (k = 0; k < s->cycle + 2; k++)
>> av_log(ctx, AV_LOG_DEBUG, "%"PRId64"\t", judbuff[k]);
>> av_log(ctx, AV_LOG_DEBUG, "next=%"PRId64", new=%"PRId64"\n",
>> next_pts, frame->pts);
>
> I am not convinced that this is "consistent across all filters".
> See e.g vf_decimate, vf_bbox, or af_aphaser (p instead of s).
> What does p or s even mean?
> You are saving a single character that yields no information whatsoever on
> what
> the variable represents as far as I can tell.
> Maybe I am missing the connection between a context and its abbreviation
> (s).
s is used as name for pointer to filter private context across bunch of filters.
So patch is to increase consistency.
>
>>
>> --
>> 1.7.11.2
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
