[FFmpeg-devel] [PATCH v3 0/2] Remove chained ogg stream header packets from demuxer

[FFmpeg-devel] [PATCH v3 0/2] Remove chained ogg stream header packets from demuxer

[Romain Beauxis]

These patches remove the ogg header packets from secondary chained ogg
streams from the demuxer.

To do so, the packet demuxing function is modified to explicitely tell
the demuxer when skip header packets

Also, the opus packet demuxing function is adapted to properly copy extra
data from the new chained streams so that decoding can keep happening.

The diff from the test output makes it possible to follow what the
changes do to the extracted streams.

## Changes since last revision:
* The base tests are now in the codebase
* Split the last commit in two, one for the API change and one for the
  corresponding codec-specific use of it.

Romain Beauxis (2):
  libavformat/oggdec.c: Changing the packet() callback API/Return value
  ogg/{vorbis,flac,opus}: Remove header packets from subsequent ogg
    streams from the demuxer output.

 libavformat/oggdec.c                       | 26 ++++++++++----------
 libavformat/oggdec.h                       |  6 +++++
 libavformat/oggparseflac.c                 | 28 ++++++++++++++++++++--
 libavformat/oggparseopus.c                 | 11 +++++++++
 libavformat/oggparsevorbis.c               | 11 +++++++--
 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, 66 insertions(+), 22 deletions(-)

-- 
2.39.5 (Apple Git-154)

_______________________________________________
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] [PATCH v3 1/2] libavformat/oggdec.c: Changing the packet() callback API/Return value

[Romain Beauxis]

---
 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.
-- 
2.39.5 (Apple Git-154)

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

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

[Michael Niedermayer]

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

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 ?


--- a/libavformat/oggparseopus.c
+++ b/libavformat/oggparseopus.c
@@ -125,6 +125,17 @@ static int opus_packet(AVFormatContext *avf, int idx)
         return AVERROR_INVALIDDATA;
     }

+     if (os->psize > 8 && !memcmp(packet, "OpusHead", 8)) {
+        if ((ret = ff_alloc_extradata(st->codecpar, os->psize)) < 0)
+            return ret;
+
+        memcpy(st->codecpar->extradata, packet, os->psize);
+        return 1;
+    }
+
+    if (os->psize > 8 && !memcmp(packet, "OpusTags", 8))
+        return 1;
+
     if ((!os->lastpts || os->lastpts == AV_NOPTS_VALUE) && !(os->flags & OGG_FLAG_EOS)) {
         int seg, d;
         int duration;


const struct ogg_codec ff_opus_codec = {
    .name             = "Opus",
    .magic            = "OpusHead",
    .magicsize        = 8,
    .header           = opus_header,
    .packet           = opus_packet,
    .nb_header        = 1,



diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c
index 9f50ab9ffc..8b4ae872d2 100644
--- a/libavformat/oggparsevorbis.c
+++ b/libavformat/oggparsevorbis.c
@@ -418,6 +418,7 @@ static int vorbis_packet(AVFormatContext *s, int idx)
     struct ogg_stream *os = ogg->streams + idx;
     struct oggvorbis_private *priv = os->private;
     int duration, flags = 0;
+    int skip_packet = 0;

     if (!priv->vp)
         return AVERROR_INVALIDDATA;
@@ -480,7 +481,13 @@ static int vorbis_packet(AVFormatContext *s, int idx)
         if (duration < 0) {
             os->pflags |= AV_PKT_FLAG_CORRUPT;
             return 0;
-        } else if (flags & VORBIS_FLAG_COMMENT) {
+        }
+
+        if (flags &
+            (VORBIS_FLAG_HEADER | VORBIS_FLAG_COMMENT | VORBIS_FLAG_SETUP))
+            skip_packet = 1;
+
+        if (flags & VORBIS_FLAG_COMMENT) {
             vorbis_update_metadata(s, idx);
             flags = 0;
         }
@@ -505,7 +512,7 @@ static int vorbis_packet(AVFormatContext *s, int idx)
         priv->final_duration += os->pduration;
     }

-    return 0;
+    return skip_packet;
 }







> -- 
> 2.39.5 (Apple Git-154)
> 
> _______________________________________________
> 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".
> 

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have often repented speaking, but never of holding my tongue.
-- Xenocrates

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

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

[Romain Beauxis]

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.

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.

Thanks,
-- Romain

> --- a/libavformat/oggparseopus.c
> +++ b/libavformat/oggparseopus.c
> @@ -125,6 +125,17 @@ static int opus_packet(AVFormatContext *avf, int idx)
>          return AVERROR_INVALIDDATA;
>      }
>
> +     if (os->psize > 8 && !memcmp(packet, "OpusHead", 8)) {
> +        if ((ret = ff_alloc_extradata(st->codecpar, os->psize)) < 0)
> +            return ret;
> +
> +        memcpy(st->codecpar->extradata, packet, os->psize);
> +        return 1;
> +    }
> +
> +    if (os->psize > 8 && !memcmp(packet, "OpusTags", 8))
> +        return 1;
> +
>      if ((!os->lastpts || os->lastpts == AV_NOPTS_VALUE) && !(os->flags &
OGG_FLAG_EOS)) {
>          int seg, d;
>          int duration;
>
>
> const struct ogg_codec ff_opus_codec = {
>     .name             = "Opus",
>     .magic            = "OpusHead",
>     .magicsize        = 8,
>     .header           = opus_header,
>     .packet           = opus_packet,
>     .nb_header        = 1,
>
>
>
> diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c
> index 9f50ab9ffc..8b4ae872d2 100644
> --- a/libavformat/oggparsevorbis.c
> +++ b/libavformat/oggparsevorbis.c
> @@ -418,6 +418,7 @@ static int vorbis_packet(AVFormatContext *s, int idx)
>      struct ogg_stream *os = ogg->streams + idx;
>      struct oggvorbis_private *priv = os->private;
>      int duration, flags = 0;
> +    int skip_packet = 0;
>
>      if (!priv->vp)
>          return AVERROR_INVALIDDATA;
> @@ -480,7 +481,13 @@ static int vorbis_packet(AVFormatContext *s, int idx)
>          if (duration < 0) {
>              os->pflags |= AV_PKT_FLAG_CORRUPT;
>              return 0;
> -        } else if (flags & VORBIS_FLAG_COMMENT) {
> +        }
> +
> +        if (flags &
> +            (VORBIS_FLAG_HEADER | VORBIS_FLAG_COMMENT |
VORBIS_FLAG_SETUP))
> +            skip_packet = 1;
> +
> +        if (flags & VORBIS_FLAG_COMMENT) {
>              vorbis_update_metadata(s, idx);
>              flags = 0;
>          }
> @@ -505,7 +512,7 @@ static int vorbis_packet(AVFormatContext *s, int idx)
>          priv->final_duration += os->pduration;
>      }
>
> -    return 0;
> +    return skip_packet;
>  }
>
>
>
>
>
>
>
> > --
> > 2.39.5 (Apple Git-154)
> >
> > _______________________________________________
> > 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".
> >
>
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> I have often repented speaking, but never of holding my tongue.
> -- Xenocrates
> _______________________________________________
> 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".

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

[Michael Niedermayer]

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

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

[Romain Beauxis]

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

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

[Romain Beauxis]

---
 libavformat/oggdec.c                       |  4 ----
 libavformat/oggparseflac.c                 | 28 ++++++++++++++++++++--
 libavformat/oggparseopus.c                 | 11 +++++++++
 libavformat/oggparsevorbis.c               | 11 +++++++--
 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 ---
 7 files changed, 46 insertions(+), 14 deletions(-)

diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
index 9baf8040a9..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;
 }
 
diff --git a/libavformat/oggparseflac.c b/libavformat/oggparseflac.c
index f25ed9cc15..d66b85b09e 100644
--- a/libavformat/oggparseflac.c
+++ b/libavformat/oggparseflac.c
@@ -27,6 +27,8 @@
 #include "oggdec.h"
 
 #define OGG_FLAC_METADATA_TYPE_STREAMINFO 0x7F
+#define OGG_FLAC_MAGIC "\177FLAC"
+#define OGG_FLAC_MAGIC_SIZE sizeof(OGG_FLAC_MAGIC)-1
 
 static int
 flac_header (AVFormatContext * s, int idx)
@@ -78,6 +80,27 @@ flac_header (AVFormatContext * s, int idx)
     return 1;
 }
 
+static int
+flac_packet (AVFormatContext * s, int idx)
+{
+    struct ogg *ogg = s->priv_data;
+    struct ogg_stream *os = ogg->streams + idx;
+
+    if (os->psize > OGG_FLAC_MAGIC_SIZE &&
+        !memcmp(
+            os->buf + os->pstart,
+            OGG_FLAC_MAGIC,
+            OGG_FLAC_MAGIC_SIZE))
+        return 1;
+
+    if (os->psize > 0 &&
+        ((os->buf[os->pstart] & 0x7F) == FLAC_METADATA_TYPE_VORBIS_COMMENT)) {
+        return 1;
+    }
+
+    return 0;
+}
+
 static int
 old_flac_header (AVFormatContext * s, int idx)
 {
@@ -127,10 +150,11 @@ fail:
 }
 
 const struct ogg_codec ff_flac_codec = {
-    .magic = "\177FLAC",
-    .magicsize = 5,
+    .magic = OGG_FLAC_MAGIC,
+    .magicsize = OGG_FLAC_MAGIC_SIZE,
     .header = flac_header,
     .nb_header = 2,
+    .packet = flac_packet,
 };
 
 const struct ogg_codec ff_old_flac_codec = {
diff --git a/libavformat/oggparseopus.c b/libavformat/oggparseopus.c
index 218e9df581..f54a9711df 100644
--- a/libavformat/oggparseopus.c
+++ b/libavformat/oggparseopus.c
@@ -125,6 +125,17 @@ static int opus_packet(AVFormatContext *avf, int idx)
         return AVERROR_INVALIDDATA;
     }
 
+     if (os->psize > 8 && !memcmp(packet, "OpusHead", 8)) {
+        if ((ret = ff_alloc_extradata(st->codecpar, os->psize)) < 0)
+            return ret;
+
+        memcpy(st->codecpar->extradata, packet, os->psize);
+        return 1;
+    }
+
+    if (os->psize > 8 && !memcmp(packet, "OpusTags", 8))
+        return 1;
+
     if ((!os->lastpts || os->lastpts == AV_NOPTS_VALUE) && !(os->flags & OGG_FLAG_EOS)) {
         int seg, d;
         int duration;
diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c
index 9f50ab9ffc..8b4ae872d2 100644
--- a/libavformat/oggparsevorbis.c
+++ b/libavformat/oggparsevorbis.c
@@ -418,6 +418,7 @@ static int vorbis_packet(AVFormatContext *s, int idx)
     struct ogg_stream *os = ogg->streams + idx;
     struct oggvorbis_private *priv = os->private;
     int duration, flags = 0;
+    int skip_packet = 0;
 
     if (!priv->vp)
         return AVERROR_INVALIDDATA;
@@ -480,7 +481,13 @@ static int vorbis_packet(AVFormatContext *s, int idx)
         if (duration < 0) {
             os->pflags |= AV_PKT_FLAG_CORRUPT;
             return 0;
-        } else if (flags & VORBIS_FLAG_COMMENT) {
+        }
+
+        if (flags &
+            (VORBIS_FLAG_HEADER | VORBIS_FLAG_COMMENT | VORBIS_FLAG_SETUP))
+            skip_packet = 1;
+
+        if (flags & VORBIS_FLAG_COMMENT) {
             vorbis_update_metadata(s, idx);
             flags = 0;
         }
@@ -505,7 +512,7 @@ static int vorbis_packet(AVFormatContext *s, int idx)
         priv->final_duration += os->pduration;
     }
 
-    return 0;
+    return skip_packet;
 }
 
 const struct ogg_codec ff_vorbis_codec = {
diff --git a/tests/ref/fate/ogg-flac-chained-meta.txt b/tests/ref/fate/ogg-flac-chained-meta.txt
index ad20ba935f..28e22aa29e 100644
--- a/tests/ref/fate/ogg-flac-chained-meta.txt
+++ b/tests/ref/fate/ogg-flac-chained-meta.txt
@@ -5,8 +5,6 @@ Stream ID: 0, frame PTS: 0, metadata: N/A
 Stream ID: 0, packet PTS: 4608, packet DTS: 4608
 Stream ID: 0, frame PTS: 4608, metadata: N/A
 Stream ID: 0, packet PTS: 0, packet DTS: 0
-Stream ID: 0, packet PTS: 0, packet DTS: 0
-Stream ID: 0, packet PTS: 0, packet DTS: 0
 Stream ID: 0, frame PTS: 0, metadata: N/A
 Stream ID: 0, packet PTS: 4608, packet DTS: 4608
 Stream ID: 0, frame PTS: 4608, metadata: N/A
diff --git a/tests/ref/fate/ogg-opus-chained-meta.txt b/tests/ref/fate/ogg-opus-chained-meta.txt
index fc84b8b703..addc41c1eb 100644
--- a/tests/ref/fate/ogg-opus-chained-meta.txt
+++ b/tests/ref/fate/ogg-opus-chained-meta.txt
@@ -13,7 +13,6 @@ Stream ID: 0, frame PTS: 3528, metadata: N/A
 Stream ID: 0, packet PTS: 4488, packet DTS: 4488
 Stream ID: 0, frame PTS: 4488, metadata: N/A
 Stream ID: 0, packet PTS: -312, packet DTS: -312
-Stream ID: 0, new metadata: encoder=Lavc61.19.100 libopus;Lavc61.19.100 libopus:title=First Stream;Second Stream
 Stream ID: 0, frame PTS: -312, metadata: N/A
 Stream ID: 0, packet PTS: 648, packet DTS: 648
 Stream ID: 0, frame PTS: 648, metadata: N/A
diff --git a/tests/ref/fate/ogg-vorbis-chained-meta.txt b/tests/ref/fate/ogg-vorbis-chained-meta.txt
index b7a97c90e2..1206f86c1f 100644
--- a/tests/ref/fate/ogg-vorbis-chained-meta.txt
+++ b/tests/ref/fate/ogg-vorbis-chained-meta.txt
@@ -6,10 +6,7 @@ Stream ID: 0, frame PTS: 128, metadata: N/A
 Stream ID: 0, packet PTS: 704, packet DTS: 704
 Stream ID: 0, frame PTS: 704, metadata: N/A
 Stream ID: 0, packet PTS: 0, packet DTS: 0
-Stream ID: 0, packet PTS: 0, packet DTS: 0
 Stream ID: 0, new metadata: encoder=Lavc61.19.100 libvorbis:title=Second Stream
-Stream ID: 0, packet PTS: 0, packet DTS: 0
-Stream ID: 0, packet PTS: 0, packet DTS: 0
 Stream ID: 0, frame PTS: 0, metadata: N/A
 Stream ID: 0, packet PTS: 128, packet DTS: 128
 Stream ID: 0, frame PTS: 128, metadata: N/A
-- 
2.39.5 (Apple Git-154)

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