| Message ID | 20230906174431.45558-2-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | AVCodecContext and AVCodecParameters side data | expand |
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); > + > /** > * @} > */
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@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.
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); + + 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 + * + * @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 + */ +typedef struct AVPacketSideDataSet { + AVPacketSideData **sd; + int nb_sd; +} 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 + * @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, + 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, + 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 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(+)