| 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 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
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
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".
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.
> 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
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".
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".
> -----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
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".
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?
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.
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.
> -----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
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".
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.
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".
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 [...]
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.
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
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
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 :)
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.
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".
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.
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,
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
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,
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".
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.
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
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".
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.
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?
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 [...]
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 --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