[FFmpeg-devel] [PATCH 4/7] lavfi/vf_spp: convert to the video_enc_params API
Michael Niedermayer
michael at niedermayer.cc
Fri Dec 4 19:24:56 EET 2020
On Fri, Nov 27, 2020 at 03:38:51PM +0100, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2020-10-09 19:17:06)
> > On Fri, Oct 09, 2020 at 08:13:24AM +0200, Anton Khirnov wrote:
> > > Quoting Michael Niedermayer (2020-10-06 00:15:06)
> > > > On Mon, Oct 05, 2020 at 10:26:24AM +0200, Anton Khirnov wrote:
> > > > > Quoting Michael Niedermayer (2020-10-03 20:23:02)
> > > > > > On Fri, Oct 02, 2020 at 08:03:28PM +0200, Anton Khirnov wrote:
> > > > > > > ---
> > > > > > > libavfilter/Makefile | 2 +-
> > > > > > > libavfilter/vf_spp.c | 57 ++++++++++++++++---------------------
> > > > > > > libavfilter/vf_spp.h | 3 +-
> > > > > > > tests/fate/filter-video.mak | 4 +--
> > > > > > > 4 files changed, 29 insertions(+), 37 deletions(-)
> > > > > > >
> > > > > > > diff --git a/libavfilter/Makefile b/libavfilter/Makefile
> > > > > > > index d20f2937b6..2669d7b84b 100644
> > > > > > > --- a/libavfilter/Makefile
> > > > > > > +++ b/libavfilter/Makefile
> > > > > > > @@ -404,7 +404,7 @@ OBJS-$(CONFIG_SOBEL_FILTER) += vf_convolution.o
> > > > > > > OBJS-$(CONFIG_SOBEL_OPENCL_FILTER) += vf_convolution_opencl.o opencl.o \
> > > > > > > opencl/convolution.o
> > > > > > > OBJS-$(CONFIG_SPLIT_FILTER) += split.o
> > > > > > > -OBJS-$(CONFIG_SPP_FILTER) += vf_spp.o
> > > > > > > +OBJS-$(CONFIG_SPP_FILTER) += vf_spp.o qp_table.o
> > > > > > > OBJS-$(CONFIG_SR_FILTER) += vf_sr.o
> > > > > > > OBJS-$(CONFIG_SSIM_FILTER) += vf_ssim.o framesync.o
> > > > > > > OBJS-$(CONFIG_STEREO3D_FILTER) += vf_stereo3d.o
> > > > > > > diff --git a/libavfilter/vf_spp.c b/libavfilter/vf_spp.c
> > > > > > > index 4bcc6429e0..2eb383be03 100644
> > > > > > > --- a/libavfilter/vf_spp.c
> > > > > > > +++ b/libavfilter/vf_spp.c
> > > > > > > @@ -36,6 +36,7 @@
> > > > > > > #include "libavutil/opt.h"
> > > > > > > #include "libavutil/pixdesc.h"
> > > > > > > #include "internal.h"
> > > > > > > +#include "qp_table.h"
> > > > > > > #include "vf_spp.h"
> > > > > > >
> > > > > > > enum mode {
> > > > > > > @@ -289,7 +290,7 @@ static void filter(SPPContext *p, uint8_t *dst, uint8_t *src,
> > > > > > > } else{
> > > > > > > const int qps = 3 + is_luma;
> > > > > > > qp = qp_table[(FFMIN(x, width - 1) >> qps) + (FFMIN(y, height - 1) >> qps) * qp_stride];
> > > > > > > - qp = FFMAX(1, ff_norm_qscale(qp, p->qscale_type));
> > > > > > > + qp = FFMAX(1, ff_norm_qscale(qp, FF_QSCALE_TYPE_MPEG2));
> > > > > >
> > > > > > wouldnt this break the cases where qscale_type is not FF_QSCALE_TYPE_MPEG2 ?
> > > > >
> > > > > There should be no such cases - in the new API, only TYPE_MPEG2 exists
> > > > > (disregarding newer codecs that were not supported by this filter
> > > > > anyway).
> > > >
> > > > before the patch the code was intended to convert the quantization step size
> > > > used in the codec to the same scale as the filter used. disregarding if the
> > > > codec was 8x8 dct based. In fact spp should not require the input to be 8x8 dct
> > > > based at all, why should it? It uses the DCT as a means to favor real images
> > > > and remove "noise" that is not part of real images. It should for example also
> > > > work if a image has blocking artifacts that look like hexagons or triangles
> > > >
> > > > I never tried but H264 with disabled loop filter should benefit from spp in
> > > > terms of subjective quality as long as the filter strength is set appropriately
> > > > That is for streams where the bitrate is low enough to see artifacts
> > >
> > > Are you saying I should expand this filter to support new codecs? That
> > > does not seem appropriate for a patch that only adapts it to new API.
> >
> > no
> > but before your patch theres a central place ff_norm_qscale()
> > that has access to both the QP type (codec type) and the QP
> > and can produce a standarized QP
> > So someone who wanted to SPP support some random new codec could
> > do it there and at the same time have 4 other filters for free also
> > support the new codec.
>
> ff_norm_qscale() is still there and the filter still calls it. But if
> someone wants to extend it to support other codecs, they still need to
> add code to ff_qp_table_extract() to handle
> AVVideoEncParams.type != AV_VIDEO_ENC_PARAMS_MPEG2.
>
> It does not seem reasonable to me to pretend the code supports something
> that it actually does not.
I see what you mean but as its done by the patch the code is not clean afterwards
at all.
For example the argument is always the same, it asks for it being removed.
If you dont want to pass the type without support in ff_qp_table_extract()
then IMHO its better to add very minimal generic support in ff_qp_table_extract()
instead of hardcoding a type leaving the code with a constant argument.
[...]
> > If you use AVVideoEncParamsType or something equivalent to it / qp_type then
> > that resolves this concern
> >
> > unless iam missing something of course ...
>
> It cannot be handled generically (at least not easily), because the data
> layout allowed by AVVideoBlockParams is more general than the one
> handled by those *pp filters. Additionally, the interpretation of qp is
> codec-dependent. So IMO the code should not pretends it can handle
> arbitrary codecs, when it is not actually true.
I dont see why generic handling wouldnt work or is hard
input is a set of rectangles, output is a ordered list of 16x16 squares
The simplest could be a mere intersect and take maximum of the QP
one wouldnt even need to use average for this to probably work reasonable
codec specific QP would need to be mapped to the expectec scale though
to avoid misunderstanding, iam not asking you to do any work here, just not to remove
things like the type. OTOH if you want it to be all functional or not at all
i think a very basic version of this should be rather simple
that said, iam not blocking any patches above is just my oppinion
Thanks
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Dictatorship naturally arises out of democracy, and the most aggravated
form of tyranny and slavery out of the most extreme liberty. -- Plato
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20201204/62674657/attachment.sig>
More information about the ffmpeg-devel
mailing list