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

[Romain Beauxis <romain.beauxis@gmail.com>]

Le dim. 4 mai 2025 à 17:23, Michael Niedermayer
<michael@niedermayer.cc> a écrit :
>
> 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

Sounds good. Just sent the updated patchset. Hopefully that one will be
acceptable.


Le dim. 4 mai 2025 à 17:23, Michael Niedermayer <michael@niedermayer.cc> a
écrit :

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