[FFmpeg-devel] [PATCH] avfilter/vf_dejudder: use the name 's' for the pointer to the private context

[Ganesh Ajjanagadde]

On Thu, Aug 20, 2015 at 11:24 AM, Paul B Mahol <onemda at gmail.com> wrote:
> This is shorter and consistent across filters.
>
> Signed-off-by: Paul B Mahol <onemda at 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).

>
> --
> 1.7.11.2
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel