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

Tue Feb 25 11:21:34 EET 2020

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

-- 
Anton Khirnov