Re: [FFmpeg-devel] [PATCH 01/10] avcodec/packet: add side data set struct and helpers

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

James Almer:
> This will be useful in the following commits.
> 
> Signed-off-by: James Almer <jamrial@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?

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

> + *
> + * @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.

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

> +    int             nb_sd;

int? Why not something unsigned given that it will never be negative?

> +} 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

> + * @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.
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?

> +                                              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);
> +
>  /**
>   * @}
>   */

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