[FFmpeg-devel] [PATCH] ogg/vorbis: implement header packet skip in chained ogg bitstreams.

[Romain Beauxis]

Le mar. 3 juin 2025 à 14:20, Andreas Rheinhardt
<andreas.rheinhardt at outlook.com> a écrit :
>
> Romain Beauxis:
> > Le mar. 3 juin 2025 à 12:12, Andreas Rheinhardt
> > <andreas.rheinhardt at outlook.com> a écrit :
> >>
> >> Romain Beauxis:
> >>> This is a redo of 574f634e49847e2225ee50013afebf0de03ef013 using a flat
> >>> memory storage for the extradata.
> >>>
> >>> PR review comments addressed:
> >>> * Use flat memory array
> >>> * Rename structure to follow convention
> >>> * Add stddef.h header
> >>>
> >>> Bonus: libavcodec/vorbisdec.c diff is greatly improved!
> >>>
> >>> ---
> >>>  libavcodec/vorbis_parser.h                 |  7 +++
> >>>  libavcodec/vorbisdec.c                     | 45 ++++++++++++-----
> >>>  libavformat/oggparsevorbis.c               | 56 ++++++++++++++++++++--
> >>>  tests/ref/fate/ogg-vorbis-chained-meta.txt |  3 --
> >>>  4 files changed, 94 insertions(+), 17 deletions(-)
> >>>
> >>> diff --git a/libavcodec/vorbis_parser.h b/libavcodec/vorbis_parser.h
> >>> index 789932ac49..9010723590 100644
> >>> --- a/libavcodec/vorbis_parser.h
> >>> +++ b/libavcodec/vorbis_parser.h
> >>> @@ -27,6 +27,7 @@
> >>>  #define AVCODEC_VORBIS_PARSER_H
> >>>
> >>>  #include <stdint.h>
> >>> +#include <stddef.h>
> >>>
> >>>  typedef struct AVVorbisParseContext AVVorbisParseContext;
> >>>
> >>> @@ -45,6 +46,12 @@ void av_vorbis_parse_free(AVVorbisParseContext **s);
> >>>  #define VORBIS_FLAG_COMMENT 0x00000002
> >>>  #define VORBIS_FLAG_SETUP   0x00000004
> >>>
> >>> +typedef struct AVVorbisHeaderExtradata {
> >>> +    unsigned int header_type;
> >>> +    size_t   header_size;
> >>> +    uint8_t header[];
> >>> +} AVVorbisHeaderExtradata;
> >>> +
> >>
> >> It seems you started this work before my latest mail
> >> https://ffmpeg.org/pipermail/ffmpeg-devel/2025-May/344422.html:
> >> AV_PKT_DATA_NEW_EXTRADATA should use the same format as ordinary
> >> extradata. (A user may e.g. use this new extradata for muxing, where the
> >> ordinary extradata is expected.) This generally allows to reuse
> >> extradata parsing functions (potentially after having factored them out
> >> of the init function).
> >
> > I'm not sure what you mean. Could you elaborate?
> >
> > If you look at https://ffmpeg.org/pipermail/ffmpeg-devel/2025-June/344446.html
> > you will see that the format seems like ordinary extradata to me. In
> > fact, as soon as the key is changed, vorbis metadata update finally
> > starts to flow as expected without changing the decoder side of
> > things, as exemplified by the test diff in the changeset.
>
> I do not get what your link to the patch exchanging
> AV_PKT_DATA_METADATA_UPDATE for AV_PKT_DATA_STRINGS_METADATA is supposed
> to mean. The format of these two types of metadata is the same, but this
> has nothing to do with AV_PKT_DATA_NEW_EXTRADATA or with extradata in
> general.

My bad.

> >
> >> Besides that, there are some problems with your approach: You are simply
> >> casting a pointer into the new extradata to AVVorbisHeaderExtradata*,
> >> although the address may not be properly aligned. You are also not
> >> checking that the extradata contains as much data as header_size claims
> >> it does. The layout of your new extradata also depends on the system
> >> (due to sizeof(header_size) and due to endianness), which makes it hard
> >> to checksum it. And you forgot the APIchanges entry/version bump for
> >> this struct (this point will become moot in a future version of this
> >> patch, when no new struct is added).
> >
> > Ok this makes sense, I can work on it.
> >
> > Let me recap the needs:
> > * extradata needs to be a flat memory array
> > * extradata needs to be platform-independent and constantly checksum-able.
> > * What's the exact requirement for alignment?
> > Considering that the array contains 2 or more sequentially stored
> > header chunks, do you require the start of each header chunk to have
> > to be aligned? Is there a technical reason for that?
>
> a) Reading or writing anything is undefined behavior unless the memory
> is suitably aligned for the type used to access it; failure to do so
> causes crashes (bus errors) on some systems.
> b) As I said: Don't add a new structure for extradata, use the already
> established format for vorbis.

The logic for vorbis is to have two header packets. Any of these 2
might be missing.

(There could also be extra packets but we can assume we'd drop the
last one of each type in this case and only pass at most two packets
at any given time).

The extradata then gets attached to the first data packet that comes
after those header packets.

Therefore, the data needs to be able to accommodate for any subset of
those two packets depending on the situation.

How do you suggest the established vorbis format should be used for
this use-case?

Thanks,
-- Romain