Re: [FFmpeg-devel] [PATCH v3 1/2] libavformat/oggdec.c: Changing the packet() callback API/Return value

[Michael Niedermayer <michael@niedermayer.cc>]

[-- Attachment #1.1: Type: text/plain, Size: 6354 bytes --]

On Sun, May 04, 2025 at 11:42:13AM -0500, Romain Beauxis wrote:
> Le dim. 4 mai 2025 à 09:04, Michael Niedermayer <michael@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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".