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 C889045AC0 for ; Fri, 14 Jul 2023 10:20:37 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 3ABCB68C617; Fri, 14 Jul 2023 13:20:34 +0300 (EEST) Received: from btbn.de (btbn.de [136.243.74.85]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id DA90F68C531 for ; Fri, 14 Jul 2023 13:20:27 +0300 (EEST) Received: from [authenticated] by btbn.de (Postfix) with ESMTPSA id 4C56E3CA258 for ; Fri, 14 Jul 2023 12:20:25 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rothenpieler.org; s=mail; t=1689330025; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=CxYXWOfQnofRrwy975rqkRYuetxNWRFukhUdSjuFOHY=; b=ogIPxpyw/6f3IktwPym3XV9rAVq7VBbxwsxuuLbxztzkSHyNbeC/73qA4bcBkY3FMSpYpi Ukf05zhXRgSWCpjFFq5zi6qJkrUfQFJ9NBEZCY07OXsoWF13HelS8wHw1DRK1sqaI/Btne ij92ZSg+K/fhlBHlbCfck0mCi0DZIpABetFAGTfZEbL6uYKvdPpoqLQ5CfW7IwHCEOd1Mo N4aZcs4/YR2qYTau1h0PVty9IS1l5DXqtC8C3QXAUNOhyMhnOEUqWoc7OTbHK1f4MNruAX vNV2pEp5Nq9aVFWeim2BrwO+0bL8kEjY5Sut5lLkd61ouK3eny6r9sDVruQuxg== Message-ID: <734cdbc8-50c9-95e1-dd5a-86ebeba3c487@rothenpieler.org> Date: Fri, 14 Jul 2023 12:20:28 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 Content-Language: en-US, de-DE To: ffmpeg-devel@ffmpeg.org References: <20230713105553.21052-1-anton@khirnov.net> <20230713105553.21052-25-anton@khirnov.net> <20230713231107.GK1093384@pb2> <168932784788.27367.2505386604250052570@lain.khirnov.net> From: Timo Rothenpieler In-Reply-To: <168932784788.27367.2505386604250052570@lain.khirnov.net> 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-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: 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. _______________________________________________ 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".