| Message ID | 20230713105553.21052-25-anton@khirnov.net |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,01/33] fftools/ffmpeg_mux_init: return errors from of_open() instead of aborting | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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 also breaks some scripts (fate is an example as it requires changes) If the removial of that feature is intended, it should be argued somewhere that this feature is never usefull. 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 thx [...]
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.
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.
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 [...]
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).
On Fri, Jul 14, 2023 at 7:06 PM Anton Khirnov <anton@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@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
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 [...]
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. diff --git a/fftools/ffmpeg_filter.c b/fftools/ffmpeg_filter.c index caf85194c5..0625a9bc92 100644 --- a/fftools/ffmpeg_filter.c +++ b/fftools/ffmpeg_filter.c @@ -277,38 +277,6 @@ static const enum AVPixelFormat *get_compliance_normal_pix_fmts(const AVCodec *c } } -static enum AVPixelFormat -choose_pixel_fmt(const AVCodec *codec, enum AVPixelFormat target, - int strict_std_compliance) -{ - if (codec && codec->pix_fmts) { - const enum AVPixelFormat *p = codec->pix_fmts; - const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(target); - //FIXME: This should check for AV_PIX_FMT_FLAG_ALPHA after PAL8 pixel format without alpha is implemented - int has_alpha = desc ? desc->nb_components % 2 == 0 : 0; - enum AVPixelFormat best= AV_PIX_FMT_NONE; - - if (strict_std_compliance > FF_COMPLIANCE_UNOFFICIAL) { - p = get_compliance_normal_pix_fmts(codec, p); - } - for (; *p != AV_PIX_FMT_NONE; p++) { - best = av_find_best_pix_fmt_of_2(best, *p, target, has_alpha, NULL); - if (*p == target) - break; - } - if (*p == AV_PIX_FMT_NONE) { - if (target != AV_PIX_FMT_NONE) - av_log(NULL, AV_LOG_WARNING, - "Incompatible pixel format '%s' for codec '%s', auto-selecting format '%s'\n", - av_get_pix_fmt_name(target), - codec->name, - av_get_pix_fmt_name(best)); - return best; - } - } - return target; -} - /* May return NULL (no pixel format found), a static string or a string * backed by the bprint. Nothing has been written to the AVBPrint in case * NULL is returned. The AVBPrint provided should be clean. */ @@ -327,8 +295,7 @@ static const char *choose_pix_fmts(OutputFilter *ofilter, AVBPrint *bprint) return av_get_pix_fmt_name(ost->enc_ctx->pix_fmt); } if (ost->enc_ctx->pix_fmt != AV_PIX_FMT_NONE) { - return av_get_pix_fmt_name(choose_pixel_fmt(enc->codec, enc->pix_fmt, - ost->enc_ctx->strict_std_compliance)); + return av_get_pix_fmt_name(enc->pix_fmt); } else if (enc->codec->pix_fmts) { const enum AVPixelFormat *p; diff --git a/tests/fate/fits.mak b/tests/fate/fits.mak index b9e99d97ee..d85946bc1a 100644 --- a/tests/fate/fits.mak +++ b/tests/fate/fits.mak @@ -8,8 +8,8 @@ tests/data/fits-multi.fits: ffmpeg$(PROGSSUF)$(EXESUF) | tests/data # TODO: Use an actual 64bit input file and fix the gbrp16 test on big-endian fits-png-map-gray := gray8 fits-png-map-gbrp := rgb24 -fits-png-map-gbrp16 := rgb48 -fits-png-map-gbrap16le := rgba64 +fits-png-map-gbrp16be := rgb48 +fits-png-map-gbrap16be := rgba64 FATE_FITS_DEC-$(call FRAMECRC, FITS, FITS, SCALE_FILTER) += fate-fitsdec-ext_data_min_max fate-fitsdec-ext_data_min_max: CMD = framecrc -i $(TARGET_SAMPLES)/fits/x0cj010ct_d0h.fit -pix_fmt gray16le -vf scale @@ -30,7 +30,7 @@ fate-fitsdec-multi: CMD = framecrc -i $(TARGET_PATH)/tests/data/fits-multi.fits fate-fitsdec%: PIXFMT = $(word 3, $(subst -, ,$(@))) fate-fitsdec%: CMD = transcode image2 $(TARGET_SAMPLES)/png1/lena-$(fits-png-map-$(PIXFMT)).png fits "-vf scale -pix_fmt $(PIXFMT)" -FATE_FITS_DEC_PIXFMT = gray gbrp gbrp16 gbrap16le +FATE_FITS_DEC_PIXFMT = gray gbrp gbrp16be gbrap16be FATE_FITS_DEC-$(call TRANSCODE, FITS, FITS, IMAGE2_DEMUXER PNG_DECODER SCALE_FILTER) += $(FATE_FITS_DEC_PIXFMT:%=fate-fitsdec-%) FATE_FITS += $(FATE_FITS_DEC-yes) diff --git a/tests/fate/lavf-video.mak b/tests/fate/lavf-video.mak index e73f8f203b..da3b114bc8 100644 --- a/tests/fate/lavf-video.mak +++ b/tests/fate/lavf-video.mak @@ -27,7 +27,7 @@ fate-lavf-gbrp.fits: CMD = lavf_video "-pix_fmt gbrp" fate-lavf-gbrap.fits: CMD = lavf_video "-pix_fmt gbrap" fate-lavf-gbrp16be.fits: CMD = lavf_video "-pix_fmt gbrp16be" fate-lavf-gbrap16be.fits: CMD = lavf_video "-pix_fmt gbrap16be" -fate-lavf-gif: CMD = lavf_video "-pix_fmt rgb24" +fate-lavf-gif: CMD = lavf_video "-pix_fmt rgb8" FATE_AVCONV += $(FATE_LAVF_VIDEO) fate-lavf-video fate-lavf: $(FATE_LAVF_VIDEO) diff --git a/tests/fate/vcodec.mak b/tests/fate/vcodec.mak index 2839e54de8..e32d28c556 100644 --- a/tests/fate/vcodec.mak +++ b/tests/fate/vcodec.mak @@ -210,9 +210,9 @@ fate-vsynth%-h263p: ENCOPTS = -qscale 2 -flags +aic -umv 1 -aiv 1 - FATE_VCODEC_SCALE-$(call ENCDEC, HUFFYUV, AVI) += huffyuv huffyuvbgr24 huffyuvbgra fate-vsynth%-huffyuv: ENCOPTS = -c:v huffyuv -pix_fmt yuv422p -sws_flags neighbor fate-vsynth%-huffyuv: DECOPTS = -sws_flags neighbor -fate-vsynth%-huffyuvbgr24: ENCOPTS = -c:v huffyuv -pix_fmt bgr24 -sws_flags neighbor +fate-vsynth%-huffyuvbgr24: ENCOPTS = -c:v huffyuv -pix_fmt rgb24 -sws_flags neighbor fate-vsynth%-huffyuvbgr24: DECOPTS = -sws_flags neighbor -fate-vsynth%-huffyuvbgra: ENCOPTS = -c:v huffyuv -pix_fmt bgr32 -sws_flags neighbor +fate-vsynth%-huffyuvbgra: ENCOPTS = -c:v huffyuv -pix_fmt rgb32 -sws_flags neighbor fate-vsynth%-huffyuvbgra: DECOPTS = -sws_flags neighbor FATE_VCODEC_SCALE-$(call ENCDEC, JPEGLS, AVI) += jpegls diff --git a/tests/ref/fate/fitsdec-gbrap16le b/tests/ref/fate/fitsdec-gbrap16be similarity index 79% rename from tests/ref/fate/fitsdec-gbrap16le rename to tests/ref/fate/fitsdec-gbrap16be index 53ef980b13..1174a0f1d8 100644 --- a/tests/ref/fate/fitsdec-gbrap16le +++ b/tests/ref/fate/fitsdec-gbrap16be @@ -1,5 +1,5 @@ -64526d8da12d1fa07ceea5725647076f *tests/data/fate/fitsdec-gbrap16le.fits -135360 tests/data/fate/fitsdec-gbrap16le.fits +64526d8da12d1fa07ceea5725647076f *tests/data/fate/fitsdec-gbrap16be.fits +135360 tests/data/fate/fitsdec-gbrap16be.fits #tb 0: 1/1 #media_type 0: video #codec_id 0: rawvideo diff --git a/tests/ref/fate/fitsdec-gbrp16 b/tests/ref/fate/fitsdec-gbrp16be similarity index 79% rename from tests/ref/fate/fitsdec-gbrp16 rename to tests/ref/fate/fitsdec-gbrp16be index 9250690e9b..ff4ca9e65c 100644 --- a/tests/ref/fate/fitsdec-gbrp16 +++ b/tests/ref/fate/fitsdec-gbrp16be @@ -1,5 +1,5 @@ -2078208c93ba417d3fe150ba42bf5a30 *tests/data/fate/fitsdec-gbrp16.fits -103680 tests/data/fate/fitsdec-gbrp16.fits +2078208c93ba417d3fe150ba42bf5a30 *tests/data/fate/fitsdec-gbrp16be.fits +103680 tests/data/fate/fitsdec-gbrp16be.fits #tb 0: 1/1 #media_type 0: video #codec_id 0: rawvideo diff --git a/tests/ref/lavf/gif b/tests/ref/lavf/gif index fc94b9df3d..7f353df286 100644 --- a/tests/ref/lavf/gif +++ b/tests/ref/lavf/gif @@ -1,3 +1,3 @@ e35f5ea283bbcb249818e0078ec72664 *tests/data/lavf/lavf.gif 2011766 tests/data/lavf/lavf.gif -tests/data/lavf/lavf.gif CRC=0x2429faff +tests/data/lavf/lavf.gif CRC=0x37f4d323
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%)