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).
>
> --
> 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
