Re: [FFmpeg-devel] [PATCH 25/33] fftools/ffmpeg_filter: stop disregarding user-specified pixel format

[Michael Niedermayer <michael@niedermayer.cc>]

[-- Attachment #1.1: Type: text/plain, Size: 8738 bytes --]

Hi

On Fri, Jul 14, 2023 at 07:06:15PM +0200, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2023-07-14 17:47:19)
> > On Fri, Jul 14, 2023 at 11:44:07AM +0200, Anton Khirnov wrote:
> > > Quoting Michael Niedermayer (2023-07-14 01:11:07)
> > > > On Thu, Jul 13, 2023 at 12:55:45PM +0200, Anton Khirnov wrote:
> > > > > When the user explicitly specifies a pixel format that is not supported
> > > > > by the encoder, ffmpeg CLI will currently use some heuristics to pick
> > > > > another supported format. This is wrong and the correct action here is
> > > > > to fail.
> > > > > 
> > > > > Surprisingly, a number of FATE tests are affected by this and actually
> > > > > use a different pixel format than is specified in the makefiles.
> > > > > ---
> > > > >  doc/ffmpeg.texi                               |  3 +-
> > > > >  fftools/ffmpeg_filter.c                       | 35 +------------------
> > > > >  tests/fate/fits.mak                           |  6 ++--
> > > > >  tests/fate/lavf-video.mak                     |  2 +-
> > > > >  tests/fate/vcodec.mak                         |  4 +--
> > > > >  .../{fitsdec-gbrap16le => fitsdec-gbrap16be}  |  4 +--
> > > > >  .../fate/{fitsdec-gbrp16 => fitsdec-gbrp16be} |  4 +--
> > > > >  tests/ref/lavf/gif                            |  2 +-
> > > > >  8 files changed, 13 insertions(+), 47 deletions(-)
> > > > >  rename tests/ref/fate/{fitsdec-gbrap16le => fitsdec-gbrap16be} (79%)
> > > > >  rename tests/ref/fate/{fitsdec-gbrp16 => fitsdec-gbrp16be} (79%)
> > > > > 
> > > > > diff --git a/doc/ffmpeg.texi b/doc/ffmpeg.texi
> > > > > index 6769f8d305..08b11097b7 100644
> > > > > --- a/doc/ffmpeg.texi
> > > > > +++ b/doc/ffmpeg.texi
> > > > > @@ -1014,8 +1014,7 @@ Disable autoscale at your own risk.
> > > > >  @item -pix_fmt[:@var{stream_specifier}] @var{format} (@emph{input/output,per-stream})
> > > > >  Set pixel format. Use @code{-pix_fmts} to show all the supported
> > > > >  pixel formats.
> > > > > -If the selected pixel format can not be selected, ffmpeg will print a
> > > > > -warning and select the best pixel format supported by the encoder.
> > > > > +
> > > > >  If @var{pix_fmt} is prefixed by a @code{+}, ffmpeg will exit with an error
> > > > >  if the requested pixel format can not be selected, and automatic conversions
> > > > >  inside filtergraphs are disabled.
> > > > 
> > > > The commit message makes this sound like a bugfix, while really this is
> > > > removing a documented feature.
> > > 
> > > It is a bugfix in my eyes. When you explicitly tell a program to perform
> > > a specific action, and the program just decides to do something else,
> > > then that program is broken.
> > > 
> > > As far as I can tell, this "feature" was added by you in 89f86379797
> > > with no explanation or documentation beyond 'fix regression with png'.
> > > It was later documented in a largely-unrelated commit that added
> > > something else.
> > > 
> > > I see no argument whatsoever for why we should have such a "smart"
> > 
> > As said previously,
> > The user cannot be expected to know if a implementation uses planar
> > or packed rgb, bgr or rgb.
> 
> Which is why
> * libavfilter will by default convert to a format supported by the
>   encoder

If the user uses -pix_fmt she likely doesnt want "any" format but has a
preferrance for some reason like 8bit or rgb for example


> * libavcodec will now helpfully print a list of formats supported by the
>   encoder if the caller gives it a wrong one

Thats good, but its a case per case adjustment.


> 
> > This is not a inherent part of the file/stream/input in many cases
> > its not a problem for you because you are a FFmpeg developer and work
> > with this every day but it is a inconvenience for users
> 
> Should we then replace any failing commandline with something that will
> not fail? Ignore any options with incorrect values? All in the name of
> convenience? Maybe you should try web development.

-pix_fmt is not "any command line"
https://en.wikipedia.org/wiki/Straw_man


> 
> Programs that try to second-guess user's explicit instructions are
> broken by design.

Maybe but the pix_fmt has 2 syntaxes one for explicit instructions
and one for a close one. Your patch modifies the "pick a close one" path


> This "convenience" argument is entirely specious:
> * users who do not know what they want get something that works by
>   default

Thats not true, 16bit yuv might not work for example for the users case


> * users who specify a wrong format get a list of correct formats they
>   can just pick from; that is as convenient as it gets for this kind of
>   a program

it works probably for most cases but its an extra step for the user
and a change in command line syntax


> * users who require yet more convenience and/or handholding can use a
>   graphical program such as Handbrake; we should not try to be
>   Handbrake, they are better at it than us

I dont understand what you are trying to say here
"require yet more convenience" is a very strange wording
I dont require convenience, i can use intels documentation, teh ELF
docs and a hex editor. But I instead use a compiler. Similarly
I surely can manually tyoe out the number of pixels for each frame
used but instead i expect ffmpeg to do that for me from the width and
height. Why should i not be able to tell FFmpeg to use a 8bit RGB format?
and instead receive a error message with a list of which format the
implementation supports than search the RGB variant be that RGB, BGR or GBRP
and write that back to ffmpeg in a 2nd call ?

This is not related to GUI vs command line interface. a GUI can show
that list too.


> 
> > > > To me as a lazy person it surely feels usefull to be able to ask for
> > > > both "exactly rgb" as well as something close to rgb (like bgr or gbrp)
> > > > without needing to know what each individual codec uses to return R,G,B
> > > 
> > 
> > > 1) This code does not give you the ability to specify "something close to rgb".
> > >    You specify a precise pixel format, and this code gives you
> > >    something. That something might be what you asked for, or something
> > >    close to it, or something completely unrelated.
> > >    E.g.
> > >      ffmpeg -i in.mkv -map 0:v -c:v libx264 -pix_fmt pal8 -t 1 out.mkv
> > >    produces yuv444p. How is it close to pal8?
> > 
> > (it is close because it can represent pal8 with little loss i think but)
> 
> pal8 and yuv444p are close? I really wonder which pair of formats would
> be not close for you then. If the set is non-empty, I'm sure I can craft
> a commandline that produces one instead of the other.

You are misunderstanding what i meant
pal8 -> yuv444p relatively little loss of information
yuv444p -> pal8 not "close" because substantial loss of information


[...]
> > > 3) No other option in ffmpeg CLI works like this. If you specify
> > >    something, you get that or an error.
> > 
> > iam not sure thats true
> > i think width and height and even vs odd have their fuzzyness at places and
> > so probably does the aspect ratio. Its not failing if it has to be rounded
> > to a close value
> > 
> > you could try
> > ./ffmpeg -i ~/videos/matrixbench_mpeg2.mpg  -aspect 512:511 test.m4v
> > there are only 8:8 bit so 512:511 cant be stored nothing fails you just
> > dont get 512:511
> > 
> > and iam pretty sure there are many more examples where "close" values
> > are taken silently
> 
> ffmpeg -i in.mkv -map 0:v -s 512x511 -c:v libx264 -f null -
> [...]
> [libx264 @ 0x55f8029a71c0] height not divisible by 2 (512x511)
> [vost#0:0/libx264 @ 0x55f802a61840] Error while opening encoder - maybe incorrect parameters such as bit_rate, rate, width or height.

maybe you want to remove "force_divisible_by" too and let the user
specify the value explicitly


> 
> Besides, you keep talking about "close" when the code in question makes
> no guarantee whatsoever that the result is in any way "close" (whatever
> that might even mean).

The code picks the "closest" format.
That can also be seen in your example of pal8 ->yuv444p. Where the encoder
supports nothing closer.

Noone seems to have been bothered before by the fact that the code makes
such choices if its fed by an impossible target.
As said previously, you can just adjust the value at which it hard fails

Do you want me to look at how that can be done and send a patch doing that ?

thx

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

No snowflake in an avalanche ever feels responsible. -- Voltaire

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".