Re: [FFmpeg-devel] [PATCH 00/10 v4][RFC] AVCodecContext and AVCodecParameters side data

[James Almer <jamrial@gmail.com>]

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