On Sun, May 04, 2025 at 11:42:13AM -0500, Romain Beauxis wrote:
> Le dim. 4 mai 2025 à 09:04, Michael Niedermayer <mich...@niedermayer.cc> a
> écrit :
> >
> > 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 = 0;
> > > os->pduration = 0;
> > > +
> > > + ret = 0;
> > > if (os->codec && os->codec->packet) {
> > > if ((ret = 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 = idx;
> > > - if (dstart)
> > > - *dstart = os->pstart;
> > > - if (dsize)
> > > - *dsize = os->psize;
> > > - if (fpos)
> > > - *fpos = os->sync_pos;
> > > +
> > > + if (!ret) {
> > > + if (sid)
> > > + *sid = idx;
> > > + if (dstart)
> > > + *dstart = os->pstart;
> > > + if (dsize)
> > > + *dsize = os->psize;
> > > + if (fpos)
> > > + *fpos = os->sync_pos;
> > > + }
> > > +
> > > os->pstart += os->psize;
> > > os->psize = 0;
> > > if(os->pstart == 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 ?
>
> I must have misinterpreted your previous message:
>
> > 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 ?
>
> Looks like you meant to do the reverse of what I sent, let me make it more
> clear:
>
> 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
>
> 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 the
> 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.
>
> 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 where
> 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
* ==0 if the packet was a regular data packet.
* ==0 or 1 if the packet was a header from a chained bitstream.
*/
Patch #2
ogg_packet() changes
and
* Attempt to process a packet as a data packet
* @return < 0 (AVERROR) code or -1 on error
* ==0 if the packet was a regular data packet.
* ==0 or 1 if the packet was a header from a chained bitstream.
+ * (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
...
Patch #6
* ==0 if the packet was a regular data packet.
- * ==0 or 1 if the packet was a header from a chained bitstream.
- * (1 will cause the packet to be skiped in calling code
(ogg_packet())
+ * ==1 if the packet was a header from a chained bitstream.
+ * This will cause the packet to be skiped in calling code
(ogg_packet()
*/
>
> 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
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
During times of universal deceit, telling the truth becomes a
revolutionary act. -- George Orwell
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
