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

[Michael Niedermayer]

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


[...]

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

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.


> 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

thx

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

Dictatorship: All citizens are under surveillance, all their steps and
actions recorded, for the politicians to enforce control.
Democracy: All politicians are under surveillance, all their steps and
actions recorded, for the citizens to enforce control.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20230714/48669bfa/attachment.sig>