Re: [FFmpeg-devel] [PATCH v2 2/2] ogg/{vorbis, flac, opus}: Remove header packets from subsequent ogg streams from the demuxer output.

[Michael Niedermayer <michael@niedermayer.cc>]

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

On Tue, Apr 29, 2025 at 07:49:54PM -0500, Romain Beauxis wrote:
> Le mar. 29 avr. 2025 à 19:17, Michael Niedermayer <michael@niedermayer.cc>
> a écrit :
> >
> > Hi Romain
> >
> > On Tue, Apr 29, 2025 at 05:42:22PM -0500, Romain Beauxis wrote:
> > > Le mar. 29 avr. 2025 à 16:35, Michael Niedermayer
> > > <michael@niedermayer.cc> a écrit :
> > > >
> > > > Hi
> > > >
> > > > On Mon, Apr 28, 2025 at 06:31:36PM -0500, Romain Beauxis wrote:
> > > > > ---
> > > > >  libavformat/oggdec.c                       | 26
> ++++++++++----------
> > > > >  libavformat/oggdec.h                       |  6 +++++
> > > > >  libavformat/oggparseflac.c                 | 28
> ++++++++++++++++++++--
> > > > >  libavformat/oggparseopus.c                 | 12 ++++++++++
> > > > >  libavformat/oggparsevorbis.c               | 12 ++++++++--
> > > > >  tests/ref/fate/ogg-flac-chained-meta.txt   |  2 --
> > > > >  tests/ref/fate/ogg-opus-chained-meta.txt   |  1 -
> > > > >  tests/ref/fate/ogg-vorbis-chained-meta.txt |  3 ---
> > > > >  8 files changed, 68 insertions(+), 22 deletions(-)
> > > > >
> > > > > diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
> > > > > index 5339fdd32c..5557eb4a14 100644
> > > > > --- a/libavformat/oggdec.c
> > > > > +++ b/libavformat/oggdec.c
> > > > > @@ -239,10 +239,6 @@ static int ogg_replace_stream(AVFormatContext
> *s, uint32_t serial, char *magic,
> > > > >      os->start_trimming = 0;
> > > > >      os->end_trimming = 0;
> > > > >
> > > > > -    /* Chained files have extradata as a new packet */
> > > > > -    if (codec == &ff_opus_codec)
> > > > > -        os->header = -1;
> > > > > -
> > > > >      return i;
> > > > >  }
> > > > >
> > > > > @@ -605,20 +601,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.
> > > >
> > > > ok, but seems unrelated
> > >
> > > This is a new convention to allow the parser to know when to skip a
> > > packet. Previous to that, return value of 1 did not have specific
> > > meaning.
> >
> > 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 ?
> >
> > I think it would make things clearer
> 
> Sure thing!
> 
> libavformat/oggdec.c: Skip packets when packet function returns 1:
> 
> https://github.com/toots/FFmpeg/commit/b78acea3b2840320bb68e065b2e712d753cb8d26


> commit 16f4a594af15aee1b68e87f17d00018592528ce6
> Author: Romain Beauxis <romain.beauxis@gmail.com>
> Date:   Mon Apr 28 18:08:58 2025 -0500
>
>     libavformat/oggdec.c: Skip packets when packet function returns 1.
>
...
> diff --git a/libavformat/oggdec.h b/libavformat/oggdec.h
> index 43df23f4cb9..09f698f99ab 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.

The commit message and the change dont seem to fit together

I would have expected the commit message to either say


"Changing the packet() callback API/Return value"
or
"Documenting the packet() callback API/Return value"

thx

[...]
--
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The smallest minority on earth is the individual. Those who deny
individual rights cannot claim to be defenders of minorities. - Ayn Rand



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