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

[Andreas Rheinhardt]

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.

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

- Andreas