[FFmpeg-devel] [PATCH 4/7] lavfi/vf_spp: convert to the video_enc_params API

[Michael Niedermayer]

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.

The patch does not replace the qp type by its replacement AVVideoEncParamsType
but instead a wrong hardcoded value
If you use AVVideoEncParamsType or something equivalent to it / qp_type then
that resolves this concern

unless iam missing something of course ...

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Good people do not need laws to tell them to act responsibly, while bad
people will find a way around the laws. -- 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/20201009/93b6dd3b/attachment.sig>