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

Paul B Mahol onemda at gmail.com
Thu Aug 20 18:59:01 CEST 2015


On 8/20/15, Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
> 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).

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 at ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>


More information about the ffmpeg-devel mailing list