diff mbox series

[FFmpeg-devel,03/12] lavfi: drop vf_qp

Message ID 20200224123739.31154-4-anton@khirnov.net
State New
Headers show
Series [FFmpeg-devel,01/12] fifo: uninline av_fifo_peek2() on the next major bump
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Anton Khirnov Feb. 24, 2020, 12:37 p.m. UTC
It fundamentally depends on an API that has been deprecated for five
years, has seen no commits since that time and is of highly dubious
usefulness.
---
 doc/filters.texi            |  32 -------
 libavfilter/Makefile        |   1 -
 libavfilter/allfilters.c    |   1 -
 libavfilter/vf_qp.c         | 183 ------------------------------------
 tests/fate/filter-video.mak |   7 +-
 tests/ref/fate/filter-pp2   |   1 -
 tests/ref/fate/filter-pp3   |   1 -
 7 files changed, 1 insertion(+), 225 deletions(-)
 delete mode 100644 libavfilter/vf_qp.c
 delete mode 100644 tests/ref/fate/filter-pp2
 delete mode 100644 tests/ref/fate/filter-pp3

Comments

Carl Eugen Hoyos Feb. 24, 2020, 12:50 p.m. UTC | #1
Am Mo., 24. Feb. 2020 um 13:40 Uhr schrieb Anton Khirnov <anton@khirnov.net>:
>
> It fundamentally depends on an API that has been deprecated for five
> years, has seen no commits since that time and is of highly dubious
> usefulness.

Please explain how the removed functionality was replaced.

Carl Eugen
Paul B Mahol Feb. 24, 2020, 12:56 p.m. UTC | #2
Filter should not be removed, it should use qp via frame side data.

On 2/24/20, Anton Khirnov <anton@khirnov.net> wrote:
> It fundamentally depends on an API that has been deprecated for five
> years, has seen no commits since that time and is of highly dubious
> usefulness.
> ---
>  doc/filters.texi            |  32 -------
>  libavfilter/Makefile        |   1 -
>  libavfilter/allfilters.c    |   1 -
>  libavfilter/vf_qp.c         | 183 ------------------------------------
>  tests/fate/filter-video.mak |   7 +-
>  tests/ref/fate/filter-pp2   |   1 -
>  tests/ref/fate/filter-pp3   |   1 -
>  7 files changed, 1 insertion(+), 225 deletions(-)
>  delete mode 100644 libavfilter/vf_qp.c
>  delete mode 100644 tests/ref/fate/filter-pp2
>  delete mode 100644 tests/ref/fate/filter-pp3
>
> diff --git a/doc/filters.texi b/doc/filters.texi
> index 70fd7a4cc7..2a1235183f 100644
> --- a/doc/filters.texi
> +++ b/doc/filters.texi
> @@ -15335,38 +15335,6 @@ telecine NTSC input:
>  ffmpeg -i input -vf pullup -r 24000/1001 ...
>  @end example
>
> -@section qp
> -
> -Change video quantization parameters (QP).
> -
> -The filter accepts the following option:
> -
> -@table @option
> -@item qp
> -Set expression for quantization parameter.
> -@end table
> -
> -The expression is evaluated through the eval API and can contain, among
> others,
> -the following constants:
> -
> -@table @var
> -@item known
> -1 if index is not 129, 0 otherwise.
> -
> -@item qp
> -Sequential index starting from -129 to 128.
> -@end table
> -
> -@subsection Examples
> -
> -@itemize
> -@item
> -Some equation like:
> -@example
> -qp=2+2*sin(PI*qp)
> -@end example
> -@end itemize
> -
>  @section random
>
>  Flush video frames from internal cache of frames into a random order.
> diff --git a/libavfilter/Makefile b/libavfilter/Makefile
> index 089880a39d..74968b32e1 100644
> --- a/libavfilter/Makefile
> +++ b/libavfilter/Makefile
> @@ -349,7 +349,6 @@ OBJS-$(CONFIG_PROGRAM_OPENCL_FILTER)         +=
> vf_program_opencl.o opencl.o fra
>  OBJS-$(CONFIG_PSEUDOCOLOR_FILTER)            += vf_pseudocolor.o
>  OBJS-$(CONFIG_PSNR_FILTER)                   += vf_psnr.o framesync.o
>  OBJS-$(CONFIG_PULLUP_FILTER)                 += vf_pullup.o
> -OBJS-$(CONFIG_QP_FILTER)                     += vf_qp.o
>  OBJS-$(CONFIG_RANDOM_FILTER)                 += vf_random.o
>  OBJS-$(CONFIG_READEIA608_FILTER)             += vf_readeia608.o
>  OBJS-$(CONFIG_READVITC_FILTER)               += vf_readvitc.o
> diff --git a/libavfilter/allfilters.c b/libavfilter/allfilters.c
> index 88ebd121ad..aa6f006ddb 100644
> --- a/libavfilter/allfilters.c
> +++ b/libavfilter/allfilters.c
> @@ -332,7 +332,6 @@ extern AVFilter ff_vf_program_opencl;
>  extern AVFilter ff_vf_pseudocolor;
>  extern AVFilter ff_vf_psnr;
>  extern AVFilter ff_vf_pullup;
> -extern AVFilter ff_vf_qp;
>  extern AVFilter ff_vf_random;
>  extern AVFilter ff_vf_readeia608;
>  extern AVFilter ff_vf_readvitc;
> diff --git a/libavfilter/vf_qp.c b/libavfilter/vf_qp.c
> deleted file mode 100644
> index 33d39493bc..0000000000
> --- a/libavfilter/vf_qp.c
> +++ /dev/null
> @@ -1,183 +0,0 @@
> -/*
> - * Copyright (C) 2004 Michael Niedermayer <michaelni@gmx.at>
> - *
> - * This file is part of FFmpeg.
> - *
> - * FFmpeg is free software; you can redistribute it and/or
> - * modify it under the terms of the GNU Lesser General Public
> - * License as published by the Free Software Foundation; either
> - * version 2.1 of the License, or (at your option) any later version.
> - *
> - * FFmpeg is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> - * Lesser General Public License for more details.
> - *
> - * You should have received a copy of the GNU Lesser General Public
> - * License along with FFmpeg; if not, write to the Free Software
> - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301
> USA
> - */
> -
> -#include <math.h>
> -#include "libavutil/eval.h"
> -#include "libavutil/imgutils.h"
> -#include "libavutil/pixdesc.h"
> -#include "libavutil/opt.h"
> -#include "avfilter.h"
> -#include "formats.h"
> -#include "internal.h"
> -#include "video.h"
> -
> -typedef struct QPContext {
> -    const AVClass *class;
> -    char *qp_expr_str;
> -    int8_t lut[257];
> -    int h, qstride;
> -    int evaluate_per_mb;
> -} QPContext;
> -
> -#define OFFSET(x) offsetof(QPContext, x)
> -#define FLAGS AV_OPT_FLAG_FILTERING_PARAM|AV_OPT_FLAG_VIDEO_PARAM
> -
> -static const AVOption qp_options[] = {
> -    { "qp", "set qp expression", OFFSET(qp_expr_str), AV_OPT_TYPE_STRING,
> {.str=NULL}, 0, 0, FLAGS },
> -    { NULL }
> -};
> -
> -AVFILTER_DEFINE_CLASS(qp);
> -
> -static int config_input(AVFilterLink *inlink)
> -{
> -    AVFilterContext *ctx = inlink->dst;
> -    QPContext *s = ctx->priv;
> -    int i;
> -    int ret;
> -    AVExpr *e = NULL;
> -    static const char *var_names[] = { "known", "qp", "x", "y", "w", "h",
> NULL };
> -
> -    if (!s->qp_expr_str)
> -        return 0;
> -
> -    ret = av_expr_parse(&e, s->qp_expr_str, var_names, NULL, NULL, NULL,
> NULL, 0, ctx);
> -    if (ret < 0)
> -        return ret;
> -
> -    s->h       = (inlink->h + 15) >> 4;
> -    s->qstride = (inlink->w + 15) >> 4;
> -    for (i = -129; i < 128; i++) {
> -        double var_values[] = { i != -129, i, NAN, NAN, s->qstride, s->h,
> 0};
> -        double temp_val = av_expr_eval(e, var_values, NULL);
> -
> -        if (isnan(temp_val)) {
> -            if(strchr(s->qp_expr_str, 'x') || strchr(s->qp_expr_str, 'y'))
> -                s->evaluate_per_mb = 1;
> -            else {
> -                av_expr_free(e);
> -                return AVERROR(EINVAL);
> -            }
> -        }
> -
> -        s->lut[i + 129] = lrintf(temp_val);
> -    }
> -    av_expr_free(e);
> -
> -    return 0;
> -}
> -
> -static int filter_frame(AVFilterLink *inlink, AVFrame *in)
> -{
> -    AVFilterContext *ctx = inlink->dst;
> -    AVFilterLink *outlink = ctx->outputs[0];
> -    QPContext *s = ctx->priv;
> -    AVBufferRef *out_qp_table_buf;
> -    AVFrame *out = NULL;
> -    const int8_t *in_qp_table;
> -    int type, stride, ret;
> -
> -    if (!s->qp_expr_str || ctx->is_disabled)
> -        return ff_filter_frame(outlink, in);
> -
> -    out_qp_table_buf = av_buffer_alloc(s->h * s->qstride);
> -    if (!out_qp_table_buf) {
> -        ret = AVERROR(ENOMEM);
> -        goto fail;
> -    }
> -
> -    out = av_frame_clone(in);
> -    if (!out) {
> -        av_buffer_unref(&out_qp_table_buf);
> -        ret = AVERROR(ENOMEM);
> -        goto fail;
> -    }
> -
> -    in_qp_table = av_frame_get_qp_table(in, &stride, &type);
> -    av_frame_set_qp_table(out, out_qp_table_buf, s->qstride, type);
> -
> -
> -    if (s->evaluate_per_mb) {
> -        int y, x;
> -
> -        for (y = 0; y < s->h; y++)
> -            for (x = 0; x < s->qstride; x++) {
> -                int qp = in_qp_table ? in_qp_table[x + stride * y] : NAN;
> -                double var_values[] = { !!in_qp_table, qp, x, y,
> s->qstride, s->h, 0};
> -                static const char *var_names[] = { "known", "qp", "x", "y",
> "w", "h", NULL };
> -                double temp_val;
> -
> -                ret = av_expr_parse_and_eval(&temp_val, s->qp_expr_str,
> -                                            var_names, var_values,
> -                                            NULL, NULL, NULL, NULL, 0, 0,
> ctx);
> -                if (ret < 0)
> -                    goto fail;
> -                out_qp_table_buf->data[x + s->qstride * y] =
> lrintf(temp_val);
> -            }
> -    } else if (in_qp_table) {
> -        int y, x;
> -
> -        for (y = 0; y < s->h; y++)
> -            for (x = 0; x < s->qstride; x++)
> -                out_qp_table_buf->data[x + s->qstride * y] = s->lut[129 +
> -                    ((int8_t)in_qp_table[x + stride * y])];
> -    } else {
> -        int y, x, qp = s->lut[0];
> -
> -        for (y = 0; y < s->h; y++)
> -            for (x = 0; x < s->qstride; x++)
> -                out_qp_table_buf->data[x + s->qstride * y] = qp;
> -    }
> -
> -    ret = ff_filter_frame(outlink, out);
> -    out = NULL;
> -fail:
> -    av_frame_free(&in);
> -    av_frame_free(&out);
> -    return ret;
> -}
> -
> -static const AVFilterPad qp_inputs[] = {
> -    {
> -        .name         = "default",
> -        .type         = AVMEDIA_TYPE_VIDEO,
> -        .filter_frame = filter_frame,
> -        .config_props = config_input,
> -    },
> -    { NULL }
> -};
> -
> -static const AVFilterPad qp_outputs[] = {
> -    {
> -        .name = "default",
> -        .type = AVMEDIA_TYPE_VIDEO,
> -    },
> -    { NULL }
> -};
> -
> -AVFilter ff_vf_qp = {
> -    .name          = "qp",
> -    .description   = NULL_IF_CONFIG_SMALL("Change video quantization
> parameters."),
> -    .priv_size     = sizeof(QPContext),
> -    .inputs        = qp_inputs,
> -    .outputs       = qp_outputs,
> -    .priv_class    = &qp_class,
> -    .flags         = AVFILTER_FLAG_SUPPORT_TIMELINE_INTERNAL,
> -};
> diff --git a/tests/fate/filter-video.mak b/tests/fate/filter-video.mak
> index 2da27f714a..5f4fd75b40 100644
> --- a/tests/fate/filter-video.mak
> +++ b/tests/fate/filter-video.mak
> @@ -531,21 +531,16 @@ fate-filter-idet: CMD = framecrc -flags bitexact -idct
> simple -i $(SRC) -vf idet
>  FATE_FILTER_VSYNTH-$(CONFIG_PAD_FILTER) += fate-filter-pad
>  fate-filter-pad: CMD = video_filter "pad=iw*1.5:ih*1.5:iw*0.3:ih*0.2"
>
> -FATE_FILTER_PP = fate-filter-pp fate-filter-pp1 fate-filter-pp2
> fate-filter-pp3 fate-filter-pp4 fate-filter-pp5 fate-filter-pp6
> +FATE_FILTER_PP = fate-filter-pp fate-filter-pp1 fate-filter-pp4
> fate-filter-pp5 fate-filter-pp6
>  FATE_FILTER_VSYNTH-$(CONFIG_PP_FILTER) += $(FATE_FILTER_PP)
>  $(FATE_FILTER_PP): fate-vsynth1-mpeg4-qprd
>
>  fate-filter-pp:  CMD = framecrc -flags bitexact -idct simple -i
> $(TARGET_PATH)/tests/data/fate/vsynth1-mpeg4-qprd.avi -frames:v 5 -flags
> +bitexact -vf "pp=be/hb/vb/tn/l5/al"
>  fate-filter-pp1: CMD = video_filter "pp=fq|4/be/hb/vb/tn/l5/al"
> -fate-filter-pp2: CMD = video_filter "qp=x+y,pp=be/h1/v1/lb"
> -fate-filter-pp3: CMD = video_filter "qp=x+y,pp=be/ha|128|7/va/li"
>  fate-filter-pp4: CMD = video_filter "pp=be/ci"
>  fate-filter-pp5: CMD = video_filter "pp=md"
>  fate-filter-pp6: CMD = video_filter "pp=be/fd"
>
> -FATE_FILTER_VSYNTH-$(call ALLYES, QP_FILTER PP_FILTER) += fate-filter-qp
> -fate-filter-qp: CMD = video_filter "qp=17,pp=be/hb/vb/tn/l5/al"
> -
>  FATE_FILTER_VSYNTH-$(CONFIG_SELECT_FILTER) += fate-filter-select
>  fate-filter-select: CMD = framecrc -flags bitexact -idct simple -i $(SRC)
> -vf "select=not(eq(mod(n\,2)\,0)+eq(mod(n\,3)\,0))" -frames:v 25 -flags
> +bitexact
>
> diff --git a/tests/ref/fate/filter-pp2 b/tests/ref/fate/filter-pp2
> deleted file mode 100644
> index ed5e77322a..0000000000
> --- a/tests/ref/fate/filter-pp2
> +++ /dev/null
> @@ -1 +0,0 @@
> -pp2                 566d48ad25dfa7a9680de933cbdf66d9
> diff --git a/tests/ref/fate/filter-pp3 b/tests/ref/fate/filter-pp3
> deleted file mode 100644
> index 536bf8e9d2..0000000000
> --- a/tests/ref/fate/filter-pp3
> +++ /dev/null
> @@ -1 +0,0 @@
> -pp3                 586fc14a52699540a865c070dd113229
> --
> 2.24.1
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Anton Khirnov Feb. 24, 2020, 2:54 p.m. UTC | #3
Quoting Carl Eugen Hoyos (2020-02-24 13:50:57)
> Am Mo., 24. Feb. 2020 um 13:40 Uhr schrieb Anton Khirnov <anton@khirnov.net>:
> >
> > It fundamentally depends on an API that has been deprecated for five
> > years, has seen no commits since that time and is of highly dubious
> > usefulness.
> 
> Please explain how the removed functionality was replaced.

It was not, for the reasons mentioned in the commit message. In my view,
the fact that nobody fixed it in all that time proves that nobody cares
about this functionality and thus that there is no value in keeping it.

Furthermore, I believe this filter (and all the associated
"postprocessing" ones) are anachronistic relics of the DivX era. They
were in fashion around ~2005 (though I doubt they were actually
improving anything even then) but nobody with a clue has used them since
H.264 took over. The value they bring to the project is actually
negative, since users who don't know any better might use them under
false impression that they would improve video quality. MPV removed
those filters eight years ago and AFAIK nobody ever missed them.
Carl Eugen Hoyos Feb. 24, 2020, 3:21 p.m. UTC | #4
> Am 24.02.2020 um 15:54 schrieb Anton Khirnov <anton@khirnov.net>:
> 
> Quoting Carl Eugen Hoyos (2020-02-24 13:50:57)
>>> Am Mo., 24. Feb. 2020 um 13:40 Uhr schrieb Anton Khirnov <anton@khirnov.net>:
>>> 
>>> It fundamentally depends on an API that has been deprecated for five
>>> years, has seen no commits since that time and is of highly dubious
>>> usefulness.
>> 
>> Please explain how the removed functionality was replaced.
> 
> It was not, for the reasons mentioned in the commit message. In my view,
> the fact that nobody fixed it in all that time proves that nobody cares
> about this functionality and thus that there is no value in keeping it.

In this case your patch set is not acceptable: I strongly suggest you work on something that improves FFmpeg instead of removing features.

Carl Eugen
James Almer Feb. 24, 2020, 3:51 p.m. UTC | #5
On Monday, February 24, 2020, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
>
>
>> Am 24.02.2020 um 15:54 schrieb Anton Khirnov <anton@khirnov.net>:
>>
>> Quoting Carl Eugen Hoyos (2020-02-24 13:50:57)
>>>> Am Mo., 24. Feb. 2020 um 13:40 Uhr schrieb Anton Khirnov <
anton@khirnov.net>:
>>>>
>>>> It fundamentally depends on an API that has been deprecated for five
>>>> years, has seen no commits since that time and is of highly dubious
>>>> usefulness.
>>>
>>> Please explain how the removed functionality was replaced.
>>
>> It was not, for the reasons mentioned in the commit message. In my view,
>> the fact that nobody fixed it in all that time proves that nobody cares
>> about this functionality and thus that there is no value in keeping it.
>
> In this case your patch set is not acceptable: I strongly suggest you
work on something that improves FFmpeg instead of removing features.
>
> Carl Eugen

Anton argued why it should be removed. You should do the same about why it
should not. Simply saying you are against removing features other
developers consider useless is not enough.

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Paul B Mahol Feb. 24, 2020, 4:02 p.m. UTC | #6
On 2/24/20, James Almer <jamrial@gmail.com> wrote:
> On Monday, February 24, 2020, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>>
>>
>>
>>> Am 24.02.2020 um 15:54 schrieb Anton Khirnov <anton@khirnov.net>:
>>>
>>> Quoting Carl Eugen Hoyos (2020-02-24 13:50:57)
>>>>> Am Mo., 24. Feb. 2020 um 13:40 Uhr schrieb Anton Khirnov <
> anton@khirnov.net>:
>>>>>
>>>>> It fundamentally depends on an API that has been deprecated for five
>>>>> years, has seen no commits since that time and is of highly dubious
>>>>> usefulness.
>>>>
>>>> Please explain how the removed functionality was replaced.
>>>
>>> It was not, for the reasons mentioned in the commit message. In my view,
>>> the fact that nobody fixed it in all that time proves that nobody cares
>>> about this functionality and thus that there is no value in keeping it.
>>
>> In this case your patch set is not acceptable: I strongly suggest you
> work on something that improves FFmpeg instead of removing features.
>>
>> Carl Eugen
>
> Anton argued why it should be removed. You should do the same about why it
> should not. Simply saying you are against removing features other
> developers consider useless is not enough.

Filter as is was simply never marked for deprecation, same applies for
removed features to other filters in this set.

>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Soft Works Feb. 24, 2020, 4:13 p.m. UTC | #7
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Anton Khirnov
> Sent: Monday, February 24, 2020 3:55 PM
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 03/12] lavfi: drop vf_qp
> 
> Quoting Carl Eugen Hoyos (2020-02-24 13:50:57)
> > Am Mo., 24. Feb. 2020 um 13:40 Uhr schrieb Anton Khirnov
> <anton@khirnov.net>:
> > >
> > > It fundamentally depends on an API that has been deprecated for five
> > > years, has seen no commits since that time and is of highly dubious
> > > usefulness.
> >
> > Please explain how the removed functionality was replaced.
> 
> It was not, for the reasons mentioned in the commit message. In my view,
> the fact that nobody fixed it in all that time proves that nobody cares about
> this functionality and thus that there is no value in keeping it.
> 
> Furthermore, I believe this filter (and all the associated "postprocessing"
> ones) are anachronistic relics of the DivX era. They were in fashion around
> ~2005 (though I doubt they were actually improving anything even then) but
> nobody with a clue has used them since

Following those or similar arguments in a consequent way, would quickly 
constitute quite a list of ffmpeg features having "no value" anymore.

Removing features from one day to another would appear to me as a bit
too extreme, no matter how useless the feature might be.

Maybe it would make sense to introduce some kind of feature category
like "legacy features" where those types of features can be 'parked'
for a while before getting removed eventually. 

(of course, allowing to configure build with or without those features).

softworkz
Paul B Mahol Feb. 24, 2020, 4:38 p.m. UTC | #8
On 2/24/20, Soft Works <softworkz@hotmail.com> wrote:
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>> Anton Khirnov
>> Sent: Monday, February 24, 2020 3:55 PM
>> To: FFmpeg development discussions and patches <ffmpeg-
>> devel@ffmpeg.org>
>> Subject: Re: [FFmpeg-devel] [PATCH 03/12] lavfi: drop vf_qp
>>
>> Quoting Carl Eugen Hoyos (2020-02-24 13:50:57)
>> > Am Mo., 24. Feb. 2020 um 13:40 Uhr schrieb Anton Khirnov
>> <anton@khirnov.net>:
>> > >
>> > > It fundamentally depends on an API that has been deprecated for five
>> > > years, has seen no commits since that time and is of highly dubious
>> > > usefulness.
>> >
>> > Please explain how the removed functionality was replaced.
>>
>> It was not, for the reasons mentioned in the commit message. In my view,
>> the fact that nobody fixed it in all that time proves that nobody cares
>> about
>> this functionality and thus that there is no value in keeping it.
>>
>> Furthermore, I believe this filter (and all the associated
>> "postprocessing"
>> ones) are anachronistic relics of the DivX era. They were in fashion
>> around
>> ~2005 (though I doubt they were actually improving anything even then)
>> but
>> nobody with a clue has used them since
>
> Following those or similar arguments in a consequent way, would quickly
> constitute quite a list of ffmpeg features having "no value" anymore.
>

Please write such a list, I'm interested.

> Removing features from one day to another would appear to me as a bit
> too extreme, no matter how useless the feature might be.
>
> Maybe it would make sense to introduce some kind of feature category
> like "legacy features" where those types of features can be 'parked'
> for a while before getting removed eventually.
>
> (of course, allowing to configure build with or without those features).
>
> softworkz
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Anton Khirnov Feb. 24, 2020, 4:50 p.m. UTC | #9
Quoting Paul B Mahol (2020-02-24 17:02:52)
> On 2/24/20, James Almer <jamrial@gmail.com> wrote:
> > On Monday, February 24, 2020, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >>
> >>
> >>
> >>> Am 24.02.2020 um 15:54 schrieb Anton Khirnov <anton@khirnov.net>:
> >>>
> >>> Quoting Carl Eugen Hoyos (2020-02-24 13:50:57)
> >>>>> Am Mo., 24. Feb. 2020 um 13:40 Uhr schrieb Anton Khirnov <
> > anton@khirnov.net>:
> >>>>>
> >>>>> It fundamentally depends on an API that has been deprecated for five
> >>>>> years, has seen no commits since that time and is of highly dubious
> >>>>> usefulness.
> >>>>
> >>>> Please explain how the removed functionality was replaced.
> >>>
> >>> It was not, for the reasons mentioned in the commit message. In my view,
> >>> the fact that nobody fixed it in all that time proves that nobody cares
> >>> about this functionality and thus that there is no value in keeping it.
> >>
> >> In this case your patch set is not acceptable: I strongly suggest you
> > work on something that improves FFmpeg instead of removing features.
> >>
> >> Carl Eugen
> >
> > Anton argued why it should be removed. You should do the same about why it
> > should not. Simply saying you are against removing features other
> > developers consider useless is not enough.
> 
> Filter as is was simply never marked for deprecation, same applies for
> removed features to other filters in this set.

So what? It produced deprecation warnings on every build for five years.

Are you claiming you have a use case for it? Or know about someone who
does?
Anton Khirnov Feb. 24, 2020, 4:55 p.m. UTC | #10
Quoting Soft Works (2020-02-24 17:13:54)
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > Anton Khirnov
> > Sent: Monday, February 24, 2020 3:55 PM
> > To: FFmpeg development discussions and patches <ffmpeg-
> > devel@ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH 03/12] lavfi: drop vf_qp
> > 
> > Quoting Carl Eugen Hoyos (2020-02-24 13:50:57)
> > > Am Mo., 24. Feb. 2020 um 13:40 Uhr schrieb Anton Khirnov
> > <anton@khirnov.net>:
> > > >
> > > > It fundamentally depends on an API that has been deprecated for five
> > > > years, has seen no commits since that time and is of highly dubious
> > > > usefulness.
> > >
> > > Please explain how the removed functionality was replaced.
> > 
> > It was not, for the reasons mentioned in the commit message. In my view,
> > the fact that nobody fixed it in all that time proves that nobody cares about
> > this functionality and thus that there is no value in keeping it.
> > 
> > Furthermore, I believe this filter (and all the associated "postprocessing"
> > ones) are anachronistic relics of the DivX era. They were in fashion around
> > ~2005 (though I doubt they were actually improving anything even then) but
> > nobody with a clue has used them since
> 
> Following those or similar arguments in a consequent way, would quickly 
> constitute quite a list of ffmpeg features having "no value" anymore.

Yes, and all features with no value should be removed. They are a burden
for both developers and users. Keeping them is bad for the project and
not good for anyone.

> 
> Removing features from one day to another would appear to me as a bit
> too extreme, no matter how useless the feature might be.

It's not "from one day to another" though. This functionality has been
deprecated for five years. And in that entire time nobody ever had
enough interest to do anything about it.

> 
> Maybe it would make sense to introduce some kind of feature category
> like "legacy features" where those types of features can be 'parked'
> for a while before getting removed eventually. 

Git history is not going anywhere. If there is ever a use case for this
(which I strongly doubt, but could happen), people are free to take the
code from history and adapt it to whatever new API they come up with.
Anton Khirnov Feb. 24, 2020, 4:57 p.m. UTC | #11
Quoting Paul B Mahol (2020-02-24 13:56:56)
> Filter should not be removed, it should use qp via frame side data.

For what purpose? I am yet to hear about any valid use case for this
filter.
Soft Works Feb. 24, 2020, 5:04 p.m. UTC | #12
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Paul B Mahol
> Sent: Monday, February 24, 2020 5:39 PM
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 03/12] lavfi: drop vf_qp
> 
> On 2/24/20, Soft Works <softworkz@hotmail.com> wrote:
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> >> Anton Khirnov
> >> Sent: Monday, February 24, 2020 3:55 PM
> >> To: FFmpeg development discussions and patches <ffmpeg-
> >> devel@ffmpeg.org>
> >> Subject: Re: [FFmpeg-devel] [PATCH 03/12] lavfi: drop vf_qp
> >>
> >> Quoting Carl Eugen Hoyos (2020-02-24 13:50:57)
> >> > Am Mo., 24. Feb. 2020 um 13:40 Uhr schrieb Anton Khirnov
> >> <anton@khirnov.net>:
> >> > >
> >> > > It fundamentally depends on an API that has been deprecated for
> >> > > five years, has seen no commits since that time and is of highly
> >> > > dubious usefulness.
> >> >
> >> > Please explain how the removed functionality was replaced.
> >>
> >> It was not, for the reasons mentioned in the commit message. In my
> >> view, the fact that nobody fixed it in all that time proves that
> >> nobody cares about this functionality and thus that there is no value
> >> in keeping it.
> >>
> >> Furthermore, I believe this filter (and all the associated
> >> "postprocessing"
> >> ones) are anachronistic relics of the DivX era. They were in fashion
> >> around
> >> ~2005 (though I doubt they were actually improving anything even
> >> then) but nobody with a clue has used them since
> >
> > Following those or similar arguments in a consequent way, would
> > quickly constitute quite a list of ffmpeg features having "no value"
> anymore.
> >
> 
> Please write such a list, I'm interested.

Excellent trap, but I won't step in ;-)

I'm not even advocating to start some kind of deprecation cycle. All I
meant to say is that _if_ there's something to be removed, it might
possibly be a good idea to have some kind of "soft-" or "2-stage-"removal
which would provide an opportunity for users to accommodate 
or complain.

softworkz
Paul B Mahol Feb. 24, 2020, 5:07 p.m. UTC | #13
On 2/24/20, Anton Khirnov <anton@khirnov.net> wrote:
> Quoting Paul B Mahol (2020-02-24 17:02:52)
>> On 2/24/20, James Almer <jamrial@gmail.com> wrote:
>> > On Monday, February 24, 2020, Carl Eugen Hoyos <ceffmpeg@gmail.com>
>> > wrote:
>> >>
>> >>
>> >>
>> >>> Am 24.02.2020 um 15:54 schrieb Anton Khirnov <anton@khirnov.net>:
>> >>>
>> >>> Quoting Carl Eugen Hoyos (2020-02-24 13:50:57)
>> >>>>> Am Mo., 24. Feb. 2020 um 13:40 Uhr schrieb Anton Khirnov <
>> > anton@khirnov.net>:
>> >>>>>
>> >>>>> It fundamentally depends on an API that has been deprecated for five
>> >>>>> years, has seen no commits since that time and is of highly dubious
>> >>>>> usefulness.
>> >>>>
>> >>>> Please explain how the removed functionality was replaced.
>> >>>
>> >>> It was not, for the reasons mentioned in the commit message. In my
>> >>> view,
>> >>> the fact that nobody fixed it in all that time proves that nobody
>> >>> cares
>> >>> about this functionality and thus that there is no value in keeping
>> >>> it.
>> >>
>> >> In this case your patch set is not acceptable: I strongly suggest you
>> > work on something that improves FFmpeg instead of removing features.
>> >>
>> >> Carl Eugen
>> >
>> > Anton argued why it should be removed. You should do the same about why
>> > it
>> > should not. Simply saying you are against removing features other
>> > developers consider useless is not enough.
>>
>> Filter as is was simply never marked for deprecation, same applies for
>> removed features to other filters in this set.
>
> So what? It produced deprecation warnings on every build for five years.
>
> Are you claiming you have a use case for it? Or know about someone who
> does?

I believe there are still usecases for this filter and other filters.

What about other filters and other deprecation warnings?
Are filters gonna be removed because of single deprecation warning in file?

I think it was mistake to set qp side data as deprecated right after
its addition.

It is hurting our reputation when users look how we removed items
after few years
of usage or when we deprecate items right in same commit that added them.

>
> --
> Anton Khirnov
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Anton Khirnov Feb. 24, 2020, 5:50 p.m. UTC | #14
Quoting Paul B Mahol (2020-02-24 18:07:26)
> On 2/24/20, Anton Khirnov <anton@khirnov.net> wrote:
> > Quoting Paul B Mahol (2020-02-24 17:02:52)
> >> On 2/24/20, James Almer <jamrial@gmail.com> wrote:
> >> > On Monday, February 24, 2020, Carl Eugen Hoyos <ceffmpeg@gmail.com>
> >> > wrote:
> >> >>
> >> >>
> >> >>
> >> >>> Am 24.02.2020 um 15:54 schrieb Anton Khirnov <anton@khirnov.net>:
> >> >>>
> >> >>> Quoting Carl Eugen Hoyos (2020-02-24 13:50:57)
> >> >>>>> Am Mo., 24. Feb. 2020 um 13:40 Uhr schrieb Anton Khirnov <
> >> > anton@khirnov.net>:
> >> >>>>>
> >> >>>>> It fundamentally depends on an API that has been deprecated for five
> >> >>>>> years, has seen no commits since that time and is of highly dubious
> >> >>>>> usefulness.
> >> >>>>
> >> >>>> Please explain how the removed functionality was replaced.
> >> >>>
> >> >>> It was not, for the reasons mentioned in the commit message. In my
> >> >>> view,
> >> >>> the fact that nobody fixed it in all that time proves that nobody
> >> >>> cares
> >> >>> about this functionality and thus that there is no value in keeping
> >> >>> it.
> >> >>
> >> >> In this case your patch set is not acceptable: I strongly suggest you
> >> > work on something that improves FFmpeg instead of removing features.
> >> >>
> >> >> Carl Eugen
> >> >
> >> > Anton argued why it should be removed. You should do the same about why
> >> > it
> >> > should not. Simply saying you are against removing features other
> >> > developers consider useless is not enough.
> >>
> >> Filter as is was simply never marked for deprecation, same applies for
> >> removed features to other filters in this set.
> >
> > So what? It produced deprecation warnings on every build for five years.
> >
> > Are you claiming you have a use case for it? Or know about someone who
> > does?
> 
> I believe there are still usecases for this filter and other filters.

Elaborate please. What use cases? Actual or theoretical?

> 
> What about other filters and other deprecation warnings?
> Are filters gonna be removed because of single deprecation warning in file?

This is sophistry, the filter is not being dropped because of a minor
deprecation warning in it. The fundamental functionality which it is
built around is to be removed.

> 
> I think it was mistake to set qp side data as deprecated right after
> its addition.

This is not an an accurate description of what happened. Exporting QP
tables wasn't deprecated at that point. Rather the preexisting
functionality for exporting QP tables (as plain points to avcodec
internal buffers) was converted to newly added side data API to keep
things working for a while and see if anyone wants to keep this. Five
years passed and nobody did. Therefore it should be removed.

> 
> It is hurting our reputation when users look how we removed items
> after few years
> of usage or when we deprecate items right in same commit that added them.

I believe it hurts our reputation a lot more when our feature list reads
like state of the art from 2002, but necessary infrastructure
maintenance cannot be done because of the burden of all these
"features".

Users hate us a lot more for confusing inconsistent poorly documented
APIs which are hard to use correctly than for deprecating obsolete
filters.
Paul B Mahol Feb. 24, 2020, 6 p.m. UTC | #15
On 2/24/20, Anton Khirnov <anton@khirnov.net> wrote:
> Quoting Paul B Mahol (2020-02-24 18:07:26)
>> On 2/24/20, Anton Khirnov <anton@khirnov.net> wrote:
>> > Quoting Paul B Mahol (2020-02-24 17:02:52)
>> >> On 2/24/20, James Almer <jamrial@gmail.com> wrote:
>> >> > On Monday, February 24, 2020, Carl Eugen Hoyos <ceffmpeg@gmail.com>
>> >> > wrote:
>> >> >>
>> >> >>
>> >> >>
>> >> >>> Am 24.02.2020 um 15:54 schrieb Anton Khirnov <anton@khirnov.net>:
>> >> >>>
>> >> >>> Quoting Carl Eugen Hoyos (2020-02-24 13:50:57)
>> >> >>>>> Am Mo., 24. Feb. 2020 um 13:40 Uhr schrieb Anton Khirnov <
>> >> > anton@khirnov.net>:
>> >> >>>>>
>> >> >>>>> It fundamentally depends on an API that has been deprecated for
>> >> >>>>> five
>> >> >>>>> years, has seen no commits since that time and is of highly
>> >> >>>>> dubious
>> >> >>>>> usefulness.
>> >> >>>>
>> >> >>>> Please explain how the removed functionality was replaced.
>> >> >>>
>> >> >>> It was not, for the reasons mentioned in the commit message. In my
>> >> >>> view,
>> >> >>> the fact that nobody fixed it in all that time proves that nobody
>> >> >>> cares
>> >> >>> about this functionality and thus that there is no value in keeping
>> >> >>> it.
>> >> >>
>> >> >> In this case your patch set is not acceptable: I strongly suggest
>> >> >> you
>> >> > work on something that improves FFmpeg instead of removing features.
>> >> >>
>> >> >> Carl Eugen
>> >> >
>> >> > Anton argued why it should be removed. You should do the same about
>> >> > why
>> >> > it
>> >> > should not. Simply saying you are against removing features other
>> >> > developers consider useless is not enough.
>> >>
>> >> Filter as is was simply never marked for deprecation, same applies for
>> >> removed features to other filters in this set.
>> >
>> > So what? It produced deprecation warnings on every build for five years.
>> >
>> > Are you claiming you have a use case for it? Or know about someone who
>> > does?
>>
>> I believe there are still usecases for this filter and other filters.
>
> Elaborate please. What use cases? Actual or theoretical?
>
>>
>> What about other filters and other deprecation warnings?
>> Are filters gonna be removed because of single deprecation warning in
>> file?
>
> This is sophistry, the filter is not being dropped because of a minor
> deprecation warning in it. The fundamental functionality which it is
> built around is to be removed.
>
>>
>> I think it was mistake to set qp side data as deprecated right after
>> its addition.
>
> This is not an an accurate description of what happened. Exporting QP
> tables wasn't deprecated at that point. Rather the preexisting
> functionality for exporting QP tables (as plain points to avcodec
> internal buffers) was converted to newly added side data API to keep
> things working for a while and see if anyone wants to keep this. Five
> years passed and nobody did. Therefore it should be removed.
>
>>
>> It is hurting our reputation when users look how we removed items
>> after few years
>> of usage or when we deprecate items right in same commit that added them.
>
> I believe it hurts our reputation a lot more when our feature list reads
> like state of the art from 2002, but necessary infrastructure
> maintenance cannot be done because of the burden of all these
> "features".
>
> Users hate us a lot more for confusing inconsistent poorly documented
> APIs which are hard to use correctly than for deprecating obsolete
> filters.

There is lot of hate here, so I'm refrain from posting further.
Do as you and technical committee wish. I'm out of the game.

>
> --
> Anton Khirnov
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Michael Niedermayer Feb. 24, 2020, 7:15 p.m. UTC | #16
On Mon, Feb 24, 2020 at 03:54:45PM +0100, Anton Khirnov wrote:
> Quoting Carl Eugen Hoyos (2020-02-24 13:50:57)
> > Am Mo., 24. Feb. 2020 um 13:40 Uhr schrieb Anton Khirnov <anton@khirnov.net>:
> > >
> > > It fundamentally depends on an API that has been deprecated for five
> > > years, has seen no commits since that time and is of highly dubious
> > > usefulness.
> > 
> > Please explain how the removed functionality was replaced.
> 
> It was not, for the reasons mentioned in the commit message. 

> In my view,
> the fact that nobody fixed it in all that time proves that nobody cares
> about this functionality and thus that there is no value in keeping it.

your reasoning only works if there is a problem that requires a fix.

Your reasoning here seems
BIG problem in A && noone fixes it -> noone cares about A

My view is
whoever sees a problem in A (i do not really) should fix it.

Maybe iam missing something and there is in fact a big problem in the
code. But if that is the case iam not aware of the problem and thats
why i did nothing for years "fixing" it. Its not that i do not care.

So what is really the issue here ?
if i build vf_qp.o i get
./libavutil/frame.h:719:1: note: 'av_frame_get_qp_table' has been explicitly marked deprecated here
attribute_deprecated

./libavutil/frame.h:721:1: note: 'av_frame_set_qp_table' has been explicitly marked deprecated here
attribute_deprecated

if i look at git history these where deprecated in
commit 7df37dd319f2d9d3e1becd5d433884e3ccfa1ee2
Author: James Almer <jamrial@gmail.com>
Date:   Mon Oct 23 11:10:48 2017 -0300

    avutil/frame: deprecate getters and setters for AVFrame fields
    
    The fields can be accessed directly, so these are not needed anymore.

This says the field can be accessed directly, so certainly its not
deprecated in favor of the side data API.    

and in fact av_frame_get_qp_table / av_frame_set_qp_table do use the 
side data API already so none of this makes sense really.
And the whole argument about five years also isnt correct as
october 2017 is not 5 years ago


> 
> Furthermore, I believe this filter (and all the associated
> "postprocessing" ones) are anachronistic relics of the DivX era. They
> were in fashion around ~2005 (though I doubt they were actually
> improving anything even then) but nobody with a clue has used them since
> H.264 took over.

well, for old videos (which still exist today) and i mean the stuff
that used 8x8 dct based codecs mpeg1 to mpeg4, divx, msmpeg4, realvideo
also jpeg and that use not very high bitrate. (very high bitrate of course
doesnt have much to improve toward)

There is a quite noticable quality improvment when using postprocessing
with the right parameters both subjective and objective (PSNR IIRC)
And at the rare but not noneexisting occurance where i do want to watch
such a video i always use one of these filters.
In realty that has often been the spp filter but thats probably not
important.
In general if you can see 8x8 blocks without the filter, these filters
will make the video simply look better.

if passing QP helps for the above usecase probably depends on how
variable QP is in the video one wants to watch or if a single fixed
hand tuned QP works well (it often does indeed)

Another usecase for passing QP was lossless re-encoding.
I do not know how common this has been used (iam not using it and its not
my idea originally), this of course also requires a encoder which
can accept motion vectors and MB types on input or intra only

Yet another use case is maintaining the input encoders choices
for quantization / quality when converting to another format.
in principle one could even have one encoder provide quantization
information to a second encoder

            -> encoder1
           /       v
raw input         QP
           \       v
            -> encoder2
           
why? i dont know, maybe for art or fun, duplicate some bad QP choices or good
QP choices, or edit QP choices ina specific area.

but i would not call the ability to pass the QP array around and
to modify it useless.

Also last but not least, if you think there really is an issue that
MUST be fixed otherwise the code must be removed. Why not ask the 
people listed in authors & copyright to look into it ?
Iam listed in the copyright it seems and unless i forgot it noone
asked me to fix some major issue in vf_qp

Thanks


[...]
Lou Logan Feb. 24, 2020, 9:41 p.m. UTC | #17
On Mon, Feb 24, 2020, at 3:37 AM, Anton Khirnov wrote:
> It fundamentally depends on an API that has been deprecated for five
> years, has seen no commits since that time and is of highly dubious
> usefulness.
> ---
>  doc/filters.texi            |  32 -------
>  libavfilter/Makefile        |   1 -
>  libavfilter/allfilters.c    |   1 -
>  libavfilter/vf_qp.c         | 183 ------------------------------------
>  tests/fate/filter-video.mak |   7 +-
>  tests/ref/fate/filter-pp2   |   1 -
>  tests/ref/fate/filter-pp3   |   1 -
>  7 files changed, 1 insertion(+), 225 deletions(-)
>  delete mode 100644 libavfilter/vf_qp.c
>  delete mode 100644 tests/ref/fate/filter-pp2
>  delete mode 100644 tests/ref/fate/filter-pp3

Fine with me. I've never seen it used by anyone.
Thilo Borgmann Feb. 24, 2020, 10:07 p.m. UTC | #18
Am 24.02.20 um 22:41 schrieb Lou Logan:
> On Mon, Feb 24, 2020, at 3:37 AM, Anton Khirnov wrote:
>> It fundamentally depends on an API that has been deprecated for five
>> years, has seen no commits since that time and is of highly dubious
>> usefulness.
>> ---
>>  doc/filters.texi            |  32 -------
>>  libavfilter/Makefile        |   1 -
>>  libavfilter/allfilters.c    |   1 -
>>  libavfilter/vf_qp.c         | 183 ------------------------------------
>>  tests/fate/filter-video.mak |   7 +-
>>  tests/ref/fate/filter-pp2   |   1 -
>>  tests/ref/fate/filter-pp3   |   1 -
>>  7 files changed, 1 insertion(+), 225 deletions(-)
>>  delete mode 100644 libavfilter/vf_qp.c
>>  delete mode 100644 tests/ref/fate/filter-pp2
>>  delete mode 100644 tests/ref/fate/filter-pp3
> 
> Fine with me. I've never seen it used by anyone.

I'm not fine with it. Declaring it's {use | use case} not existent is no arguments whatsoever in reality.

Also, removing some functionality needs an argument - it is not keeping some functionality needs an argument.

Nobody technically elaborates Paul's statement that it should go into side data. WTF? The compromise isn't even considered?

Let's dig some trenches, shall we?

And how come some obvious "use cases" / "needs" like [1] come into play? Or do we declare not continued discussions non-existent now, too?

And how comes, if Michael's investigation, that all of this is based on use of _a function_ that is deprecated instead of direct access of AVFrame's fields is the cause of all of this?

Shame on all of us.

-Thilo

[1] https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2019-August/247401.html
Hendrik Leppkes Feb. 24, 2020, 11:46 p.m. UTC | #19
On Mon, Feb 24, 2020 at 11:08 PM Thilo Borgmann <thilo.borgmann@mail.de> wrote:
> And how comes, if Michael's investigation, that all of this is based on use of _a function_ that is deprecated instead of direct access of AVFrame's fields is the cause of all of this?
>

The entire functionality is deprecated, that the functions in use here
were deprecated in another commit than the fields they internally
access does not change that.

- Hendrik
Vittorio Giovara Feb. 25, 2020, 4:30 a.m. UTC | #20
On Mon, Feb 24, 2020 at 5:07 PM Thilo Borgmann <thilo.borgmann@mail.de>
wrote:

> Am 24.02.20 um 22:41 schrieb Lou Logan:
> > On Mon, Feb 24, 2020, at 3:37 AM, Anton Khirnov wrote:
> >> It fundamentally depends on an API that has been deprecated for five
> >> years, has seen no commits since that time and is of highly dubious
> >> usefulness.
> >> ---
> >>  doc/filters.texi            |  32 -------
> >>  libavfilter/Makefile        |   1 -
> >>  libavfilter/allfilters.c    |   1 -
> >>  libavfilter/vf_qp.c         | 183 ------------------------------------
> >>  tests/fate/filter-video.mak |   7 +-
> >>  tests/ref/fate/filter-pp2   |   1 -
> >>  tests/ref/fate/filter-pp3   |   1 -
> >>  7 files changed, 1 insertion(+), 225 deletions(-)
> >>  delete mode 100644 libavfilter/vf_qp.c
> >>  delete mode 100644 tests/ref/fate/filter-pp2
> >>  delete mode 100644 tests/ref/fate/filter-pp3
> >
> > Fine with me. I've never seen it used by anyone.
>
> I'm not fine with it. Declaring it's {use | use case} not existent is no
> arguments whatsoever in reality.
>
> Also, removing some functionality needs an argument - it is not keeping
> some functionality needs an argument.
>
> Nobody technically elaborates Paul's statement that it should go into side
> data. WTF? The compromise isn't even considered?
>
> Let's dig some trenches, shall we?
>
> And how come some obvious "use cases" / "needs" like [1] come into play?
> Or do we declare not continued discussions non-existent now, too?
>
> And how comes, if Michael's investigation, that all of this is based on
> use of _a function_ that is deprecated instead of direct access of
> AVFrame's fields is the cause of all of this?
>
> Shame on all of us.
>

If I may add my two cents, I feel like we are overreacting a bit and we
should take a step back.

It comes to no surprise that I do not agree that being so user-complacent
is beneficial to the overall health of the project, and that sometimes the
need to drop antiquate technologies arises. First of all this does not mean
that we're backward-removing this feature from older applications, old
ffmpeg installs will keep working. Secondly we have to accept that making
every user always happy is 100% not achievable. In general we should treat
this as an engineering problem and accept its trade-offs: how many users
will get angry that any given functionality is removed, how many will even
notice, and how beneficial it is that a feature is actually removed. And
let's not forget each functionality has a cost, not much for code overhead
but maintenance and review too: most people in this thread had to spend
time (the most precious resource of all!) to analyze the problem, find
alternatives and argue about this topic, while it could probably have been
spent doing better things.

For the case at hand, removing a filter that is using a deprecated
functionality seems perfectly fine, it's has happened in the past and will
definitely happen in the future. If the definitive need arises that a
filter is absolutely needed for these very old files, and users can't just
use an older ffmpeg install, then I'm sure some version a
correctly-implemented filter will magically appear on the mailing list.
For a more general picture, I hope the project will not take such a
conservative stance against removal and deprecation.

After all, we're in 2020 and not using floppy disks just fine :)
Anton Khirnov Feb. 25, 2020, 9:21 a.m. UTC | #21
Quoting Michael Niedermayer (2020-02-24 20:15:43)
> On Mon, Feb 24, 2020 at 03:54:45PM +0100, Anton Khirnov wrote:
> > Quoting Carl Eugen Hoyos (2020-02-24 13:50:57)
> > > Am Mo., 24. Feb. 2020 um 13:40 Uhr schrieb Anton Khirnov <anton@khirnov.net>:
> > > >
> > > > It fundamentally depends on an API that has been deprecated for five
> > > > years, has seen no commits since that time and is of highly dubious
> > > > usefulness.
> > > 
> > > Please explain how the removed functionality was replaced.
> > 
> > It was not, for the reasons mentioned in the commit message. 
> 
> > In my view,
> > the fact that nobody fixed it in all that time proves that nobody cares
> > about this functionality and thus that there is no value in keeping it.
> 
> your reasoning only works if there is a problem that requires a fix.
> 
> Your reasoning here seems
> BIG problem in A && noone fixes it -> noone cares about A
> 
> My view is
> whoever sees a problem in A (i do not really) should fix it.
> 
> Maybe iam missing something and there is in fact a big problem in the
> code. But if that is the case iam not aware of the problem and thats
> why i did nothing for years "fixing" it. Its not that i do not care.
> 
> So what is really the issue here ?
> if i build vf_qp.o i get
> ./libavutil/frame.h:719:1: note: 'av_frame_get_qp_table' has been explicitly marked deprecated here
> attribute_deprecated
> 
> ./libavutil/frame.h:721:1: note: 'av_frame_set_qp_table' has been explicitly marked deprecated here
> attribute_deprecated

Yes, I believe there is a problem, or more precisely a bunch of related
problems. Not really that big, but real ones nevertheless.

One aspect of the problem is precisely the fact that this functionality
has been deprecated and nobody has addressed this deprecation in many
years. Paul was concerned about our reputation - I believe having so
many deprecation warnings during build is very bad for our reputation.
But more importantly, it is confusing the developers and users about
what they should use and what not. If you cared about keeping this code,
you should have undeprecated it.

Two ather aspects of the problem are:
- this API is only really workable for MPEG1/2 and closely related
  codecs like JPEG/H.263/MPEG4 ASP/RV
- it is undocumented, the data layout is not defined
If you really want to keep it, those two points should be addressed.

> 
> if i look at git history these where deprecated in
> commit 7df37dd319f2d9d3e1becd5d433884e3ccfa1ee2
> Author: James Almer <jamrial@gmail.com>
> Date:   Mon Oct 23 11:10:48 2017 -0300
> 
>     avutil/frame: deprecate getters and setters for AVFrame fields
>     
>     The fields can be accessed directly, so these are not needed anymore.
> 
> This says the field can be accessed directly, so certainly its not
> deprecated in favor of the side data API.    
> 
> and in fact av_frame_get_qp_table / av_frame_set_qp_table do use the 
> side data API already so none of this makes sense really.
> And the whole argument about five years also isnt correct as
> october 2017 is not 5 years ago

The accessors may have been deprecated in 2017, but the entire
"exporting QP tables" functionality was deprecated long before that. In
any case, it does not matter when exactly that was.

> 
> 
> > 
> > Furthermore, I believe this filter (and all the associated
> > "postprocessing" ones) are anachronistic relics of the DivX era. They
> > were in fashion around ~2005 (though I doubt they were actually
> > improving anything even then) but nobody with a clue has used them since
> > H.264 took over.
> 
> well, for old videos (which still exist today) and i mean the stuff
> that used 8x8 dct based codecs mpeg1 to mpeg4, divx, msmpeg4, realvideo
> also jpeg and that use not very high bitrate. (very high bitrate of course
> doesnt have much to improve toward)
> 
> There is a quite noticable quality improvment when using postprocessing
> with the right parameters both subjective and objective (PSNR IIRC)
> And at the rare but not noneexisting occurance where i do want to watch
> such a video i always use one of these filters.
> In realty that has often been the spp filter but thats probably not
> important.
> In general if you can see 8x8 blocks without the filter, these filters
> will make the video simply look better.
> 
> if passing QP helps for the above usecase probably depends on how
> variable QP is in the video one wants to watch or if a single fixed
> hand tuned QP works well (it often does indeed)

But that is precisely the question at hand. Is passing QP tables a
useful thing to have?
Also, do note that MPV removed those filters and according to its
developer nobody ever missed them or complained about their removal.
Furthermore, people in https://github.com/mpv-player/mpv/issues/2792
suggest that other filters may do as good or better job.

> 
> Another usecase for passing QP was lossless re-encoding.
> I do not know how common this has been used (iam not using it and its not
> my idea originally), this of course also requires a encoder which
> can accept motion vectors and MB types on input or intra only
> 
> Yet another use case is maintaining the input encoders choices
> for quantization / quality when converting to another format.
> in principle one could even have one encoder provide quantization
> information to a second encoder
> 
>             -> encoder1
>            /       v
> raw input         QP
>            \       v
>             -> encoder2
>            
> why? i dont know, maybe for art or fun, duplicate some bad QP choices or good
> QP choices, or edit QP choices ina specific area.
> 
> but i would not call the ability to pass the QP array around and
> to modify it useless.

I would disagree about that. All those cases you described are
theoretical - "someone might want to do it this way". But so far there
doesn't seem to be anyone who actualy does that or wants to do that.

Theoretical features that nobody actually uses are not useful - hence
they are useless. I would even say they are worse then useless, since
they
- clutter the API namespace, making it harder to find actually useful
  things
- provide additional attack surface for potential security issues
- make maintenance and refactoring harder, preventing actually useful
  changes

> 
> Also last but not least, if you think there really is an issue that
> MUST be fixed otherwise the code must be removed. Why not ask the 
> people listed in authors & copyright to look into it ?
> Iam listed in the copyright it seems and unless i forgot it noone
> asked me to fix some major issue in vf_qp

That is exactly what I am doing with this patch set. I do not have a
personal vendetta against this code. I do not intend to go over dead
bodies to see it removed.

It is marked for removal and we are planning a major bump, hence I am
setting patches that remove it. It is an opportunity for people who want
to keep it to step up and do something about it.

But so far all the objections except yours have been pure feature
hoarding. "Someone might conceivably use this so it must not be
removed". I do not believe this way of thinking is good for the project.
Either someone should show a clear valid use case for this, or it should
be dropped.

And I am repeating yet again that the code remains in git history and
can always be resurrected in the future if someone wants it. It is not
gone forever. Adding new features is easier than removing them.
Paul B Mahol Feb. 25, 2020, 9:54 a.m. UTC | #22
On 2/25/20, Anton Khirnov <anton@khirnov.net> wrote:
> Quoting Michael Niedermayer (2020-02-24 20:15:43)
>> On Mon, Feb 24, 2020 at 03:54:45PM +0100, Anton Khirnov wrote:
>> > Quoting Carl Eugen Hoyos (2020-02-24 13:50:57)
>> > > Am Mo., 24. Feb. 2020 um 13:40 Uhr schrieb Anton Khirnov
>> > > <anton@khirnov.net>:
>> > > >
>> > > > It fundamentally depends on an API that has been deprecated for five
>> > > > years, has seen no commits since that time and is of highly dubious
>> > > > usefulness.
>> > >
>> > > Please explain how the removed functionality was replaced.
>> >
>> > It was not, for the reasons mentioned in the commit message.
>>
>> > In my view,
>> > the fact that nobody fixed it in all that time proves that nobody cares
>> > about this functionality and thus that there is no value in keeping it.
>>
>> your reasoning only works if there is a problem that requires a fix.
>>
>> Your reasoning here seems
>> BIG problem in A && noone fixes it -> noone cares about A
>>
>> My view is
>> whoever sees a problem in A (i do not really) should fix it.
>>
>> Maybe iam missing something and there is in fact a big problem in the
>> code. But if that is the case iam not aware of the problem and thats
>> why i did nothing for years "fixing" it. Its not that i do not care.
>>
>> So what is really the issue here ?
>> if i build vf_qp.o i get
>> ./libavutil/frame.h:719:1: note: 'av_frame_get_qp_table' has been
>> explicitly marked deprecated here
>> attribute_deprecated
>>
>> ./libavutil/frame.h:721:1: note: 'av_frame_set_qp_table' has been
>> explicitly marked deprecated here
>> attribute_deprecated
>
> Yes, I believe there is a problem, or more precisely a bunch of related
> problems. Not really that big, but real ones nevertheless.
>
> One aspect of the problem is precisely the fact that this functionality
> has been deprecated and nobody has addressed this deprecation in many
> years. Paul was concerned about our reputation - I believe having so
> many deprecation warnings during build is very bad for our reputation.
> But more importantly, it is confusing the developers and users about
> what they should use and what not. If you cared about keeping this code,
> you should have undeprecated it.
>
> Two ather aspects of the problem are:
> - this API is only really workable for MPEG1/2 and closely related
>   codecs like JPEG/H.263/MPEG4 ASP/RV
> - it is undocumented, the data layout is not defined
> If you really want to keep it, those two points should be addressed.
>
>>
>> if i look at git history these where deprecated in
>> commit 7df37dd319f2d9d3e1becd5d433884e3ccfa1ee2
>> Author: James Almer <jamrial@gmail.com>
>> Date:   Mon Oct 23 11:10:48 2017 -0300
>>
>>     avutil/frame: deprecate getters and setters for AVFrame fields
>>
>>     The fields can be accessed directly, so these are not needed anymore.
>>
>> This says the field can be accessed directly, so certainly its not
>> deprecated in favor of the side data API.
>>
>> and in fact av_frame_get_qp_table / av_frame_set_qp_table do use the
>> side data API already so none of this makes sense really.
>> And the whole argument about five years also isnt correct as
>> october 2017 is not 5 years ago
>
> The accessors may have been deprecated in 2017, but the entire
> "exporting QP tables" functionality was deprecated long before that. In
> any case, it does not matter when exactly that was.
>
>>
>>
>> >
>> > Furthermore, I believe this filter (and all the associated
>> > "postprocessing" ones) are anachronistic relics of the DivX era. They
>> > were in fashion around ~2005 (though I doubt they were actually
>> > improving anything even then) but nobody with a clue has used them since
>> > H.264 took over.
>>
>> well, for old videos (which still exist today) and i mean the stuff
>> that used 8x8 dct based codecs mpeg1 to mpeg4, divx, msmpeg4, realvideo
>> also jpeg and that use not very high bitrate. (very high bitrate of course
>> doesnt have much to improve toward)
>>
>> There is a quite noticable quality improvment when using postprocessing
>> with the right parameters both subjective and objective (PSNR IIRC)
>> And at the rare but not noneexisting occurance where i do want to watch
>> such a video i always use one of these filters.
>> In realty that has often been the spp filter but thats probably not
>> important.
>> In general if you can see 8x8 blocks without the filter, these filters
>> will make the video simply look better.
>>
>> if passing QP helps for the above usecase probably depends on how
>> variable QP is in the video one wants to watch or if a single fixed
>> hand tuned QP works well (it often does indeed)
>
> But that is precisely the question at hand. Is passing QP tables a
> useful thing to have?
> Also, do note that MPV removed those filters and according to its
> developer nobody ever missed them or complained about their removal.
> Furthermore, people in https://github.com/mpv-player/mpv/issues/2792
> suggest that other filters may do as good or better job.

lol, I must comment on this. You just mentioned single usecase that
use qp tables.
Original Github issue poster was not 100% happy with alternative results.
You would need to also remove pp filter and entire libpostprocess
otherwise because they
are mostly using qp table to function properly.
Good luck with that.

>
>>
>> Another usecase for passing QP was lossless re-encoding.
>> I do not know how common this has been used (iam not using it and its not
>> my idea originally), this of course also requires a encoder which
>> can accept motion vectors and MB types on input or intra only
>>
>> Yet another use case is maintaining the input encoders choices
>> for quantization / quality when converting to another format.
>> in principle one could even have one encoder provide quantization
>> information to a second encoder
>>
>>             -> encoder1
>>            /       v
>> raw input         QP
>>            \       v
>>             -> encoder2
>>
>> why? i dont know, maybe for art or fun, duplicate some bad QP choices or
>> good
>> QP choices, or edit QP choices ina specific area.
>>
>> but i would not call the ability to pass the QP array around and
>> to modify it useless.
>
> I would disagree about that. All those cases you described are
> theoretical - "someone might want to do it this way". But so far there
> doesn't seem to be anyone who actualy does that or wants to do that.
>
> Theoretical features that nobody actually uses are not useful - hence
> they are useless. I would even say they are worse then useless, since
> they
> - clutter the API namespace, making it harder to find actually useful
>   things
> - provide additional attack surface for potential security issues
> - make maintenance and refactoring harder, preventing actually useful
>   changes
>
>>
>> Also last but not least, if you think there really is an issue that
>> MUST be fixed otherwise the code must be removed. Why not ask the
>> people listed in authors & copyright to look into it ?
>> Iam listed in the copyright it seems and unless i forgot it noone
>> asked me to fix some major issue in vf_qp
>
> That is exactly what I am doing with this patch set. I do not have a
> personal vendetta against this code. I do not intend to go over dead
> bodies to see it removed.
>
> It is marked for removal and we are planning a major bump, hence I am
> setting patches that remove it. It is an opportunity for people who want
> to keep it to step up and do something about it.
>
> But so far all the objections except yours have been pure feature
> hoarding. "Someone might conceivably use this so it must not be
> removed". I do not believe this way of thinking is good for the project.
> Either someone should show a clear valid use case for this, or it should
> be dropped.
>
> And I am repeating yet again that the code remains in git history and
> can always be resurrected in the future if someone wants it. It is not
> gone forever. Adding new features is easier than removing them.
>
> --
> Anton Khirnov
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Anton Khirnov Feb. 25, 2020, 10:28 a.m. UTC | #23
Quoting Thilo Borgmann (2020-02-24 23:07:48)
> Am 24.02.20 um 22:41 schrieb Lou Logan:
> > On Mon, Feb 24, 2020, at 3:37 AM, Anton Khirnov wrote:
> >> It fundamentally depends on an API that has been deprecated for five
> >> years, has seen no commits since that time and is of highly dubious
> >> usefulness.
> >> ---
> >>  doc/filters.texi            |  32 -------
> >>  libavfilter/Makefile        |   1 -
> >>  libavfilter/allfilters.c    |   1 -
> >>  libavfilter/vf_qp.c         | 183 ------------------------------------
> >>  tests/fate/filter-video.mak |   7 +-
> >>  tests/ref/fate/filter-pp2   |   1 -
> >>  tests/ref/fate/filter-pp3   |   1 -
> >>  7 files changed, 1 insertion(+), 225 deletions(-)
> >>  delete mode 100644 libavfilter/vf_qp.c
> >>  delete mode 100644 tests/ref/fate/filter-pp2
> >>  delete mode 100644 tests/ref/fate/filter-pp3
> > 
> > Fine with me. I've never seen it used by anyone.
> 
> I'm not fine with it. Declaring it's {use | use case} not existent is
> no arguments whatsoever in reality.
> 
> Also, removing some functionality needs an argument - it is not
> keeping some functionality needs an argument.

I disagree with that. Keeping code around is not free, as Vittorio
already said - it is a burden in many ways. So I believe all code needs
continued justification for its existence - not just "it was added in
the past so it stays in forever". Note that I'm not saying it needs to
be mainstream or very popular - I am fine with obscure features that
are only useful to a few people in highly special cases (given they are
not unduly intrusive and those people are willing to maintain them). But
so far in this thread, there has been no actual use presented for
exporting and passing around QP tables. None whatsoever.

Most objections here are like yours - it's a) a feature and b) someone
somewhere sometime might conceivably want to use it, so it must not be
removed. Michael's reponse is the only one that comes close to having a
use case, but even he says that he's unsure of the usefulness of the
actual QP tables and that it's largely theoretical.

I believe I did more structural changes to the libraries than most
people and in my experience these obsolete features from mplayer days
are a MASSIVE maintenance pain. The amount of effort required to keep
them working is simply not justified when essentially nobody uses them.

IMO these demands that they all be preserved forever is endangering the
project. If making changes becomes too hard, people stop making them.
They move to other places that are less hostile to change. We are at
risk of turning into a repository of obscure old codecs and filters and
getting overtaken by leaner projects like dav1d (yes it's supposed to be
AV1-only, but that can change).

> 
> Nobody technically elaborates Paul's statement that it should go into side data. WTF? The compromise isn't even considered?
> 
> Let's dig some trenches, shall we?
> 
> And how come some obvious "use cases" / "needs" like [1] come into play? Or do we declare not continued discussions non-existent now, too?

The patch in your link is not using this API. Precisely because this API
is inadequate for anything newer than MPEG4 ASP. If anything, it's an
additional argument in favor of my patches.

> 
> And how comes, if Michael's investigation, that all of this is based on use of _a function_ that is deprecated instead of direct access of AVFrame's fields is the cause of all of this?
> 
> Shame on all of us.

WTF? What shame?
I am sending patches, in good faith, that I believe will improve the
project. People may (and do) object to them. As long as the discussion
is civil, constructive and in good faith (so far it mostly is), I see no
reason for any shame.
Nicolas George Feb. 25, 2020, 11:44 a.m. UTC | #24
Anton Khirnov (12020-02-25):
>					      So I believe all code needs
> continued justification for its existence - not just "it was added in
> the past so it stays in forever". Note that I'm not saying it needs to
> be mainstream or very popular - I am fine with obscure features that
> are only useful to a few people in highly special cases (given they are
> not unduly intrusive and those people are willing to maintain them). But
> so far in this thread, there has been no actual use presented for
> exporting and passing around QP tables. None whatsoever.

You are ignoring a significant bias in making this argument: satisfied
people don't complain.

The feature is there, it works. If people need it, they use it. They
don't come on the mailing list complaining they have no problem. They
don't even consider fixing a compilation warning among many others,
especially when they possibly don't compile themselves.

If we think a feature is unused and want to remove it, I think we need
to announce it in a way that its users can't miss, and then let them
sufficient time to make their needs known. In this case, "in a way that
its users can't miss" is not true.

You are right in saying that features are a maintenance burden, and
therefore we should remove features that are unused. But for that, you
need to actually make a case for the fact that they are unused. For now,
AFAICS, you case amounts to the fact that nobody spontaneously told you
they use this filter.

Regards,
Thilo Borgmann Feb. 26, 2020, 1:27 p.m. UTC | #25
Am 25.02.20 um 11:28 schrieb Anton Khirnov:
> Quoting Thilo Borgmann (2020-02-24 23:07:48)
>> Am 24.02.20 um 22:41 schrieb Lou Logan:
>>> On Mon, Feb 24, 2020, at 3:37 AM, Anton Khirnov wrote:
>>>> It fundamentally depends on an API that has been deprecated for five
>>>> years, has seen no commits since that time and is of highly dubious
>>>> usefulness.
>>>> ---
>>>>  doc/filters.texi            |  32 -------
>>>>  libavfilter/Makefile        |   1 -
>>>>  libavfilter/allfilters.c    |   1 -
>>>>  libavfilter/vf_qp.c         | 183 ------------------------------------
>>>>  tests/fate/filter-video.mak |   7 +-
>>>>  tests/ref/fate/filter-pp2   |   1 -
>>>>  tests/ref/fate/filter-pp3   |   1 -
>>>>  7 files changed, 1 insertion(+), 225 deletions(-)
>>>>  delete mode 100644 libavfilter/vf_qp.c
>>>>  delete mode 100644 tests/ref/fate/filter-pp2
>>>>  delete mode 100644 tests/ref/fate/filter-pp3
>>>
>>> Fine with me. I've never seen it used by anyone.
>>
>> I'm not fine with it. Declaring it's {use | use case} not existent is
>> no arguments whatsoever in reality.
>>
>> Also, removing some functionality needs an argument - it is not
>> keeping some functionality needs an argument.
> 
> I disagree with that. Keeping code around is not free, as Vittorio
> already said - it is a burden in many ways. So I believe all code needs
> continued justification for its existence - not just "it was added in
> the past so it stays in forever". Note that I'm not saying it needs to
> be mainstream or very popular - I am fine with obscure features that
> are only useful to a few people in highly special cases (given they are
> not unduly intrusive and those people are willing to maintain them). But
> so far in this thread, there has been no actual use presented for
> exporting and passing around QP tables. None whatsoever.
> 
> Most objections here are like yours - it's a) a feature and b) someone
> somewhere sometime might conceivably want to use it, so it must not be
> removed. Michael's reponse is the only one that comes close to having a
> use case, but even he says that he's unsure of the usefulness of the
> actual QP tables and that it's largely theoretical.
> 
> I believe I did more structural changes to the libraries than most
> people and in my experience these obsolete features from mplayer days
> are a MASSIVE maintenance pain. The amount of effort required to keep
> them working is simply not justified when essentially nobody uses them.
> 
> IMO these demands that they all be preserved forever is endangering the
> project. If making changes becomes too hard, people stop making them.
> They move to other places that are less hostile to change. We are at
> risk of turning into a repository of obscure old codecs and filters and
> getting overtaken by leaner projects like dav1d (yes it's supposed to be
> AV1-only, but that can change).
> 
>>
>> Nobody technically elaborates Paul's statement that it should go into side data. WTF? The compromise isn't even considered?
>>
>> Let's dig some trenches, shall we?
>>
>> And how come some obvious "use cases" / "needs" like [1] come into play? Or do we declare not continued discussions non-existent now, too?
> 
> The patch in your link is not using this API. Precisely because this API
> is inadequate for anything newer than MPEG4 ASP. If anything, it's an
> additional argument in favor of my patches.

My actual point about that patch was that there is a use case to extract QP tables for more current codecs. Suggests this use case is also there for less current ones which says we should not just remove this possibility.

If that patch is avoiding the deprecated things, could it be applied to the older codes and supersede vf_qp? (And I have no clue whether or not)
And would that not go into the direction Paul suggested?


>> And how comes, if Michael's investigation, that all of this is based on use of _a function_ that is deprecated instead of direct access of AVFrame's fields is the cause of all of this?
>>
>> Shame on all of us.
> 
> WTF? What shame?

I am ashamed that I do read most arguments here as merely black-and-white statements.
That might be considered, as I admit, as an overreaction and I probably should not have written.
However, that is also why I suggested to dig trenches.


> I am sending patches, in good faith, that I believe will improve the
> project. People may (and do) object to them. As long as the discussion
> is civil, constructive and in good faith (so far it mostly is), I see no
> reason for any shame.

Yes and please continue to do so. That I did not criticize.
Also yes, I don't see it non-civil and hopefully it will stay that way.
I see it, however, way less constructive as it could and should be.

-Thilo
Jean-Baptiste Kempf Feb. 26, 2020, 2:03 p.m. UTC | #26
Yo,

On Wed, Feb 26, 2020, at 14:27, Thilo Borgmann wrote:
> > The patch in your link is not using this API. Precisely because this API
> > is inadequate for anything newer than MPEG4 ASP. If anything, it's an
> > additional argument in favor of my patches.
> 
> My actual point about that patch was that there is a use case to 
> extract QP tables for more current codecs. Suggests this use case is 
> also there for less current ones which says we should not just remove 
> this possibility.

I think this is the right question:
"Are there actual valid use cases to do it?"

I remember that last year, there was an extractqp for h264 patches from Google, for example.

Any other question is basically less important.

If that is useful, then, we should update the API and remove the old API.
If it is not, then, let's remove everything.

Best,
Paul B Mahol Feb. 26, 2020, 2:46 p.m. UTC | #27
On 2/26/20, Jean-Baptiste Kempf <jb@videolan.org> wrote:
> Yo,
>
> On Wed, Feb 26, 2020, at 14:27, Thilo Borgmann wrote:
>> > The patch in your link is not using this API. Precisely because this API
>> > is inadequate for anything newer than MPEG4 ASP. If anything, it's an
>> > additional argument in favor of my patches.
>>
>> My actual point about that patch was that there is a use case to
>> extract QP tables for more current codecs. Suggests this use case is
>> also there for less current ones which says we should not just remove
>> this possibility.
>
> I think this is the right question:
> "Are there actual valid use cases to do it?"
>
> I remember that last year, there was an extractqp for h264 patches from
> Google, for example.
>
> Any other question is basically less important.
>
> If that is useful, then, we should update the API and remove the old API.
> If it is not, then, let's remove everything.
>

It is useful as already proved. Please refrain from ignoring obvious facts.


> Best,
>
> --
> Jean-Baptiste Kempf -  President
> +33 672 704 734
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Jean-Baptiste Kempf Feb. 26, 2020, 3:26 p.m. UTC | #28
On Wed, Feb 26, 2020, at 15:46, Paul B Mahol wrote:
> On 2/26/20, Jean-Baptiste Kempf <jb@videolan.org> wrote:
> > Yo,
> >
> > On Wed, Feb 26, 2020, at 14:27, Thilo Borgmann wrote:
> >> > The patch in your link is not using this API. Precisely because this API
> >> > is inadequate for anything newer than MPEG4 ASP. If anything, it's an
> >> > additional argument in favor of my patches.
> >>
> >> My actual point about that patch was that there is a use case to
> >> extract QP tables for more current codecs. Suggests this use case is
> >> also there for less current ones which says we should not just remove
> >> this possibility.
> >
> > I think this is the right question:
> > "Are there actual valid use cases to do it?"
> >
> > I remember that last year, there was an extractqp for h264 patches from
> > Google, for example.
> >
> > Any other question is basically less important.
> >
> > If that is useful, then, we should update the API and remove the old API.
> > If it is not, then, let's remove everything.
> >
> 
> It is useful as already proved. Please refrain from ignoring obvious facts.

Please learn how to talk nicely to people, that will help quite a bit.
Thilo Borgmann Feb. 26, 2020, 4:16 p.m. UTC | #29
Am 26.02.20 um 16:26 schrieb Jean-Baptiste Kempf:
> On Wed, Feb 26, 2020, at 15:46, Paul B Mahol wrote:
>> On 2/26/20, Jean-Baptiste Kempf <jb@videolan.org> wrote:
>>> Yo,
>>>
>>> On Wed, Feb 26, 2020, at 14:27, Thilo Borgmann wrote:
>>>>> The patch in your link is not using this API. Precisely because this API
>>>>> is inadequate for anything newer than MPEG4 ASP. If anything, it's an
>>>>> additional argument in favor of my patches.
>>>>
>>>> My actual point about that patch was that there is a use case to
>>>> extract QP tables for more current codecs. Suggests this use case is
>>>> also there for less current ones which says we should not just remove
>>>> this possibility.
>>>
>>> I think this is the right question:
>>> "Are there actual valid use cases to do it?"
>>>
>>> I remember that last year, there was an extractqp for h264 patches from
>>> Google, for example.

That should be the patch I linked to.

-Thilo
Thierry Foucu Feb. 26, 2020, 6:04 p.m. UTC | #30
First of all, thanks for trying to clean up deprecated API

On Wed, Feb 26, 2020 at 6:03 AM Jean-Baptiste Kempf <jb@videolan.org> wrote:

> Yo,
>
> On Wed, Feb 26, 2020, at 14:27, Thilo Borgmann wrote:
> > > The patch in your link is not using this API. Precisely because this
> API
> > > is inadequate for anything newer than MPEG4 ASP. If anything, it's an
> > > additional argument in favor of my patches.
> >
> > My actual point about that patch was that there is a use case to
> > extract QP tables for more current codecs. Suggests this use case is
> > also there for less current ones which says we should not just remove
> > this possibility.
>
> I think this is the right question:
> "Are there actual valid use cases to do it?"
>

yes, there is a use case for extracting QP per block:
To analyze and visualize QP to validate/analyze Rate Control when we enable
Adaptive QP and ROI for example.
Most people who works on Rate Control know that the choice of the right
MV/QP/block type is really important for quality.
In today world, you will have to license existing software to visualize the
QP, but if you want to do this over +10k video, and analyze QP based on
some stats we expect to have, you need some libraries to do it.
People could write some codec parser to extract the QP per block, but this
is almost re-implementing part of libavcodec.
Today, for example, when we want to visualize QP, we use libavcodec to
decode the frame, use another piece of code to extract the QP and overlay
both of them. This is almost (and i say ALMOST) decoding the frame twice.
(BTW, we can do that already with libavcodec for MV, so why not for QP)

Last summer, an intern from google tried to come up with a metadata
structure to store information per block.
This would have allow to store per block, the MV, QP, block type, etc.. And
it could have work for any codec with any different block size.
He was trying to implement what was done for MV and replace the QP with
this new metadata.

I agree that the current QP code (not feature) should be deprecated as it
does not work with newer codec.
But before removing the deprecated code, it will be nice to have the same
feature available with a newer API, so the features of
extracting/analyzing/overlaying QP still work.

>
> I remember that last year, there was an extractqp for h264 patches from
> Google, for example.
>
> Any other question is basically less important.
>
> If that is useful, then, we should update the API and remove the old API.
> If it is not, then, let's remove everything.
>
> Best,
>
> --
> Jean-Baptiste Kempf -  President
> +33 672 704 734
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Anton Khirnov Feb. 27, 2020, 11:30 a.m. UTC | #31
Quoting Thierry Foucu (2020-02-26 19:04:57)
> First of all, thanks for trying to clean up deprecated API
> 
> On Wed, Feb 26, 2020 at 6:03 AM Jean-Baptiste Kempf <jb@videolan.org> wrote:
> 
> > Yo,
> >
> > On Wed, Feb 26, 2020, at 14:27, Thilo Borgmann wrote:
> > > > The patch in your link is not using this API. Precisely because this
> > API
> > > > is inadequate for anything newer than MPEG4 ASP. If anything, it's an
> > > > additional argument in favor of my patches.
> > >
> > > My actual point about that patch was that there is a use case to
> > > extract QP tables for more current codecs. Suggests this use case is
> > > also there for less current ones which says we should not just remove
> > > this possibility.
> >
> > I think this is the right question:
> > "Are there actual valid use cases to do it?"
> >
> 
> yes, there is a use case for extracting QP per block:
> To analyze and visualize QP to validate/analyze Rate Control when we enable
> Adaptive QP and ROI for example.
> Most people who works on Rate Control know that the choice of the right
> MV/QP/block type is really important for quality.
> In today world, you will have to license existing software to visualize the
> QP, but if you want to do this over +10k video, and analyze QP based on
> some stats we expect to have, you need some libraries to do it.
> People could write some codec parser to extract the QP per block, but this
> is almost re-implementing part of libavcodec.
> Today, for example, when we want to visualize QP, we use libavcodec to
> decode the frame, use another piece of code to extract the QP and overlay
> both of them. This is almost (and i say ALMOST) decoding the frame twice.
> (BTW, we can do that already with libavcodec for MV, so why not for QP)

Thank you, finally there is a clear use case.

> 
> Last summer, an intern from google tried to come up with a metadata
> structure to store information per block.
> This would have allow to store per block, the MV, QP, block type, etc.. And
> it could have work for any codec with any different block size.
> He was trying to implement what was done for MV and replace the QP with
> this new metadata.
> 
> I agree that the current QP code (not feature) should be deprecated as it
> does not work with newer codec.
> But before removing the deprecated code, it will be nice to have the same
> feature available with a newer API, so the features of
> extracting/analyzing/overlaying QP still work.

Yes, it does indeed sound reasonable to push this new API and convert
the old code to it. The question remains if any of the people arguing
for keeping those filters are also willing to do the conversion.
Jean-Baptiste Kempf Feb. 27, 2020, 11:32 a.m. UTC | #32
On Thu, Feb 27, 2020, at 12:30, Anton Khirnov wrote:
> > Today, for example, when we want to visualize QP, we use libavcodec to
> > decode the frame, use another piece of code to extract the QP and overlay
> > both of them. This is almost (and i say ALMOST) decoding the frame twice.
> > (BTW, we can do that already with libavcodec for MV, so why not for QP)
> 
> Thank you, finally there is a clear use case.

Thanks Thierry!

> > I agree that the current QP code (not feature) should be deprecated as it
> > does not work with newer codec.
> > But before removing the deprecated code, it will be nice to have the same
> > feature available with a newer API, so the features of
> > extracting/analyzing/overlaying QP still work.
> 
> Yes, it does indeed sound reasonable to push this new API and convert
> the old code to it. The question remains if any of the people arguing
> for keeping those filters are also willing to do the conversion.

Lynne? Juan? Can it be done with the new API patches?
Michael Niedermayer Feb. 27, 2020, 5:33 p.m. UTC | #33
On Thu, Feb 27, 2020 at 12:30:22PM +0100, Anton Khirnov wrote:
> Quoting Thierry Foucu (2020-02-26 19:04:57)
> > First of all, thanks for trying to clean up deprecated API
> > 
> > On Wed, Feb 26, 2020 at 6:03 AM Jean-Baptiste Kempf <jb@videolan.org> wrote:
> > 
> > > Yo,
> > >
> > > On Wed, Feb 26, 2020, at 14:27, Thilo Borgmann wrote:
> > > > > The patch in your link is not using this API. Precisely because this
> > > API
> > > > > is inadequate for anything newer than MPEG4 ASP. If anything, it's an
> > > > > additional argument in favor of my patches.
> > > >
> > > > My actual point about that patch was that there is a use case to
> > > > extract QP tables for more current codecs. Suggests this use case is
> > > > also there for less current ones which says we should not just remove
> > > > this possibility.
> > >
> > > I think this is the right question:
> > > "Are there actual valid use cases to do it?"
> > >
> > 
> > yes, there is a use case for extracting QP per block:
> > To analyze and visualize QP to validate/analyze Rate Control when we enable
> > Adaptive QP and ROI for example.
> > Most people who works on Rate Control know that the choice of the right
> > MV/QP/block type is really important for quality.
> > In today world, you will have to license existing software to visualize the
> > QP, but if you want to do this over +10k video, and analyze QP based on
> > some stats we expect to have, you need some libraries to do it.
> > People could write some codec parser to extract the QP per block, but this
> > is almost re-implementing part of libavcodec.
> > Today, for example, when we want to visualize QP, we use libavcodec to
> > decode the frame, use another piece of code to extract the QP and overlay
> > both of them. This is almost (and i say ALMOST) decoding the frame twice.
> > (BTW, we can do that already with libavcodec for MV, so why not for QP)
> 
> Thank you, finally there is a clear use case.
> 

> > 
> > Last summer, an intern from google tried to come up with a metadata
> > structure to store information per block.
> > This would have allow to store per block, the MV, QP, block type, etc.. And
> > it could have work for any codec with any different block size.
> > He was trying to implement what was done for MV and replace the QP with
> > this new metadata.
> > 
> > I agree that the current QP code (not feature) should be deprecated as it
> > does not work with newer codec.
> > But before removing the deprecated code, it will be nice to have the same
> > feature available with a newer API, so the features of
> > extracting/analyzing/overlaying QP still work.
> 
> Yes, it does indeed sound reasonable to push this new API and convert
> the old code to it. The question remains if any of the people arguing
> for keeping those filters are also willing to do the conversion.

I am willing to help, the time ATM is not great though because theres
alot i want/need to do both related to FFmpeg and unrelated.
(for example iam behind with making a new release which should happen
 before the bump ...)

so i suspect by the time i get around to look into qp API updating
someone else probably will already have done it ...

Thanks

 [...]
Jean-Baptiste Kempf Feb. 27, 2020, 5:45 p.m. UTC | #34
On Thu, Feb 27, 2020, at 18:33, Michael Niedermayer wrote:
> > > I agree that the current QP code (not feature) should be deprecated as it
> > > does not work with newer codec.
> > > But before removing the deprecated code, it will be nice to have the same
> > > feature available with a newer API, so the features of
> > > extracting/analyzing/overlaying QP still work.
> > 
> > Yes, it does indeed sound reasonable to push this new API and convert
> > the old code to it. The question remains if any of the people arguing
> > for keeping those filters are also willing to do the conversion.
> 
> I am willing to help, the time ATM is not great though because theres

Thanks.

I think the next major API bump is months away, so I'm not sure there is any urgency here :)

--
Jean-Baptiste Kempf - President
+33 672 704 734
diff mbox series

Patch

diff --git a/doc/filters.texi b/doc/filters.texi
index 70fd7a4cc7..2a1235183f 100644
--- a/doc/filters.texi
+++ b/doc/filters.texi
@@ -15335,38 +15335,6 @@  telecine NTSC input:
 ffmpeg -i input -vf pullup -r 24000/1001 ...
 @end example
 
-@section qp
-
-Change video quantization parameters (QP).
-
-The filter accepts the following option:
-
-@table @option
-@item qp
-Set expression for quantization parameter.
-@end table
-
-The expression is evaluated through the eval API and can contain, among others,
-the following constants:
-
-@table @var
-@item known
-1 if index is not 129, 0 otherwise.
-
-@item qp
-Sequential index starting from -129 to 128.
-@end table
-
-@subsection Examples
-
-@itemize
-@item
-Some equation like:
-@example
-qp=2+2*sin(PI*qp)
-@end example
-@end itemize
-
 @section random
 
 Flush video frames from internal cache of frames into a random order.
diff --git a/libavfilter/Makefile b/libavfilter/Makefile
index 089880a39d..74968b32e1 100644
--- a/libavfilter/Makefile
+++ b/libavfilter/Makefile
@@ -349,7 +349,6 @@  OBJS-$(CONFIG_PROGRAM_OPENCL_FILTER)         += vf_program_opencl.o opencl.o fra
 OBJS-$(CONFIG_PSEUDOCOLOR_FILTER)            += vf_pseudocolor.o
 OBJS-$(CONFIG_PSNR_FILTER)                   += vf_psnr.o framesync.o
 OBJS-$(CONFIG_PULLUP_FILTER)                 += vf_pullup.o
-OBJS-$(CONFIG_QP_FILTER)                     += vf_qp.o
 OBJS-$(CONFIG_RANDOM_FILTER)                 += vf_random.o
 OBJS-$(CONFIG_READEIA608_FILTER)             += vf_readeia608.o
 OBJS-$(CONFIG_READVITC_FILTER)               += vf_readvitc.o
diff --git a/libavfilter/allfilters.c b/libavfilter/allfilters.c
index 88ebd121ad..aa6f006ddb 100644
--- a/libavfilter/allfilters.c
+++ b/libavfilter/allfilters.c
@@ -332,7 +332,6 @@  extern AVFilter ff_vf_program_opencl;
 extern AVFilter ff_vf_pseudocolor;
 extern AVFilter ff_vf_psnr;
 extern AVFilter ff_vf_pullup;
-extern AVFilter ff_vf_qp;
 extern AVFilter ff_vf_random;
 extern AVFilter ff_vf_readeia608;
 extern AVFilter ff_vf_readvitc;
diff --git a/libavfilter/vf_qp.c b/libavfilter/vf_qp.c
deleted file mode 100644
index 33d39493bc..0000000000
--- a/libavfilter/vf_qp.c
+++ /dev/null
@@ -1,183 +0,0 @@ 
-/*
- * Copyright (C) 2004 Michael Niedermayer <michaelni@gmx.at>
- *
- * This file is part of FFmpeg.
- *
- * FFmpeg is free software; you can redistribute it and/or
- * modify it under the terms of the GNU Lesser General Public
- * License as published by the Free Software Foundation; either
- * version 2.1 of the License, or (at your option) any later version.
- *
- * FFmpeg is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
- * Lesser General Public License for more details.
- *
- * You should have received a copy of the GNU Lesser General Public
- * License along with FFmpeg; if not, write to the Free Software
- * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
- */
-
-#include <math.h>
-#include "libavutil/eval.h"
-#include "libavutil/imgutils.h"
-#include "libavutil/pixdesc.h"
-#include "libavutil/opt.h"
-#include "avfilter.h"
-#include "formats.h"
-#include "internal.h"
-#include "video.h"
-
-typedef struct QPContext {
-    const AVClass *class;
-    char *qp_expr_str;
-    int8_t lut[257];
-    int h, qstride;
-    int evaluate_per_mb;
-} QPContext;
-
-#define OFFSET(x) offsetof(QPContext, x)
-#define FLAGS AV_OPT_FLAG_FILTERING_PARAM|AV_OPT_FLAG_VIDEO_PARAM
-
-static const AVOption qp_options[] = {
-    { "qp", "set qp expression", OFFSET(qp_expr_str), AV_OPT_TYPE_STRING, {.str=NULL}, 0, 0, FLAGS },
-    { NULL }
-};
-
-AVFILTER_DEFINE_CLASS(qp);
-
-static int config_input(AVFilterLink *inlink)
-{
-    AVFilterContext *ctx = inlink->dst;
-    QPContext *s = ctx->priv;
-    int i;
-    int ret;
-    AVExpr *e = NULL;
-    static const char *var_names[] = { "known", "qp", "x", "y", "w", "h", NULL };
-
-    if (!s->qp_expr_str)
-        return 0;
-
-    ret = av_expr_parse(&e, s->qp_expr_str, var_names, NULL, NULL, NULL, NULL, 0, ctx);
-    if (ret < 0)
-        return ret;
-
-    s->h       = (inlink->h + 15) >> 4;
-    s->qstride = (inlink->w + 15) >> 4;
-    for (i = -129; i < 128; i++) {
-        double var_values[] = { i != -129, i, NAN, NAN, s->qstride, s->h, 0};
-        double temp_val = av_expr_eval(e, var_values, NULL);
-
-        if (isnan(temp_val)) {
-            if(strchr(s->qp_expr_str, 'x') || strchr(s->qp_expr_str, 'y'))
-                s->evaluate_per_mb = 1;
-            else {
-                av_expr_free(e);
-                return AVERROR(EINVAL);
-            }
-        }
-
-        s->lut[i + 129] = lrintf(temp_val);
-    }
-    av_expr_free(e);
-
-    return 0;
-}
-
-static int filter_frame(AVFilterLink *inlink, AVFrame *in)
-{
-    AVFilterContext *ctx = inlink->dst;
-    AVFilterLink *outlink = ctx->outputs[0];
-    QPContext *s = ctx->priv;
-    AVBufferRef *out_qp_table_buf;
-    AVFrame *out = NULL;
-    const int8_t *in_qp_table;
-    int type, stride, ret;
-
-    if (!s->qp_expr_str || ctx->is_disabled)
-        return ff_filter_frame(outlink, in);
-
-    out_qp_table_buf = av_buffer_alloc(s->h * s->qstride);
-    if (!out_qp_table_buf) {
-        ret = AVERROR(ENOMEM);
-        goto fail;
-    }
-
-    out = av_frame_clone(in);
-    if (!out) {
-        av_buffer_unref(&out_qp_table_buf);
-        ret = AVERROR(ENOMEM);
-        goto fail;
-    }
-
-    in_qp_table = av_frame_get_qp_table(in, &stride, &type);
-    av_frame_set_qp_table(out, out_qp_table_buf, s->qstride, type);
-
-
-    if (s->evaluate_per_mb) {
-        int y, x;
-
-        for (y = 0; y < s->h; y++)
-            for (x = 0; x < s->qstride; x++) {
-                int qp = in_qp_table ? in_qp_table[x + stride * y] : NAN;
-                double var_values[] = { !!in_qp_table, qp, x, y, s->qstride, s->h, 0};
-                static const char *var_names[] = { "known", "qp", "x", "y", "w", "h", NULL };
-                double temp_val;
-
-                ret = av_expr_parse_and_eval(&temp_val, s->qp_expr_str,
-                                            var_names, var_values,
-                                            NULL, NULL, NULL, NULL, 0, 0, ctx);
-                if (ret < 0)
-                    goto fail;
-                out_qp_table_buf->data[x + s->qstride * y] = lrintf(temp_val);
-            }
-    } else if (in_qp_table) {
-        int y, x;
-
-        for (y = 0; y < s->h; y++)
-            for (x = 0; x < s->qstride; x++)
-                out_qp_table_buf->data[x + s->qstride * y] = s->lut[129 +
-                    ((int8_t)in_qp_table[x + stride * y])];
-    } else {
-        int y, x, qp = s->lut[0];
-
-        for (y = 0; y < s->h; y++)
-            for (x = 0; x < s->qstride; x++)
-                out_qp_table_buf->data[x + s->qstride * y] = qp;
-    }
-
-    ret = ff_filter_frame(outlink, out);
-    out = NULL;
-fail:
-    av_frame_free(&in);
-    av_frame_free(&out);
-    return ret;
-}
-
-static const AVFilterPad qp_inputs[] = {
-    {
-        .name         = "default",
-        .type         = AVMEDIA_TYPE_VIDEO,
-        .filter_frame = filter_frame,
-        .config_props = config_input,
-    },
-    { NULL }
-};
-
-static const AVFilterPad qp_outputs[] = {
-    {
-        .name = "default",
-        .type = AVMEDIA_TYPE_VIDEO,
-    },
-    { NULL }
-};
-
-AVFilter ff_vf_qp = {
-    .name          = "qp",
-    .description   = NULL_IF_CONFIG_SMALL("Change video quantization parameters."),
-    .priv_size     = sizeof(QPContext),
-    .inputs        = qp_inputs,
-    .outputs       = qp_outputs,
-    .priv_class    = &qp_class,
-    .flags         = AVFILTER_FLAG_SUPPORT_TIMELINE_INTERNAL,
-};
diff --git a/tests/fate/filter-video.mak b/tests/fate/filter-video.mak
index 2da27f714a..5f4fd75b40 100644
--- a/tests/fate/filter-video.mak
+++ b/tests/fate/filter-video.mak
@@ -531,21 +531,16 @@  fate-filter-idet: CMD = framecrc -flags bitexact -idct simple -i $(SRC) -vf idet
 FATE_FILTER_VSYNTH-$(CONFIG_PAD_FILTER) += fate-filter-pad
 fate-filter-pad: CMD = video_filter "pad=iw*1.5:ih*1.5:iw*0.3:ih*0.2"
 
-FATE_FILTER_PP = fate-filter-pp fate-filter-pp1 fate-filter-pp2 fate-filter-pp3 fate-filter-pp4 fate-filter-pp5 fate-filter-pp6
+FATE_FILTER_PP = fate-filter-pp fate-filter-pp1 fate-filter-pp4 fate-filter-pp5 fate-filter-pp6
 FATE_FILTER_VSYNTH-$(CONFIG_PP_FILTER) += $(FATE_FILTER_PP)
 $(FATE_FILTER_PP): fate-vsynth1-mpeg4-qprd
 
 fate-filter-pp:  CMD = framecrc -flags bitexact -idct simple -i $(TARGET_PATH)/tests/data/fate/vsynth1-mpeg4-qprd.avi -frames:v 5 -flags +bitexact -vf "pp=be/hb/vb/tn/l5/al"
 fate-filter-pp1: CMD = video_filter "pp=fq|4/be/hb/vb/tn/l5/al"
-fate-filter-pp2: CMD = video_filter "qp=x+y,pp=be/h1/v1/lb"
-fate-filter-pp3: CMD = video_filter "qp=x+y,pp=be/ha|128|7/va/li"
 fate-filter-pp4: CMD = video_filter "pp=be/ci"
 fate-filter-pp5: CMD = video_filter "pp=md"
 fate-filter-pp6: CMD = video_filter "pp=be/fd"
 
-FATE_FILTER_VSYNTH-$(call ALLYES, QP_FILTER PP_FILTER) += fate-filter-qp
-fate-filter-qp: CMD = video_filter "qp=17,pp=be/hb/vb/tn/l5/al"
-
 FATE_FILTER_VSYNTH-$(CONFIG_SELECT_FILTER) += fate-filter-select
 fate-filter-select: CMD = framecrc -flags bitexact -idct simple -i $(SRC) -vf "select=not(eq(mod(n\,2)\,0)+eq(mod(n\,3)\,0))" -frames:v 25 -flags +bitexact
 
diff --git a/tests/ref/fate/filter-pp2 b/tests/ref/fate/filter-pp2
deleted file mode 100644
index ed5e77322a..0000000000
--- a/tests/ref/fate/filter-pp2
+++ /dev/null
@@ -1 +0,0 @@ 
-pp2                 566d48ad25dfa7a9680de933cbdf66d9
diff --git a/tests/ref/fate/filter-pp3 b/tests/ref/fate/filter-pp3
deleted file mode 100644
index 536bf8e9d2..0000000000
--- a/tests/ref/fate/filter-pp3
+++ /dev/null
@@ -1 +0,0 @@ 
-pp3                 586fc14a52699540a865c070dd113229