[FFmpeg-devel] [PATCH 01/10] avcodec/packet: add side data set struct and helpers
James Almer
jamrial at gmail.com
Mon Sep 11 21:00:33 EEST 2023
On 9/11/2023 2:41 PM, Andreas Rheinhardt wrote:
> James Almer:
>> This will be useful in the following commits.
>>
>> Signed-off-by: James Almer <jamrial at gmail.com>
>> ---
>> libavcodec/avpacket.c | 99 +++++++++++++++++++++++++++++++++++++++++++
>> libavcodec/packet.h | 74 ++++++++++++++++++++++++++++++++
>> 2 files changed, 173 insertions(+)
>>
>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>> index 5fef65e97a..5b133c5d8a 100644
>> --- a/libavcodec/avpacket.c
>> +++ b/libavcodec/avpacket.c
>> @@ -645,3 +645,102 @@ int ff_side_data_set_prft(AVPacket *pkt, int64_t timestamp)
>>
>> return 0;
>> }
>> +
>> +AVPacketSideData *av_packet_side_data_set_get(const AVPacketSideDataSet *set,
>> + enum AVPacketSideDataType type)
>> +{
>> + for (int i = 0; i < set->nb_sd; i++)
>> + if (set->sd[i]->type == type)
>> + return set->sd[i];
>> +
>> + return NULL;
>> +}
>> +
>> +static AVPacketSideData *add_side_data_to_set(AVPacketSideDataSet *set,
>> + enum AVPacketSideDataType type,
>> + uint8_t *data, size_t size)
>> +{
>> + AVPacketSideData *sd, **tmp;
>> +
>> + for (int i = 0; i < set->nb_sd; i++) {
>> + sd = set->sd[i];
>> + if (sd->type != type)
>> + continue;
>> +
>> + av_freep(&sd->data);
>> + sd->data = data;
>> + sd->size = size;
>> + return sd;
>> + }
>> +
>> + if (set->nb_sd + 1U > INT_MAX)
>> + return NULL;
>> +
>> + tmp = av_realloc_array(set->sd, set->nb_sd + 1, sizeof(*tmp));
>> + if (!tmp)
>> + return NULL;
>> +
>> + set->sd = tmp;
>> +
>> + sd = av_mallocz(sizeof(*sd));
>> + if (!sd)
>> + return NULL;
>> +
>> + sd->type = type;
>> + sd->data = data;
>> + sd->size = size;
>> +
>> + set->sd[set->nb_sd++] = sd;
>> +
>> + return sd;
>> +}
>> +
>> +AVPacketSideData *av_packet_side_data_set_add(AVPacketSideDataSet *set,
>> + enum AVPacketSideDataType type,
>> + uint8_t *data, size_t size,
>> + int flags)
>> +{
>> + return add_side_data_to_set(set, type, data, size);
>> +}
>> +
>> +AVPacketSideData *av_packet_side_data_set_new(AVPacketSideDataSet *set,
>> + enum AVPacketSideDataType type,
>> + size_t size, int flags)
>> +{
>> + AVPacketSideData *sd = NULL;
>> + uint8_t *data = av_malloc(size);
>
> How about padding in case we have new extradata?
Ok.
Despite side data of new extradata type not being something we'll see in
global side data, in the future AVPacket could also start using this
struct and helpers, so it's worth including padding now.
>
>> +
>> + if (!data)
>> + return NULL;
>> +
>> + sd = add_side_data_to_set(set, type, data, size);
>> + if (!sd)
>> + av_freep(&data);
>> +
>> + return sd;
>> +}
>> +
>> +void av_packet_side_data_set_remove(AVPacketSideDataSet *set,
>> + enum AVPacketSideDataType type)
>> +{
>> + for (int i = set->nb_sd - 1; i >= 0; i--) {
>> + AVPacketSideData *sd = set->sd[i];
>> + if (sd->type != type)
>> + continue;
>> + av_free(set->sd[i]->data);
>> + av_free(set->sd[i]);
>> + set->sd[i] = set->sd[--set->nb_sd];
>> + break;
>> + }
>> +}
>> +
>> +void av_packet_side_data_set_free(AVPacketSideDataSet *set)
>> +{
>> + for (int i = 0; i < set->nb_sd; i++) {
>> + av_free(set->sd[i]->data);
>> + av_free(set->sd[i]);
>> + }
>> + set->nb_sd = 0;
>> +
>> + av_freep(&set->sd);
>> +}
>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
>> index f28e7e7011..87720ab909 100644
>> --- a/libavcodec/packet.h
>> +++ b/libavcodec/packet.h
>> @@ -318,6 +318,20 @@ typedef struct AVPacketSideData {
>> enum AVPacketSideDataType type;
>> } AVPacketSideData;
>>
>> +/**
>> + * Structure to hold a set of AVPacketSideData
>
> This should mention whether it is legal or not to have multiple side
> datas of the same type in the same set.
Ok.
>
>> + *
>> + * @see av_packet_side_data_set_new
>> + * @see av_packet_side_data_set_add
>> + * @see av_packet_side_data_set_get
>> + * @see av_packet_side_data_set_remove
>> + * @see av_packet_side_data_set_free
>> + */
>
> This should mention that its size is part of the ABI.
Ok.
>
>> +typedef struct AVPacketSideDataSet {
>> + AVPacketSideData **sd;
>
> Allocating the AVPacketSideDatas independently is a big break from how
> it is done with AVPackets. Is this really intended? IMO not worth it.
I did it to have this struct in line with Jan's frame one (and frame
side data in general). This way helpers can also share a common signature.
>
>> + int nb_sd;
>
> int? Why not something unsigned given that it will never be negative?
Entry count is int pretty much everywhere. Is it worth making it
unsigned only here?
>
>> +} AVPacketSideDataSet;
>> +
>> /**
>> * This structure stores compressed data. It is typically exported by demuxers
>> * and then passed as input to decoders, or received as output from encoders and
>> @@ -724,6 +738,66 @@ int av_packet_make_writable(AVPacket *pkt);
>> */
>> void av_packet_rescale_ts(AVPacket *pkt, AVRational tb_src, AVRational tb_dst);
>>
>> +/**
>> + * Allocate a new side data entry into to a set.
>> + *
>> + * @param set a set to which the side data should be added
>> + * @param type side data type
>> + * @param size side data size
>> + * @param flags currently unused
>
> must be zero
Will add.
>
>> + * @return pointer to freshly allocated side data entry on success, or NULL
>> + * otherwise.
>> + */
>> +AVPacketSideData *av_packet_side_data_set_new(AVPacketSideDataSet *set,
>> + enum AVPacketSideDataType type,
>> + size_t size, int flags);
>> +
>> +/**
>> + * Wrap an existing array as a packet side data into a set.
>> + *
>> + * @param set a set to which the side data should be added
>> + * @param type side data type
>> + * @param data a data array. It must be allocated with the av_malloc() family
>> + * of functions. The ownership of the data is transferred to the
>> + * set on success
>> + * @param size size of the data array
>> + * @param flags currently unused
>> + * @return pointer to freshly allocated side data entry on success, or NULL
>> + * otherwise. On failure, the set is unchanged and the data remains
>> + * owned by the caller.
>> + */
>> +AVPacketSideData *av_packet_side_data_set_add(AVPacketSideDataSet *set,
>> + enum AVPacketSideDataType type,
>> + uint8_t *data, size_t size,
>
> data should be void*; the days in which side data was just a buffer and
> not a structure are long gone.
Ok.
> Changing AVPacketSideData is a different issue.
>
>> + int flags);
>> +
>> +/**
>> + * Remove side data of the given type from a set.
>> + *
>> + * @param set a set from which the side data should be removed
>> + * @param type side information type
>> + */
>> +void av_packet_side_data_set_remove(AVPacketSideDataSet *set,
>> + enum AVPacketSideDataType type);
>> +
>> +/**
>> + * Get side information from set.
>> + *
>> + * @param set a set from which the side data should be fetched
>> + * @param type desired side information type
>> + *
>> + * @return pointer to side data if present or NULL otherwise
>> + */
>> +AVPacketSideData *av_packet_side_data_set_get(const AVPacketSideDataSet *set,
>
> Is it really good to give the user the right to modify the entries of a
> set that they don't own?
Will make it const.
>
>> + enum AVPacketSideDataType type);
>> +
>> +/**
>> + * Convenience function to free all the side data stored in a set.
>> + *
>> + * @param set the set to free
>> + */
>> +void av_packet_side_data_set_free(AVPacketSideDataSet *set);
>> +
>> /**
>> * @}
>> */
This aside, do you have an opinion regarding the inclusion of a new
field of AVPacketSideDataSet to AVCodecContext, like in the previous
iteration of this set, instead of using coded_side_data? I really prefer
the former, as currently there's no way for users to allocate or fill
coded_side_data other than doing it manually, and that's pretty ugly and
unintuitive.
Since coded_side_data is barely used right now, deprecating it will be
completely painless.
Anton suggested dropping the set struct altogether and just use the
underlying fields directly in AVCodecParameters, then make the helpers
take IN|OUT pointer to pointer parameters to change the array and the
element count fields, which IMO is also pretty ugly.
More information about the ffmpeg-devel
mailing list