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 ESMTPS id 923644BDDD for ; Sun, 4 May 2025 22:23:36 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 2F6B768B405; Mon, 5 May 2025 01:23:33 +0300 (EEST) Received: from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net [217.70.183.197]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 7F6DB687B45 for ; Mon, 5 May 2025 01:23:26 +0300 (EEST) Received: by mail.gandi.net (Postfix) with ESMTPSA id 98F3C42EF2 for ; Sun, 4 May 2025 22:23:25 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=niedermayer.cc; s=gm1; t=1746397405; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=j/aVoaRZes1U6rxQrUV1odMmPAYHK5TFyu1zSV/hDB0=; b=XCZfdAr/+fikejo9OzoKZXzj5idozJNI3OXhZZycWVgOC6FKYjnyxGH5ZZGBPVpQTj9hHr 130R+CWCDvi2IRf57gGyag5uKi6x1xWtftjMwi/JSR+6KwzSA3bBKYOtY5S3cA7Uy/xnFa UtKzFWMDegZTyKnZ5EUX+EZ0CoGT0RLmnkE5/guuIfrYj9+cOhDmUhqCnYv6XpMaQMvMau kR4qtP+Cb26lgT7OgxlRO8jGYs5uJtvBSXeSpvKGD3/p4UCMl5nK5E8zHlkoP9Ih9pGlQC hBhim2a/eNqWS0xgfQx7tEGU1tYQ3xOAPfl5Qw7VOyM5eZoWU3frtjN0q1HLuw== Date: Mon, 5 May 2025 00:23:24 +0200 From: Michael Niedermayer To: FFmpeg development discussions and patches Message-ID: <20250504222324.GC29660@pb2> References: <20250503170329.46135-1-romain.beauxis@gmail.com> <20250503170329.46135-2-romain.beauxis@gmail.com> <20250504140433.GB29660@pb2> MIME-Version: 1.0 In-Reply-To: X-GND-State: clean X-GND-Score: -70 X-GND-Cause: gggruggvucftvghtrhhoucdtuddrgeefvddrtddtgddvjeelgedtucetufdoteggodetrfdotffvucfrrhhofhhilhgvmecuifetpfffkfdpucggtfgfnhhsuhgsshgtrhhisggvnecuuegrihhlohhuthemuceftddunecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenfghrlhcuvffnffculdeftddmnecujfgurhepfffhvffukfhfgggtuggjsehgtderredttddunecuhfhrohhmpefoihgthhgrvghlucfpihgvuggvrhhmrgihvghruceomhhitghhrggvlhesnhhivgguvghrmhgrhigvrhdrtggtqeenucggtffrrghtthgvrhhnpedutedvhfduuedugedufefghefhvedvgffgffekhfdvgfdvtefftdejkeehteefheenucfkphepgedurdeiiedrieejrdduudefnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehinhgvthepgedurdeiiedrieejrdduudefpdhhvghloheplhhotggrlhhhohhsthdpmhgrihhlfhhrohhmpehmihgthhgrvghlsehnihgvuggvrhhmrgihvghrrdgttgdpnhgspghrtghpthhtohepuddprhgtphhtthhopehffhhmphgvghdquggvvhgvlhesfhhfmhhpvghgrdhorhhg X-GND-Sasl: michael@niedermayer.cc Subject: Re: [FFmpeg-devel] [PATCH v3 1/2] libavformat/oggdec.c: Changing the packet() callback API/Return value 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="===============1862463503933748958==" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: --===============1862463503933748958== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="S1BNGpv0yoYahz37" Content-Disposition: inline --S1BNGpv0yoYahz37 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sun, May 04, 2025 at 11:42:13AM -0500, Romain Beauxis wrote: > Le dim. 4 mai 2025 =E0 09:04, Michael Niedermayer a > =E9crit : > > > > On Sat, May 03, 2025 at 12:03:28PM -0500, Romain Beauxis wrote: > > > --- > > > libavformat/oggdec.c | 22 ++++++++++++++-------- > > > libavformat/oggdec.h | 6 ++++++ > > > 2 files changed, 20 insertions(+), 8 deletions(-) > > > > > > diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c > > > index 5339fdd32c..9baf8040a9 100644 > > > --- a/libavformat/oggdec.c > > > +++ b/libavformat/oggdec.c > > > @@ -605,20 +605,26 @@ static int ogg_packet(AVFormatContext *s, int > *sid, int *dstart, int *dsize, > > > } else { > > > os->pflags =3D 0; > > > os->pduration =3D 0; > > > + > > > + ret =3D 0; > > > if (os->codec && os->codec->packet) { > > > if ((ret =3D os->codec->packet(s, idx)) < 0) { > > > av_log(s, AV_LOG_ERROR, "Packet processing failed: > %s\n", av_err2str(ret)); > > > return ret; > > > } > > > } > > > - if (sid) > > > - *sid =3D idx; > > > - if (dstart) > > > - *dstart =3D os->pstart; > > > - if (dsize) > > > - *dsize =3D os->psize; > > > - if (fpos) > > > - *fpos =3D os->sync_pos; > > > + > > > + if (!ret) { > > > + if (sid) > > > + *sid =3D idx; > > > + if (dstart) > > > + *dstart =3D os->pstart; > > > + if (dsize) > > > + *dsize =3D os->psize; > > > + if (fpos) > > > + *fpos =3D os->sync_pos; > > > + } > > > + > > > os->pstart +=3D os->psize; > > > os->psize =3D 0; > > > if(os->pstart =3D=3D os->bufpos) > > > diff --git a/libavformat/oggdec.h b/libavformat/oggdec.h > > > index 43df23f4cb..09f698f99a 100644 > > > --- a/libavformat/oggdec.h > > > +++ b/libavformat/oggdec.h > > > @@ -38,6 +38,12 @@ struct ogg_codec { > > > * -1 if an error occurred or for unsupported stream > > > */ > > > int (*header)(AVFormatContext *, int); > > > + /** > > > + * Attempt to process a packet as a data packet > > > + * @return 1 if the packet was a header from a chained bitstream. > > > + * 0 if the packet was a regular data packet. > > > + * -1 if an error occurred or for unsupported stream > > > + */ > > > int (*packet)(AVFormatContext *, int); > > > /** > > > * Translate a granule into a timestamp. > > > > Iam still confused by this > > > > If this changes the API for ogg_codec.packet() > > and in the same patch theres a change to ogg_packet() which uses > > ogg_codec.packet() > > > > but then there is a 2nd patch that actually changes the implementations > > of ogg_codec.packet() : > > > > so after the first patch, the API documentation is not correct, > > I thought that documentation and implementation change would happen > > in the same patch and the use of the more refined API would then be in > > a 2nd patch > > am i missing something here ? >=20 > I must have misinterpreted your previous message: >=20 > > Do you think this would merit a seperate patch ? > > I mean patch #1 changing the packet() return value and clearly stating > > that in the commit message > > and patch #2 using the new value ? >=20 > Looks like you meant to do the reverse of what I sent, let me make it more > clear: >=20 > Order #1: > 1. Change the ogg_codec.packet() implementations to return 1 when skipping > should occur > 2. Consume that value in ogg_packet and implement the actual skip in the > ogg bitstream >=20 > Order #2: > 1. Introduce a new API functionality in ogg_packet that is opt-in, i.e. > returning 1 in the ogg packet function to optionally skip a packet from t= he > ogg bitstream. API is available but not used yet. > 2. Implement the functionality in all the different ogg_codec.packet() > codec-specific files to skip header packets. >=20 > Order #2 is what I sent. To me it makes more sense. One of the reasons for > this order is that, also, the test diff would be colocated with the change > in the atual codec-specific code. If you do order #1, the test diff would > occur in the generic changes to ogg_packet, making it harder to track whe= re > the code that actually implements the test output changes comes from. If you prefer order #2 then i think, it could be done like this: (what bothers me is that API doc missmatching the actual implementation) Patch #1 docs only /** * Attempt to process a packet as a data packet * @return < 0 (AVERROR) code or -1 on error * =3D=3D0 if the packet was a regular data packet. * =3D=3D0 or 1 if the packet was a header from a chained bits= tream. */ Patch #2 ogg_packet() changes and * Attempt to process a packet as a data packet * @return < 0 (AVERROR) code or -1 on error * =3D=3D0 if the packet was a regular data packet. * =3D=3D0 or 1 if the packet was a header from a chained bits= tream. + * (1 will cause the packet to be skiped in calling code (ogg= _packet()) */ Patch #3 oggparseopus.c changes and corresponding fate change Patch #4 oggparsevorbis.c changes and corresponding fate change =2E.. Patch #6 * =3D=3D0 if the packet was a regular data packet. - * =3D=3D0 or 1 if the packet was a header from a chained bits= tream. - * (1 will cause the packet to be skiped in calling code (ogg= _packet()) + * =3D=3D1 if the packet was a header from a chained bitstream. + * This will cause the packet to be skiped in calling code = (ogg_packet() */ >=20 > At the end of the day, I'm happy to commit these changes in any shape or > form so we can move forward with the rest of this work. I like to ensure the changes are understandable on a first glance, and the = whole logic in ogg* is a bit confusing with all the *packet stuff thx [...] --=20 Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB During times of universal deceit, telling the truth becomes a revolutionary act. -- George Orwell --S1BNGpv0yoYahz37 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iF0EABEKAB0WIQSf8hKLFH72cwut8TNhHseHBAsPqwUCaBfo1QAKCRBhHseHBAsP qx/hAJ9zbVy3oi0gp1eK623Dw9uEHQAoegCaA8KUWZZSedPR12G1mWH3eNioyvA= =cKui -----END PGP SIGNATURE----- --S1BNGpv0yoYahz37-- --===============1862463503933748958== 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". --===============1862463503933748958==--