[FFmpeg-devel] [PATCH] avfilter/vf_dejudder: use the name 's' for the pointer to the private context
Ganesh Ajjanagadde
gajjanag at mit.edu
Thu Aug 20 19:10:43 CEST 2015
On Thu, Aug 20, 2015 at 12:59 PM, Paul B Mahol <onemda at gmail.com> wrote:
> 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.
I know what s is doing, I just fail to see the connection between the name
"s" and "filter private context".
"p" is slightly better (p for private).
The name should reflect what it is in my view.
Leaving that aside, I doubt I was highly skewed in my choice of examples.
Stats on this would be helpful: what percentage of current filters use "s"?
IMHO, if you want to improve consistency,
do so, but do it in a way that improves the code.
I find "bbox", "dm", or "p" far more informative as variable names than the "s".
Why don't you try to get all filters to use short abbreviations (like
"bbox" or "dm"),
or use "p" which at least stands for private?
>
>>
>>>
>>> --
>>> 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
>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
More information about the ffmpeg-devel
mailing list