From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id 5A029463D0 for ; Sat, 15 Jul 2023 20:02:00 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 4551268C026; Sat, 15 Jul 2023 23:01:58 +0300 (EEST) Received: from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net [217.70.183.194]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id C219168C1E7 for ; Sat, 15 Jul 2023 23:01:51 +0300 (EEST) Received: by mail.gandi.net (Postfix) with ESMTPSA id A9F0A40002 for ; Sat, 15 Jul 2023 20:01:50 +0000 (UTC) Date: Sat, 15 Jul 2023 22:01:48 +0200 From: Michael Niedermayer To: FFmpeg development discussions and patches Message-ID: <20230715200148.GS1093384@pb2> References: <20230713105553.21052-1-anton@khirnov.net> <20230713105553.21052-25-anton@khirnov.net> <20230713231107.GK1093384@pb2> <168932784788.27367.2505386604250052570@lain.khirnov.net> <20230714154719.GM1093384@pb2> <168935437525.27367.7939731618097704358@lain.khirnov.net> MIME-Version: 1.0 In-Reply-To: <168935437525.27367.7939731618097704358@lain.khirnov.net> X-GND-Sasl: michael@niedermayer.cc Subject: Re: [FFmpeg-devel] [PATCH 25/33] fftools/ffmpeg_filter: stop disregarding user-specified pixel format X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Type: multipart/mixed; boundary="===============2537316631283472858==" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: --===============2537316631283472858== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="qoa3+4LHNtB0VmQh" Content-Disposition: inline --qoa3+4LHNtB0VmQh Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 sup= ported > > > > > by the encoder, ffmpeg CLI will currently use some heuristics to = pick > > > > > another supported format. This is wrong and the correct action he= re is > > > > > to fail. > > > > >=20 > > > > > Surprisingly, a number of FATE tests are affected by this and act= ually > > > > > 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 =3D> fitsdec-gbrap16be} | 4 +-- > > > > > .../fate/{fitsdec-gbrp16 =3D> fitsdec-gbrp16be} | 4 +-- > > > > > tests/ref/lavf/gif | 2 +- > > > > > 8 files changed, 13 insertions(+), 47 deletions(-) > > > > > rename tests/ref/fate/{fitsdec-gbrap16le =3D> fitsdec-gbrap16be}= (79%) > > > > > rename tests/ref/fate/{fitsdec-gbrp16 =3D> fitsdec-gbrp16be} (79= %) > > > > >=20 > > > > > 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{inpu= t/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 pr= int a > > > > > -warning and select the best pixel format supported by the encode= r. > > > > > + > > > > > If @var{pix_fmt} is prefixed by a @code{+}, ffmpeg will exit wit= h an error > > > > > if the requested pixel format can not be selected, and automatic= conversions > > > > > inside filtergraphs are disabled. > > > >=20 > > > > The commit message makes this sound like a bugfix, while really thi= s is > > > > removing a documented feature. > > >=20 > > > It is a bugfix in my eyes. When you explicitly tell a program to perf= orm > > > a specific action, and the program just decides to do something else, > > > then that program is broken. > > >=20 > > > 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. > > >=20 > > > I see no argument whatsoever for why we should have such a "smart" > >=20 > > As said previously, > > The user cannot be expected to know if a implementation uses planar > > or packed rgb, bgr or rgb. >=20 > 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. >=20 > > 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 >=20 > 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 >=20 > 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. >=20 > > > > 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 g= brp) > > > > without needing to know what each individual codec uses to return R= ,G,B > > >=20 > >=20 > > > 1) This code does not give you the ability to specify "something clos= e 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? > >=20 > > (it is close because it can represent pal8 with little loss i think but) >=20 > 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. > >=20 > > 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 roun= ded > > to a close value > >=20 > > 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 > >=20 > > and iam pretty sure there are many more examples where "close" values > > are taken silently >=20 > 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 i= ncorrect 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 >=20 > 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 [...] --=20 Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB No snowflake in an avalanche ever feels responsible. -- Voltaire --qoa3+4LHNtB0VmQh Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iF0EABEIAB0WIQSf8hKLFH72cwut8TNhHseHBAsPqwUCZLL7JwAKCRBhHseHBAsP q/arAJoCxk4Mp91sZ4eFEPbptxGKlZ2tFgCeNOn3r9Y1UznuJicNJOirgk0TuvY= =r+Wa -----END PGP SIGNATURE----- --qoa3+4LHNtB0VmQh-- --===============2537316631283472858== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ 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". --===============2537316631283472858==--