| Message ID | 20220919094604.4645-1-thilo.borgmann@mail.de |
|---|---|
| Headers | show |
| Series | Add display_matrix option | expand |
I still see no convincing arguments in favor of adding all this complexity just so that we can stuff multiple options into one and get a WORSE user interface. Just add multiple options.
Anton Khirnov (12022-09-20): > I still see no convincing arguments in favor of adding all this > complexity just so that we can stuff multiple options into one and get a > WORSE user interface. > > Just add multiple options. We agree, except on the last point. All user-visible FFmpeg types should have a way for the user to input them, self-contained and convenient with regard with the logic of the type, and a function to print them in a way that can be parsed back. Channels layouts do not have multiple options, they have av_channel_layout_from_string(). Rationals are not entered with -framerate_num 24000 -framerate_den 1001, they are entered as a ratio, and so on. It should be systematic. Regards,
Quoting Nicolas George (2022-09-20 11:05:59) > Anton Khirnov (12022-09-20): > > I still see no convincing arguments in favor of adding all this > > complexity just so that we can stuff multiple options into one and get a > > WORSE user interface. > > > > Just add multiple options. > > We agree, except on the last point. > > All user-visible FFmpeg types should have a way for the user to input > them, self-contained and convenient with regard with the logic of the > type, and a function to print them in a way that can be parsed back. > > Channels layouts do not have multiple options, they have > av_channel_layout_from_string(). Rationals are not entered with > -framerate_num 24000 -framerate_den 1001, they are entered as a ratio, > and so on. It should be systematic. I have no problem with adding a -display_matrix option that allows expert users to specify all nine values of the display transformation matrix directly. But this set is not adding such an option, it is only handling rotation, flipping and scaling. Most users will think of these as three different operations and so it will be simplest - both for us and for our users - for them to be three separate options (with documentation stating the order in which they are applied).
Hi, this is an updated and cleaned-up version of Jan's patchset discussed in [1], now v4... I'd especially appreciate any comments on 5/6, ffmpeg_opt.c:119ff which is pretty ugly as-is. Previous reviews were split between threads [2][3] and are merged into. Now it comes with cover letter as the topic would have changed again, keep track of revision and notget reviews on the same stuff in seperate threads. Should fix #8329 and #6370. Thanks, Thilo [1] https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2022-May/296553.html [2] https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2022-August/300076.html [3] https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2022-August/300122.html Jan Ekström (3): fftools: Add support for dictionary options ffmpeg: Add display_matrix option ffmpeg: Deprecate display rotation override with a metadata key Thilo Borgmann (3): lavu/opt: Allow options to be arguments of other options fftools/cmdutils: Print arguments of options lavu/display: Add horizontal and vertical scaling to the display matrix doc/APIchanges | 7 ++ doc/ffmpeg.texi | 18 ++++ fftools/cmdutils.c | 22 +++++ fftools/cmdutils.h | 3 + fftools/ffmpeg.c | 2 + fftools/ffmpeg.h | 7 ++ fftools/ffmpeg_filter.c | 13 +++ fftools/ffmpeg_opt.c | 162 +++++++++++++++++++++++++++++++++++- libavutil/display.c | 35 ++++++++ libavutil/display.h | 26 ++++++ libavutil/opt.c | 14 +++- libavutil/opt.h | 8 ++ libavutil/version.h | 4 +- tests/fate/filter-video.mak | 2 +- 14 files changed, 316 insertions(+), 7 deletions(-)