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 D009D463D0 for ; Tue, 16 May 2023 17:41:18 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 609A568BF84; Tue, 16 May 2023 20:41:15 +0300 (EEST) Received: from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net [217.70.183.199]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 3A16368A084 for ; Tue, 16 May 2023 20:41:09 +0300 (EEST) Received: (Authenticated sender: michael@niedermayer.cc) by mail.gandi.net (Postfix) with ESMTPSA id 28100FF802 for ; Tue, 16 May 2023 17:41:07 +0000 (UTC) Date: Tue, 16 May 2023 19:41:04 +0200 From: Michael Niedermayer To: FFmpeg development discussions and patches Message-ID: <20230516174104.GT1391451@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> <168418349631.3843.9492232114728800405@lain.khirnov.net> MIME-Version: 1.0 In-Reply-To: <168418349631.3843.9492232114728800405@lain.khirnov.net> 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: multipart/mixed; boundary="===============3221021990527371056==" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: --===============3221021990527371056== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="5OY1VMnxNaoJKAjw" Content-Disposition: inline --5OY1VMnxNaoJKAjw Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, May 15, 2023 at 10:44:56PM +0200, Anton Khirnov wrote: > 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, whic= h would > > > > > cancel out. > > > > > Updating lavf and not the parsers would result in correct value i= n 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(-) > > > >=20 > > > > Doesnt this sort of change need a major ABI bump ? > > > > it sounds like lavc and lavf interdepend here both ways > > >=20 > > > No, we do not guarantee bug compatibility. > > >=20 > > > Libavformat seeing ticks_per_frame=3D1 for codecs that set it to 2 up= on > > > being opened is a bug. Same for the parser. > > >=20 > > > 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). > >=20 > > 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 > >=20 > > with=20 > > ./ffmpeg -i fate-suite//h264/lossless.h264 -f framecrc - > >=20 > > The output uses 1/60 thats odd because if every frame can be represente= d 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 > >=20 > > maybe iam missing something but the new value seems worse and also > > not consistent with what ffmpeg actually uses >=20 > 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. Iam not sure how it is consistent if the value used is different than the value displayed >=20 > 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. >=20 > 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. I think the important part is provide the user with what (s)he wants If more files work better, thats a win. The world of multimedia is a bit messy in some corners (as you know) so i am not sure if true beauty, cleanliness and consistency can be achieved while having well working/fast code But i certainly support making the code nicer. about "correctness", iam not even sure what exactly is "correct" in some cases. just the recent hls case, the first 4 links i tried used 2 mime types the rfc would consider wrong. id say adding them is "correct" with the "SHOULD" recommandition but others surely will disagree about tbr, i think its a usefull field, It wont be the global optimal value for some videos but neither would width and height be, if they change midstream. But any improvment is good and i support that, in this case here i saw one file change and i reported it. Thanks [...] --=20 Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I am the wisest man alive, for I know one thing, and that is that I know nothing. -- Socrates --5OY1VMnxNaoJKAjw Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iF0EABEIAB0WIQSf8hKLFH72cwut8TNhHseHBAsPqwUCZGPALAAKCRBhHseHBAsP q+shAJ9J+DgOm1qY50rFvxS8TpuLf6jA1QCePtx+oIWY+kDZQ6dYbIQ/nDrOk1g= =ARvX -----END PGP SIGNATURE----- --5OY1VMnxNaoJKAjw-- --===============3221021990527371056== 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". --===============3221021990527371056==--