| Message ID | 20230904220848.21900-1-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,01/17] avcodec/avcodec: add side data to AVCodecContext | expand |
Quoting James Almer (2023-09-05 00:08:48) > This will allow the propagation of global side data within the AVCodecContext > instead of having to do it inside packets, and thus be available during init(). > Global and frame specific side data will therefore be distinct. This commit message is misleading - there is already AVCodecContext.coded_side_data for exactly this purpose. And after the changes from the last iteration I see even less of a reason to replace it with a new field.
On 9/5/2023 8:07 AM, Anton Khirnov wrote: > Quoting James Almer (2023-09-05 00:08:48) >> This will allow the propagation of global side data within the AVCodecContext >> instead of having to do it inside packets, and thus be available during init(). >> Global and frame specific side data will therefore be distinct. > > This commit message is misleading - there is already > AVCodecContext.coded_side_data for exactly this purpose. And after the > changes from the last iteration I see even less of a reason to replace > it with a new field. I insist the new field in the form of a set is better, for the sake of unified helpers that can be used in avctx, codecpar, avstream, and potentially others in the future. It will also be the packet counterpart of Jan's frame side data set field. coded_side_data is currently used only to export CPB props, so the amount of users is probably very small (Maybe only lavf, even). I think the benefits in the long run outweigh the cons from the breakage that would mean replacing coded_side_data. Also, my interpretation of coded is still that it refers to a coded stream, much like we make a distinction between coded and raw for bits_per_sample, and in decoding scenarios, side data entries would have information that refer to the decoded raw stream (hdr, etc). That said, I don't want to keep delaying this set much longer, so if you're really against that change I'll try to remove it from the set and keep the rest.
Quoting James Almer (2023-09-05 13:26:22) > On 9/5/2023 8:07 AM, Anton Khirnov wrote: > > Quoting James Almer (2023-09-05 00:08:48) > >> This will allow the propagation of global side data within the AVCodecContext > >> instead of having to do it inside packets, and thus be available during init(). > >> Global and frame specific side data will therefore be distinct. > > > > This commit message is misleading - there is already > > AVCodecContext.coded_side_data for exactly this purpose. And after the > > changes from the last iteration I see even less of a reason to replace > > it with a new field. > > I insist the new field in the form of a set is better, for the sake of > unified helpers that can be used in avctx, codecpar, avstream, and > potentially others in the future. The big problem with this is that AVPacket is left as is. And since changing it would be a huge break for very little gain, we'll have different handling for packets and everything else for the foreseeable future. I think you'd get almost the same benefits with downsides by making the helpers accept array+count as parameters. It's slightly less elegant, but not hugely so IMO. > It will also be the packet counterpart of Jan's frame side data set > field. coded_side_data is currently used only to export CPB props, so > the amount of users is probably very small (Maybe only lavf, even). I > think the benefits in the long run outweigh the cons from the breakage > that would mean replacing coded_side_data. > > Also, my interpretation of coded is still that it refers to a coded > stream, much like we make a distinction between coded and raw for > bits_per_sample, and in decoding scenarios, side data entries would have > information that refer to the decoded raw stream (hdr, etc). The intent was for it to be a direct counterpart to AVStream.side_data, as is mentioned in the relevant commit message, so your interpretation is objectively wrong. > That said, I don't want to keep delaying this set much longer, so if > you're really against that change I'll try to remove it from the set and > keep the rest. I'd appreciate more opinions on this, from whoever cares.
On 9/5/2023 8:37 AM, Anton Khirnov wrote: > Quoting James Almer (2023-09-05 13:26:22) >> On 9/5/2023 8:07 AM, Anton Khirnov wrote: >>> Quoting James Almer (2023-09-05 00:08:48) >>>> This will allow the propagation of global side data within the AVCodecContext >>>> instead of having to do it inside packets, and thus be available during init(). >>>> Global and frame specific side data will therefore be distinct. >>> >>> This commit message is misleading - there is already >>> AVCodecContext.coded_side_data for exactly this purpose. And after the >>> changes from the last iteration I see even less of a reason to replace >>> it with a new field. >> >> I insist the new field in the form of a set is better, for the sake of >> unified helpers that can be used in avctx, codecpar, avstream, and >> potentially others in the future. > > The big problem with this is that AVPacket is left as is. And since > changing it would be a huge break for very little gain, we'll have > different handling for packets and everything else for the foreseeable > future. Packets and frames have their own helpers, so there will be a distinction between those and side data set helpers initially used in structs meant for global side data. The API user doesn't need to access the fields directly. > > I think you'd get almost the same benefits with downsides by making the > helpers accept array+count as parameters. It's slightly less elegant, > but not hugely so IMO. > >> It will also be the packet counterpart of Jan's frame side data set >> field. coded_side_data is currently used only to export CPB props, so >> the amount of users is probably very small (Maybe only lavf, even). I >> think the benefits in the long run outweigh the cons from the breakage >> that would mean replacing coded_side_data. >> >> Also, my interpretation of coded is still that it refers to a coded >> stream, much like we make a distinction between coded and raw for >> bits_per_sample, and in decoding scenarios, side data entries would have >> information that refer to the decoded raw stream (hdr, etc). > > The intent was for it to be a direct counterpart to AVStream.side_data, Since that one is being deprecated and removed, doing the same with its less used counterpart should not be that much of a problem. > as is mentioned in the relevant commit message, so your interpretation > is objectively wrong. Fair, but i question your choice of name :p > >> That said, I don't want to keep delaying this set much longer, so if >> you're really against that change I'll try to remove it from the set and >> keep the rest. > > I'd appreciate more opinions on this, from whoever cares. Ok, but like i mentioned, i really doubt there are many coded_side_data users out there, for being limited to only exporting CPB props.
diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c index 131834b6de..7e1ef99234 100644 --- a/libavcodec/avcodec.c +++ b/libavcodec/avcodec.c @@ -475,6 +475,8 @@ av_cold int avcodec_close(AVCodecContext *avctx) av_freep(&avctx->internal); } + av_packet_side_data_set_free(&avctx->packet_side_data); + 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 649411ac79..dda8a2412b 100644 --- a/libavcodec/avcodec.h +++ b/libavcodec/avcodec.h @@ -2100,6 +2100,14 @@ typedef struct AVCodecContext { * an error. */ int64_t frame_num; + + /** + * Additional data associated with the entire stream. + * + * - decoding: set by user + * - encoding: unused + */ + AVPacketSideDataSet packet_side_data; } AVCodecContext; /** 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..63b402d7ea 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 **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 +732,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 allow the propagation of global side data within the AVCodecContext instead of having to do it inside packets, and thus be available during init(). Global and frame specific side data will therefore be distinct. Signed-off-by: James Almer <jamrial@gmail.com> --- libavcodec/avcodec.c | 2 + libavcodec/avcodec.h | 8 ++++ libavcodec/avpacket.c | 99 +++++++++++++++++++++++++++++++++++++++++++ libavcodec/packet.h | 68 +++++++++++++++++++++++++++++ 4 files changed, 177 insertions(+)