[FFmpeg-devel] [PATCH 25/33] fftools/ffmpeg_filter: stop disregarding user-specified pixel format
Paul B Mahol
onemda at gmail.com
Sat Jul 15 11:59:50 EEST 2023
On Fri, Jul 14, 2023 at 7:06 PM Anton Khirnov <anton at khirnov.net> 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
> * 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).
>
Agreed, this patch is good to go into repo.
>
> --
> 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".
>
More information about the ffmpeg-devel
mailing list