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

[Anton Khirnov]

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
* libavcodec will now helpfully print a list of formats supported by the
  encoder if the caller gives it a wrong one

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

Programs that try to second-guess user's explicit instructions are
broken by design. This "convenience" argument is entirely specious:
* users who do not know what they want get something that works by
  default
* 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
* 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

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

> your patch fixes this and adjusts the threshold to not accept formats
> that are too different ?
> i agree that would be a bugfix and also it should be quite easy to do
> as teh code already computes what the differences are and computes a score
> 
> 
> > 2) If you want syntax for fuzzy format specification, patches are
> >    welcome. But it should not interfere with specifying an exact format.
> 
> the code is there already
> you found a case where it behaves unreasonable, so change the threshold
> at which it accepts differences, you can even specify what differences
> it accepts
> theres also get_pix_fmt_score() that takes 2 pixel formats and gives
> you a bitmask discribing their differences together with a numeric score
> 
> And once the threshold / mask is adjusted to accept what is considered
> similar enough for the pixfmt ffmpeg command line use case.
> You can move this to whatever syntax you prefer. You are the expert when
> it comes to the ffmpeg command line.
> Simply removing the code and calling for someone to add it back in
> is a bit odd.

Then my expert opinion is this - this code:
* is broken by design
* is actively harmful
* solves no actual problem
* adds unnecessary complexity
It should be removed in its entirety.

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

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

-- 
Anton Khirnov