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

[Timo Rothenpieler]

On 14/07/2023 11:44, 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"
> selection for pixel formats, but nothing else. It goes entirely against
> the way any other option works.
> 
>> It also breaks some scripts (fate is an example as it requires changes)
> 
> No, its removal reveals already broken FATE tests, as I see no
> indication that those tests intended to use a different format than
> indicated on the commandline. It seems far more likely that this
> misfeature confused the tests' authors as to what format is actually
> used. That is IMO yet another argument for removing this.
> 
>> If the removial of that feature is intended, it should be argued somewhere
>> that this feature is never usefull.
> 
> https://xkcd.com/1172/
> 
>> 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?
> 2) If you want syntax for fuzzy format specification, patches are
>     welcome. But it should not interfere with specifying an exact format.
> 3) No other option in ffmpeg CLI works like this. If you specify
>     something, you get that or an error.

I completely agree. I didn't even realize this behaviour was a thing.
Getting a more or less random other pixel format than requested seems 
very surprising and undesireable.

However, this is unarguably an API break and can and will break existing 
scripts and applications.
So it should go through the usual deprecation cycle.
Maybe intensify the warning a bit, make it hang executing for a few 
seconds so people take notice.