* [FFmpeg-devel] [PATCH v8 1/5] Add FATE test for sample from track ticket #2739
2025-05-24 18:14 [FFmpeg-devel] [PATCH v8 0/5] Remove chained ogg stream header packets from the demuxer Romain Beauxis
@ 2025-05-24 18:14 ` Romain Beauxis
2025-05-26 18:38 ` Michael Niedermayer
2025-05-24 18:14 ` [FFmpeg-devel] [PATCH v8 2/5] libavformat/oggdec.{c, h}: Add new_extradata, use it to pass extradata to the next decoded packet Romain Beauxis
` (3 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Romain Beauxis @ 2025-05-24 18:14 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: samples-request, Romain Beauxis
Sample available at: https://www.dropbox.com/scl/fo/xrtrna2rxr1j354hrtymq/AGwemlxHYecBLNmQ8Fsy--4?rlkey=lzilr4m9w4gfdqygoe172vvy8&dl=0
---
tests/Makefile | 2 ++
tests/fate/trac.mak | 11 ++++++++
tests/ref/fate/trac-2739.txt | 53 ++++++++++++++++++++++++++++++++++++
3 files changed, 66 insertions(+)
create mode 100644 tests/fate/trac.mak
create mode 100644 tests/ref/fate/trac-2739.txt
diff --git a/tests/Makefile b/tests/Makefile
index 505d7f9c6d..1e32020eb0 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -241,6 +241,7 @@ include $(SRC_PATH)/tests/fate/source.mak
include $(SRC_PATH)/tests/fate/spdif.mak
include $(SRC_PATH)/tests/fate/speedhq.mak
include $(SRC_PATH)/tests/fate/subtitles.mak
+include $(SRC_PATH)/tests/fate/trac.mak
include $(SRC_PATH)/tests/fate/truehd.mak
include $(SRC_PATH)/tests/fate/utvideo.mak
include $(SRC_PATH)/tests/fate/vbn.mak
@@ -282,6 +283,7 @@ $(FATE_SAMPLES_FASTSTART): tools/qt-faststart$(EXESUF)
$(FATE_SAMPLES_DUMP_DATA) $(FATE_SAMPLES_DUMP_DATA-yes): tools/venc_data_dump$(EXESUF)
$(FATE_SAMPLES_SCALE_SLICE): tools/scale_slice_test$(EXESUF)
$(FATE_SAMPLES_DUMP_STREAM_META): tests/api/api-dump-stream-meta-test$(EXESUF)
+$(FATE_AVFORMAT_TESTS_SEEK): libavformat/tests/seek$(EXESUF)
ifdef SAMPLES
FATE += $(FATE_EXTERN)
diff --git a/tests/fate/trac.mak b/tests/fate/trac.mak
new file mode 100644
index 0000000000..32c5b21cd7
--- /dev/null
+++ b/tests/fate/trac.mak
@@ -0,0 +1,11 @@
+FATE_TRAC_2739 += fate-trac-2739-borked-seek
+fate-trac-2739-borked-seek: REF = $(SRC_PATH)/tests/ref/fate/trac-2739.txt
+fate-trac-2739-borked-seek: CMD = run $(SRC_PATH)/libavformat/tests/seek$(EXESUF) $(TARGET_SAMPLES)/trac/2739/lavf_ogm_seeking_borked.ogm
+
+FATE_TRAC_2739-$(call DEMDEC, OGG, VORBIS) += $(FATE_TRAC_2739)
+
+FATE_AVFORMAT_TESTS_SEEK += $(FATE_TRAC_2739-yes)
+
+FATE_EXTERN += $(FATE_TRAC_2739-yes)
+
+fate-trac-2739: $(FATE_TRAC_2739-yes)
diff --git a/tests/ref/fate/trac-2739.txt b/tests/ref/fate/trac-2739.txt
new file mode 100644
index 0000000000..86d339c880
--- /dev/null
+++ b/tests/ref/fate/trac-2739.txt
@@ -0,0 +1,53 @@
+ret: 0 st: 0 flags:1 dts:-0.083417 pts: 0.000000 pos: 887 size: 3347
+ret: 0 st:-1 flags:0 ts:-1.000000
+ret: 0 st: 0 flags:1 dts: 0.000000 pts: 0.000000 pos: 887 size: 3347
+ret: 0 st:-1 flags:1 ts: 1.894167
+ret: 0 st: 0 flags:1 dts: 0.000000 pts: 0.000000 pos: 887 size: 3347
+ret: 0 st: 0 flags:0 ts: 0.792458
+ret: 0 st: 1 flags:1 dts: 12.481333 pts: 12.481333 pos:2855209 size: 269
+ret: 0 st: 0 flags:1 ts:-0.333666
+ret: 0 st: 0 flags:1 dts: 0.000000 pts: 0.000000 pos: 887 size: 3347
+ret: 0 st: 1 flags:0 ts: 2.576667
+ret: 0 st: 1 flags:1 dts: 2.785333 pts: 2.785333 pos: 559697 size: 255
+ret: 0 st: 1 flags:1 ts: 1.470833
+ret: 0 st: 1 flags:1 dts: 1.164000 pts: 1.164000 pos: 192278 size: 275
+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: 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
+ret: 0 st: 3 flags:1 dts: 0.000000 pts: 0.000000 pos: 10247 size: 2
+ret: 0 st: 4 flags:0 ts:-0.058000
+ret: 0 st: 4 flags:1 dts: 0.000000 pts: 0.000000 pos: 10281 size: 2
+ret: 0 st: 4 flags:1 ts: 2.836000
+ret: 0 st: 4 flags:1 dts: 0.000000 pts: 0.000000 pos: 10281 size: 2
+ret: 0 st:-1 flags:0 ts: 1.730004
+ret: 0 st: 1 flags:1 dts: 12.481333 pts: 12.481333 pos:2855209 size: 269
+ret: 0 st:-1 flags:1 ts: 0.624171
+ret: 0 st: 0 flags:1 dts: 0.000000 pts: 0.000000 pos: 887 size: 3347
+ret: 0 st: 0 flags:0 ts:-0.500500
+ret: 0 st: 0 flags:1 dts: 0.000000 pts: 0.000000 pos: 887 size: 3347
+ret: 0 st: 0 flags:1 ts: 2.419081
+ret: 0 st: 0 flags:1 dts: 0.000000 pts: 0.000000 pos: 887 size: 3347
+ret: 0 st: 1 flags:0 ts: 1.306667
+ret: 0 st: 1 flags:1 dts: 1.164000 pts: 1.164000 pos: 192278 size: 275
+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 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
+ret: 0 st: 3 flags:1 dts: 113.023000 pts: 113.023000 pos:25335211 size: 47
+ret: 0 st: 3 flags:1 ts:-0.222000
+ret: 0 st: 3 flags:1 dts: 0.000000 pts: 0.000000 pos: 10247 size: 2
+ret: 0 st: 4 flags:0 ts: 2.672000
+ret: 0 st: 4 flags:1 dts: 113.023000 pts: 113.023000 pos:25335289 size: 38
+ret: 0 st: 4 flags:1 ts: 1.566000
+ret: 0 st: 4 flags:1 dts: 0.000000 pts: 0.000000 pos: 10281 size: 2
+ret: 0 st:-1 flags:0 ts: 0.460008
+ret: 0 st: 1 flags:1 dts: 12.481333 pts: 12.481333 pos:2855209 size: 269
+ret: 0 st:-1 flags:1 ts:-0.645825
+ret: 0 st: 0 flags:1 dts: 0.000000 pts: 0.000000 pos: 887 size: 3347
--
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] 14+ messages in thread* Re: [FFmpeg-devel] [PATCH v8 1/5] Add FATE test for sample from track ticket #2739
2025-05-24 18:14 ` [FFmpeg-devel] [PATCH v8 1/5] Add FATE test for sample from track ticket #2739 Romain Beauxis
@ 2025-05-26 18:38 ` Michael Niedermayer
2025-05-26 22:28 ` Romain Beauxis
0 siblings, 1 reply; 14+ messages in thread
From: Michael Niedermayer @ 2025-05-26 18:38 UTC (permalink / raw)
To: FFmpeg development discussions and patches
[-- Attachment #1.1: Type: text/plain, Size: 655 bytes --]
On Sat, May 24, 2025 at 01:14:04PM -0500, Romain Beauxis wrote:
> Sample available at: https://www.dropbox.com/scl/fo/xrtrna2rxr1j354hrtymq/AGwemlxHYecBLNmQ8Fsy--4?rlkey=lzilr4m9w4gfdqygoe172vvy8&dl=0
that file is too big for fate IMHO
du -m trac/2739/lavf_ogm_seeking_borked.ogm
231 trac/2739/lavf_ogm_seeking_borked.ogm
can it be made (alot) smaller and still test whats relevant ?
thx
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
In fact, the RIAA has been known to suggest that students drop out
of college or go to community college in order to be able to afford
settlements. -- The RIAA
[-- 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] 14+ messages in thread
* Re: [FFmpeg-devel] [PATCH v8 1/5] Add FATE test for sample from track ticket #2739
2025-05-26 18:38 ` Michael Niedermayer
@ 2025-05-26 22:28 ` Romain Beauxis
2025-05-30 20:06 ` Michael Niedermayer
0 siblings, 1 reply; 14+ messages in thread
From: Romain Beauxis @ 2025-05-26 22:28 UTC (permalink / raw)
To: FFmpeg development discussions and patches
Le lun. 26 mai 2025 à 13:38, Michael Niedermayer <michael@niedermayer.cc> a
écrit :
>
> On Sat, May 24, 2025 at 01:14:04PM -0500, Romain Beauxis wrote:
> > Sample available at:
https://www.dropbox.com/scl/fo/xrtrna2rxr1j354hrtymq/AGwemlxHYecBLNmQ8Fsy--4?rlkey=lzilr4m9w4gfdqygoe172vvy8&dl=0
>
> that file is too big for fate IMHO
>
> du -m trac/2739/lavf_ogm_seeking_borked.ogm
> 231 trac/2739/lavf_ogm_seeking_borked.ogm
>
> can it be made (alot) smaller and still test whats relevant ?
Sure thing.
In this case, would you mind having a look at the rest of the patchset
first?
I wouldn't want this new test to hold up the rest of the work. All you'd
have to do is ignore the diff related to this test in the penultimate patch.
Thanks,
-- Romain
> thx
>
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> In fact, the RIAA has been known to suggest that students drop out
> of college or go to community college in order to be able to afford
> settlements. -- The RIAA
> _______________________________________________
> 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] 14+ messages in thread
* Re: [FFmpeg-devel] [PATCH v8 1/5] Add FATE test for sample from track ticket #2739
2025-05-26 22:28 ` Romain Beauxis
@ 2025-05-30 20:06 ` Michael Niedermayer
2025-05-31 19:33 ` Romain Beauxis
0 siblings, 1 reply; 14+ messages in thread
From: Michael Niedermayer @ 2025-05-30 20:06 UTC (permalink / raw)
To: FFmpeg development discussions and patches
[-- Attachment #1.1: Type: text/plain, Size: 1125 bytes --]
Hi
On Mon, May 26, 2025 at 05:28:50PM -0500, Romain Beauxis wrote:
> Le lun. 26 mai 2025 à 13:38, Michael Niedermayer <michael@niedermayer.cc> a
> écrit :
> >
> > On Sat, May 24, 2025 at 01:14:04PM -0500, Romain Beauxis wrote:
> > > Sample available at:
> https://www.dropbox.com/scl/fo/xrtrna2rxr1j354hrtymq/AGwemlxHYecBLNmQ8Fsy--4?rlkey=lzilr4m9w4gfdqygoe172vvy8&dl=0
> >
> > that file is too big for fate IMHO
> >
> > du -m trac/2739/lavf_ogm_seeking_borked.ogm
> > 231 trac/2739/lavf_ogm_seeking_borked.ogm
> >
> > can it be made (alot) smaller and still test whats relevant ?
>
> Sure thing.
>
> In this case, would you mind having a look at the rest of the patchset
> first?
>
> I wouldn't want this new test to hold up the rest of the work. All you'd
> have to do is ignore the diff related to this test in the penultimate patch.
will apply teh rest of the patchset
thx
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- 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] 14+ messages in thread
* Re: [FFmpeg-devel] [PATCH v8 1/5] Add FATE test for sample from track ticket #2739
2025-05-30 20:06 ` Michael Niedermayer
@ 2025-05-31 19:33 ` Romain Beauxis
0 siblings, 0 replies; 14+ messages in thread
From: Romain Beauxis @ 2025-05-31 19:33 UTC (permalink / raw)
To: FFmpeg development discussions and patches
Le ven. 30 mai 2025 à 15:07, Michael Niedermayer
<michael@niedermayer.cc> a écrit :
>
> Hi
>
> On Mon, May 26, 2025 at 05:28:50PM -0500, Romain Beauxis wrote:
> > Le lun. 26 mai 2025 à 13:38, Michael Niedermayer <michael@niedermayer.cc> a
> > écrit :
> > >
> > > On Sat, May 24, 2025 at 01:14:04PM -0500, Romain Beauxis wrote:
> > > > Sample available at:
> > https://www.dropbox.com/scl/fo/xrtrna2rxr1j354hrtymq/AGwemlxHYecBLNmQ8Fsy--4?rlkey=lzilr4m9w4gfdqygoe172vvy8&dl=0
> > >
> > > that file is too big for fate IMHO
> > >
> > > du -m trac/2739/lavf_ogm_seeking_borked.ogm
> > > 231 trac/2739/lavf_ogm_seeking_borked.ogm
> > >
> > > can it be made (alot) smaller and still test whats relevant ?
> >
> > Sure thing.
> >
> > In this case, would you mind having a look at the rest of the patchset
> > first?
> >
> > I wouldn't want this new test to hold up the rest of the work. All you'd
> > have to do is ignore the diff related to this test in the penultimate patch.
>
> will apply teh rest of the patchset
Thanks! I'll resume the work on this text and the rest of the patchset soon.
Appreciate the patience and guidance.
-- Romain
> thx
>
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> it is not once nor twice but times without number that the same ideas make
> their appearance in the world. -- 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] 14+ messages in thread
* [FFmpeg-devel] [PATCH v8 2/5] libavformat/oggdec.{c, h}: Add new_extradata, use it to pass extradata to the next decoded packet.
2025-05-24 18:14 [FFmpeg-devel] [PATCH v8 0/5] Remove chained ogg stream header packets from the demuxer Romain Beauxis
2025-05-24 18:14 ` [FFmpeg-devel] [PATCH v8 1/5] Add FATE test for sample from track ticket #2739 Romain Beauxis
@ 2025-05-24 18:14 ` Romain Beauxis
2025-05-24 18:14 ` [FFmpeg-devel] [PATCH v8 3/5] ogg/vorbis: factor out header processing logic Romain Beauxis
` (2 subsequent siblings)
4 siblings, 0 replies; 14+ messages in thread
From: Romain Beauxis @ 2025-05-24 18:14 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..da3ef815db 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] 14+ messages in thread* [FFmpeg-devel] [PATCH v8 3/5] ogg/vorbis: factor out header processing logic.
2025-05-24 18:14 [FFmpeg-devel] [PATCH v8 0/5] Remove chained ogg stream header packets from the demuxer Romain Beauxis
2025-05-24 18:14 ` [FFmpeg-devel] [PATCH v8 1/5] Add FATE test for sample from track ticket #2739 Romain Beauxis
2025-05-24 18:14 ` [FFmpeg-devel] [PATCH v8 2/5] libavformat/oggdec.{c, h}: Add new_extradata, use it to pass extradata to the next decoded packet Romain Beauxis
@ 2025-05-24 18:14 ` Romain Beauxis
2025-05-24 18:14 ` [FFmpeg-devel] [PATCH v8 4/5] ogg/vorbis: implement header packet skip in chained ogg bitstreams Romain Beauxis
2025-05-24 18:14 ` [FFmpeg-devel] [PATCH v8 5/5] libavformat/oggdec.h: Change paket function documentation to return 1 on header packets only Romain Beauxis
4 siblings, 0 replies; 14+ messages in thread
From: Romain Beauxis @ 2025-05-24 18:14 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: Romain Beauxis
---
libavformat/oggparsevorbis.c | 104 ++++++++++++++++++++---------------
1 file changed, 60 insertions(+), 44 deletions(-)
diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c
index 9f50ab9ffc..62cc2da6de 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;
@@ -329,50 +385,10 @@ static int vorbis_header(AVFormatContext *s, int idx)
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;
--
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] 14+ messages in thread* [FFmpeg-devel] [PATCH v8 4/5] ogg/vorbis: implement header packet skip in chained ogg bitstreams.
2025-05-24 18:14 [FFmpeg-devel] [PATCH v8 0/5] Remove chained ogg stream header packets from the demuxer Romain Beauxis
` (2 preceding siblings ...)
2025-05-24 18:14 ` [FFmpeg-devel] [PATCH v8 3/5] ogg/vorbis: factor out header processing logic Romain Beauxis
@ 2025-05-24 18:14 ` Romain Beauxis
2025-05-31 0:44 ` Andreas Rheinhardt
2025-05-24 18:14 ` [FFmpeg-devel] [PATCH v8 5/5] libavformat/oggdec.h: Change paket function documentation to return 1 on header packets only Romain Beauxis
4 siblings, 1 reply; 14+ messages in thread
From: Romain Beauxis @ 2025-05-24 18:14 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: Romain Beauxis
---
libavcodec/vorbis_parser.h | 11 ++++
libavcodec/vorbisdec.c | 75 +++++++++++++---------
libavformat/oggparsevorbis.c | 67 ++++++++++++++++++-
tests/ref/fate/ogg-vorbis-chained-meta.txt | 3 -
tests/ref/fate/trac-2739.txt | 4 +-
5 files changed, 121 insertions(+), 39 deletions(-)
diff --git a/libavcodec/vorbis_parser.h b/libavcodec/vorbis_parser.h
index 789932ac49..b176fe536c 100644
--- a/libavcodec/vorbis_parser.h
+++ b/libavcodec/vorbis_parser.h
@@ -30,6 +30,17 @@
typedef struct AVVorbisParseContext AVVorbisParseContext;
+/**
+ * Used by the vorbis parser to pass new chained stream headers
+ * as extradata.
+ */
+typedef struct vorbis_new_extradata {
+ uint8_t *header;
+ size_t header_size;
+ uint8_t *setup;
+ size_t setup_size;
+} vorbis_new_extradata;
+
/**
* Allocate and initialize the Vorbis parser using headers in the extradata.
*/
diff --git a/libavcodec/vorbisdec.c b/libavcodec/vorbisdec.c
index adbd726183..a4b159ba9b 100644
--- a/libavcodec/vorbisdec.c
+++ b/libavcodec/vorbisdec.c
@@ -43,6 +43,7 @@
#include "vorbis.h"
#include "vorbisdsp.h"
#include "vorbis_data.h"
+#include "vorbis_parser.h"
#include "xiph.h"
#define V_NB_BITS 8
@@ -1778,47 +1779,59 @@ static int vorbis_decode_frame(AVCodecContext *avctx, AVFrame *frame,
GetBitContext *gb = &vc->gb;
float *channel_ptrs[255];
int i, len, ret;
+ size_t new_extradata_size;
+ vorbis_new_extradata *new_extradata;
+ const uint8_t *header;
+ const uint8_t *setup;
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;
+ new_extradata = (vorbis_new_extradata *)av_packet_get_side_data(
+ avpkt, AV_PKT_DATA_NEW_EXTRADATA, &new_extradata_size);
- 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;
- }
+ if (new_extradata) {
+ header = new_extradata->header;
+ setup = new_extradata->setup;
- 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]);
- }
+ if (new_extradata->header_size > 7 && *header == 1) {
+ if ((ret = init_get_bits8(
+ gb, header + 1,
+ new_extradata->header_size - 1)) < 0)
+ return ret;
- avctx->sample_rate = vc->audio_samplerate;
- return buf_size;
- }
+ 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;
+ }
- if (*buf == 3 && buf_size > 7) {
- av_log(avctx, AV_LOG_DEBUG, "Ignoring comment header\n");
- return buf_size;
- }
+ 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]);
+ }
- if (*buf == 5 && buf_size > 7 && vc->channel_residues && !vc->modes) {
- if ((ret = init_get_bits8(gb, buf + 1, buf_size - 1)) < 0)
- return ret;
+ avctx->sample_rate = vc->audio_samplerate;
+ }
- if ((ret = vorbis_parse_setup_hdr(vc))) {
- av_log(avctx, AV_LOG_ERROR, "Setup header corrupt.\n");
- vorbis_free(vc);
- return ret;
+ if (new_extradata->setup_size > 7 && *setup == 5 &&
+ vc->channel_residues && !vc->modes) {
+ if ((ret = init_get_bits8(
+ gb, setup + 1,
+ new_extradata->setup_size - 1)) < 0)
+ return ret;
+
+ if ((ret = vorbis_parse_setup_hdr(vc))) {
+ av_log(avctx, AV_LOG_ERROR, "Setup header corrupt.\n");
+ 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 62cc2da6de..f8e66e8127 100644
--- a/libavformat/oggparsevorbis.c
+++ b/libavformat/oggparsevorbis.c
@@ -255,12 +255,19 @@ static void vorbis_cleanup(AVFormatContext *s, int idx)
struct ogg *ogg = s->priv_data;
struct ogg_stream *os = ogg->streams + idx;
struct oggvorbis_private *priv = os->private;
+ vorbis_new_extradata *new_extradata;
int i;
if (os->private) {
av_vorbis_parse_free(&priv->vp);
for (i = 0; i < 3; i++)
av_freep(&priv->packet[i]);
}
+
+ if (os->new_extradata) {
+ new_extradata = (vorbis_new_extradata *)os->new_extradata;
+ av_freep(&new_extradata->header);
+ av_freep(&new_extradata->setup);
+ }
}
static int vorbis_update_metadata(AVFormatContext *s, int idx)
@@ -433,7 +440,10 @@ static int vorbis_packet(AVFormatContext *s, int idx)
struct ogg *ogg = s->priv_data;
struct ogg_stream *os = ogg->streams + idx;
struct oggvorbis_private *priv = os->private;
+ vorbis_new_extradata *new_extradata;
int duration, flags = 0;
+ int skip_packet = 0;
+ int ret;
if (!priv->vp)
return AVERROR_INVALIDDATA;
@@ -496,10 +506,61 @@ 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;
+
+ if (!os->new_extradata) {
+ os->new_extradata = av_mallocz(sizeof(vorbis_new_extradata));
+ if (!os->new_extradata)
+ return AVERROR(ENOMEM);
+ }
+
+ os->new_extradata_size = sizeof(vorbis_new_extradata);
+ new_extradata = (vorbis_new_extradata *)os->new_extradata;
+
+ ret = av_reallocp(&new_extradata->header, os->psize);
+ if (ret < 0)
+ return ret;
+
+ memcpy(new_extradata->header, os->buf + os->pstart, os->psize);
+ new_extradata->header_size = os->psize;
+
+ 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) {
+ if (!os->new_extradata) {
+ os->new_extradata = av_mallocz(sizeof(vorbis_new_extradata));
+ if (!os->new_extradata)
+ return AVERROR(ENOMEM);
+ }
+
+ os->new_extradata_size = sizeof(vorbis_new_extradata);
+ new_extradata = (vorbis_new_extradata *)os->new_extradata;
+
+ ret = av_reallocp(&new_extradata->setup, os->psize);
+ if (ret < 0)
+ return ret;
+
+ memcpy(new_extradata->setup, os->buf + os->pstart, os->psize);
+ new_extradata->setup_size = os->psize;
+
+ skip_packet = 1;
}
+
os->pduration = duration;
}
@@ -521,7 +582,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
diff --git a/tests/ref/fate/trac-2739.txt b/tests/ref/fate/trac-2739.txt
index 86d339c880..8cf601f051 100644
--- a/tests/ref/fate/trac-2739.txt
+++ b/tests/ref/fate/trac-2739.txt
@@ -14,7 +14,7 @@ ret: 0 st: 1 flags:1 dts: 1.164000 pts: 1.164000 pos: 192278 size: 275
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
@@ -36,7 +36,7 @@ ret: 0 st: 1 flags:1 dts: 1.164000 pts: 1.164000 pos: 192278 size: 275
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
--
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] 14+ messages in thread* Re: [FFmpeg-devel] [PATCH v8 4/5] ogg/vorbis: implement header packet skip in chained ogg bitstreams.
2025-05-24 18:14 ` [FFmpeg-devel] [PATCH v8 4/5] ogg/vorbis: implement header packet skip in chained ogg bitstreams Romain Beauxis
@ 2025-05-31 0:44 ` Andreas Rheinhardt
2025-05-31 19:36 ` Romain Beauxis
0 siblings, 1 reply; 14+ messages in thread
From: Andreas Rheinhardt @ 2025-05-31 0:44 UTC (permalink / raw)
To: ffmpeg-devel
Romain Beauxis:
> ---
> libavcodec/vorbis_parser.h | 11 ++++
> libavcodec/vorbisdec.c | 75 +++++++++++++---------
> libavformat/oggparsevorbis.c | 67 ++++++++++++++++++-
> tests/ref/fate/ogg-vorbis-chained-meta.txt | 3 -
> tests/ref/fate/trac-2739.txt | 4 +-
> 5 files changed, 121 insertions(+), 39 deletions(-)
>
> diff --git a/libavcodec/vorbis_parser.h b/libavcodec/vorbis_parser.h
> index 789932ac49..b176fe536c 100644
> --- a/libavcodec/vorbis_parser.h
> +++ b/libavcodec/vorbis_parser.h
> @@ -30,6 +30,17 @@
>
> typedef struct AVVorbisParseContext AVVorbisParseContext;
>
> +/**
> + * Used by the vorbis parser to pass new chained stream headers
> + * as extradata.
> + */
> +typedef struct vorbis_new_extradata {
> + uint8_t *header;
> + size_t header_size;
> + uint8_t *setup;
> + size_t setup_size;
> +} vorbis_new_extradata;
> +
> /**
> * Allocate and initialize the Vorbis parser using headers in the extradata.
> */
> diff --git a/libavcodec/vorbisdec.c b/libavcodec/vorbisdec.c
> index adbd726183..a4b159ba9b 100644
> --- a/libavcodec/vorbisdec.c
> +++ b/libavcodec/vorbisdec.c
> @@ -43,6 +43,7 @@
> #include "vorbis.h"
> #include "vorbisdsp.h"
> #include "vorbis_data.h"
> +#include "vorbis_parser.h"
> #include "xiph.h"
>
> #define V_NB_BITS 8
> @@ -1778,47 +1779,59 @@ static int vorbis_decode_frame(AVCodecContext *avctx, AVFrame *frame,
> GetBitContext *gb = &vc->gb;
> float *channel_ptrs[255];
> int i, len, ret;
> + size_t new_extradata_size;
> + vorbis_new_extradata *new_extradata;
> + const uint8_t *header;
> + const uint8_t *setup;
>
> 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;
> + new_extradata = (vorbis_new_extradata *)av_packet_get_side_data(
> + avpkt, AV_PKT_DATA_NEW_EXTRADATA, &new_extradata_size);
>
> - 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;
> - }
> + if (new_extradata) {
> + header = new_extradata->header;
> + setup = new_extradata->setup;
>
> - 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]);
> - }
> + if (new_extradata->header_size > 7 && *header == 1) {
> + if ((ret = init_get_bits8(
> + gb, header + 1,
> + new_extradata->header_size - 1)) < 0)
> + return ret;
>
> - avctx->sample_rate = vc->audio_samplerate;
> - return buf_size;
> - }
> + 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;
> + }
>
> - if (*buf == 3 && buf_size > 7) {
> - av_log(avctx, AV_LOG_DEBUG, "Ignoring comment header\n");
> - return buf_size;
> - }
> + 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]);
> + }
>
> - if (*buf == 5 && buf_size > 7 && vc->channel_residues && !vc->modes) {
> - if ((ret = init_get_bits8(gb, buf + 1, buf_size - 1)) < 0)
> - return ret;
> + avctx->sample_rate = vc->audio_samplerate;
> + }
>
> - if ((ret = vorbis_parse_setup_hdr(vc))) {
> - av_log(avctx, AV_LOG_ERROR, "Setup header corrupt.\n");
> - vorbis_free(vc);
> - return ret;
> + if (new_extradata->setup_size > 7 && *setup == 5 &&
> + vc->channel_residues && !vc->modes) {
> + if ((ret = init_get_bits8(
> + gb, setup + 1,
> + new_extradata->setup_size - 1)) < 0)
> + return ret;
> +
> + if ((ret = vorbis_parse_setup_hdr(vc))) {
> + av_log(avctx, AV_LOG_ERROR, "Setup header corrupt.\n");
> + 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 62cc2da6de..f8e66e8127 100644
> --- a/libavformat/oggparsevorbis.c
> +++ b/libavformat/oggparsevorbis.c
> @@ -255,12 +255,19 @@ static void vorbis_cleanup(AVFormatContext *s, int idx)
> struct ogg *ogg = s->priv_data;
> struct ogg_stream *os = ogg->streams + idx;
> struct oggvorbis_private *priv = os->private;
> + vorbis_new_extradata *new_extradata;
> int i;
> if (os->private) {
> av_vorbis_parse_free(&priv->vp);
> for (i = 0; i < 3; i++)
> av_freep(&priv->packet[i]);
> }
> +
> + if (os->new_extradata) {
> + new_extradata = (vorbis_new_extradata *)os->new_extradata;
> + av_freep(&new_extradata->header);
> + av_freep(&new_extradata->setup);
> + }
> }
>
> static int vorbis_update_metadata(AVFormatContext *s, int idx)
> @@ -433,7 +440,10 @@ static int vorbis_packet(AVFormatContext *s, int idx)
> struct ogg *ogg = s->priv_data;
> struct ogg_stream *os = ogg->streams + idx;
> struct oggvorbis_private *priv = os->private;
> + vorbis_new_extradata *new_extradata;
> int duration, flags = 0;
> + int skip_packet = 0;
> + int ret;
>
> if (!priv->vp)
> return AVERROR_INVALIDDATA;
> @@ -496,10 +506,61 @@ 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;
> +
> + if (!os->new_extradata) {
> + os->new_extradata = av_mallocz(sizeof(vorbis_new_extradata));
> + if (!os->new_extradata)
> + return AVERROR(ENOMEM);
> + }
> +
> + os->new_extradata_size = sizeof(vorbis_new_extradata);
> + new_extradata = (vorbis_new_extradata *)os->new_extradata;
> +
> + ret = av_reallocp(&new_extradata->header, os->psize);
> + if (ret < 0)
> + return ret;
> +
> + memcpy(new_extradata->header, os->buf + os->pstart, os->psize);
> + new_extradata->header_size = os->psize;
> +
> + 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) {
> + if (!os->new_extradata) {
> + os->new_extradata = av_mallocz(sizeof(vorbis_new_extradata));
> + if (!os->new_extradata)
> + return AVERROR(ENOMEM);
> + }
> +
> + os->new_extradata_size = sizeof(vorbis_new_extradata);
> + new_extradata = (vorbis_new_extradata *)os->new_extradata;
> +
> + ret = av_reallocp(&new_extradata->setup, os->psize);
> + if (ret < 0)
> + return ret;
> +
> + memcpy(new_extradata->setup, os->buf + os->pstart, os->psize);
> + new_extradata->setup_size = os->psize;
> +
> + skip_packet = 1;
> }
> +
> os->pduration = duration;
> }
>
> @@ -521,7 +582,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 = {
There are multiple issues with this patch:
1. The side data structures are not padded, leading to
heap-buffer-overflows in the fate-ogg-vorbis-chained-meta test.
2. The side data structures are not flat and therefore not suitable for
use as AVPacketSideData. (The setup and header arrays are currently
owned by the demuxer, yet an AVPacket is supposed to be valid on its
own. But this side data becomes invalid when the demuxer encounters a
new side data (and reallocates its internal buffers) or when the demuxer
is closed.)
3. The vorbis_new_extradata name does not abide by our naming
conventions; given that this is a public header, it would need an AV prefix.
4. You forgot to add an APIchanges entry and bump lavc minor for your
addition.
5. You forgot to add stddef.h to vorbis_parser.h.
6. You should be aware that our parsing API does not work with
AVPackets, but with raw buffers. It (or rather: parse_packet() in
libavformat/demux.c) therefore has a tendency to put the side data to
the wrong packets or completely omit it (this happens when the parser
introduces a delay). Vorbis-in-ogg does not enable the parser, so it is
(currently) unaffected by this.
- Andreas
_______________________________________________
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] 14+ messages in thread* Re: [FFmpeg-devel] [PATCH v8 4/5] ogg/vorbis: implement header packet skip in chained ogg bitstreams.
2025-05-31 0:44 ` Andreas Rheinhardt
@ 2025-05-31 19:36 ` Romain Beauxis
2025-05-31 20:08 ` Andreas Rheinhardt
0 siblings, 1 reply; 14+ messages in thread
From: Romain Beauxis @ 2025-05-31 19:36 UTC (permalink / raw)
To: FFmpeg development discussions and patches
Le ven. 30 mai 2025 à 19:44, Andreas Rheinhardt
<andreas.rheinhardt@outlook.com> a écrit :
>
> Romain Beauxis:
> > ---
> > libavcodec/vorbis_parser.h | 11 ++++
> > libavcodec/vorbisdec.c | 75 +++++++++++++---------
> > libavformat/oggparsevorbis.c | 67 ++++++++++++++++++-
> > tests/ref/fate/ogg-vorbis-chained-meta.txt | 3 -
> > tests/ref/fate/trac-2739.txt | 4 +-
> > 5 files changed, 121 insertions(+), 39 deletions(-)
> >
> > diff --git a/libavcodec/vorbis_parser.h b/libavcodec/vorbis_parser.h
> > index 789932ac49..b176fe536c 100644
> > --- a/libavcodec/vorbis_parser.h
> > +++ b/libavcodec/vorbis_parser.h
> > @@ -30,6 +30,17 @@
> >
> > typedef struct AVVorbisParseContext AVVorbisParseContext;
> >
> > +/**
> > + * Used by the vorbis parser to pass new chained stream headers
> > + * as extradata.
> > + */
> > +typedef struct vorbis_new_extradata {
> > + uint8_t *header;
> > + size_t header_size;
> > + uint8_t *setup;
> > + size_t setup_size;
> > +} vorbis_new_extradata;
> > +
> > /**
> > * Allocate and initialize the Vorbis parser using headers in the extradata.
> > */
> > diff --git a/libavcodec/vorbisdec.c b/libavcodec/vorbisdec.c
> > index adbd726183..a4b159ba9b 100644
> > --- a/libavcodec/vorbisdec.c
> > +++ b/libavcodec/vorbisdec.c
> > @@ -43,6 +43,7 @@
> > #include "vorbis.h"
> > #include "vorbisdsp.h"
> > #include "vorbis_data.h"
> > +#include "vorbis_parser.h"
> > #include "xiph.h"
> >
> > #define V_NB_BITS 8
> > @@ -1778,47 +1779,59 @@ static int vorbis_decode_frame(AVCodecContext *avctx, AVFrame *frame,
> > GetBitContext *gb = &vc->gb;
> > float *channel_ptrs[255];
> > int i, len, ret;
> > + size_t new_extradata_size;
> > + vorbis_new_extradata *new_extradata;
> > + const uint8_t *header;
> > + const uint8_t *setup;
> >
> > 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;
> > + new_extradata = (vorbis_new_extradata *)av_packet_get_side_data(
> > + avpkt, AV_PKT_DATA_NEW_EXTRADATA, &new_extradata_size);
> >
> > - 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;
> > - }
> > + if (new_extradata) {
> > + header = new_extradata->header;
> > + setup = new_extradata->setup;
> >
> > - 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]);
> > - }
> > + if (new_extradata->header_size > 7 && *header == 1) {
> > + if ((ret = init_get_bits8(
> > + gb, header + 1,
> > + new_extradata->header_size - 1)) < 0)
> > + return ret;
> >
> > - avctx->sample_rate = vc->audio_samplerate;
> > - return buf_size;
> > - }
> > + 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;
> > + }
> >
> > - if (*buf == 3 && buf_size > 7) {
> > - av_log(avctx, AV_LOG_DEBUG, "Ignoring comment header\n");
> > - return buf_size;
> > - }
> > + 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]);
> > + }
> >
> > - if (*buf == 5 && buf_size > 7 && vc->channel_residues && !vc->modes) {
> > - if ((ret = init_get_bits8(gb, buf + 1, buf_size - 1)) < 0)
> > - return ret;
> > + avctx->sample_rate = vc->audio_samplerate;
> > + }
> >
> > - if ((ret = vorbis_parse_setup_hdr(vc))) {
> > - av_log(avctx, AV_LOG_ERROR, "Setup header corrupt.\n");
> > - vorbis_free(vc);
> > - return ret;
> > + if (new_extradata->setup_size > 7 && *setup == 5 &&
> > + vc->channel_residues && !vc->modes) {
> > + if ((ret = init_get_bits8(
> > + gb, setup + 1,
> > + new_extradata->setup_size - 1)) < 0)
> > + return ret;
> > +
> > + if ((ret = vorbis_parse_setup_hdr(vc))) {
> > + av_log(avctx, AV_LOG_ERROR, "Setup header corrupt.\n");
> > + 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 62cc2da6de..f8e66e8127 100644
> > --- a/libavformat/oggparsevorbis.c
> > +++ b/libavformat/oggparsevorbis.c
> > @@ -255,12 +255,19 @@ static void vorbis_cleanup(AVFormatContext *s, int idx)
> > struct ogg *ogg = s->priv_data;
> > struct ogg_stream *os = ogg->streams + idx;
> > struct oggvorbis_private *priv = os->private;
> > + vorbis_new_extradata *new_extradata;
> > int i;
> > if (os->private) {
> > av_vorbis_parse_free(&priv->vp);
> > for (i = 0; i < 3; i++)
> > av_freep(&priv->packet[i]);
> > }
> > +
> > + if (os->new_extradata) {
> > + new_extradata = (vorbis_new_extradata *)os->new_extradata;
> > + av_freep(&new_extradata->header);
> > + av_freep(&new_extradata->setup);
> > + }
> > }
> >
> > static int vorbis_update_metadata(AVFormatContext *s, int idx)
> > @@ -433,7 +440,10 @@ static int vorbis_packet(AVFormatContext *s, int idx)
> > struct ogg *ogg = s->priv_data;
> > struct ogg_stream *os = ogg->streams + idx;
> > struct oggvorbis_private *priv = os->private;
> > + vorbis_new_extradata *new_extradata;
> > int duration, flags = 0;
> > + int skip_packet = 0;
> > + int ret;
> >
> > if (!priv->vp)
> > return AVERROR_INVALIDDATA;
> > @@ -496,10 +506,61 @@ 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;
> > +
> > + if (!os->new_extradata) {
> > + os->new_extradata = av_mallocz(sizeof(vorbis_new_extradata));
> > + if (!os->new_extradata)
> > + return AVERROR(ENOMEM);
> > + }
> > +
> > + os->new_extradata_size = sizeof(vorbis_new_extradata);
> > + new_extradata = (vorbis_new_extradata *)os->new_extradata;
> > +
> > + ret = av_reallocp(&new_extradata->header, os->psize);
> > + if (ret < 0)
> > + return ret;
> > +
> > + memcpy(new_extradata->header, os->buf + os->pstart, os->psize);
> > + new_extradata->header_size = os->psize;
> > +
> > + 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) {
> > + if (!os->new_extradata) {
> > + os->new_extradata = av_mallocz(sizeof(vorbis_new_extradata));
> > + if (!os->new_extradata)
> > + return AVERROR(ENOMEM);
> > + }
> > +
> > + os->new_extradata_size = sizeof(vorbis_new_extradata);
> > + new_extradata = (vorbis_new_extradata *)os->new_extradata;
> > +
> > + ret = av_reallocp(&new_extradata->setup, os->psize);
> > + if (ret < 0)
> > + return ret;
> > +
> > + memcpy(new_extradata->setup, os->buf + os->pstart, os->psize);
> > + new_extradata->setup_size = os->psize;
> > +
> > + skip_packet = 1;
> > }
> > +
> > os->pduration = duration;
> > }
> >
> > @@ -521,7 +582,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 = {
>
> There are multiple issues with this patch:
Thank you for your feedback.
> 1. The side data structures are not padded, leading to
> heap-buffer-overflows in the fate-ogg-vorbis-chained-meta test.
Do you have a pointer to this issue? Is there a failing test here:
https://fate.ffmpeg.org/ ?
> 2. The side data structures are not flat and therefore not suitable for
> use as AVPacketSideData. (The setup and header arrays are currently
> owned by the demuxer, yet an AVPacket is supposed to be valid on its
> own. But this side data becomes invalid when the demuxer encounters a
> new side data (and reallocates its internal buffers) or when the demuxer
> is closed.)
I can work on that.
> 3. The vorbis_new_extradata name does not abide by our naming
> conventions; given that this is a public header, it would need an AV prefix.
Noted.
> 4. You forgot to add an APIchanges entry and bump lavc minor for your
> addition.
Will add
> 5. You forgot to add stddef.h to vorbis_parser.h.
Will add
> 6. You should be aware that our parsing API does not work with
> AVPackets, but with raw buffers. It (or rather: parse_packet() in
> libavformat/demux.c) therefore has a tendency to put the side data to
> the wrong packets or completely omit it (this happens when the parser
> introduces a delay). Vorbis-in-ogg does not enable the parser, so it is
> (currently) unaffected by this.
Noted, glad it's not impacted. I'll have a look at the corresponding code.
I'll send a fix for these as part of the next row of patches. I'll
make sure to CC/ you so I can get your opinion before it gets merged.
-- Romain
> - Andreas
>
> _______________________________________________
> 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] 14+ messages in thread* Re: [FFmpeg-devel] [PATCH v8 4/5] ogg/vorbis: implement header packet skip in chained ogg bitstreams.
2025-05-31 19:36 ` Romain Beauxis
@ 2025-05-31 20:08 ` Andreas Rheinhardt
2025-06-01 16:21 ` Romain Beauxis
0 siblings, 1 reply; 14+ messages in thread
From: Andreas Rheinhardt @ 2025-05-31 20:08 UTC (permalink / raw)
To: ffmpeg-devel
Romain Beauxis:
> Le ven. 30 mai 2025 à 19:44, Andreas Rheinhardt
> <andreas.rheinhardt@outlook.com> a écrit :
>>
>> Romain Beauxis:
>>> ---
>>> libavcodec/vorbis_parser.h | 11 ++++
>>> libavcodec/vorbisdec.c | 75 +++++++++++++---------
>>> libavformat/oggparsevorbis.c | 67 ++++++++++++++++++-
>>> tests/ref/fate/ogg-vorbis-chained-meta.txt | 3 -
>>> tests/ref/fate/trac-2739.txt | 4 +-
>>> 5 files changed, 121 insertions(+), 39 deletions(-)
>>>
>>> diff --git a/libavcodec/vorbis_parser.h b/libavcodec/vorbis_parser.h
>>> index 789932ac49..b176fe536c 100644
>>> --- a/libavcodec/vorbis_parser.h
>>> +++ b/libavcodec/vorbis_parser.h
>>> @@ -30,6 +30,17 @@
>>>
>>> typedef struct AVVorbisParseContext AVVorbisParseContext;
>>>
>>> +/**
>>> + * Used by the vorbis parser to pass new chained stream headers
>>> + * as extradata.
>>> + */
>>> +typedef struct vorbis_new_extradata {
>>> + uint8_t *header;
>>> + size_t header_size;
>>> + uint8_t *setup;
>>> + size_t setup_size;
>>> +} vorbis_new_extradata;
>>> +
>>> /**
>>> * Allocate and initialize the Vorbis parser using headers in the extradata.
>>> */
>>> diff --git a/libavcodec/vorbisdec.c b/libavcodec/vorbisdec.c
>>> index adbd726183..a4b159ba9b 100644
>>> --- a/libavcodec/vorbisdec.c
>>> +++ b/libavcodec/vorbisdec.c
>>> @@ -43,6 +43,7 @@
>>> #include "vorbis.h"
>>> #include "vorbisdsp.h"
>>> #include "vorbis_data.h"
>>> +#include "vorbis_parser.h"
>>> #include "xiph.h"
>>>
>>> #define V_NB_BITS 8
>>> @@ -1778,47 +1779,59 @@ static int vorbis_decode_frame(AVCodecContext *avctx, AVFrame *frame,
>>> GetBitContext *gb = &vc->gb;
>>> float *channel_ptrs[255];
>>> int i, len, ret;
>>> + size_t new_extradata_size;
>>> + vorbis_new_extradata *new_extradata;
>>> + const uint8_t *header;
>>> + const uint8_t *setup;
>>>
>>> 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;
>>> + new_extradata = (vorbis_new_extradata *)av_packet_get_side_data(
>>> + avpkt, AV_PKT_DATA_NEW_EXTRADATA, &new_extradata_size);
>>>
>>> - 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;
>>> - }
>>> + if (new_extradata) {
>>> + header = new_extradata->header;
>>> + setup = new_extradata->setup;
>>>
>>> - 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]);
>>> - }
>>> + if (new_extradata->header_size > 7 && *header == 1) {
>>> + if ((ret = init_get_bits8(
>>> + gb, header + 1,
>>> + new_extradata->header_size - 1)) < 0)
>>> + return ret;
>>>
>>> - avctx->sample_rate = vc->audio_samplerate;
>>> - return buf_size;
>>> - }
>>> + 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;
>>> + }
>>>
>>> - if (*buf == 3 && buf_size > 7) {
>>> - av_log(avctx, AV_LOG_DEBUG, "Ignoring comment header\n");
>>> - return buf_size;
>>> - }
>>> + 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]);
>>> + }
>>>
>>> - if (*buf == 5 && buf_size > 7 && vc->channel_residues && !vc->modes) {
>>> - if ((ret = init_get_bits8(gb, buf + 1, buf_size - 1)) < 0)
>>> - return ret;
>>> + avctx->sample_rate = vc->audio_samplerate;
>>> + }
>>>
>>> - if ((ret = vorbis_parse_setup_hdr(vc))) {
>>> - av_log(avctx, AV_LOG_ERROR, "Setup header corrupt.\n");
>>> - vorbis_free(vc);
>>> - return ret;
>>> + if (new_extradata->setup_size > 7 && *setup == 5 &&
>>> + vc->channel_residues && !vc->modes) {
>>> + if ((ret = init_get_bits8(
>>> + gb, setup + 1,
>>> + new_extradata->setup_size - 1)) < 0)
>>> + return ret;
>>> +
>>> + if ((ret = vorbis_parse_setup_hdr(vc))) {
>>> + av_log(avctx, AV_LOG_ERROR, "Setup header corrupt.\n");
>>> + 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 62cc2da6de..f8e66e8127 100644
>>> --- a/libavformat/oggparsevorbis.c
>>> +++ b/libavformat/oggparsevorbis.c
>>> @@ -255,12 +255,19 @@ static void vorbis_cleanup(AVFormatContext *s, int idx)
>>> struct ogg *ogg = s->priv_data;
>>> struct ogg_stream *os = ogg->streams + idx;
>>> struct oggvorbis_private *priv = os->private;
>>> + vorbis_new_extradata *new_extradata;
>>> int i;
>>> if (os->private) {
>>> av_vorbis_parse_free(&priv->vp);
>>> for (i = 0; i < 3; i++)
>>> av_freep(&priv->packet[i]);
>>> }
>>> +
>>> + if (os->new_extradata) {
>>> + new_extradata = (vorbis_new_extradata *)os->new_extradata;
>>> + av_freep(&new_extradata->header);
>>> + av_freep(&new_extradata->setup);
>>> + }
>>> }
>>>
>>> static int vorbis_update_metadata(AVFormatContext *s, int idx)
>>> @@ -433,7 +440,10 @@ static int vorbis_packet(AVFormatContext *s, int idx)
>>> struct ogg *ogg = s->priv_data;
>>> struct ogg_stream *os = ogg->streams + idx;
>>> struct oggvorbis_private *priv = os->private;
>>> + vorbis_new_extradata *new_extradata;
>>> int duration, flags = 0;
>>> + int skip_packet = 0;
>>> + int ret;
>>>
>>> if (!priv->vp)
>>> return AVERROR_INVALIDDATA;
>>> @@ -496,10 +506,61 @@ 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;
>>> +
>>> + if (!os->new_extradata) {
>>> + os->new_extradata = av_mallocz(sizeof(vorbis_new_extradata));
>>> + if (!os->new_extradata)
>>> + return AVERROR(ENOMEM);
>>> + }
>>> +
>>> + os->new_extradata_size = sizeof(vorbis_new_extradata);
>>> + new_extradata = (vorbis_new_extradata *)os->new_extradata;
>>> +
>>> + ret = av_reallocp(&new_extradata->header, os->psize);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + memcpy(new_extradata->header, os->buf + os->pstart, os->psize);
>>> + new_extradata->header_size = os->psize;
>>> +
>>> + 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) {
>>> + if (!os->new_extradata) {
>>> + os->new_extradata = av_mallocz(sizeof(vorbis_new_extradata));
>>> + if (!os->new_extradata)
>>> + return AVERROR(ENOMEM);
>>> + }
>>> +
>>> + os->new_extradata_size = sizeof(vorbis_new_extradata);
>>> + new_extradata = (vorbis_new_extradata *)os->new_extradata;
>>> +
>>> + ret = av_reallocp(&new_extradata->setup, os->psize);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + memcpy(new_extradata->setup, os->buf + os->pstart, os->psize);
>>> + new_extradata->setup_size = os->psize;
>>> +
>>> + skip_packet = 1;
>>> }
>>> +
>>> os->pduration = duration;
>>> }
>>>
>>> @@ -521,7 +582,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 = {
>>
>> There are multiple issues with this patch:
>
> Thank you for your feedback.
>
>> 1. The side data structures are not padded, leading to
>> heap-buffer-overflows in the fate-ogg-vorbis-chained-meta test.
>
> Do you have a pointer to this issue? Is there a failing test here:
> https://fate.ffmpeg.org/ ?
>
I noted it when I ran FATE with (Clang-)ASAN locally. Seems like none of
the ASAN/valgrind fate boxes tested your commit.
>> 2. The side data structures are not flat and therefore not suitable for
>> use as AVPacketSideData. (The setup and header arrays are currently
>> owned by the demuxer, yet an AVPacket is supposed to be valid on its
>> own. But this side data becomes invalid when the demuxer encounters a
>> new side data (and reallocates its internal buffers) or when the demuxer
>> is closed.)
>
> I can work on that.
Actually, thinking about this a bit more: New extradata via side data
should use the same format as ordinary extradata, so there is no need to
add a new struct and APIchanges for that.
- Andreas
_______________________________________________
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] 14+ messages in thread* Re: [FFmpeg-devel] [PATCH v8 4/5] ogg/vorbis: implement header packet skip in chained ogg bitstreams.
2025-05-31 20:08 ` Andreas Rheinhardt
@ 2025-06-01 16:21 ` Romain Beauxis
0 siblings, 0 replies; 14+ messages in thread
From: Romain Beauxis @ 2025-06-01 16:21 UTC (permalink / raw)
To: FFmpeg development discussions and patches
Le sam. 31 mai 2025 à 15:08, Andreas Rheinhardt
<andreas.rheinhardt@outlook.com> a écrit :
>
> Romain Beauxis:
> > Le ven. 30 mai 2025 à 19:44, Andreas Rheinhardt
> > <andreas.rheinhardt@outlook.com> a écrit :
> >>
> >> Romain Beauxis:
> >>> ---
> >>> libavcodec/vorbis_parser.h | 11 ++++
> >>> libavcodec/vorbisdec.c | 75 +++++++++++++---------
> >>> libavformat/oggparsevorbis.c | 67 ++++++++++++++++++-
> >>> tests/ref/fate/ogg-vorbis-chained-meta.txt | 3 -
> >>> tests/ref/fate/trac-2739.txt | 4 +-
> >>> 5 files changed, 121 insertions(+), 39 deletions(-)
> >>>
> >>> diff --git a/libavcodec/vorbis_parser.h b/libavcodec/vorbis_parser.h
> >>> index 789932ac49..b176fe536c 100644
> >>> --- a/libavcodec/vorbis_parser.h
> >>> +++ b/libavcodec/vorbis_parser.h
> >>> @@ -30,6 +30,17 @@
> >>>
> >>> typedef struct AVVorbisParseContext AVVorbisParseContext;
> >>>
> >>> +/**
> >>> + * Used by the vorbis parser to pass new chained stream headers
> >>> + * as extradata.
> >>> + */
> >>> +typedef struct vorbis_new_extradata {
> >>> + uint8_t *header;
> >>> + size_t header_size;
> >>> + uint8_t *setup;
> >>> + size_t setup_size;
> >>> +} vorbis_new_extradata;
> >>> +
> >>> /**
> >>> * Allocate and initialize the Vorbis parser using headers in the extradata.
> >>> */
> >>> diff --git a/libavcodec/vorbisdec.c b/libavcodec/vorbisdec.c
> >>> index adbd726183..a4b159ba9b 100644
> >>> --- a/libavcodec/vorbisdec.c
> >>> +++ b/libavcodec/vorbisdec.c
> >>> @@ -43,6 +43,7 @@
> >>> #include "vorbis.h"
> >>> #include "vorbisdsp.h"
> >>> #include "vorbis_data.h"
> >>> +#include "vorbis_parser.h"
> >>> #include "xiph.h"
> >>>
> >>> #define V_NB_BITS 8
> >>> @@ -1778,47 +1779,59 @@ static int vorbis_decode_frame(AVCodecContext *avctx, AVFrame *frame,
> >>> GetBitContext *gb = &vc->gb;
> >>> float *channel_ptrs[255];
> >>> int i, len, ret;
> >>> + size_t new_extradata_size;
> >>> + vorbis_new_extradata *new_extradata;
> >>> + const uint8_t *header;
> >>> + const uint8_t *setup;
> >>>
> >>> 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;
> >>> + new_extradata = (vorbis_new_extradata *)av_packet_get_side_data(
> >>> + avpkt, AV_PKT_DATA_NEW_EXTRADATA, &new_extradata_size);
> >>>
> >>> - 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;
> >>> - }
> >>> + if (new_extradata) {
> >>> + header = new_extradata->header;
> >>> + setup = new_extradata->setup;
> >>>
> >>> - 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]);
> >>> - }
> >>> + if (new_extradata->header_size > 7 && *header == 1) {
> >>> + if ((ret = init_get_bits8(
> >>> + gb, header + 1,
> >>> + new_extradata->header_size - 1)) < 0)
> >>> + return ret;
> >>>
> >>> - avctx->sample_rate = vc->audio_samplerate;
> >>> - return buf_size;
> >>> - }
> >>> + 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;
> >>> + }
> >>>
> >>> - if (*buf == 3 && buf_size > 7) {
> >>> - av_log(avctx, AV_LOG_DEBUG, "Ignoring comment header\n");
> >>> - return buf_size;
> >>> - }
> >>> + 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]);
> >>> + }
> >>>
> >>> - if (*buf == 5 && buf_size > 7 && vc->channel_residues && !vc->modes) {
> >>> - if ((ret = init_get_bits8(gb, buf + 1, buf_size - 1)) < 0)
> >>> - return ret;
> >>> + avctx->sample_rate = vc->audio_samplerate;
> >>> + }
> >>>
> >>> - if ((ret = vorbis_parse_setup_hdr(vc))) {
> >>> - av_log(avctx, AV_LOG_ERROR, "Setup header corrupt.\n");
> >>> - vorbis_free(vc);
> >>> - return ret;
> >>> + if (new_extradata->setup_size > 7 && *setup == 5 &&
> >>> + vc->channel_residues && !vc->modes) {
> >>> + if ((ret = init_get_bits8(
> >>> + gb, setup + 1,
> >>> + new_extradata->setup_size - 1)) < 0)
> >>> + return ret;
> >>> +
> >>> + if ((ret = vorbis_parse_setup_hdr(vc))) {
> >>> + av_log(avctx, AV_LOG_ERROR, "Setup header corrupt.\n");
> >>> + 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 62cc2da6de..f8e66e8127 100644
> >>> --- a/libavformat/oggparsevorbis.c
> >>> +++ b/libavformat/oggparsevorbis.c
> >>> @@ -255,12 +255,19 @@ static void vorbis_cleanup(AVFormatContext *s, int idx)
> >>> struct ogg *ogg = s->priv_data;
> >>> struct ogg_stream *os = ogg->streams + idx;
> >>> struct oggvorbis_private *priv = os->private;
> >>> + vorbis_new_extradata *new_extradata;
> >>> int i;
> >>> if (os->private) {
> >>> av_vorbis_parse_free(&priv->vp);
> >>> for (i = 0; i < 3; i++)
> >>> av_freep(&priv->packet[i]);
> >>> }
> >>> +
> >>> + if (os->new_extradata) {
> >>> + new_extradata = (vorbis_new_extradata *)os->new_extradata;
> >>> + av_freep(&new_extradata->header);
> >>> + av_freep(&new_extradata->setup);
> >>> + }
> >>> }
> >>>
> >>> static int vorbis_update_metadata(AVFormatContext *s, int idx)
> >>> @@ -433,7 +440,10 @@ static int vorbis_packet(AVFormatContext *s, int idx)
> >>> struct ogg *ogg = s->priv_data;
> >>> struct ogg_stream *os = ogg->streams + idx;
> >>> struct oggvorbis_private *priv = os->private;
> >>> + vorbis_new_extradata *new_extradata;
> >>> int duration, flags = 0;
> >>> + int skip_packet = 0;
> >>> + int ret;
> >>>
> >>> if (!priv->vp)
> >>> return AVERROR_INVALIDDATA;
> >>> @@ -496,10 +506,61 @@ 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;
> >>> +
> >>> + if (!os->new_extradata) {
> >>> + os->new_extradata = av_mallocz(sizeof(vorbis_new_extradata));
> >>> + if (!os->new_extradata)
> >>> + return AVERROR(ENOMEM);
> >>> + }
> >>> +
> >>> + os->new_extradata_size = sizeof(vorbis_new_extradata);
> >>> + new_extradata = (vorbis_new_extradata *)os->new_extradata;
> >>> +
> >>> + ret = av_reallocp(&new_extradata->header, os->psize);
> >>> + if (ret < 0)
> >>> + return ret;
> >>> +
> >>> + memcpy(new_extradata->header, os->buf + os->pstart, os->psize);
> >>> + new_extradata->header_size = os->psize;
> >>> +
> >>> + 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) {
> >>> + if (!os->new_extradata) {
> >>> + os->new_extradata = av_mallocz(sizeof(vorbis_new_extradata));
> >>> + if (!os->new_extradata)
> >>> + return AVERROR(ENOMEM);
> >>> + }
> >>> +
> >>> + os->new_extradata_size = sizeof(vorbis_new_extradata);
> >>> + new_extradata = (vorbis_new_extradata *)os->new_extradata;
> >>> +
> >>> + ret = av_reallocp(&new_extradata->setup, os->psize);
> >>> + if (ret < 0)
> >>> + return ret;
> >>> +
> >>> + memcpy(new_extradata->setup, os->buf + os->pstart, os->psize);
> >>> + new_extradata->setup_size = os->psize;
> >>> +
> >>> + skip_packet = 1;
> >>> }
> >>> +
> >>> os->pduration = duration;
> >>> }
> >>>
> >>> @@ -521,7 +582,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 = {
> >>
> >> There are multiple issues with this patch:
> >
> > Thank you for your feedback.
> >
> >> 1. The side data structures are not padded, leading to
> >> heap-buffer-overflows in the fate-ogg-vorbis-chained-meta test.
> >
> > Do you have a pointer to this issue? Is there a failing test here:
> > https://fate.ffmpeg.org/ ?
> >
>
> I noted it when I ran FATE with (Clang-)ASAN locally. Seems like none of
> the ASAN/valgrind fate boxes tested your commit.
>
> >> 2. The side data structures are not flat and therefore not suitable for
> >> use as AVPacketSideData. (The setup and header arrays are currently
> >> owned by the demuxer, yet an AVPacket is supposed to be valid on its
> >> own. But this side data becomes invalid when the demuxer encounters a
> >> new side data (and reallocates its internal buffers) or when the demuxer
> >> is closed.)
> >
> > I can work on that.
>
> Actually, thinking about this a bit more: New extradata via side data
> should use the same format as ordinary extradata, so there is no need to
> add a new struct and APIchanges for that.
Just sent an updated patch, let me know what you think!
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] 14+ messages in thread
* [FFmpeg-devel] [PATCH v8 5/5] libavformat/oggdec.h: Change paket function documentation to return 1 on header packets only.
2025-05-24 18:14 [FFmpeg-devel] [PATCH v8 0/5] Remove chained ogg stream header packets from the demuxer Romain Beauxis
` (3 preceding siblings ...)
2025-05-24 18:14 ` [FFmpeg-devel] [PATCH v8 4/5] ogg/vorbis: implement header packet skip in chained ogg bitstreams Romain Beauxis
@ 2025-05-24 18:14 ` Romain Beauxis
4 siblings, 0 replies; 14+ messages in thread
From: Romain Beauxis @ 2025-05-24 18:14 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] 14+ messages in thread