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

[Paul B Mahol]

On 2/25/20, Anton Khirnov <anton at 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 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.

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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".