Re: [FFmpeg-devel] [PATCH 01/13] avcodec/avcodec: add side data to AVCodecContext

[James Almer <jamrial@gmail.com>]

On 7/20/2023 5:34 PM, James Almer wrote:
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>   libavcodec/avcodec.c  |  2 ++
>   libavcodec/avcodec.h  |  8 +++++
>   libavcodec/avpacket.c | 84 +++++++++++++++++++++++++++++++++++++++++++
>   libavcodec/packet.h   | 54 ++++++++++++++++++++++++++++
>   4 files changed, 148 insertions(+)
> 
> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
> index 340abe830e..16869e97f2 100644
> --- a/libavcodec/avcodec.c
> +++ b/libavcodec/avcodec.c
> @@ -467,6 +467,8 @@ av_cold int avcodec_close(AVCodecContext *avctx)
>           av_freep(&avctx->internal);
>       }
>   
> +    av_packet_free_side_data_set(&avctx->side_data_set);
> +
>       for (i = 0; i < avctx->nb_coded_side_data; i++)
>           av_freep(&avctx->coded_side_data[i].data);
>       av_freep(&avctx->coded_side_data);
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index fe41ecc3c9..2902ecf6cb 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -2102,6 +2102,14 @@ typedef struct AVCodecContext {
>        *   an error.
>        */
>       int64_t frame_num;
> +
> +    /**
> +     * Additional data associated with the entire stream.
> +     *
> +     * - decoding: set by user
> +     * - encoding: unused
> +     */
> +    AVPacketSideDataSet side_data_set;

This name is also used in a patchset by Jan Ekström that added a similar 
struct but for AVFrameSideData. I used it here for this first iteration, 
but if his patchset also goes in, we need non conflicting names.

>   } AVCodecContext;
>   
>   /**
> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> index 5fef65e97a..f569731403 100644
> --- a/libavcodec/avpacket.c
> +++ b/libavcodec/avpacket.c
> @@ -645,3 +645,87 @@ int ff_side_data_set_prft(AVPacket *pkt, int64_t timestamp)
>   
>       return 0;
>   }
> +
> +AVPacketSideData *av_packet_get_side_data_from_set(const AVPacketSideDataSet *set,
> +                                                   enum AVPacketSideDataType type)
> +{
> +    for (int i = 0; i < set->nb_side_data; i++)
> +        if (set->side_data[i].type == type)
> +            return &set->side_data[i];
> +
> +    return NULL;
> +}
> +
> +static int add_side_data_to_set(AVPacketSideDataSet *set,
> +                                AVPacketSideData **psd,
> +                                enum AVPacketSideDataType type,
> +                                uint8_t *data, size_t size)
> +{
> +    AVPacketSideData *sd, *tmp;
> +
> +    for (int i = 0; i < set->nb_side_data; i++) {
> +        sd = &set->side_data[i];
> +
> +        if (sd->type == type) {
> +            av_freep(&sd->data);
> +            sd->data = data;
> +            sd->size = size;
> +            *psd = sd;
> +            return 0;
> +        }
> +    }
> +
> +    if (set->nb_side_data + 1U > FFMIN(INT_MAX, SIZE_MAX / sizeof(*tmp)))
> +        return AVERROR(ERANGE);
> +
> +    tmp = av_realloc_array(set->side_data, set->nb_side_data + 1, sizeof(*tmp));
> +    if (!tmp)
> +        return AVERROR(ENOMEM);
> +
> +    set->side_data = tmp;
> +    set->nb_side_data++;
> +
> +    sd = &set->side_data[set->nb_side_data - 1];
> +    sd->type = type;
> +    sd->data = data;
> +    sd->size = size;
> +    *psd = sd;
> +
> +    return 0;
> +}
> +
> +int av_packet_add_side_data_to_set(AVPacketSideDataSet *set,
> +                                   enum AVPacketSideDataType type,
> +                                   uint8_t *data, size_t size)
> +{
> +    AVPacketSideData *sd;
> +
> +    return add_side_data_to_set(set, &sd, type, data, size);
> +}
> +
> +AVPacketSideData *av_packet_new_side_data_to_set(AVPacketSideDataSet *set,
> +                                                 enum AVPacketSideDataType type,
> +                                                 size_t size)
> +{
> +    AVPacketSideData *sd = NULL;
> +    uint8_t *data = av_malloc(size);
> +    int ret;
> +
> +    if (!data)
> +        return NULL;
> +
> +    ret = add_side_data_to_set(set, &sd, type, data, size);
> +    if (ret < 0)
> +        av_freep(&data);
> +
> +    return sd;
> +}
> +
> +void av_packet_free_side_data_set(AVPacketSideDataSet *set)
> +{
> +    int i;
> +    for (i = 0; i < set->nb_side_data; i++)
> +        av_freep(&set->side_data[i].data);
> +    av_freep(&set->side_data);
> +    set->nb_side_data = 0;
> +}
> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
> index f28e7e7011..590c2bf15a 100644
> --- a/libavcodec/packet.h
> +++ b/libavcodec/packet.h
> @@ -318,6 +318,14 @@ typedef struct AVPacketSideData {
>       enum AVPacketSideDataType type;
>   } AVPacketSideData;
>   
> +/**
> + * Structure to hold a set of AVPacketSideDataSet
> + */
> +typedef struct AVPacketSideDataSet {
> +    AVPacketSideData *side_data;

I can alternatively make this pointer to pointer, to be in line with 
AVFrameSideData in AVFrame. It would make it simple to add a function to 
remove a single entry from the array, but it may not be worth the extra 
allocation overhead.

> +    int            nb_side_data;
> +} AVPacketSideDataSet;

The reason i introduced this struct is to simplify the helper functions 
defined below, to be generic and usable for both the avctx and codecpar 
side data arrays also introduced in this patchset.
I was told said helpers could take a pointer to pointer to 
AVPacketSideData and a pointer to a size_t, essentially emulating the 
struct as separate function arguments, but personally i find that pretty 
ugly.

In the long run, AVPacket could use this struct too, to make it 
consistent across the codebase. It would give us the chance to make 
packet side data reference counted too, as we can't expect existing 
users to change how they currently access avpacket->side_data, but 
that's a discussion for another time.

> +
>   /**
>    * 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 +732,52 @@ 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
> + * @return pointer to freshly allocated side data entry or NULL otherwise.
> + */
> +AVPacketSideData *av_packet_new_side_data_to_set(AVPacketSideDataSet *set,
> +                                                 enum AVPacketSideDataType type,
> +                                                 size_t size);

Alternatively, these functions could be 
av_packet_side_data_{new,add,get}(), to use a new namespace based on the 
new struct. Opinions welcome.

> +
> +/**
> + * 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 information type
> + * @param data the side data array. It must be allocated with the av_malloc()
> + *             family of functions. The ownership of the data is transferred to
> + *             pkt.
> + * @param size side information size
> + * @return a non-negative number on success, a negative AVERROR code on
> + *         failure. On failure, the set is unchanged and the data remains
> + *         owned by the caller.
> + */
> +int av_packet_add_side_data_to_set(AVPacketSideDataSet *set,
> +                                   enum AVPacketSideDataType type,
> +                                   uint8_t *data, size_t size);
> +
> +/**
> + * 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_get_side_data_from_set(const AVPacketSideDataSet *set,
> +                                                   enum AVPacketSideDataType type);
> +
> +/**
> + * Convenience function to free all the side data stored in a set.
> + *
> + * @param set a set
> + */
> +void av_packet_free_side_data_set(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".