| Message ID | 20230906174431.45558-1-jamrial@gmail.com |
|---|---|
| Headers | show |
| Series | AVCodecContext and AVCodecParameters side data | expand |
On 9/6/2023 2:44 PM, James Almer wrote: > Changes since the previous version: > - Removed the AVPacketSideDataSet from AVCodecContext, using instead the > existing coded_side_data field, at Anton's request. > > I still prefer using a new field of type AVPacketSideDataSet, given that the > user can currently only fill coded_side_data manually (See the implementation > in the codecpar copy functions, which non lavf users would need to replicate), > and more so considering coded_side_data is a field pretty much nothing but lavf > uses. > > Opinions are very welcome and needed. Ping.
Quoting James Almer (2023-09-06 19:44:20) > Changes since the previous version: > - Removed the AVPacketSideDataSet from AVCodecContext, using instead the > existing coded_side_data field, at Anton's request. > > I still prefer using a new field of type AVPacketSideDataSet, given that the > user can currently only fill coded_side_data manually (See the implementation > in the codecpar copy functions, which non lavf users would need to replicate), > and more so considering coded_side_data is a field pretty much nothing but lavf > uses. > > Opinions are very welcome and needed. To provide more context on the issue: AVPackets can have side data attached to them, in the form of AVPacket.{side_data,side_data_elems} array+count. Some of this side data can occasionally be stored in global headers, e.g. in containers or extradata. We have some limited support for this: * AVStream.{side_data,nb_side_data} array+count allows demuxers to export this to user, and muxers to accept it from the user * AVCodecContext.{coded_side_data,nb_coded_side_data} allows decoders to accept it from the user (in principle, in practice it is not used), and encoders to export it to the user (this is used, but not very much) The broad idea for this set is adding stream-global "coded/packet" side data to AVCodecParameters, so that it can be naturally communicated from demuxers to bitstream filters to decoders, and from encoders to bitstream filters to muxers. Since AVStream already contains an AVCodecParameters instance, the abovementioned AVStream.side_data becomes redundant and is deprecated, the muxer/demuxer stream-global side data would be passed through AVStream.codecpar. The original version proposed by James adds a new struct, that bundles the side data array+count, and a set of helpers operating on this struct. Then this new struct is added to AVCodecParameters and AVCodecContext, which replaces the abovementioned AVStream and AVCodecContext side data array+count. Notably AVPacket is left as-is, because its side data is widely used and replacing array+count by this new struct would be a very intrusive break for little gain. My objections to this approach are * coded side data is now stored differently in AVPacket and in everything else * the case for replacing AVCodecContext.coded_side_data does not seem sufficiently strong to me My alternative suggestion was: * avoid adding a new struct (which merely bundles array+count) * add a set of helpers that accept array+count as parameters and operate on them * use these helpers for all operations on side data across AVPacket, AVCodecContext, and AVCodecParameters I have a moderately strong preference for this approach, but James disagrees. More opinions are very much welcome.
On 9/14/2023 12:43 PM, Anton Khirnov wrote: > Quoting James Almer (2023-09-06 19:44:20) >> Changes since the previous version: >> - Removed the AVPacketSideDataSet from AVCodecContext, using instead the >> existing coded_side_data field, at Anton's request. >> >> I still prefer using a new field of type AVPacketSideDataSet, given that the >> user can currently only fill coded_side_data manually (See the implementation >> in the codecpar copy functions, which non lavf users would need to replicate), >> and more so considering coded_side_data is a field pretty much nothing but lavf >> uses. >> >> Opinions are very welcome and needed. > > To provide more context on the issue: > > AVPackets can have side data attached to them, in the form of > AVPacket.{side_data,side_data_elems} array+count. > > Some of this side data can occasionally be stored in global headers, > e.g. in containers or extradata. We have some limited support for this: > * AVStream.{side_data,nb_side_data} array+count allows demuxers to > export this to user, and muxers to accept it from the user > * AVCodecContext.{coded_side_data,nb_coded_side_data} allows decoders to > accept it from the user (in principle, in practice it is not used), > and encoders to export it to the user (this is used, but not very > much) > > The broad idea for this set is adding stream-global "coded/packet" side > data to AVCodecParameters, so that it can be naturally communicated from > demuxers to bitstream filters to decoders, and from encoders to > bitstream filters to muxers. Since AVStream already contains an > AVCodecParameters instance, the abovementioned AVStream.side_data > becomes redundant and is deprecated, the muxer/demuxer stream-global > side data would be passed through AVStream.codecpar. > > The original version proposed by James adds a new struct, that bundles > the side data array+count, and a set of helpers operating on this > struct. Then this new struct is added to AVCodecParameters and > AVCodecContext, which replaces the abovementioned AVStream and > AVCodecContext side data array+count. Notably AVPacket is left as-is, > because its side data is widely used and replacing array+count by this > new struct would be a very intrusive break for little gain. In the future, packet side data could be made ref counted. For this, we'd need to add a new AVBufferRef field to AVPacketSideData, which is currently tied to the ABI. Ideally, if this happens, sizeof(AVPacketSideData) would be made not a part of the ABI, putting it in line with the AVFrame counterpart. Doing this is pretty much not an option for the existing array+count fields in AVPacket and AVCodecContext, because everyone, from external API users to our own API users like the CLI last i checked, loop through it manually as nothing prevents them to. But if a new field of the new set struct is added as replacement (a struct where the AVPacketSideData field is an array of pointers, and whose helpers can be defined as "must use to handle this struct"), then this would not be an issue. I probably said it several times, including in the above paragraph, but one of my objectives is trying to sync frame and packet side data structs, fields and helpers, since right now both are considerably different. Jan has a patchset also introducing a side data set for frames, so this is the time to try and align both as much as possible (which i already talked to him about) and laying the ground for future changes. > > My objections to this approach are > * coded side data is now stored differently in AVPacket and in > everything else > * the case for replacing AVCodecContext.coded_side_data does not seem > sufficiently strong to me It's used only by a handful of encoders to export CPB side data, which probably only lavf looks at, and by no decoder at all. It's a field that would be completely painless to replace. > > My alternative suggestion was: > * avoid adding a new struct (which merely bundles array+count) > * add a set of helpers that accept array+count as parameters and operate > on them See above my comment about sizeof(AVPacketSideData) and users accessing the array it directly. > * use these helpers for all operations on side data across AVPacket, > AVCodecContext, and AVCodecParameters > > I have a moderately strong preference for this approach, but James > disagrees. More opinions are very much welcome. >
On 9/14/2023 1:40 PM, James Almer wrote: > On 9/14/2023 12:43 PM, Anton Khirnov wrote: >> Quoting James Almer (2023-09-06 19:44:20) >>> Changes since the previous version: >>> - Removed the AVPacketSideDataSet from AVCodecContext, using instead the >>> existing coded_side_data field, at Anton's request. >>> >>> I still prefer using a new field of type AVPacketSideDataSet, given >>> that the >>> user can currently only fill coded_side_data manually (See the >>> implementation >>> in the codecpar copy functions, which non lavf users would need to >>> replicate), >>> and more so considering coded_side_data is a field pretty much >>> nothing but lavf >>> uses. >>> >>> Opinions are very welcome and needed. >> >> To provide more context on the issue: >> >> AVPackets can have side data attached to them, in the form of >> AVPacket.{side_data,side_data_elems} array+count. >> >> Some of this side data can occasionally be stored in global headers, >> e.g. in containers or extradata. We have some limited support for this: >> * AVStream.{side_data,nb_side_data} array+count allows demuxers to >> export this to user, and muxers to accept it from the user >> * AVCodecContext.{coded_side_data,nb_coded_side_data} allows decoders to >> accept it from the user (in principle, in practice it is not used), >> and encoders to export it to the user (this is used, but not very >> much) >> >> The broad idea for this set is adding stream-global "coded/packet" side >> data to AVCodecParameters, so that it can be naturally communicated from >> demuxers to bitstream filters to decoders, and from encoders to >> bitstream filters to muxers. Since AVStream already contains an >> AVCodecParameters instance, the abovementioned AVStream.side_data >> becomes redundant and is deprecated, the muxer/demuxer stream-global >> side data would be passed through AVStream.codecpar. >> >> The original version proposed by James adds a new struct, that bundles >> the side data array+count, and a set of helpers operating on this >> struct. Then this new struct is added to AVCodecParameters and >> AVCodecContext, which replaces the abovementioned AVStream and >> AVCodecContext side data array+count. Notably AVPacket is left as-is, >> because its side data is widely used and replacing array+count by this >> new struct would be a very intrusive break for little gain. > > In the future, packet side data could be made ref counted. For this, > we'd need to add a new AVBufferRef field to AVPacketSideData, which is > currently tied to the ABI. > Ideally, if this happens, sizeof(AVPacketSideData) would be made not a > part of the ABI, putting it in line with the AVFrame counterpart. Doing > this is pretty much not an option for the existing array+count fields in > AVPacket and AVCodecContext, because everyone, from external API users > to our own API users like the CLI last i checked, loop through it > manually as nothing prevents them to. But if a new field of the new set > struct is added as replacement (a struct where the AVPacketSideData > field is an array of pointers, and whose helpers can be defined as "must > use to handle this struct"), then this would not be an issue. > > I probably said it several times, including in the above paragraph, but > one of my objectives is trying to sync frame and packet side data > structs, fields and helpers, since right now both are considerably > different. > Jan has a patchset also introducing a side data set for frames, so this > is the time to try and align both as much as possible (which i already > talked to him about) and laying the ground for future changes. > >> >> My objections to this approach are >> * coded side data is now stored differently in AVPacket and in >> everything else >> * the case for replacing AVCodecContext.coded_side_data does not seem >> sufficiently strong to me > > It's used only by a handful of encoders to export CPB side data, which > probably only lavf looks at, and by no decoder at all. It's a field that > would be completely painless to replace. > >> >> My alternative suggestion was: >> * avoid adding a new struct (which merely bundles array+count) >> * add a set of helpers that accept array+count as parameters and operate >> on them > > See above my comment about sizeof(AVPacketSideData) and users accessing > the array it directly. > >> * use these helpers for all operations on side data across AVPacket, >> AVCodecContext, and AVCodecParameters >> >> I have a moderately strong preference for this approach, but James >> disagrees. More opinions are very much welcome. Ping. We really need opinions on what approach to use. Here's some visual representation of what is being discussed. Currently, things look like this: ######### Side data definition ----- struct AVPacketSideData { uint8_t *data; size_t size; enum AVPacketSideDataType type; }; ----- Users ----- struct AVCodecContext { [...] } struct AVCodecParameters { [...] } struct AVPacket { [...] AVPacketSideData *side_data; int side_data_elems; [...] } uint8_t* av_packet_new_side_data(AVPacket *pkt, enum AVPacketSideDataType type, size_t size); int av_packet_add_side_data(AVPacket *pkt, enum AVPacketSideDataType type, uint8_t *data, size_t size); uint8_t* av_packet_get_side_data(const AVPacket *pkt, enum AVPacketSideDataType type, size_t *size); struct AVStream { [...] AVCodecParameters *codec_par; AVPacketSideData *side_data; int nb_side_data; [...] }; uint8_t* av_stream_new_side_data(AVStream *stream, enum AVPacketSideDataType type, size_t size); int av_stream_add_side_data(AVStream *st, enum AVPacketSideDataType type, uint8_t *data, size_t size); uint8_t* av_stream_get_side_data(const AVStream *stream, enum AVPacketSideDataType type, size_t *size); ----- ######### The only way for global side data exported by demuxers to hit decoders or bsfs is by injecting it into the first output packet. This means it's not available at init(). The solution is obviously being able to store the side data in the decoder context, which the caller would copy from the demuxer context. My suggestion is to introduce a new struct to hold a set of side data, and helpers that work with it, whereas Anton's suggestion is to just storing the pointer to side data array and side data array size directly without wrapping them. My suggestion would have things look like this: ######### Side data definition ----- struct AVPacketSideData { uint8_t *data; size_t size; enum AVPacketSideDataType type; }; struct AVPacketSideDataSet { AVPacketSideData **sd; int nb_sd; }; AVPacketSideData *av_packet_side_data_set_new(AVPacketSideDataSet *set, enum AVPacketSideDataType type, size_t size); AVPacketSideData *av_packet_side_data_set_add(AVPacketSideDataSet *set, enum AVPacketSideDataType type, uint8_t *data, size_t size); AVPacketSideData *av_packet_side_data_set_get(AVPacketSideDataSet *set, enum AVPacketSideDataType type); void av_packet_side_data_set_remove(AVPacketSideDataSet *set, enum AVPacketSideDataType type); void av_packet_side_data_set_uninit(AVPacketSideDataSet *set); ----- Users ----- struct AVCodecContext { [...] AVPacketSideDataSet *side_data; [...] } struct AVCodecParameters { [...] AVPacketSideDataSet *side_data; [...] } AVPacket and its helpers untouched. struct AVStream { [...] AVCodecParameters *codec_par; [...] }; ----- In which you'd do for example av_packet_side_data_set_new(avctx->side_data, AV_PKT_DATA_DISPLAYMATRIX, sizeof(int32_t) * 9); av_packet_side_data_set_new(st->codec_par->side_data, AV_PKT_DATA_DISPLAYMATRIX, sizeof(int32_t) * 9); av_packet_side_data_set_get(avctx->side_data, AV_PKT_DATA_DISPLAYMATRIX); av_packet_side_data_set_get(st->codec_par->side_data, AV_PKT_DATA_DISPLAYMATRIX); ######### Whereas Anton's would have things look like this: ######### Side data definition ----- struct AVPacketSideData { uint8_t *data; size_t size; enum AVPacketSideDataType type; }; AVPacketSideData *av_packet_side_data_set_new( AVPacketSideData **sd, // INOUT size_t *sd_size, // INOUT enum AVPacketSideDataType type, size_t size); AVPacketSideData *av_packet_side_data_set_add( AVPacketSideData **sd, // INOUT size_t *sd_size, // INOUT enum AVPacketSideDataType type, uint8_t *data, size_t size); AVPacketSideData *av_packet_side_data_set_get(AVPacketSideData *sd, size_t sd_size, enum AVPacketSideDataType type); void av_packet_side_data_set_remove(AVPacketSideData *sd, size_t *sd_size, enum AVPacketSideDataType type); void av_packet_side_data_set_uninit(AVPacketSideData *sd); ----- Users ----- struct AVCodecContext { [...] AVPacketSideData *coded_side_data; int nb_coded_side_data; [...] } struct AVCodecParameters { [...] AVPacketSideData *side_data; int nb_side_data; [...] } AVPacket and its helpers untouched. struct AVStream { [...] AVCodecParameters *codec_par; [...] }; ----- In which you'd do for example av_packet_side_data_set_new(&avctx->coded_side_data, &avctx->nb_coded_side_data, AV_PKT_DATA_DISPLAYMATRIX, av_packet_side_data_set_new(&st->codec_par->side_data, &st->codec_par->nb_side_data, AV_PKT_DATA_DISPLAYMATRIX, sizeof(int32_t) * 9); av_packet_side_data_set_get(avctx->coded_side_data, avctx->nb_coded_side_data, AV_PKT_DATA_DISPLAYMATRIX); av_packet_side_data_set_get(st->codec_par->side_data, st->codec_par->nb_side_data, AV_PKT_DATA_DISPLAYMATRIX); ######### Does anyone have a preference?
On 9/25/23, James Almer <jamrial@gmail.com> wrote: > On 9/14/2023 1:40 PM, James Almer wrote: >> On 9/14/2023 12:43 PM, Anton Khirnov wrote: >>> Quoting James Almer (2023-09-06 19:44:20) >>>> Changes since the previous version: >>>> - Removed the AVPacketSideDataSet from AVCodecContext, using instead the >>>> existing coded_side_data field, at Anton's request. >>>> >>>> I still prefer using a new field of type AVPacketSideDataSet, given >>>> that the >>>> user can currently only fill coded_side_data manually (See the >>>> implementation >>>> in the codecpar copy functions, which non lavf users would need to >>>> replicate), >>>> and more so considering coded_side_data is a field pretty much >>>> nothing but lavf >>>> uses. >>>> >>>> Opinions are very welcome and needed. >>> >>> To provide more context on the issue: >>> >>> AVPackets can have side data attached to them, in the form of >>> AVPacket.{side_data,side_data_elems} array+count. >>> >>> Some of this side data can occasionally be stored in global headers, >>> e.g. in containers or extradata. We have some limited support for this: >>> * AVStream.{side_data,nb_side_data} array+count allows demuxers to >>> export this to user, and muxers to accept it from the user >>> * AVCodecContext.{coded_side_data,nb_coded_side_data} allows decoders to >>> accept it from the user (in principle, in practice it is not used), >>> and encoders to export it to the user (this is used, but not very >>> much) >>> >>> The broad idea for this set is adding stream-global "coded/packet" side >>> data to AVCodecParameters, so that it can be naturally communicated from >>> demuxers to bitstream filters to decoders, and from encoders to >>> bitstream filters to muxers. Since AVStream already contains an >>> AVCodecParameters instance, the abovementioned AVStream.side_data >>> becomes redundant and is deprecated, the muxer/demuxer stream-global >>> side data would be passed through AVStream.codecpar. >>> >>> The original version proposed by James adds a new struct, that bundles >>> the side data array+count, and a set of helpers operating on this >>> struct. Then this new struct is added to AVCodecParameters and >>> AVCodecContext, which replaces the abovementioned AVStream and >>> AVCodecContext side data array+count. Notably AVPacket is left as-is, >>> because its side data is widely used and replacing array+count by this >>> new struct would be a very intrusive break for little gain. >> >> In the future, packet side data could be made ref counted. For this, >> we'd need to add a new AVBufferRef field to AVPacketSideData, which is >> currently tied to the ABI. >> Ideally, if this happens, sizeof(AVPacketSideData) would be made not a >> part of the ABI, putting it in line with the AVFrame counterpart. Doing >> this is pretty much not an option for the existing array+count fields in >> AVPacket and AVCodecContext, because everyone, from external API users >> to our own API users like the CLI last i checked, loop through it >> manually as nothing prevents them to. But if a new field of the new set >> struct is added as replacement (a struct where the AVPacketSideData >> field is an array of pointers, and whose helpers can be defined as "must >> use to handle this struct"), then this would not be an issue. >> >> I probably said it several times, including in the above paragraph, but >> one of my objectives is trying to sync frame and packet side data >> structs, fields and helpers, since right now both are considerably >> different. >> Jan has a patchset also introducing a side data set for frames, so this >> is the time to try and align both as much as possible (which i already >> talked to him about) and laying the ground for future changes. >> >>> >>> My objections to this approach are >>> * coded side data is now stored differently in AVPacket and in >>> everything else >>> * the case for replacing AVCodecContext.coded_side_data does not seem >>> sufficiently strong to me >> >> It's used only by a handful of encoders to export CPB side data, which >> probably only lavf looks at, and by no decoder at all. It's a field that >> would be completely painless to replace. >> >>> >>> My alternative suggestion was: >>> * avoid adding a new struct (which merely bundles array+count) >>> * add a set of helpers that accept array+count as parameters and operate >>> on them >> >> See above my comment about sizeof(AVPacketSideData) and users accessing >> the array it directly. >> >>> * use these helpers for all operations on side data across AVPacket, >>> AVCodecContext, and AVCodecParameters >>> >>> I have a moderately strong preference for this approach, but James >>> disagrees. More opinions are very much welcome. > > Ping. We really need opinions on what approach to use. > > Here's some visual representation of what is being discussed. > Currently, things look like this: > > ######### > Side data definition > ----- > struct AVPacketSideData { > uint8_t *data; > size_t size; > enum AVPacketSideDataType type; > }; > ----- > > Users > ----- > struct AVCodecContext { > [...] > } > > struct AVCodecParameters { > [...] > } > > struct AVPacket { > [...] > AVPacketSideData *side_data; > int side_data_elems; > [...] > } > uint8_t* av_packet_new_side_data(AVPacket *pkt, enum > AVPacketSideDataType type, > size_t size); > int av_packet_add_side_data(AVPacket *pkt, > enum AVPacketSideDataType type, > uint8_t *data, size_t size); > uint8_t* av_packet_get_side_data(const AVPacket *pkt, > enum AVPacketSideDataType type, > size_t *size); > > struct AVStream { > [...] > AVCodecParameters *codec_par; > AVPacketSideData *side_data; > int nb_side_data; > [...] > }; > uint8_t* av_stream_new_side_data(AVStream *stream, > enum AVPacketSideDataType type, > size_t size); > int av_stream_add_side_data(AVStream *st, > enum AVPacketSideDataType type, > uint8_t *data, size_t size); > uint8_t* av_stream_get_side_data(const AVStream *stream, > enum AVPacketSideDataType type, > size_t *size); > ----- > ######### > > The only way for global side data exported by demuxers to hit decoders > or bsfs is by injecting it into the first output packet. This means it's > not available at init(). > The solution is obviously being able to store the side data in the > decoder context, which the caller would copy from the demuxer context. > My suggestion is to introduce a new struct to hold a set of side data, > and helpers that work with it, whereas Anton's suggestion is to just > storing the pointer to side data array and side data array size directly > without wrapping them. > > My suggestion would have things look like this: > > ######### > Side data definition > ----- > struct AVPacketSideData { > uint8_t *data; > size_t size; > enum AVPacketSideDataType type; > }; > > struct AVPacketSideDataSet { > AVPacketSideData **sd; > int nb_sd; > }; > > AVPacketSideData *av_packet_side_data_set_new(AVPacketSideDataSet *set, > enum AVPacketSideDataType type, > size_t size); > AVPacketSideData *av_packet_side_data_set_add(AVPacketSideDataSet *set, > enum AVPacketSideDataType type, > uint8_t *data, size_t size); > AVPacketSideData *av_packet_side_data_set_get(AVPacketSideDataSet *set, > enum AVPacketSideDataType type); > void av_packet_side_data_set_remove(AVPacketSideDataSet *set, > enum AVPacketSideDataType type); > void av_packet_side_data_set_uninit(AVPacketSideDataSet *set); > ----- > > Users > ----- > struct AVCodecContext { > [...] > AVPacketSideDataSet *side_data; > [...] > } > > struct AVCodecParameters { > [...] > AVPacketSideDataSet *side_data; > [...] > } > > AVPacket and its helpers untouched. > > struct AVStream { > [...] > AVCodecParameters *codec_par; > [...] > }; > ----- > > In which you'd do for example > > av_packet_side_data_set_new(avctx->side_data, > AV_PKT_DATA_DISPLAYMATRIX, > sizeof(int32_t) * 9); > av_packet_side_data_set_new(st->codec_par->side_data, > AV_PKT_DATA_DISPLAYMATRIX, > sizeof(int32_t) * 9); > av_packet_side_data_set_get(avctx->side_data, > AV_PKT_DATA_DISPLAYMATRIX); > av_packet_side_data_set_get(st->codec_par->side_data, > AV_PKT_DATA_DISPLAYMATRIX); > ######### > > Whereas Anton's would have things look like this: > > ######### > Side data definition > ----- > struct AVPacketSideData { > uint8_t *data; > size_t size; > enum AVPacketSideDataType type; > }; > > AVPacketSideData *av_packet_side_data_set_new( > AVPacketSideData **sd, // INOUT > size_t *sd_size, // INOUT > enum AVPacketSideDataType type, > size_t size); > AVPacketSideData *av_packet_side_data_set_add( > AVPacketSideData **sd, // INOUT > size_t *sd_size, // INOUT > enum AVPacketSideDataType type, > uint8_t *data, size_t size); > AVPacketSideData *av_packet_side_data_set_get(AVPacketSideData *sd, > size_t sd_size, > enum AVPacketSideDataType type); > void av_packet_side_data_set_remove(AVPacketSideData *sd, > size_t *sd_size, > enum AVPacketSideDataType type); > void av_packet_side_data_set_uninit(AVPacketSideData *sd); > ----- > > Users > ----- > struct AVCodecContext { > [...] > AVPacketSideData *coded_side_data; > int nb_coded_side_data; > [...] > } > > struct AVCodecParameters { > [...] > AVPacketSideData *side_data; > int nb_side_data; > [...] > } > > AVPacket and its helpers untouched. > > struct AVStream { > [...] > AVCodecParameters *codec_par; > [...] > }; > ----- > > In which you'd do for example > > av_packet_side_data_set_new(&avctx->coded_side_data, > &avctx->nb_coded_side_data, > AV_PKT_DATA_DISPLAYMATRIX, > av_packet_side_data_set_new(&st->codec_par->side_data, > &st->codec_par->nb_side_data, > AV_PKT_DATA_DISPLAYMATRIX, > sizeof(int32_t) * 9); > av_packet_side_data_set_get(avctx->coded_side_data, > avctx->nb_coded_side_data, > AV_PKT_DATA_DISPLAYMATRIX); > av_packet_side_data_set_get(st->codec_par->side_data, > st->codec_par->nb_side_data, > AV_PKT_DATA_DISPLAYMATRIX); > ######### > > Does anyone have a preference? Addition of newer API to just reduce number of arguments when using it looks to me too much for little gain. > _______________________________________________ > 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". >
On 9/25/2023 5:02 PM, Paul B Mahol wrote: > On 9/25/23, James Almer <jamrial@gmail.com> wrote: >> On 9/14/2023 1:40 PM, James Almer wrote: >>> On 9/14/2023 12:43 PM, Anton Khirnov wrote: >>>> Quoting James Almer (2023-09-06 19:44:20) >>>>> Changes since the previous version: >>>>> - Removed the AVPacketSideDataSet from AVCodecContext, using instead the >>>>> existing coded_side_data field, at Anton's request. >>>>> >>>>> I still prefer using a new field of type AVPacketSideDataSet, given >>>>> that the >>>>> user can currently only fill coded_side_data manually (See the >>>>> implementation >>>>> in the codecpar copy functions, which non lavf users would need to >>>>> replicate), >>>>> and more so considering coded_side_data is a field pretty much >>>>> nothing but lavf >>>>> uses. >>>>> >>>>> Opinions are very welcome and needed. >>>> >>>> To provide more context on the issue: >>>> >>>> AVPackets can have side data attached to them, in the form of >>>> AVPacket.{side_data,side_data_elems} array+count. >>>> >>>> Some of this side data can occasionally be stored in global headers, >>>> e.g. in containers or extradata. We have some limited support for this: >>>> * AVStream.{side_data,nb_side_data} array+count allows demuxers to >>>> export this to user, and muxers to accept it from the user >>>> * AVCodecContext.{coded_side_data,nb_coded_side_data} allows decoders to >>>> accept it from the user (in principle, in practice it is not used), >>>> and encoders to export it to the user (this is used, but not very >>>> much) >>>> >>>> The broad idea for this set is adding stream-global "coded/packet" side >>>> data to AVCodecParameters, so that it can be naturally communicated from >>>> demuxers to bitstream filters to decoders, and from encoders to >>>> bitstream filters to muxers. Since AVStream already contains an >>>> AVCodecParameters instance, the abovementioned AVStream.side_data >>>> becomes redundant and is deprecated, the muxer/demuxer stream-global >>>> side data would be passed through AVStream.codecpar. >>>> >>>> The original version proposed by James adds a new struct, that bundles >>>> the side data array+count, and a set of helpers operating on this >>>> struct. Then this new struct is added to AVCodecParameters and >>>> AVCodecContext, which replaces the abovementioned AVStream and >>>> AVCodecContext side data array+count. Notably AVPacket is left as-is, >>>> because its side data is widely used and replacing array+count by this >>>> new struct would be a very intrusive break for little gain. >>> >>> In the future, packet side data could be made ref counted. For this, >>> we'd need to add a new AVBufferRef field to AVPacketSideData, which is >>> currently tied to the ABI. >>> Ideally, if this happens, sizeof(AVPacketSideData) would be made not a >>> part of the ABI, putting it in line with the AVFrame counterpart. Doing >>> this is pretty much not an option for the existing array+count fields in >>> AVPacket and AVCodecContext, because everyone, from external API users >>> to our own API users like the CLI last i checked, loop through it >>> manually as nothing prevents them to. But if a new field of the new set >>> struct is added as replacement (a struct where the AVPacketSideData >>> field is an array of pointers, and whose helpers can be defined as "must >>> use to handle this struct"), then this would not be an issue. >>> >>> I probably said it several times, including in the above paragraph, but >>> one of my objectives is trying to sync frame and packet side data >>> structs, fields and helpers, since right now both are considerably >>> different. >>> Jan has a patchset also introducing a side data set for frames, so this >>> is the time to try and align both as much as possible (which i already >>> talked to him about) and laying the ground for future changes. >>> >>>> >>>> My objections to this approach are >>>> * coded side data is now stored differently in AVPacket and in >>>> everything else >>>> * the case for replacing AVCodecContext.coded_side_data does not seem >>>> sufficiently strong to me >>> >>> It's used only by a handful of encoders to export CPB side data, which >>> probably only lavf looks at, and by no decoder at all. It's a field that >>> would be completely painless to replace. >>> >>>> >>>> My alternative suggestion was: >>>> * avoid adding a new struct (which merely bundles array+count) >>>> * add a set of helpers that accept array+count as parameters and operate >>>> on them >>> >>> See above my comment about sizeof(AVPacketSideData) and users accessing >>> the array it directly. >>> >>>> * use these helpers for all operations on side data across AVPacket, >>>> AVCodecContext, and AVCodecParameters >>>> >>>> I have a moderately strong preference for this approach, but James >>>> disagrees. More opinions are very much welcome. >> >> Ping. We really need opinions on what approach to use. >> >> Here's some visual representation of what is being discussed. >> Currently, things look like this: >> >> ######### >> Side data definition >> ----- >> struct AVPacketSideData { >> uint8_t *data; >> size_t size; >> enum AVPacketSideDataType type; >> }; >> ----- >> >> Users >> ----- >> struct AVCodecContext { >> [...] >> } >> >> struct AVCodecParameters { >> [...] >> } >> >> struct AVPacket { >> [...] >> AVPacketSideData *side_data; >> int side_data_elems; >> [...] >> } >> uint8_t* av_packet_new_side_data(AVPacket *pkt, enum >> AVPacketSideDataType type, >> size_t size); >> int av_packet_add_side_data(AVPacket *pkt, >> enum AVPacketSideDataType type, >> uint8_t *data, size_t size); >> uint8_t* av_packet_get_side_data(const AVPacket *pkt, >> enum AVPacketSideDataType type, >> size_t *size); >> >> struct AVStream { >> [...] >> AVCodecParameters *codec_par; >> AVPacketSideData *side_data; >> int nb_side_data; >> [...] >> }; >> uint8_t* av_stream_new_side_data(AVStream *stream, >> enum AVPacketSideDataType type, >> size_t size); >> int av_stream_add_side_data(AVStream *st, >> enum AVPacketSideDataType type, >> uint8_t *data, size_t size); >> uint8_t* av_stream_get_side_data(const AVStream *stream, >> enum AVPacketSideDataType type, >> size_t *size); >> ----- >> ######### >> >> The only way for global side data exported by demuxers to hit decoders >> or bsfs is by injecting it into the first output packet. This means it's >> not available at init(). >> The solution is obviously being able to store the side data in the >> decoder context, which the caller would copy from the demuxer context. >> My suggestion is to introduce a new struct to hold a set of side data, >> and helpers that work with it, whereas Anton's suggestion is to just >> storing the pointer to side data array and side data array size directly >> without wrapping them. >> >> My suggestion would have things look like this: >> >> ######### >> Side data definition >> ----- >> struct AVPacketSideData { >> uint8_t *data; >> size_t size; >> enum AVPacketSideDataType type; >> }; >> >> struct AVPacketSideDataSet { >> AVPacketSideData **sd; >> int nb_sd; >> }; >> >> AVPacketSideData *av_packet_side_data_set_new(AVPacketSideDataSet *set, >> enum AVPacketSideDataType type, >> size_t size); >> AVPacketSideData *av_packet_side_data_set_add(AVPacketSideDataSet *set, >> enum AVPacketSideDataType type, >> uint8_t *data, size_t size); >> AVPacketSideData *av_packet_side_data_set_get(AVPacketSideDataSet *set, >> enum AVPacketSideDataType type); >> void av_packet_side_data_set_remove(AVPacketSideDataSet *set, >> enum AVPacketSideDataType type); >> void av_packet_side_data_set_uninit(AVPacketSideDataSet *set); >> ----- >> >> Users >> ----- >> struct AVCodecContext { >> [...] >> AVPacketSideDataSet *side_data; >> [...] >> } >> >> struct AVCodecParameters { >> [...] >> AVPacketSideDataSet *side_data; >> [...] >> } >> >> AVPacket and its helpers untouched. >> >> struct AVStream { >> [...] >> AVCodecParameters *codec_par; >> [...] >> }; >> ----- >> >> In which you'd do for example >> >> av_packet_side_data_set_new(avctx->side_data, >> AV_PKT_DATA_DISPLAYMATRIX, >> sizeof(int32_t) * 9); >> av_packet_side_data_set_new(st->codec_par->side_data, >> AV_PKT_DATA_DISPLAYMATRIX, >> sizeof(int32_t) * 9); >> av_packet_side_data_set_get(avctx->side_data, >> AV_PKT_DATA_DISPLAYMATRIX); >> av_packet_side_data_set_get(st->codec_par->side_data, >> AV_PKT_DATA_DISPLAYMATRIX); >> ######### >> >> Whereas Anton's would have things look like this: >> >> ######### >> Side data definition >> ----- >> struct AVPacketSideData { >> uint8_t *data; >> size_t size; >> enum AVPacketSideDataType type; >> }; >> >> AVPacketSideData *av_packet_side_data_set_new( >> AVPacketSideData **sd, // INOUT >> size_t *sd_size, // INOUT >> enum AVPacketSideDataType type, >> size_t size); >> AVPacketSideData *av_packet_side_data_set_add( >> AVPacketSideData **sd, // INOUT >> size_t *sd_size, // INOUT >> enum AVPacketSideDataType type, >> uint8_t *data, size_t size); >> AVPacketSideData *av_packet_side_data_set_get(AVPacketSideData *sd, >> size_t sd_size, >> enum AVPacketSideDataType type); >> void av_packet_side_data_set_remove(AVPacketSideData *sd, >> size_t *sd_size, >> enum AVPacketSideDataType type); >> void av_packet_side_data_set_uninit(AVPacketSideData *sd); >> ----- >> >> Users >> ----- >> struct AVCodecContext { >> [...] >> AVPacketSideData *coded_side_data; >> int nb_coded_side_data; >> [...] >> } >> >> struct AVCodecParameters { >> [...] >> AVPacketSideData *side_data; >> int nb_side_data; >> [...] >> } >> >> AVPacket and its helpers untouched. >> >> struct AVStream { >> [...] >> AVCodecParameters *codec_par; >> [...] >> }; >> ----- >> >> In which you'd do for example >> >> av_packet_side_data_set_new(&avctx->coded_side_data, >> &avctx->nb_coded_side_data, >> AV_PKT_DATA_DISPLAYMATRIX, >> av_packet_side_data_set_new(&st->codec_par->side_data, >> &st->codec_par->nb_side_data, >> AV_PKT_DATA_DISPLAYMATRIX, >> sizeof(int32_t) * 9); >> av_packet_side_data_set_get(avctx->coded_side_data, >> avctx->nb_coded_side_data, >> AV_PKT_DATA_DISPLAYMATRIX); >> av_packet_side_data_set_get(st->codec_par->side_data, >> st->codec_par->nb_side_data, >> AV_PKT_DATA_DISPLAYMATRIX); >> ######### >> >> Does anyone have a preference? > > Addition of newer API to just reduce number of arguments when > using it looks to me too much for little gain. The difference also lies in the fact the new struct will hold an array of pointers of AVPacketSideData, meaning sizeof(AVPacketSideData) can be made non ABI fixed in the long run, whereas the side data in avctx and codecpar are and will be an array of AVPacketSideData, meaning sizeof(AVPacketSideData) must remain tied to the ABI. This means adding new fields becomes much harder, and we lose an opportunity to put AVPacketSideData in line with AVFrameSideData. And as i stated, Jan will add his own set of side data, for AVFrameSideData, and that one needs to be in a new set struct as the alternative is having helpers with arguments like "AVFrameSideData ***side_data".
Changes since the previous version: - Removed the AVPacketSideDataSet from AVCodecContext, using instead the existing coded_side_data field, at Anton's request. I still prefer using a new field of type AVPacketSideDataSet, given that the user can currently only fill coded_side_data manually (See the implementation in the codecpar copy functions, which non lavf users would need to replicate), and more so considering coded_side_data is a field pretty much nothing but lavf uses. Opinions are very welcome and needed. James Almer (10): avcodec/packet: add side data set struct and helpers avcodec/codec_par: add side data to AVCodecParameters avformat/avformat: use the side data from AVStream.codecpar fftools/ffmpeg: stop using AVStream.side_data fftools/ffplay: stop using AVStream.side_data fftools/ffprobe: stop using AVStream.side_data avcodec/hevcdec: check for DOVI configuration record in AVCodecContext side data avcodec/decode: check for global side data in AVCodecContext side data fftools/ffmpeg: stop injecting stream side data in packets fftools/ffplay: stop injecting stream side data in packets fftools/ffmpeg_demux.c | 29 +----- fftools/ffmpeg_enc.c | 31 ++---- fftools/ffmpeg_filter.c | 5 +- fftools/ffmpeg_mux_init.c | 19 ++-- fftools/ffplay.c | 10 +- fftools/ffprobe.c | 30 +++--- libavcodec/avcodec.h | 2 +- libavcodec/avpacket.c | 99 +++++++++++++++++++ libavcodec/codec_par.c | 81 +++++++++++++++ libavcodec/codec_par.h | 6 ++ libavcodec/decode.c | 55 +++++++++-- libavcodec/decode.h | 2 +- libavcodec/hevcdec.c | 15 ++- libavcodec/internal.h | 3 + libavcodec/packet.h | 74 ++++++++++++++ libavcodec/utils.c | 10 ++ libavdevice/android_camera.c | 9 +- libavformat/avformat.c | 42 ++------ libavformat/avformat.h | 40 +++++++- libavformat/concatdec.c | 1 - libavformat/dashdec.c | 11 --- libavformat/demux.c | 54 ++++++---- libavformat/demux_utils.c | 4 + libavformat/dovi_isom.c | 8 +- libavformat/dump.c | 6 +- libavformat/hls.c | 11 --- libavformat/hlsenc.c | 11 ++- libavformat/internal.h | 4 + libavformat/matroskadec.c | 45 ++++----- libavformat/matroskaenc.c | 48 +++++---- libavformat/mov.c | 81 ++++++++------- libavformat/movenc.c | 73 +++++++------- libavformat/mp3enc.c | 8 +- libavformat/mpegenc.c | 18 ++-- libavformat/mpegts.c | 8 +- libavformat/mux.c | 19 ++++ libavformat/mxfdec.c | 22 +++-- libavformat/mxfenc.c | 8 +- libavformat/options.c | 2 + libavformat/replaygain.c | 9 +- libavformat/seek.c | 2 + libavformat/version_major.h | 1 + tests/ref/fate/autorotate | 4 +- tests/ref/fate/copy-trac3074 | 2 +- tests/ref/fate/matroska-avoid-negative-ts | 2 +- tests/ref/fate/matroska-dovi-write-config7 | 2 +- tests/ref/fate/matroska-dovi-write-config8 | 2 +- tests/ref/fate/matroska-encoding-delay | 2 +- .../fate/matroska-mastering-display-metadata | 4 +- tests/ref/fate/matroska-spherical-mono-remux | 4 +- tests/ref/fate/matroska-stereo_mode | 8 +- tests/ref/fate/matroska-vp8-alpha-remux | 2 +- .../ref/fate/mov-mp4-disposition-mpegts-remux | 4 +- tests/ref/fate/mxf-d10-user-comments | 2 +- tests/ref/fate/mxf-remux-applehdr10 | 2 +- tests/ref/fate/vp9-superframe-bsf | 2 +- 56 files changed, 698 insertions(+), 360 deletions(-)