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 E863F430D7 for ; Tue, 18 Oct 2022 20:05:50 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 1910E680134; Tue, 18 Oct 2022 23:05:47 +0300 (EEST) Received: from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net [217.70.183.193]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 0D8B068BAA0 for ; Tue, 18 Oct 2022 23:05:40 +0300 (EEST) Received: (Authenticated sender: michael@niedermayer.cc) by mail.gandi.net (Postfix) with ESMTPSA id EB642240004 for ; Tue, 18 Oct 2022 20:05:39 +0000 (UTC) Date: Tue, 18 Oct 2022 22:05:39 +0200 From: Michael Niedermayer To: FFmpeg development discussions and patches Message-ID: <20221018200539.GB907335@pb2> References: <20221017203853.GA907335@pb2> MIME-Version: 1.0 In-Reply-To: Subject: Re: [FFmpeg-devel] [PATCH] avcodec/svq1: fix interframe mean VLC symbols 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="===============6149861760464915570==" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: --===============6149861760464915570== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="BwCQnh7xodEAoBMC" Content-Disposition: inline --BwCQnh7xodEAoBMC Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Oct 18, 2022 at 12:44:20PM +1100, Peter Ross wrote: > On Mon, Oct 17, 2022 at 10:38:53PM +0200, Michael Niedermayer wrote: > > On Mon, Oct 17, 2022 at 08:33:28PM +1100, Peter Ross wrote: > > > On Mon, Oct 17, 2022 at 05:04:29AM +0200, Andreas Rheinhardt wrote: > > > > Peter Ross: > > > > > Fixes ticket #128. > > > > >=20 > > > > > The SVQ1 interframe mean VLC symbols -128 and 128 are incorrectly= swapped > > > > > in our SVQ1 implementation, resulting in visible artifacts for so= me videos. > > > > > This patch unswaps the order of these two symbols. > > > > >=20 > > > > > The most noticable example of the artiacts caused by this error c= an be observed in > > > > > https://trac.ffmpeg.org/attachment/ticket/128/svq1_set.7z '352_28= 8_k_50.mov'. > > > > > The artifacts are not observed when using the reference decoder > > > > > (QuickTime 7.7.9 x86 binary). > > > > >=20 > > > > > As a result of this patch, the reference data for the fate-svq1 t= est > > > > > ($SAMPLES/svq1/marymary-shackles.mov) must be modified. For this = file, our > > > > > decoder output is now bitwise identical to the reference decoder.= I have > > > > > tested patch with various other samples and they are all now bitw= ise identical. > > > >=20 > > > > Seems like this is not the only test whose reference needs to be > > > > updated. There are also the fate-vsynth%-svq1 tests. > > >=20 > > > Right, the encoder. This change will cause previous videos created by= the FFmpeg > > > encoder to playback *with* artifacts in newer builds of FFmpeg with t= his patch > > > applied. There is not much we can do about this, since there isn't pl= ace for a > > > version string in the SVQ3 bitstream (unlike MPEG-4 etc). > >=20 > > it is possible i think to avoid this > >=20 > > we just need anything that differs reasonably consistently between the = binary > > encoder and the ffmpeg encoder > > the first thing that comes to mind is the temporal reference values, we= seem > > to always store 0 but there may be other things we do that differ >=20 > for the temporal reference field, the official binary encoder emits an in= creasing > sequence of numbers for each frame, and they wrap at 8-bits. i don't thin= k we can > use a simple '=3D=3D 0 case is FFmpeg', as there's a good chance that a l= egitimate > interframe will occur whe the field is 0. With a single frame a value of 1 to 255 has a 0% chance to be ffmpeg and a 100% chance to be the official encoder (this is only true if we asume=20 no damage/corruption) a 0 value has a chance that matches how often you assume these 2 encoded files are encountered. with 2 frames a 0, 0 will very likely be FFmpeg while all others are 100% not FFmpeg. You can get a 0,0 by droping 255 frames that does not seem like a probably thing also if 255 frames are lost artifacts already exist likely and being wrong on the detection should not matter i belive the TR is worth a try for detection. If it fails iam curious of the case in which it does so. Maybe its not that easy and there are unexpected cases >=20 > there's not much else to detect whether its a FFmpeg file, just the check= sum and > unknown extradata bits, both which the buggy FFmpeg encoder set to zero. = Problem > is, some legitimate samples also have both these fields set to zero. >=20 > i am open to what other ideas you have! >=20 > > This fix will need some caution so everything keeps working=20 > > 1. we need the decoder to detect the official and our old buggy encoder= and=20 > > handle each appropriately > > 2. when we fix our encoder there are at least 3 ways > > 2a. avoid the ambigous codes, these will decode fine in all cases >=20 > i like this option. encoder performance is already abysmal, so it won't h= urt too much. >=20 > > 2b. whatever we used to detect the official encoder we can mimic that t= oo > > while fixing this. but that may be a one shot opertunity > > 2c. find a cleaner way to store our version first, implment that in the > > decoder and then use it in the encoder when we fix this. This would > > need some testing to make sure it works with the official decoder > > I see a few things that might work for the version. the decoder dec= odes > > some "embedded message", theres the temporal reference which seems = not > > mattering > > there are various encoder choices that allow embeding some message = if one > > really wanted > >=20 > > thx >=20 > going forward i propose we set the mov container extradata field to LIBAV= CODEC_IDENT. > "just" in case there is another error in the encoder that needs addressin= g in future. > my analysis of the binary decoder suggests it not inspect the container e= xtradata field. good idea! thx [...] --=20 Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Many things microsoft did are stupid, but not doing something just because microsoft did it is even more stupid. If everything ms did were stupid they would be bankrupt already. --BwCQnh7xodEAoBMC Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iF0EABEIAB0WIQSf8hKLFH72cwut8TNhHseHBAsPqwUCY08HCwAKCRBhHseHBAsP q69/AJ9pabgaZU+HqyiWynPRLmFPDegsEwCdHnzFNCXxth7AJIMDRNY4ZRk6NOs= =3CRF -----END PGP SIGNATURE----- --BwCQnh7xodEAoBMC-- --===============6149861760464915570== 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". --===============6149861760464915570==--