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 A88ED4638F for ; Mon, 15 May 2023 20:45:06 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id E74F868BF7E; Mon, 15 May 2023 23:45:03 +0300 (EEST) Received: from mail0.khirnov.net (red.khirnov.net [176.97.15.12]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id CDAF968BEAE for ; Mon, 15 May 2023 23:44:57 +0300 (EEST) Received: from localhost (localhost [IPv6:::1]) by mail0.khirnov.net (Postfix) with ESMTP id 4464B2404EC for ; Mon, 15 May 2023 22:44:57 +0200 (CEST) Received: from mail0.khirnov.net ([IPv6:::1]) by localhost (mail0.khirnov.net [IPv6:::1]) (amavisd-new, port 10024) with ESMTP id XcPP_2Sl065F for ; Mon, 15 May 2023 22:44:56 +0200 (CEST) Received: from lain.khirnov.net (lain.khirnov.net [IPv6:2001:67c:1138:4306::3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "lain.khirnov.net", Issuer "smtp.khirnov.net SMTP CA" (verified OK)) by mail0.khirnov.net (Postfix) with ESMTPS id 6ABCC240177 for ; Mon, 15 May 2023 22:44:56 +0200 (CEST) Received: by lain.khirnov.net (Postfix, from userid 1000) id 54EF01601B2; Mon, 15 May 2023 22:44:56 +0200 (CEST) From: Anton Khirnov To: FFmpeg development discussions and patches In-Reply-To: <20230515185942.GO1391451@pb2> References: <20230507133255.20881-1-anton@khirnov.net> <20230507133255.20881-4-anton@khirnov.net> <20230508141542.GS1391451@pb2> <168362189020.3843.14417317264187506189@lain.khirnov.net> <20230515185942.GO1391451@pb2> Mail-Followup-To: FFmpeg development discussions and patches Date: Mon, 15 May 2023 22:44:56 +0200 Message-ID: <168418349631.3843.9492232114728800405@lain.khirnov.net> User-Agent: alot/0.8.1 MIME-Version: 1.0 Subject: Re: [FFmpeg-devel] [PATCH 04/13] lavf: use AV_CODEC_PROP_FIELDS where appropriate 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: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: Quoting Michael Niedermayer (2023-05-15 20:59:42) > On Tue, May 09, 2023 at 10:44:50AM +0200, Anton Khirnov wrote: > > Quoting Michael Niedermayer (2023-05-08 16:15:42) > > > On Sun, May 07, 2023 at 03:32:46PM +0200, Anton Khirnov wrote: > > > > H.264 and mpeg12 parsers need to be adjusted at the same time to stop > > > > using the value of AVCodecContext.ticks_per_frame, because it is not set > > > > correctly unless the codec has been opened. Previously this would result > > > > in both the parser and lavf seeing the same incorrect value, which would > > > > cancel out. > > > > Updating lavf and not the parsers would result in correct value in lavf, > > > > but the wrong one in parsers, which would break some tests. > > > > --- > > > > libavcodec/h264_parser.c | 4 ++-- > > > > libavcodec/mpegvideo_parser.c | 2 +- > > > > libavformat/avformat.c | 9 ++++++--- > > > > libavformat/demux.c | 29 +++++++++++++++++++---------- > > > > libavformat/internal.h | 3 +++ > > > > 5 files changed, 31 insertions(+), 16 deletions(-) > > > > > > Doesnt this sort of change need a major ABI bump ? > > > it sounds like lavc and lavf interdepend here both ways > > > > No, we do not guarantee bug compatibility. > > > > Libavformat seeing ticks_per_frame=1 for codecs that set it to 2 upon > > being opened is a bug. Same for the parser. > > > > It just so happens that libavformat AND its internal parser instance see > > the same incorrect value and this cancels out in cases that are tested > > by FATE (it would break if we had more thorough tests for repeating > > single fields). > > This patch seems to change tbr > ./ffmpeg -i fate-suite//h264/lossless.h264 > Stream #0:0: Video: h264 (High 4:4:4 Predictive), yuv420p(progressive), 640x480, 25 fps, 60 tbr, 1200k tbn > vs. > Stream #0:0: Video: h264 (High 4:4:4 Predictive), yuv420p(progressive), 640x480, 25 fps, 120 tbr, 1200k tbn > > with > ./ffmpeg -i fate-suite//h264/lossless.h264 -f framecrc - > > The output uses 1/60 thats odd because if every frame can be represented in > 1/60 then tbr is 1/60 or more course > OTOH if tbr is finer than 1/60 then not every frame can be represented in 1/60 > > maybe iam missing something but the new value seems worse and also > not consistent with what ffmpeg actually uses ticks_per_frame was added by you in 3797c74ba53, and according to your code it's supposed to be 2 for H.264. It just so happens that for this specific sample libavformat invokes the parser without opening the decoder, so it sees the default value of 1. If it did open the decoder, it would see 2. This patch at least makes it consistent, even if it might not always be the optimal choice. As far as I'm concerned, the entire notion of 'tbr' is fundamentally flawed and should be abandoned. There is no magical way for the code to know what timebase is truly the right one here, without reading the whole file. Furthermore, the entire approach of "some sample X is now getting slightly worse arbitrary numbers than before" seems highly questionable to me. Our timestamps code is a unholy mess of hacks upon hacks upon hacks. For pretty much ANY change one can find or construct a sample that decodes worse after it. We should stop focusing on individual samples and prioritize overall consistency and correctness. -- 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".