* [FFmpeg-devel] [PATCH v5 1/7] libavformat/oggdec.h: Document packet function return value.
2025-05-09 23:43 [FFmpeg-devel] [PATCH v5 0/7] Remove chained ogg stream header packets from the demuxer Romain Beauxis
@ 2025-05-09 23:43 ` Romain Beauxis
2025-05-14 22:32 ` Michael Niedermayer
2025-05-09 23:43 ` [FFmpeg-devel] [PATCH v5 2/7] libavformat/oggdec.{c, h}: Implement packet skip on packet return value of 1 Romain Beauxis
` (5 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Romain Beauxis @ 2025-05-09 23:43 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: Romain Beauxis
---
libavformat/oggdec.h | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/libavformat/oggdec.h b/libavformat/oggdec.h
index 43df23f4cb..5225b77a07 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 < 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.
+ */
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".
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [FFmpeg-devel] [PATCH v5 1/7] libavformat/oggdec.h: Document packet function return value.
2025-05-09 23:43 ` [FFmpeg-devel] [PATCH v5 1/7] libavformat/oggdec.h: Document packet function return value Romain Beauxis
@ 2025-05-14 22:32 ` Michael Niedermayer
2025-05-16 17:39 ` Romain Beauxis
0 siblings, 1 reply; 16+ messages in thread
From: Michael Niedermayer @ 2025-05-14 22:32 UTC (permalink / raw)
To: FFmpeg development discussions and patches
[-- Attachment #1.1: Type: text/plain, Size: 1139 bytes --]
On Fri, May 09, 2025 at 06:43:21PM -0500, Romain Beauxis wrote:
> ---
> libavformat/oggdec.h | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/libavformat/oggdec.h b/libavformat/oggdec.h
> index 43df23f4cb..5225b77a07 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 < 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.
> + */
> int (*packet)(AVFormatContext *, int);
> /**
> * Translate a granule into a timestamp.
> --
will apply, so the patchset becomes smaller
thx
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Good people do not need laws to tell them to act responsibly, while bad
people will find a way around the laws. -- Plato
[-- 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".
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [FFmpeg-devel] [PATCH v5 1/7] libavformat/oggdec.h: Document packet function return value.
2025-05-14 22:32 ` Michael Niedermayer
@ 2025-05-16 17:39 ` Romain Beauxis
2025-05-17 22:08 ` Michael Niedermayer
0 siblings, 1 reply; 16+ messages in thread
From: Romain Beauxis @ 2025-05-16 17:39 UTC (permalink / raw)
To: FFmpeg development discussions and patches
Le mer. 14 mai 2025 à 17:32, Michael Niedermayer <michael@niedermayer.cc> a
écrit :
>
> On Fri, May 09, 2025 at 06:43:21PM -0500, Romain Beauxis wrote:
> > ---
> > libavformat/oggdec.h | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/libavformat/oggdec.h b/libavformat/oggdec.h
> > index 43df23f4cb..5225b77a07 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 < 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.
> > + */
> > int (*packet)(AVFormatContext *, int);
> > /**
> > * Translate a granule into a timestamp.
> > --
>
> will apply, so the patchset becomes smaller
Thanks!
I'll address the comments on the vorbis diff.
Opus and flac could go in too if you want.
-- Romain
_______________________________________________
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".
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [FFmpeg-devel] [PATCH v5 1/7] libavformat/oggdec.h: Document packet function return value.
2025-05-16 17:39 ` Romain Beauxis
@ 2025-05-17 22:08 ` Michael Niedermayer
0 siblings, 0 replies; 16+ messages in thread
From: Michael Niedermayer @ 2025-05-17 22:08 UTC (permalink / raw)
To: FFmpeg development discussions and patches
[-- Attachment #1.1: Type: text/plain, Size: 1547 bytes --]
On Fri, May 16, 2025 at 12:39:36PM -0500, Romain Beauxis wrote:
> Le mer. 14 mai 2025 à 17:32, Michael Niedermayer <michael@niedermayer.cc> a
> écrit :
> >
> > On Fri, May 09, 2025 at 06:43:21PM -0500, Romain Beauxis wrote:
> > > ---
> > > libavformat/oggdec.h | 6 ++++++
> > > 1 file changed, 6 insertions(+)
> > >
> > > diff --git a/libavformat/oggdec.h b/libavformat/oggdec.h
> > > index 43df23f4cb..5225b77a07 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 < 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.
> > > + */
> > > int (*packet)(AVFormatContext *, int);
> > > /**
> > > * Translate a granule into a timestamp.
> > > --
> >
> > will apply, so the patchset becomes smaller
>
> Thanks!
>
> I'll address the comments on the vorbis diff.
>
> Opus and flac could go in too if you want.
no objection, if someone wants to apply them
thx
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire
[-- 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".
^ permalink raw reply [flat|nested] 16+ messages in thread
* [FFmpeg-devel] [PATCH v5 2/7] libavformat/oggdec.{c, h}: Implement packet skip on packet return value of 1
2025-05-09 23:43 [FFmpeg-devel] [PATCH v5 0/7] Remove chained ogg stream header packets from the demuxer Romain Beauxis
2025-05-09 23:43 ` [FFmpeg-devel] [PATCH v5 1/7] libavformat/oggdec.h: Document packet function return value Romain Beauxis
@ 2025-05-09 23:43 ` Romain Beauxis
2025-05-14 22:34 ` Michael Niedermayer
2025-05-09 23:43 ` [FFmpeg-devel] [PATCH v5 3/7] ogg/opus: implement header packet skip in chained ogg bitstreams Romain Beauxis
` (4 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Romain Beauxis @ 2025-05-09 23:43 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: Romain Beauxis
---
libavformat/oggdec.c | 22 ++++++++++++++--------
libavformat/oggdec.h | 1 +
2 files changed, 15 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 5225b77a07..bc670d0f1e 100644
--- a/libavformat/oggdec.h
+++ b/libavformat/oggdec.h
@@ -43,6 +43,7 @@ struct ogg_codec {
* @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())
*/
int (*packet)(AVFormatContext *, int);
/**
--
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".
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [FFmpeg-devel] [PATCH v5 2/7] libavformat/oggdec.{c, h}: Implement packet skip on packet return value of 1
2025-05-09 23:43 ` [FFmpeg-devel] [PATCH v5 2/7] libavformat/oggdec.{c, h}: Implement packet skip on packet return value of 1 Romain Beauxis
@ 2025-05-14 22:34 ` Michael Niedermayer
0 siblings, 0 replies; 16+ messages in thread
From: Michael Niedermayer @ 2025-05-14 22:34 UTC (permalink / raw)
To: FFmpeg development discussions and patches
[-- Attachment #1.1: Type: text/plain, Size: 415 bytes --]
On Fri, May 09, 2025 at 06:43:22PM -0500, Romain Beauxis wrote:
> ---
> libavformat/oggdec.c | 22 ++++++++++++++--------
> libavformat/oggdec.h | 1 +
> 2 files changed, 15 insertions(+), 8 deletions(-)
will apply
thx
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Republics decline into democracies and democracies degenerate into
despotisms. -- Aristotle
[-- 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".
^ permalink raw reply [flat|nested] 16+ messages in thread
* [FFmpeg-devel] [PATCH v5 3/7] ogg/opus: implement header packet skip in chained ogg bitstreams.
2025-05-09 23:43 [FFmpeg-devel] [PATCH v5 0/7] Remove chained ogg stream header packets from the demuxer Romain Beauxis
2025-05-09 23:43 ` [FFmpeg-devel] [PATCH v5 1/7] libavformat/oggdec.h: Document packet function return value Romain Beauxis
2025-05-09 23:43 ` [FFmpeg-devel] [PATCH v5 2/7] libavformat/oggdec.{c, h}: Implement packet skip on packet return value of 1 Romain Beauxis
@ 2025-05-09 23:43 ` Romain Beauxis
2025-05-09 23:43 ` [FFmpeg-devel] [PATCH v5 4/7] ogg/flac: " Romain Beauxis
` (3 subsequent siblings)
6 siblings, 0 replies; 16+ messages in thread
From: Romain Beauxis @ 2025-05-09 23:43 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: Romain Beauxis
---
libavformat/oggdec.c | 4 --
libavformat/oggparseopus.c | 87 ++++++++++++++++--------
tests/ref/fate/ogg-opus-chained-meta.txt | 1 -
3 files changed, 60 insertions(+), 32 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/oggparseopus.c b/libavformat/oggparseopus.c
index 218e9df581..65b93b4053 100644
--- a/libavformat/oggparseopus.c
+++ b/libavformat/oggparseopus.c
@@ -36,6 +36,51 @@ struct oggopus_private {
#define OPUS_SEEK_PREROLL_MS 80
#define OPUS_HEAD_SIZE 19
+static int parse_opus_header(AVFormatContext *avf, AVStream *st, struct ogg_stream *os,
+ struct oggopus_private *priv, uint8_t *packet,
+ size_t psize)
+{
+ int channels;
+ int ret;
+
+ if (psize < OPUS_HEAD_SIZE || (AV_RL8(packet + 8) & 0xF0) != 0)
+ return AVERROR_INVALIDDATA;
+
+ st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO;
+ st->codecpar->codec_id = AV_CODEC_ID_OPUS;
+
+ channels = AV_RL8(packet + 9);
+ if (st->codecpar->ch_layout.nb_channels &&
+ channels != st->codecpar->ch_layout.nb_channels) {
+ av_log(avf, AV_LOG_ERROR, "Channel change is not supported\n");
+ return AVERROR_PATCHWELCOME;
+ }
+
+ st->codecpar->ch_layout.nb_channels = channels;
+
+ priv->pre_skip = AV_RL16(packet + 10);
+ st->codecpar->initial_padding = priv->pre_skip;
+ os->start_trimming = priv->pre_skip;
+ /*orig_sample_rate = AV_RL32(packet + 12);*/
+ /*gain = AV_RL16(packet + 16);*/
+ /*channel_map = AV_RL8 (packet + 18);*/
+
+ ret = ff_alloc_extradata(st->codecpar, os->psize);
+ if (ret < 0)
+ return ret;
+
+ memcpy(st->codecpar->extradata, packet, os->psize);
+
+ st->codecpar->sample_rate = 48000;
+ st->codecpar->seek_preroll = av_rescale(OPUS_SEEK_PREROLL_MS,
+ st->codecpar->sample_rate, 1000);
+ avpriv_set_pts_info(st, 64, 1, 48000);
+
+ priv->need_comments = 1;
+
+ return 1;
+}
+
static int opus_header(AVFormatContext *avf, int idx)
{
struct ogg *ogg = avf->priv_data;
@@ -43,7 +88,6 @@ static int opus_header(AVFormatContext *avf, int idx)
AVStream *st = avf->streams[idx];
struct oggopus_private *priv = os->private;
uint8_t *packet = os->buf + os->pstart;
- int ret;
if (!priv) {
priv = os->private = av_mallocz(sizeof(*priv));
@@ -51,32 +95,8 @@ static int opus_header(AVFormatContext *avf, int idx)
return AVERROR(ENOMEM);
}
- if (os->flags & OGG_FLAG_BOS) {
- if (os->psize < OPUS_HEAD_SIZE || (AV_RL8(packet + 8) & 0xF0) != 0)
- return AVERROR_INVALIDDATA;
- st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO;
- st->codecpar->codec_id = AV_CODEC_ID_OPUS;
- st->codecpar->ch_layout.nb_channels = AV_RL8(packet + 9);
-
- priv->pre_skip = AV_RL16(packet + 10);
- st->codecpar->initial_padding = priv->pre_skip;
- os->start_trimming = priv->pre_skip;
- /*orig_sample_rate = AV_RL32(packet + 12);*/
- /*gain = AV_RL16(packet + 16);*/
- /*channel_map = AV_RL8 (packet + 18);*/
-
- if ((ret = ff_alloc_extradata(st->codecpar, os->psize)) < 0)
- return ret;
-
- memcpy(st->codecpar->extradata, packet, os->psize);
-
- st->codecpar->sample_rate = 48000;
- st->codecpar->seek_preroll = av_rescale(OPUS_SEEK_PREROLL_MS,
- st->codecpar->sample_rate, 1000);
- avpriv_set_pts_info(st, 64, 1, 48000);
- priv->need_comments = 1;
- return 1;
- }
+ if (os->flags & OGG_FLAG_BOS)
+ return parse_opus_header(avf, st, os, priv, packet, os->psize);
if (priv->need_comments) {
if (os->psize < 8 || memcmp(packet, "OpusTags", 8))
@@ -125,6 +145,19 @@ static int opus_packet(AVFormatContext *avf, int idx)
return AVERROR_INVALIDDATA;
}
+ if (os->psize > 8 && !memcmp(packet, "OpusHead", 8)) {
+ ret = parse_opus_header(avf, st, os, priv, packet, os->psize);
+ if (ret < 0)
+ return ret;
+
+ return 1;
+ }
+
+ if (os->psize > 8 && !memcmp(packet, "OpusTags", 8)) {
+ priv->need_comments = 0;
+ return 1;
+ }
+
if ((!os->lastpts || os->lastpts == AV_NOPTS_VALUE) && !(os->flags & OGG_FLAG_EOS)) {
int seg, d;
int duration;
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
--
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".
^ permalink raw reply [flat|nested] 16+ messages in thread* [FFmpeg-devel] [PATCH v5 4/7] ogg/flac: implement header packet skip in chained ogg bitstreams.
2025-05-09 23:43 [FFmpeg-devel] [PATCH v5 0/7] Remove chained ogg stream header packets from the demuxer Romain Beauxis
` (2 preceding siblings ...)
2025-05-09 23:43 ` [FFmpeg-devel] [PATCH v5 3/7] ogg/opus: implement header packet skip in chained ogg bitstreams Romain Beauxis
@ 2025-05-09 23:43 ` Romain Beauxis
2025-05-09 23:43 ` [FFmpeg-devel] [PATCH v5 5/7] libavformat/oggdec.{c, h}: Add new_extradata, use it to pass extradata to the next decoded packet Romain Beauxis
` (2 subsequent siblings)
6 siblings, 0 replies; 16+ messages in thread
From: Romain Beauxis @ 2025-05-09 23:43 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: Romain Beauxis
---
libavformat/oggparseflac.c | 28 ++++++++++++++++++++++--
tests/ref/fate/ogg-flac-chained-meta.txt | 2 --
2 files changed, 26 insertions(+), 4 deletions(-)
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/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
--
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".
^ permalink raw reply [flat|nested] 16+ messages in thread* [FFmpeg-devel] [PATCH v5 5/7] libavformat/oggdec.{c, h}: Add new_extradata, use it to pass extradata to the next decoded packet.
2025-05-09 23:43 [FFmpeg-devel] [PATCH v5 0/7] Remove chained ogg stream header packets from the demuxer Romain Beauxis
` (3 preceding siblings ...)
2025-05-09 23:43 ` [FFmpeg-devel] [PATCH v5 4/7] ogg/flac: " Romain Beauxis
@ 2025-05-09 23:43 ` Romain Beauxis
2025-05-09 23:43 ` [FFmpeg-devel] [PATCH v5 6/7] ogg/vorbis: implement header packet skip in chained ogg bitstreams Romain Beauxis
2025-05-09 23:43 ` [FFmpeg-devel] [PATCH v5 7/7] libavformat/oggdec.h: Change paket function documentation to return 1 on header packets only Romain Beauxis
6 siblings, 0 replies; 16+ messages in thread
From: Romain Beauxis @ 2025-05-09 23:43 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: Romain Beauxis
---
libavformat/oggdec.c | 11 +++++++++++
libavformat/oggdec.h | 2 ++
2 files changed, 13 insertions(+)
diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
index 5557eb4a14..cb77cdd994 100644
--- a/libavformat/oggdec.c
+++ b/libavformat/oggdec.c
@@ -77,6 +77,7 @@ static void free_stream(AVFormatContext *s, int i)
av_freep(&stream->private);
av_freep(&stream->new_metadata);
+ av_freep(&stream->new_extradata);
}
//FIXME We could avoid some structure duplication
@@ -888,6 +889,16 @@ retry:
os->new_metadata_size = 0;
}
+ if (os->new_extradata) {
+ ret = av_packet_add_side_data(pkt, AV_PKT_DATA_NEW_EXTRADATA,
+ os->new_extradata, os->new_extradata_size);
+ if (ret < 0)
+ return ret;
+
+ os->new_extradata = NULL;
+ os->new_extradata_size = 0;
+ }
+
return psize;
}
diff --git a/libavformat/oggdec.h b/libavformat/oggdec.h
index bc670d0f1e..5083de646c 100644
--- a/libavformat/oggdec.h
+++ b/libavformat/oggdec.h
@@ -94,6 +94,8 @@ struct ogg_stream {
int end_trimming; ///< set the number of packets to drop from the end
uint8_t *new_metadata;
size_t new_metadata_size;
+ uint8_t *new_extradata;
+ size_t new_extradata_size;
void *private;
};
--
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".
^ permalink raw reply [flat|nested] 16+ messages in thread* [FFmpeg-devel] [PATCH v5 6/7] ogg/vorbis: implement header packet skip in chained ogg bitstreams.
2025-05-09 23:43 [FFmpeg-devel] [PATCH v5 0/7] Remove chained ogg stream header packets from the demuxer Romain Beauxis
` (4 preceding siblings ...)
2025-05-09 23:43 ` [FFmpeg-devel] [PATCH v5 5/7] libavformat/oggdec.{c, h}: Add new_extradata, use it to pass extradata to the next decoded packet Romain Beauxis
@ 2025-05-09 23:43 ` Romain Beauxis
2025-05-13 19:23 ` Michael Niedermayer
2025-05-09 23:43 ` [FFmpeg-devel] [PATCH v5 7/7] libavformat/oggdec.h: Change paket function documentation to return 1 on header packets only Romain Beauxis
6 siblings, 1 reply; 16+ messages in thread
From: Romain Beauxis @ 2025-05-09 23:43 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: Romain Beauxis
---
libavcodec/vorbisdec.c | 37 +----
libavformat/oggparsevorbis.c | 174 +++++++++++++--------
tests/ref/fate/ogg-vorbis-chained-meta.txt | 3 -
3 files changed, 117 insertions(+), 97 deletions(-)
diff --git a/libavcodec/vorbisdec.c b/libavcodec/vorbisdec.c
index a778dc6b58..f069ac6ab3 100644
--- a/libavcodec/vorbisdec.c
+++ b/libavcodec/vorbisdec.c
@@ -1776,39 +1776,17 @@ static int vorbis_decode_frame(AVCodecContext *avctx, AVFrame *frame,
GetBitContext *gb = &vc->gb;
float *channel_ptrs[255];
int i, len, ret;
+ const int8_t *new_extradata;
+ size_t new_extradata_size;
ff_dlog(NULL, "packet length %d \n", buf_size);
- if (*buf == 1 && buf_size > 7) {
- if ((ret = init_get_bits8(gb, buf + 1, buf_size - 1)) < 0)
- return ret;
-
- vorbis_free(vc);
- if ((ret = vorbis_parse_id_hdr(vc))) {
- av_log(avctx, AV_LOG_ERROR, "Id header corrupt.\n");
- vorbis_free(vc);
- return ret;
- }
-
- av_channel_layout_uninit(&avctx->ch_layout);
- if (vc->audio_channels > 8) {
- avctx->ch_layout.order = AV_CHANNEL_ORDER_UNSPEC;
- avctx->ch_layout.nb_channels = vc->audio_channels;
- } else {
- av_channel_layout_copy(&avctx->ch_layout, &ff_vorbis_ch_layouts[vc->audio_channels - 1]);
- }
-
- avctx->sample_rate = vc->audio_samplerate;
- return buf_size;
- }
-
- if (*buf == 3 && buf_size > 7) {
- av_log(avctx, AV_LOG_DEBUG, "Ignoring comment header\n");
- return buf_size;
- }
+ new_extradata = av_packet_get_side_data(avpkt, AV_PKT_DATA_NEW_EXTRADATA,
+ &new_extradata_size);
- if (*buf == 5 && buf_size > 7 && vc->channel_residues && !vc->modes) {
- if ((ret = init_get_bits8(gb, buf + 1, buf_size - 1)) < 0)
+ if (new_extradata && *new_extradata == 5 && new_extradata_size > 7 &&
+ vc->channel_residues && !vc->modes) {
+ if ((ret = init_get_bits8(gb, new_extradata + 1, new_extradata_size - 1)) < 0)
return ret;
if ((ret = vorbis_parse_setup_hdr(vc))) {
@@ -1816,7 +1794,6 @@ static int vorbis_decode_frame(AVCodecContext *avctx, AVFrame *frame,
vorbis_free(vc);
return ret;
}
- return buf_size;
}
if (!vc->channel_residues || !vc->modes) {
diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c
index 9f50ab9ffc..452728b54d 100644
--- a/libavformat/oggparsevorbis.c
+++ b/libavformat/oggparsevorbis.c
@@ -293,6 +293,62 @@ static int vorbis_update_metadata(AVFormatContext *s, int idx)
return ret;
}
+static int vorbis_parse_header(AVFormatContext *s, AVStream *st,
+ const uint8_t *p, unsigned int psize)
+{
+ unsigned blocksize, bs0, bs1;
+ int srate;
+ int channels;
+
+ if (psize != 30)
+ return AVERROR_INVALIDDATA;
+
+ p += 7; /* skip "\001vorbis" tag */
+
+ if (bytestream_get_le32(&p) != 0) /* vorbis_version */
+ return AVERROR_INVALIDDATA;
+
+ channels = bytestream_get_byte(&p);
+ if (st->codecpar->ch_layout.nb_channels &&
+ channels != st->codecpar->ch_layout.nb_channels) {
+ av_log(s, AV_LOG_ERROR, "Channel change is not supported\n");
+ return AVERROR_PATCHWELCOME;
+ }
+ st->codecpar->ch_layout.nb_channels = channels;
+ srate = bytestream_get_le32(&p);
+ p += 4; // skip maximum bitrate
+ st->codecpar->bit_rate = bytestream_get_le32(&p); // nominal bitrate
+ p += 4; // skip minimum bitrate
+
+ blocksize = bytestream_get_byte(&p);
+ bs0 = blocksize & 15;
+ bs1 = blocksize >> 4;
+
+ if (bs0 > bs1)
+ return AVERROR_INVALIDDATA;
+ if (bs0 < 6 || bs1 > 13)
+ return AVERROR_INVALIDDATA;
+
+ if (bytestream_get_byte(&p) != 1) /* framing_flag */
+ return AVERROR_INVALIDDATA;
+
+ st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO;
+ st->codecpar->codec_id = AV_CODEC_ID_VORBIS;
+
+ if (srate > 0) {
+ if (st->codecpar->sample_rate &&
+ srate != st->codecpar->sample_rate) {
+ av_log(s, AV_LOG_ERROR, "Sample rate change is not supported\n");
+ return AVERROR_PATCHWELCOME;
+ }
+
+ st->codecpar->sample_rate = srate;
+ avpriv_set_pts_info(st, 64, 1, srate);
+ }
+
+ return 1;
+}
+
static int vorbis_header(AVFormatContext *s, int idx)
{
struct ogg *ogg = s->priv_data;
@@ -300,6 +356,7 @@ static int vorbis_header(AVFormatContext *s, int idx)
struct ogg_stream *os = ogg->streams + idx;
struct oggvorbis_private *priv;
int pkt_type = os->buf[os->pstart];
+ int ret;
if (!os->private) {
os->private = av_mallocz(sizeof(struct oggvorbis_private));
@@ -327,56 +384,18 @@ static int vorbis_header(AVFormatContext *s, int idx)
priv->len[pkt_type >> 1] = os->psize;
priv->packet[pkt_type >> 1] = av_memdup(os->buf + os->pstart, os->psize);
+
if (!priv->packet[pkt_type >> 1])
return AVERROR(ENOMEM);
- if (os->buf[os->pstart] == 1) {
- const uint8_t *p = os->buf + os->pstart + 7; /* skip "\001vorbis" tag */
- unsigned blocksize, bs0, bs1;
- int srate;
- int channels;
-
- if (os->psize != 30)
- return AVERROR_INVALIDDATA;
-
- if (bytestream_get_le32(&p) != 0) /* vorbis_version */
- return AVERROR_INVALIDDATA;
-
- channels = bytestream_get_byte(&p);
- if (st->codecpar->ch_layout.nb_channels &&
- channels != st->codecpar->ch_layout.nb_channels) {
- av_log(s, AV_LOG_ERROR, "Channel change is not supported\n");
- return AVERROR_PATCHWELCOME;
- }
- st->codecpar->ch_layout.nb_channels = channels;
- srate = bytestream_get_le32(&p);
- p += 4; // skip maximum bitrate
- st->codecpar->bit_rate = bytestream_get_le32(&p); // nominal bitrate
- p += 4; // skip minimum bitrate
-
- blocksize = bytestream_get_byte(&p);
- bs0 = blocksize & 15;
- bs1 = blocksize >> 4;
-
- if (bs0 > bs1)
- return AVERROR_INVALIDDATA;
- if (bs0 < 6 || bs1 > 13)
- return AVERROR_INVALIDDATA;
-
- if (bytestream_get_byte(&p) != 1) /* framing_flag */
- return AVERROR_INVALIDDATA;
-
- st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO;
- st->codecpar->codec_id = AV_CODEC_ID_VORBIS;
-
- if (srate > 0) {
- st->codecpar->sample_rate = srate;
- avpriv_set_pts_info(st, 64, 1, srate);
- }
- } else if (os->buf[os->pstart] == 3) {
+
+ if (pkt_type == 1)
+ return vorbis_parse_header(s, st, os->buf + os->pstart, os->psize);
+
+ if (pkt_type == 3) {
if (vorbis_update_metadata(s, idx) >= 0 && priv->len[1] > 10) {
unsigned new_len;
- int ret = ff_replaygain_export(st, st->metadata);
+ ret = ff_replaygain_export(st, st->metadata);
if (ret < 0)
return ret;
@@ -388,25 +407,25 @@ static int vorbis_header(AVFormatContext *s, int idx)
priv->len[1] = new_len;
}
}
- } else {
- int ret;
- if (priv->vp)
- return AVERROR_INVALIDDATA;
+ return 1;
+ }
- ret = fixup_vorbis_headers(s, priv, &st->codecpar->extradata);
- if (ret < 0) {
- st->codecpar->extradata_size = 0;
- return ret;
- }
- st->codecpar->extradata_size = ret;
+ if (priv->vp)
+ return AVERROR_INVALIDDATA;
- priv->vp = av_vorbis_parse_init(st->codecpar->extradata, st->codecpar->extradata_size);
- if (!priv->vp) {
- av_freep(&st->codecpar->extradata);
- st->codecpar->extradata_size = 0;
- return AVERROR_UNKNOWN;
- }
+ ret = fixup_vorbis_headers(s, priv, &st->codecpar->extradata);
+ if (ret < 0) {
+ st->codecpar->extradata_size = 0;
+ return ret;
+ }
+ st->codecpar->extradata_size = ret;
+
+ priv->vp = av_vorbis_parse_init(st->codecpar->extradata, st->codecpar->extradata_size);
+ if (!priv->vp) {
+ av_freep(&st->codecpar->extradata);
+ st->codecpar->extradata_size = 0;
+ return AVERROR_UNKNOWN;
}
return 1;
@@ -418,6 +437,8 @@ 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;
+ int ret;
if (!priv->vp)
return AVERROR_INVALIDDATA;
@@ -480,10 +501,35 @@ 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) {
- vorbis_update_metadata(s, idx);
+ }
+
+ if (flags & VORBIS_FLAG_HEADER) {
+ ret = vorbis_parse_header(s, s->streams[idx], os->buf + os->pstart, os->psize);
+ if (ret < 0)
+ return ret;
+
+ skip_packet = 1;
+ }
+
+ if (flags & VORBIS_FLAG_COMMENT) {
+ ret = vorbis_update_metadata(s, idx);
+ if (ret < 0)
+ return ret;
+
flags = 0;
+ skip_packet = 1;
}
+
+ if (flags & VORBIS_FLAG_SETUP) {
+ ret = av_reallocp(&os->new_extradata, os->psize);
+ if (ret < 0)
+ return ret;
+
+ memcpy(os->new_extradata, os->buf + os->pstart, os->psize);
+ os->new_extradata_size = os->psize;
+ skip_packet = 1;
+ }
+
os->pduration = duration;
}
@@ -505,7 +551,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-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".
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [FFmpeg-devel] [PATCH v5 6/7] ogg/vorbis: implement header packet skip in chained ogg bitstreams.
2025-05-09 23:43 ` [FFmpeg-devel] [PATCH v5 6/7] ogg/vorbis: implement header packet skip in chained ogg bitstreams Romain Beauxis
@ 2025-05-13 19:23 ` Michael Niedermayer
2025-05-17 18:10 ` Romain Beauxis
0 siblings, 1 reply; 16+ messages in thread
From: Michael Niedermayer @ 2025-05-13 19:23 UTC (permalink / raw)
To: FFmpeg development discussions and patches
[-- Attachment #1.1: Type: text/plain, Size: 9084 bytes --]
On Fri, May 09, 2025 at 06:43:26PM -0500, Romain Beauxis wrote:
> ---
> libavcodec/vorbisdec.c | 37 +----
> libavformat/oggparsevorbis.c | 174 +++++++++++++--------
> tests/ref/fate/ogg-vorbis-chained-meta.txt | 3 -
> 3 files changed, 117 insertions(+), 97 deletions(-)
>
> diff --git a/libavcodec/vorbisdec.c b/libavcodec/vorbisdec.c
> index a778dc6b58..f069ac6ab3 100644
> --- a/libavcodec/vorbisdec.c
> +++ b/libavcodec/vorbisdec.c
> @@ -1776,39 +1776,17 @@ static int vorbis_decode_frame(AVCodecContext *avctx, AVFrame *frame,
> GetBitContext *gb = &vc->gb;
> float *channel_ptrs[255];
> int i, len, ret;
> + const int8_t *new_extradata;
> + size_t new_extradata_size;
>
> ff_dlog(NULL, "packet length %d \n", buf_size);
>
> - if (*buf == 1 && buf_size > 7) {
> - if ((ret = init_get_bits8(gb, buf + 1, buf_size - 1)) < 0)
> - return ret;
> -
> - vorbis_free(vc);
> - if ((ret = vorbis_parse_id_hdr(vc))) {
> - av_log(avctx, AV_LOG_ERROR, "Id header corrupt.\n");
> - vorbis_free(vc);
> - return ret;
> - }
> -
> - av_channel_layout_uninit(&avctx->ch_layout);
> - if (vc->audio_channels > 8) {
> - avctx->ch_layout.order = AV_CHANNEL_ORDER_UNSPEC;
> - avctx->ch_layout.nb_channels = vc->audio_channels;
> - } else {
> - av_channel_layout_copy(&avctx->ch_layout, &ff_vorbis_ch_layouts[vc->audio_channels - 1]);
> - }
> -
> - avctx->sample_rate = vc->audio_samplerate;
> - return buf_size;
> - }
> -
> - if (*buf == 3 && buf_size > 7) {
> - av_log(avctx, AV_LOG_DEBUG, "Ignoring comment header\n");
> - return buf_size;
> - }
> + new_extradata = av_packet_get_side_data(avpkt, AV_PKT_DATA_NEW_EXTRADATA,
> + &new_extradata_size);
>
> - if (*buf == 5 && buf_size > 7 && vc->channel_residues && !vc->modes) {
> - if ((ret = init_get_bits8(gb, buf + 1, buf_size - 1)) < 0)
> + if (new_extradata && *new_extradata == 5 && new_extradata_size > 7 &&
> + vc->channel_residues && !vc->modes) {
> + if ((ret = init_get_bits8(gb, new_extradata + 1, new_extradata_size - 1)) < 0)
> return ret;
>
> if ((ret = vorbis_parse_setup_hdr(vc))) {
> @@ -1816,7 +1794,6 @@ static int vorbis_decode_frame(AVCodecContext *avctx, AVFrame *frame,
> vorbis_free(vc);
> return ret;
> }
> - return buf_size;
> }
>
> if (!vc->channel_residues || !vc->modes) {
> diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c
> index 9f50ab9ffc..452728b54d 100644
> --- a/libavformat/oggparsevorbis.c
> +++ b/libavformat/oggparsevorbis.c
> @@ -293,6 +293,62 @@ static int vorbis_update_metadata(AVFormatContext *s, int idx)
> return ret;
> }
>
> +static int vorbis_parse_header(AVFormatContext *s, AVStream *st,
> + const uint8_t *p, unsigned int psize)
> +{
> + unsigned blocksize, bs0, bs1;
> + int srate;
> + int channels;
> +
> + if (psize != 30)
> + return AVERROR_INVALIDDATA;
> +
> + p += 7; /* skip "\001vorbis" tag */
> +
> + if (bytestream_get_le32(&p) != 0) /* vorbis_version */
> + return AVERROR_INVALIDDATA;
> +
> + channels = bytestream_get_byte(&p);
> + if (st->codecpar->ch_layout.nb_channels &&
> + channels != st->codecpar->ch_layout.nb_channels) {
> + av_log(s, AV_LOG_ERROR, "Channel change is not supported\n");
> + return AVERROR_PATCHWELCOME;
> + }
> + st->codecpar->ch_layout.nb_channels = channels;
> + srate = bytestream_get_le32(&p);
> + p += 4; // skip maximum bitrate
> + st->codecpar->bit_rate = bytestream_get_le32(&p); // nominal bitrate
> + p += 4; // skip minimum bitrate
> +
> + blocksize = bytestream_get_byte(&p);
> + bs0 = blocksize & 15;
> + bs1 = blocksize >> 4;
> +
> + if (bs0 > bs1)
> + return AVERROR_INVALIDDATA;
> + if (bs0 < 6 || bs1 > 13)
> + return AVERROR_INVALIDDATA;
> +
> + if (bytestream_get_byte(&p) != 1) /* framing_flag */
> + return AVERROR_INVALIDDATA;
> +
> + st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO;
> + st->codecpar->codec_id = AV_CODEC_ID_VORBIS;
> +
> + if (srate > 0) {
> + if (st->codecpar->sample_rate &&
> + srate != st->codecpar->sample_rate) {
> + av_log(s, AV_LOG_ERROR, "Sample rate change is not supported\n");
> + return AVERROR_PATCHWELCOME;
> + }
> +
> + st->codecpar->sample_rate = srate;
> + avpriv_set_pts_info(st, 64, 1, srate);
> + }
> +
> + return 1;
> +}
> +
> static int vorbis_header(AVFormatContext *s, int idx)
> {
> struct ogg *ogg = s->priv_data;
> @@ -300,6 +356,7 @@ static int vorbis_header(AVFormatContext *s, int idx)
> struct ogg_stream *os = ogg->streams + idx;
> struct oggvorbis_private *priv;
> int pkt_type = os->buf[os->pstart];
> + int ret;
>
> if (!os->private) {
> os->private = av_mallocz(sizeof(struct oggvorbis_private));
> @@ -327,56 +384,18 @@ static int vorbis_header(AVFormatContext *s, int idx)
>
> priv->len[pkt_type >> 1] = os->psize;
> priv->packet[pkt_type >> 1] = av_memdup(os->buf + os->pstart, os->psize);
> +
> if (!priv->packet[pkt_type >> 1])
> return AVERROR(ENOMEM);
> - if (os->buf[os->pstart] == 1) {
> - const uint8_t *p = os->buf + os->pstart + 7; /* skip "\001vorbis" tag */
> - unsigned blocksize, bs0, bs1;
> - int srate;
> - int channels;
> -
> - if (os->psize != 30)
> - return AVERROR_INVALIDDATA;
> -
> - if (bytestream_get_le32(&p) != 0) /* vorbis_version */
> - return AVERROR_INVALIDDATA;
> -
> - channels = bytestream_get_byte(&p);
> - if (st->codecpar->ch_layout.nb_channels &&
> - channels != st->codecpar->ch_layout.nb_channels) {
> - av_log(s, AV_LOG_ERROR, "Channel change is not supported\n");
> - return AVERROR_PATCHWELCOME;
> - }
> - st->codecpar->ch_layout.nb_channels = channels;
> - srate = bytestream_get_le32(&p);
> - p += 4; // skip maximum bitrate
> - st->codecpar->bit_rate = bytestream_get_le32(&p); // nominal bitrate
> - p += 4; // skip minimum bitrate
> -
> - blocksize = bytestream_get_byte(&p);
> - bs0 = blocksize & 15;
> - bs1 = blocksize >> 4;
> -
> - if (bs0 > bs1)
> - return AVERROR_INVALIDDATA;
> - if (bs0 < 6 || bs1 > 13)
> - return AVERROR_INVALIDDATA;
> -
> - if (bytestream_get_byte(&p) != 1) /* framing_flag */
> - return AVERROR_INVALIDDATA;
> -
> - st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO;
> - st->codecpar->codec_id = AV_CODEC_ID_VORBIS;
> -
> - if (srate > 0) {
> - st->codecpar->sample_rate = srate;
> - avpriv_set_pts_info(st, 64, 1, srate);
> - }
> - } else if (os->buf[os->pstart] == 3) {
moving code should be in a seperate patch so any changes can be clearly
seen in the diff which does not move the code.
The output from:
libavformat/tests/seek $TICKETS/2739/lavf_ogm_seeking_borked.ogm
chanegs by:
@@ -273,7 +273,7 @@
ret: 0 st: 2 flags:0 ts: 0.365000
ret: 0 st: 2 flags:1 dts: 0.332000 pts: 0.332000 pos: 18959 size: 271
ret: 0 st: 2 flags:1 ts:-0.740833
-ret: 0 st: 2 flags:1 dts: 0.000000 pts: 0.000000 pos: 7691 size: 2519
+ret: 0 st: 2 flags:1 dts:-0.002667 pts:-0.002667 pos: 14580 size: 39
ret: 0 st: 3 flags:0 ts: 2.153000
ret: 0 st: 3 flags:1 dts: 113.023000 pts: 113.023000 pos:25335211 size: 47
ret: 0 st: 3 flags:1 ts: 1.048000
@@ -295,7 +295,7 @@
ret: 0 st: 1 flags:1 ts: 0.200833
ret: 0 st: 1 flags:1 dts:-0.002667 pts:-0.002667 pos: 10315 size: 32
ret: 0 st: 2 flags:0 ts:-0.905000
-ret: 0 st: 2 flags:1 dts: 0.000000 pts: 0.000000 pos: 7691 size: 2519
+ret: 0 st: 2 flags:1 dts:-0.002667 pts:-0.002667 pos: 14580 size: 39
ret: 0 st: 2 flags:1 ts: 1.989167
ret: 0 st: 2 flags:1 dts: 1.782667 pts: 1.782667 pos: 321518 size: 241
ret: 0 st: 3 flags:0 ts: 0.883000
Command exited with non-zero status 1
Is this correct&intended ?
thx
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Rewriting code that is poorly written but fully understood is good.
Rewriting code that one doesnt understand is a sign that one is less smart
than the original author, trying to rewrite it will not make it better.
[-- 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".
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [FFmpeg-devel] [PATCH v5 6/7] ogg/vorbis: implement header packet skip in chained ogg bitstreams.
2025-05-13 19:23 ` Michael Niedermayer
@ 2025-05-17 18:10 ` Romain Beauxis
2025-05-17 22:07 ` Michael Niedermayer
0 siblings, 1 reply; 16+ messages in thread
From: Romain Beauxis @ 2025-05-17 18:10 UTC (permalink / raw)
To: FFmpeg development discussions and patches
Le mar. 13 mai 2025 à 14:23, Michael Niedermayer <michael@niedermayer.cc> a
écrit :
>
> On Fri, May 09, 2025 at 06:43:26PM -0500, Romain Beauxis wrote:
> > ---
> > libavcodec/vorbisdec.c | 37 +----
> > libavformat/oggparsevorbis.c | 174 +++++++++++++--------
> > tests/ref/fate/ogg-vorbis-chained-meta.txt | 3 -
> > 3 files changed, 117 insertions(+), 97 deletions(-)
> >
> > diff --git a/libavcodec/vorbisdec.c b/libavcodec/vorbisdec.c
> > index a778dc6b58..f069ac6ab3 100644
> > --- a/libavcodec/vorbisdec.c
> > +++ b/libavcodec/vorbisdec.c
> > @@ -1776,39 +1776,17 @@ static int vorbis_decode_frame(AVCodecContext
*avctx, AVFrame *frame,
> > GetBitContext *gb = &vc->gb;
> > float *channel_ptrs[255];
> > int i, len, ret;
> > + const int8_t *new_extradata;
> > + size_t new_extradata_size;
> >
> > ff_dlog(NULL, "packet length %d \n", buf_size);
> >
> > - if (*buf == 1 && buf_size > 7) {
> > - if ((ret = init_get_bits8(gb, buf + 1, buf_size - 1)) < 0)
> > - return ret;
> > -
> > - vorbis_free(vc);
> > - if ((ret = vorbis_parse_id_hdr(vc))) {
> > - av_log(avctx, AV_LOG_ERROR, "Id header corrupt.\n");
> > - vorbis_free(vc);
> > - return ret;
> > - }
> > -
> > - av_channel_layout_uninit(&avctx->ch_layout);
> > - if (vc->audio_channels > 8) {
> > - avctx->ch_layout.order = AV_CHANNEL_ORDER_UNSPEC;
> > - avctx->ch_layout.nb_channels = vc->audio_channels;
> > - } else {
> > - av_channel_layout_copy(&avctx->ch_layout,
&ff_vorbis_ch_layouts[vc->audio_channels - 1]);
> > - }
> > -
> > - avctx->sample_rate = vc->audio_samplerate;
> > - return buf_size;
> > - }
> > -
> > - if (*buf == 3 && buf_size > 7) {
> > - av_log(avctx, AV_LOG_DEBUG, "Ignoring comment header\n");
> > - return buf_size;
> > - }
> > + new_extradata = av_packet_get_side_data(avpkt,
AV_PKT_DATA_NEW_EXTRADATA,
> > + &new_extradata_size);
> >
> > - if (*buf == 5 && buf_size > 7 && vc->channel_residues &&
!vc->modes) {
> > - if ((ret = init_get_bits8(gb, buf + 1, buf_size - 1)) < 0)
> > + if (new_extradata && *new_extradata == 5 && new_extradata_size > 7
&&
> > + vc->channel_residues && !vc->modes) {
> > + if ((ret = init_get_bits8(gb, new_extradata + 1,
new_extradata_size - 1)) < 0)
> > return ret;
> >
> > if ((ret = vorbis_parse_setup_hdr(vc))) {
> > @@ -1816,7 +1794,6 @@ static int vorbis_decode_frame(AVCodecContext
*avctx, AVFrame *frame,
> > vorbis_free(vc);
> > return ret;
> > }
> > - return buf_size;
> > }
> >
> > if (!vc->channel_residues || !vc->modes) {
> > diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c
> > index 9f50ab9ffc..452728b54d 100644
> > --- a/libavformat/oggparsevorbis.c
> > +++ b/libavformat/oggparsevorbis.c
> > @@ -293,6 +293,62 @@ static int vorbis_update_metadata(AVFormatContext
*s, int idx)
> > return ret;
> > }
> >
> > +static int vorbis_parse_header(AVFormatContext *s, AVStream *st,
> > + const uint8_t *p, unsigned int psize)
> > +{
> > + unsigned blocksize, bs0, bs1;
> > + int srate;
> > + int channels;
> > +
> > + if (psize != 30)
> > + return AVERROR_INVALIDDATA;
> > +
> > + p += 7; /* skip "\001vorbis" tag */
> > +
> > + if (bytestream_get_le32(&p) != 0) /* vorbis_version */
> > + return AVERROR_INVALIDDATA;
> > +
> > + channels = bytestream_get_byte(&p);
> > + if (st->codecpar->ch_layout.nb_channels &&
> > + channels != st->codecpar->ch_layout.nb_channels) {
> > + av_log(s, AV_LOG_ERROR, "Channel change is not supported\n");
> > + return AVERROR_PATCHWELCOME;
> > + }
> > + st->codecpar->ch_layout.nb_channels = channels;
> > + srate = bytestream_get_le32(&p);
> > + p += 4; // skip maximum bitrate
> > + st->codecpar->bit_rate = bytestream_get_le32(&p); // nominal
bitrate
> > + p += 4; // skip minimum bitrate
> > +
> > + blocksize = bytestream_get_byte(&p);
> > + bs0 = blocksize & 15;
> > + bs1 = blocksize >> 4;
> > +
> > + if (bs0 > bs1)
> > + return AVERROR_INVALIDDATA;
> > + if (bs0 < 6 || bs1 > 13)
> > + return AVERROR_INVALIDDATA;
> > +
> > + if (bytestream_get_byte(&p) != 1) /* framing_flag */
> > + return AVERROR_INVALIDDATA;
> > +
> > + st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO;
> > + st->codecpar->codec_id = AV_CODEC_ID_VORBIS;
> > +
> > + if (srate > 0) {
> > + if (st->codecpar->sample_rate &&
> > + srate != st->codecpar->sample_rate) {
> > + av_log(s, AV_LOG_ERROR, "Sample rate change is not
supported\n");
> > + return AVERROR_PATCHWELCOME;
> > + }
> > +
> > + st->codecpar->sample_rate = srate;
> > + avpriv_set_pts_info(st, 64, 1, srate);
> > + }
> > +
> > + return 1;
> > +}
> > +
> > static int vorbis_header(AVFormatContext *s, int idx)
> > {
> > struct ogg *ogg = s->priv_data;
> > @@ -300,6 +356,7 @@ static int vorbis_header(AVFormatContext *s, int
idx)
> > struct ogg_stream *os = ogg->streams + idx;
> > struct oggvorbis_private *priv;
> > int pkt_type = os->buf[os->pstart];
> > + int ret;
> >
> > if (!os->private) {
> > os->private = av_mallocz(sizeof(struct oggvorbis_private));
> > @@ -327,56 +384,18 @@ static int vorbis_header(AVFormatContext *s, int
idx)
> >
> > priv->len[pkt_type >> 1] = os->psize;
> > priv->packet[pkt_type >> 1] = av_memdup(os->buf + os->pstart,
os->psize);
> > +
> > if (!priv->packet[pkt_type >> 1])
> > return AVERROR(ENOMEM);
> > - if (os->buf[os->pstart] == 1) {
> > - const uint8_t *p = os->buf + os->pstart + 7; /* skip
"\001vorbis" tag */
> > - unsigned blocksize, bs0, bs1;
> > - int srate;
> > - int channels;
> > -
> > - if (os->psize != 30)
> > - return AVERROR_INVALIDDATA;
> > -
> > - if (bytestream_get_le32(&p) != 0) /* vorbis_version */
> > - return AVERROR_INVALIDDATA;
> > -
> > - channels = bytestream_get_byte(&p);
> > - if (st->codecpar->ch_layout.nb_channels &&
> > - channels != st->codecpar->ch_layout.nb_channels) {
> > - av_log(s, AV_LOG_ERROR, "Channel change is not
supported\n");
> > - return AVERROR_PATCHWELCOME;
> > - }
> > - st->codecpar->ch_layout.nb_channels = channels;
> > - srate = bytestream_get_le32(&p);
> > - p += 4; // skip maximum bitrate
> > - st->codecpar->bit_rate = bytestream_get_le32(&p); // nominal
bitrate
> > - p += 4; // skip minimum bitrate
> > -
> > - blocksize = bytestream_get_byte(&p);
> > - bs0 = blocksize & 15;
> > - bs1 = blocksize >> 4;
> > -
> > - if (bs0 > bs1)
> > - return AVERROR_INVALIDDATA;
> > - if (bs0 < 6 || bs1 > 13)
> > - return AVERROR_INVALIDDATA;
> > -
> > - if (bytestream_get_byte(&p) != 1) /* framing_flag */
> > - return AVERROR_INVALIDDATA;
> > -
> > - st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO;
> > - st->codecpar->codec_id = AV_CODEC_ID_VORBIS;
> > -
> > - if (srate > 0) {
> > - st->codecpar->sample_rate = srate;
> > - avpriv_set_pts_info(st, 64, 1, srate);
> > - }
> > - } else if (os->buf[os->pstart] == 3) {
>
>
> moving code should be in a seperate patch so any changes can be clearly
> seen in the diff which does not move the code.
>
> The output from:
> libavformat/tests/seek $TICKETS/2739/lavf_ogm_seeking_borked.ogm
>
> chanegs by:
> @@ -273,7 +273,7 @@
> ret: 0 st: 2 flags:0 ts: 0.365000
> ret: 0 st: 2 flags:1 dts: 0.332000 pts: 0.332000 pos: 18959
size: 271
> ret: 0 st: 2 flags:1 ts:-0.740833
> -ret: 0 st: 2 flags:1 dts: 0.000000 pts: 0.000000 pos: 7691
size: 2519
> +ret: 0 st: 2 flags:1 dts:-0.002667 pts:-0.002667 pos: 14580
size: 39
> ret: 0 st: 3 flags:0 ts: 2.153000
> ret: 0 st: 3 flags:1 dts: 113.023000 pts: 113.023000
pos:25335211 size: 47
> ret: 0 st: 3 flags:1 ts: 1.048000
> @@ -295,7 +295,7 @@
> ret: 0 st: 1 flags:1 ts: 0.200833
> ret: 0 st: 1 flags:1 dts:-0.002667 pts:-0.002667 pos: 10315
size: 32
> ret: 0 st: 2 flags:0 ts:-0.905000
> -ret: 0 st: 2 flags:1 dts: 0.000000 pts: 0.000000 pos: 7691
size: 2519
> +ret: 0 st: 2 flags:1 dts:-0.002667 pts:-0.002667 pos: 14580
size: 39
> ret: 0 st: 2 flags:1 ts: 1.989167
> ret: 0 st: 2 flags:1 dts: 1.782667 pts: 1.782667 pos: 321518
size: 241
> ret: 0 st: 3 flags:0 ts: 0.883000
> Command exited with non-zero status 1
>
> Is this correct&intended ?
Sorry I thought that this sample was part of FATE but it isn't their nor is
it linked in the corresponding ticket.
It would be nice if there was a definite set of samples/tests to check
before sending patches, I do my best to check my work against the
established set of validation checks but I am unable to test non-existing
files!
Would you provide the file for reference? Is it possible to have an
extensive set of the checks that you are intending to see passed before
considering for inclusion?
I have the split up patchset ready to re-submit when I can confirm that it
works as expected.
Thanks,
-- Romain
_______________________________________________
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".
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [FFmpeg-devel] [PATCH v5 6/7] ogg/vorbis: implement header packet skip in chained ogg bitstreams.
2025-05-17 18:10 ` Romain Beauxis
@ 2025-05-17 22:07 ` Michael Niedermayer
2025-05-19 14:40 ` Romain Beauxis
0 siblings, 1 reply; 16+ messages in thread
From: Michael Niedermayer @ 2025-05-17 22:07 UTC (permalink / raw)
To: FFmpeg development discussions and patches
[-- Attachment #1.1: Type: text/plain, Size: 10576 bytes --]
On Sat, May 17, 2025 at 01:10:26PM -0500, Romain Beauxis wrote:
> Le mar. 13 mai 2025 à 14:23, Michael Niedermayer <michael@niedermayer.cc> a
> écrit :
> >
> > On Fri, May 09, 2025 at 06:43:26PM -0500, Romain Beauxis wrote:
> > > ---
> > > libavcodec/vorbisdec.c | 37 +----
> > > libavformat/oggparsevorbis.c | 174 +++++++++++++--------
> > > tests/ref/fate/ogg-vorbis-chained-meta.txt | 3 -
> > > 3 files changed, 117 insertions(+), 97 deletions(-)
> > >
> > > diff --git a/libavcodec/vorbisdec.c b/libavcodec/vorbisdec.c
> > > index a778dc6b58..f069ac6ab3 100644
> > > --- a/libavcodec/vorbisdec.c
> > > +++ b/libavcodec/vorbisdec.c
> > > @@ -1776,39 +1776,17 @@ static int vorbis_decode_frame(AVCodecContext
> *avctx, AVFrame *frame,
> > > GetBitContext *gb = &vc->gb;
> > > float *channel_ptrs[255];
> > > int i, len, ret;
> > > + const int8_t *new_extradata;
> > > + size_t new_extradata_size;
> > >
> > > ff_dlog(NULL, "packet length %d \n", buf_size);
> > >
> > > - if (*buf == 1 && buf_size > 7) {
> > > - if ((ret = init_get_bits8(gb, buf + 1, buf_size - 1)) < 0)
> > > - return ret;
> > > -
> > > - vorbis_free(vc);
> > > - if ((ret = vorbis_parse_id_hdr(vc))) {
> > > - av_log(avctx, AV_LOG_ERROR, "Id header corrupt.\n");
> > > - vorbis_free(vc);
> > > - return ret;
> > > - }
> > > -
> > > - av_channel_layout_uninit(&avctx->ch_layout);
> > > - if (vc->audio_channels > 8) {
> > > - avctx->ch_layout.order = AV_CHANNEL_ORDER_UNSPEC;
> > > - avctx->ch_layout.nb_channels = vc->audio_channels;
> > > - } else {
> > > - av_channel_layout_copy(&avctx->ch_layout,
> &ff_vorbis_ch_layouts[vc->audio_channels - 1]);
> > > - }
> > > -
> > > - avctx->sample_rate = vc->audio_samplerate;
> > > - return buf_size;
> > > - }
> > > -
> > > - if (*buf == 3 && buf_size > 7) {
> > > - av_log(avctx, AV_LOG_DEBUG, "Ignoring comment header\n");
> > > - return buf_size;
> > > - }
> > > + new_extradata = av_packet_get_side_data(avpkt,
> AV_PKT_DATA_NEW_EXTRADATA,
> > > + &new_extradata_size);
> > >
> > > - if (*buf == 5 && buf_size > 7 && vc->channel_residues &&
> !vc->modes) {
> > > - if ((ret = init_get_bits8(gb, buf + 1, buf_size - 1)) < 0)
> > > + if (new_extradata && *new_extradata == 5 && new_extradata_size > 7
> &&
> > > + vc->channel_residues && !vc->modes) {
> > > + if ((ret = init_get_bits8(gb, new_extradata + 1,
> new_extradata_size - 1)) < 0)
> > > return ret;
> > >
> > > if ((ret = vorbis_parse_setup_hdr(vc))) {
> > > @@ -1816,7 +1794,6 @@ static int vorbis_decode_frame(AVCodecContext
> *avctx, AVFrame *frame,
> > > vorbis_free(vc);
> > > return ret;
> > > }
> > > - return buf_size;
> > > }
> > >
> > > if (!vc->channel_residues || !vc->modes) {
> > > diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c
> > > index 9f50ab9ffc..452728b54d 100644
> > > --- a/libavformat/oggparsevorbis.c
> > > +++ b/libavformat/oggparsevorbis.c
> > > @@ -293,6 +293,62 @@ static int vorbis_update_metadata(AVFormatContext
> *s, int idx)
> > > return ret;
> > > }
> > >
> > > +static int vorbis_parse_header(AVFormatContext *s, AVStream *st,
> > > + const uint8_t *p, unsigned int psize)
> > > +{
> > > + unsigned blocksize, bs0, bs1;
> > > + int srate;
> > > + int channels;
> > > +
> > > + if (psize != 30)
> > > + return AVERROR_INVALIDDATA;
> > > +
> > > + p += 7; /* skip "\001vorbis" tag */
> > > +
> > > + if (bytestream_get_le32(&p) != 0) /* vorbis_version */
> > > + return AVERROR_INVALIDDATA;
> > > +
> > > + channels = bytestream_get_byte(&p);
> > > + if (st->codecpar->ch_layout.nb_channels &&
> > > + channels != st->codecpar->ch_layout.nb_channels) {
> > > + av_log(s, AV_LOG_ERROR, "Channel change is not supported\n");
> > > + return AVERROR_PATCHWELCOME;
> > > + }
> > > + st->codecpar->ch_layout.nb_channels = channels;
> > > + srate = bytestream_get_le32(&p);
> > > + p += 4; // skip maximum bitrate
> > > + st->codecpar->bit_rate = bytestream_get_le32(&p); // nominal
> bitrate
> > > + p += 4; // skip minimum bitrate
> > > +
> > > + blocksize = bytestream_get_byte(&p);
> > > + bs0 = blocksize & 15;
> > > + bs1 = blocksize >> 4;
> > > +
> > > + if (bs0 > bs1)
> > > + return AVERROR_INVALIDDATA;
> > > + if (bs0 < 6 || bs1 > 13)
> > > + return AVERROR_INVALIDDATA;
> > > +
> > > + if (bytestream_get_byte(&p) != 1) /* framing_flag */
> > > + return AVERROR_INVALIDDATA;
> > > +
> > > + st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO;
> > > + st->codecpar->codec_id = AV_CODEC_ID_VORBIS;
> > > +
> > > + if (srate > 0) {
> > > + if (st->codecpar->sample_rate &&
> > > + srate != st->codecpar->sample_rate) {
> > > + av_log(s, AV_LOG_ERROR, "Sample rate change is not
> supported\n");
> > > + return AVERROR_PATCHWELCOME;
> > > + }
> > > +
> > > + st->codecpar->sample_rate = srate;
> > > + avpriv_set_pts_info(st, 64, 1, srate);
> > > + }
> > > +
> > > + return 1;
> > > +}
> > > +
> > > static int vorbis_header(AVFormatContext *s, int idx)
> > > {
> > > struct ogg *ogg = s->priv_data;
> > > @@ -300,6 +356,7 @@ static int vorbis_header(AVFormatContext *s, int
> idx)
> > > struct ogg_stream *os = ogg->streams + idx;
> > > struct oggvorbis_private *priv;
> > > int pkt_type = os->buf[os->pstart];
> > > + int ret;
> > >
> > > if (!os->private) {
> > > os->private = av_mallocz(sizeof(struct oggvorbis_private));
> > > @@ -327,56 +384,18 @@ static int vorbis_header(AVFormatContext *s, int
> idx)
> > >
> > > priv->len[pkt_type >> 1] = os->psize;
> > > priv->packet[pkt_type >> 1] = av_memdup(os->buf + os->pstart,
> os->psize);
> > > +
> > > if (!priv->packet[pkt_type >> 1])
> > > return AVERROR(ENOMEM);
> > > - if (os->buf[os->pstart] == 1) {
> > > - const uint8_t *p = os->buf + os->pstart + 7; /* skip
> "\001vorbis" tag */
> > > - unsigned blocksize, bs0, bs1;
> > > - int srate;
> > > - int channels;
> > > -
> > > - if (os->psize != 30)
> > > - return AVERROR_INVALIDDATA;
> > > -
> > > - if (bytestream_get_le32(&p) != 0) /* vorbis_version */
> > > - return AVERROR_INVALIDDATA;
> > > -
> > > - channels = bytestream_get_byte(&p);
> > > - if (st->codecpar->ch_layout.nb_channels &&
> > > - channels != st->codecpar->ch_layout.nb_channels) {
> > > - av_log(s, AV_LOG_ERROR, "Channel change is not
> supported\n");
> > > - return AVERROR_PATCHWELCOME;
> > > - }
> > > - st->codecpar->ch_layout.nb_channels = channels;
> > > - srate = bytestream_get_le32(&p);
> > > - p += 4; // skip maximum bitrate
> > > - st->codecpar->bit_rate = bytestream_get_le32(&p); // nominal
> bitrate
> > > - p += 4; // skip minimum bitrate
> > > -
> > > - blocksize = bytestream_get_byte(&p);
> > > - bs0 = blocksize & 15;
> > > - bs1 = blocksize >> 4;
> > > -
> > > - if (bs0 > bs1)
> > > - return AVERROR_INVALIDDATA;
> > > - if (bs0 < 6 || bs1 > 13)
> > > - return AVERROR_INVALIDDATA;
> > > -
> > > - if (bytestream_get_byte(&p) != 1) /* framing_flag */
> > > - return AVERROR_INVALIDDATA;
> > > -
> > > - st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO;
> > > - st->codecpar->codec_id = AV_CODEC_ID_VORBIS;
> > > -
> > > - if (srate > 0) {
> > > - st->codecpar->sample_rate = srate;
> > > - avpriv_set_pts_info(st, 64, 1, srate);
> > > - }
> > > - } else if (os->buf[os->pstart] == 3) {
> >
> >
> > moving code should be in a seperate patch so any changes can be clearly
> > seen in the diff which does not move the code.
> >
> > The output from:
> > libavformat/tests/seek $TICKETS/2739/lavf_ogm_seeking_borked.ogm
> >
> > chanegs by:
> > @@ -273,7 +273,7 @@
> > ret: 0 st: 2 flags:0 ts: 0.365000
> > ret: 0 st: 2 flags:1 dts: 0.332000 pts: 0.332000 pos: 18959
> size: 271
> > ret: 0 st: 2 flags:1 ts:-0.740833
> > -ret: 0 st: 2 flags:1 dts: 0.000000 pts: 0.000000 pos: 7691
> size: 2519
> > +ret: 0 st: 2 flags:1 dts:-0.002667 pts:-0.002667 pos: 14580
> size: 39
> > ret: 0 st: 3 flags:0 ts: 2.153000
> > ret: 0 st: 3 flags:1 dts: 113.023000 pts: 113.023000
> pos:25335211 size: 47
> > ret: 0 st: 3 flags:1 ts: 1.048000
> > @@ -295,7 +295,7 @@
> > ret: 0 st: 1 flags:1 ts: 0.200833
> > ret: 0 st: 1 flags:1 dts:-0.002667 pts:-0.002667 pos: 10315
> size: 32
> > ret: 0 st: 2 flags:0 ts:-0.905000
> > -ret: 0 st: 2 flags:1 dts: 0.000000 pts: 0.000000 pos: 7691
> size: 2519
> > +ret: 0 st: 2 flags:1 dts:-0.002667 pts:-0.002667 pos: 14580
> size: 39
> > ret: 0 st: 2 flags:1 ts: 1.989167
> > ret: 0 st: 2 flags:1 dts: 1.782667 pts: 1.782667 pos: 321518
> size: 241
> > ret: 0 st: 3 flags:0 ts: 0.883000
> > Command exited with non-zero status 1
> >
> > Is this correct&intended ?
>
> Sorry I thought that this sample was part of FATE but it isn't their nor is
> it linked in the corresponding ticket.
samples.ffmpeg.org/ffmpeg-bugs/trac/ticket2739/lavf_ogm_seeking_borked.ogm
>
> It would be nice if there was a definite set of samples/tests to check
> before sending patches,
yes, ideally every (or most) fixed tickets would result in a new fate test
It is something that someone should do as a STF task or as a volunteer.
Its not something i have teh time or patience for
thx
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Republics decline into democracies and democracies degenerate into
despotisms. -- Aristotle
[-- 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".
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [FFmpeg-devel] [PATCH v5 6/7] ogg/vorbis: implement header packet skip in chained ogg bitstreams.
2025-05-17 22:07 ` Michael Niedermayer
@ 2025-05-19 14:40 ` Romain Beauxis
0 siblings, 0 replies; 16+ messages in thread
From: Romain Beauxis @ 2025-05-19 14:40 UTC (permalink / raw)
To: FFmpeg development discussions and patches
Le sam. 17 mai 2025 à 17:07, Michael Niedermayer
<michael@niedermayer.cc> a écrit :
>
> On Sat, May 17, 2025 at 01:10:26PM -0500, Romain Beauxis wrote:
> > Le mar. 13 mai 2025 à 14:23, Michael Niedermayer <michael@niedermayer.cc> a
> > écrit :
> > >
> > > On Fri, May 09, 2025 at 06:43:26PM -0500, Romain Beauxis wrote:
> > > > ---
> > > > libavcodec/vorbisdec.c | 37 +----
> > > > libavformat/oggparsevorbis.c | 174 +++++++++++++--------
> > > > tests/ref/fate/ogg-vorbis-chained-meta.txt | 3 -
> > > > 3 files changed, 117 insertions(+), 97 deletions(-)
> > > >
> > > > diff --git a/libavcodec/vorbisdec.c b/libavcodec/vorbisdec.c
> > > > index a778dc6b58..f069ac6ab3 100644
> > > > --- a/libavcodec/vorbisdec.c
> > > > +++ b/libavcodec/vorbisdec.c
> > > > @@ -1776,39 +1776,17 @@ static int vorbis_decode_frame(AVCodecContext
> > *avctx, AVFrame *frame,
> > > > GetBitContext *gb = &vc->gb;
> > > > float *channel_ptrs[255];
> > > > int i, len, ret;
> > > > + const int8_t *new_extradata;
> > > > + size_t new_extradata_size;
> > > >
> > > > ff_dlog(NULL, "packet length %d \n", buf_size);
> > > >
> > > > - if (*buf == 1 && buf_size > 7) {
> > > > - if ((ret = init_get_bits8(gb, buf + 1, buf_size - 1)) < 0)
> > > > - return ret;
> > > > -
> > > > - vorbis_free(vc);
> > > > - if ((ret = vorbis_parse_id_hdr(vc))) {
> > > > - av_log(avctx, AV_LOG_ERROR, "Id header corrupt.\n");
> > > > - vorbis_free(vc);
> > > > - return ret;
> > > > - }
> > > > -
> > > > - av_channel_layout_uninit(&avctx->ch_layout);
> > > > - if (vc->audio_channels > 8) {
> > > > - avctx->ch_layout.order = AV_CHANNEL_ORDER_UNSPEC;
> > > > - avctx->ch_layout.nb_channels = vc->audio_channels;
> > > > - } else {
> > > > - av_channel_layout_copy(&avctx->ch_layout,
> > &ff_vorbis_ch_layouts[vc->audio_channels - 1]);
> > > > - }
> > > > -
> > > > - avctx->sample_rate = vc->audio_samplerate;
> > > > - return buf_size;
> > > > - }
> > > > -
> > > > - if (*buf == 3 && buf_size > 7) {
> > > > - av_log(avctx, AV_LOG_DEBUG, "Ignoring comment header\n");
> > > > - return buf_size;
> > > > - }
> > > > + new_extradata = av_packet_get_side_data(avpkt,
> > AV_PKT_DATA_NEW_EXTRADATA,
> > > > + &new_extradata_size);
> > > >
> > > > - if (*buf == 5 && buf_size > 7 && vc->channel_residues &&
> > !vc->modes) {
> > > > - if ((ret = init_get_bits8(gb, buf + 1, buf_size - 1)) < 0)
> > > > + if (new_extradata && *new_extradata == 5 && new_extradata_size > 7
> > &&
> > > > + vc->channel_residues && !vc->modes) {
> > > > + if ((ret = init_get_bits8(gb, new_extradata + 1,
> > new_extradata_size - 1)) < 0)
> > > > return ret;
> > > >
> > > > if ((ret = vorbis_parse_setup_hdr(vc))) {
> > > > @@ -1816,7 +1794,6 @@ static int vorbis_decode_frame(AVCodecContext
> > *avctx, AVFrame *frame,
> > > > vorbis_free(vc);
> > > > return ret;
> > > > }
> > > > - return buf_size;
> > > > }
> > > >
> > > > if (!vc->channel_residues || !vc->modes) {
> > > > diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c
> > > > index 9f50ab9ffc..452728b54d 100644
> > > > --- a/libavformat/oggparsevorbis.c
> > > > +++ b/libavformat/oggparsevorbis.c
> > > > @@ -293,6 +293,62 @@ static int vorbis_update_metadata(AVFormatContext
> > *s, int idx)
> > > > return ret;
> > > > }
> > > >
> > > > +static int vorbis_parse_header(AVFormatContext *s, AVStream *st,
> > > > + const uint8_t *p, unsigned int psize)
> > > > +{
> > > > + unsigned blocksize, bs0, bs1;
> > > > + int srate;
> > > > + int channels;
> > > > +
> > > > + if (psize != 30)
> > > > + return AVERROR_INVALIDDATA;
> > > > +
> > > > + p += 7; /* skip "\001vorbis" tag */
> > > > +
> > > > + if (bytestream_get_le32(&p) != 0) /* vorbis_version */
> > > > + return AVERROR_INVALIDDATA;
> > > > +
> > > > + channels = bytestream_get_byte(&p);
> > > > + if (st->codecpar->ch_layout.nb_channels &&
> > > > + channels != st->codecpar->ch_layout.nb_channels) {
> > > > + av_log(s, AV_LOG_ERROR, "Channel change is not supported\n");
> > > > + return AVERROR_PATCHWELCOME;
> > > > + }
> > > > + st->codecpar->ch_layout.nb_channels = channels;
> > > > + srate = bytestream_get_le32(&p);
> > > > + p += 4; // skip maximum bitrate
> > > > + st->codecpar->bit_rate = bytestream_get_le32(&p); // nominal
> > bitrate
> > > > + p += 4; // skip minimum bitrate
> > > > +
> > > > + blocksize = bytestream_get_byte(&p);
> > > > + bs0 = blocksize & 15;
> > > > + bs1 = blocksize >> 4;
> > > > +
> > > > + if (bs0 > bs1)
> > > > + return AVERROR_INVALIDDATA;
> > > > + if (bs0 < 6 || bs1 > 13)
> > > > + return AVERROR_INVALIDDATA;
> > > > +
> > > > + if (bytestream_get_byte(&p) != 1) /* framing_flag */
> > > > + return AVERROR_INVALIDDATA;
> > > > +
> > > > + st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO;
> > > > + st->codecpar->codec_id = AV_CODEC_ID_VORBIS;
> > > > +
> > > > + if (srate > 0) {
> > > > + if (st->codecpar->sample_rate &&
> > > > + srate != st->codecpar->sample_rate) {
> > > > + av_log(s, AV_LOG_ERROR, "Sample rate change is not
> > supported\n");
> > > > + return AVERROR_PATCHWELCOME;
> > > > + }
> > > > +
> > > > + st->codecpar->sample_rate = srate;
> > > > + avpriv_set_pts_info(st, 64, 1, srate);
> > > > + }
> > > > +
> > > > + return 1;
> > > > +}
> > > > +
> > > > static int vorbis_header(AVFormatContext *s, int idx)
> > > > {
> > > > struct ogg *ogg = s->priv_data;
> > > > @@ -300,6 +356,7 @@ static int vorbis_header(AVFormatContext *s, int
> > idx)
> > > > struct ogg_stream *os = ogg->streams + idx;
> > > > struct oggvorbis_private *priv;
> > > > int pkt_type = os->buf[os->pstart];
> > > > + int ret;
> > > >
> > > > if (!os->private) {
> > > > os->private = av_mallocz(sizeof(struct oggvorbis_private));
> > > > @@ -327,56 +384,18 @@ static int vorbis_header(AVFormatContext *s, int
> > idx)
> > > >
> > > > priv->len[pkt_type >> 1] = os->psize;
> > > > priv->packet[pkt_type >> 1] = av_memdup(os->buf + os->pstart,
> > os->psize);
> > > > +
> > > > if (!priv->packet[pkt_type >> 1])
> > > > return AVERROR(ENOMEM);
> > > > - if (os->buf[os->pstart] == 1) {
> > > > - const uint8_t *p = os->buf + os->pstart + 7; /* skip
> > "\001vorbis" tag */
> > > > - unsigned blocksize, bs0, bs1;
> > > > - int srate;
> > > > - int channels;
> > > > -
> > > > - if (os->psize != 30)
> > > > - return AVERROR_INVALIDDATA;
> > > > -
> > > > - if (bytestream_get_le32(&p) != 0) /* vorbis_version */
> > > > - return AVERROR_INVALIDDATA;
> > > > -
> > > > - channels = bytestream_get_byte(&p);
> > > > - if (st->codecpar->ch_layout.nb_channels &&
> > > > - channels != st->codecpar->ch_layout.nb_channels) {
> > > > - av_log(s, AV_LOG_ERROR, "Channel change is not
> > supported\n");
> > > > - return AVERROR_PATCHWELCOME;
> > > > - }
> > > > - st->codecpar->ch_layout.nb_channels = channels;
> > > > - srate = bytestream_get_le32(&p);
> > > > - p += 4; // skip maximum bitrate
> > > > - st->codecpar->bit_rate = bytestream_get_le32(&p); // nominal
> > bitrate
> > > > - p += 4; // skip minimum bitrate
> > > > -
> > > > - blocksize = bytestream_get_byte(&p);
> > > > - bs0 = blocksize & 15;
> > > > - bs1 = blocksize >> 4;
> > > > -
> > > > - if (bs0 > bs1)
> > > > - return AVERROR_INVALIDDATA;
> > > > - if (bs0 < 6 || bs1 > 13)
> > > > - return AVERROR_INVALIDDATA;
> > > > -
> > > > - if (bytestream_get_byte(&p) != 1) /* framing_flag */
> > > > - return AVERROR_INVALIDDATA;
> > > > -
> > > > - st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO;
> > > > - st->codecpar->codec_id = AV_CODEC_ID_VORBIS;
> > > > -
> > > > - if (srate > 0) {
> > > > - st->codecpar->sample_rate = srate;
> > > > - avpriv_set_pts_info(st, 64, 1, srate);
> > > > - }
> > > > - } else if (os->buf[os->pstart] == 3) {
> > >
> > >
> > > moving code should be in a seperate patch so any changes can be clearly
> > > seen in the diff which does not move the code.
> > >
> > > The output from:
> > > libavformat/tests/seek $TICKETS/2739/lavf_ogm_seeking_borked.ogm
> > >
> > > chanegs by:
> > > @@ -273,7 +273,7 @@
> > > ret: 0 st: 2 flags:0 ts: 0.365000
> > > ret: 0 st: 2 flags:1 dts: 0.332000 pts: 0.332000 pos: 18959
> > size: 271
> > > ret: 0 st: 2 flags:1 ts:-0.740833
> > > -ret: 0 st: 2 flags:1 dts: 0.000000 pts: 0.000000 pos: 7691
> > size: 2519
> > > +ret: 0 st: 2 flags:1 dts:-0.002667 pts:-0.002667 pos: 14580
> > size: 39
> > > ret: 0 st: 3 flags:0 ts: 2.153000
> > > ret: 0 st: 3 flags:1 dts: 113.023000 pts: 113.023000
> > pos:25335211 size: 47
> > > ret: 0 st: 3 flags:1 ts: 1.048000
> > > @@ -295,7 +295,7 @@
> > > ret: 0 st: 1 flags:1 ts: 0.200833
> > > ret: 0 st: 1 flags:1 dts:-0.002667 pts:-0.002667 pos: 10315
> > size: 32
> > > ret: 0 st: 2 flags:0 ts:-0.905000
> > > -ret: 0 st: 2 flags:1 dts: 0.000000 pts: 0.000000 pos: 7691
> > size: 2519
> > > +ret: 0 st: 2 flags:1 dts:-0.002667 pts:-0.002667 pos: 14580
> > size: 39
> > > ret: 0 st: 2 flags:1 ts: 1.989167
> > > ret: 0 st: 2 flags:1 dts: 1.782667 pts: 1.782667 pos: 321518
> > size: 241
> > > ret: 0 st: 3 flags:0 ts: 0.883000
> > > Command exited with non-zero status 1
> > >
> > > Is this correct&intended ?
> >
> > Sorry I thought that this sample was part of FATE but it isn't their nor is
> > it linked in the corresponding ticket.
>
> samples.ffmpeg.org/ffmpeg-bugs/trac/ticket2739/lavf_ogm_seeking_borked.ogm
Thanks. I just had a look. It's hard to know what's really going on
because this stream is highly invalid:
[ogg @ 0x12d804080] Headers mismatch for stream 1: expected 3 received 2.
[ogg @ 0x12d804080] Headers mismatch for stream 2: expected 3 received 2.
The logic for packet passing is changed with this patch series and new
headers from subsequent chained streams are only passed with the first
data packet following the headers.
I haven't debugged down to the de-packetization but, a typical change
would be: if the stream is invalid and has a series of headers without
any data packet in-between, then the decoder would never see the
intermediate headers with the new changes.
However, I have rewritten my changes to actually pass both header and
setup packet as extradata. This way, the decoder will see whatever we
are able to pass as header and setup packet from the parser.
This should make sure that we are sticking as close as possible to the
existing behavior.
The seek diff is still the same with it but that makes me more
confident that this is, indeed, intended.
-- Romain
> >
> > It would be nice if there was a definite set of samples/tests to check
> > before sending patches,
>
> yes, ideally every (or most) fixed tickets would result in a new fate test
>
> It is something that someone should do as a STF task or as a volunteer.
> Its not something i have teh time or patience for
>
> thx
>
> [...]
>
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Republics decline into democracies and democracies degenerate into
> despotisms. -- Aristotle
> _______________________________________________
> 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".
^ permalink raw reply [flat|nested] 16+ messages in thread
* [FFmpeg-devel] [PATCH v5 7/7] libavformat/oggdec.h: Change paket function documentation to return 1 on header packets only.
2025-05-09 23:43 [FFmpeg-devel] [PATCH v5 0/7] Remove chained ogg stream header packets from the demuxer Romain Beauxis
` (5 preceding siblings ...)
2025-05-09 23:43 ` [FFmpeg-devel] [PATCH v5 6/7] ogg/vorbis: implement header packet skip in chained ogg bitstreams Romain Beauxis
@ 2025-05-09 23:43 ` Romain Beauxis
6 siblings, 0 replies; 16+ messages in thread
From: Romain Beauxis @ 2025-05-09 23:43 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: Romain Beauxis
---
libavformat/oggdec.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/libavformat/oggdec.h b/libavformat/oggdec.h
index 5083de646c..c15fbe738e 100644
--- a/libavformat/oggdec.h
+++ b/libavformat/oggdec.h
@@ -42,8 +42,8 @@ struct ogg_codec {
* 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())
+ * == 1 if the packet was a header from a chained bitstream.
+ * This will cause the packet to be skiped in calling code (ogg_packet()
*/
int (*packet)(AVFormatContext *, int);
/**
--
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".
^ permalink raw reply [flat|nested] 16+ messages in thread