[FFmpeg-devel] [PATCH 09/11] avcodec/decode: check for global side data in AVCodecContext side data

[James Almer]

On 10/3/2023 3:46 PM, James Almer wrote:
> On 10/3/2023 9:34 AM, Anton Khirnov wrote:
>> Quoting James Almer (2023-09-27 15:12:40)
>>> avcodec/decode: check for global side data in AVCodecContext side data
>>
>> I don't think this makes it clear what this commit actually does.
>> Make it something like 'propagate global side data to frames'.
>>
>>> Signed-off-by: James Almer <jamrial at gmail.com>
>>> ---
>>>   libavcodec/decode.c | 48 +++++++++++++++++++++++++++++++++++++--------
>>>   1 file changed, 40 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
>>> index a7196b5740..3b4bb70689 100644
>>> --- a/libavcodec/decode.c
>>> +++ b/libavcodec/decode.c
>>> @@ -1422,6 +1422,20 @@ static int add_metadata_from_side_data(const 
>>> AVPacket *avpkt, AVFrame *frame)
>>>       return av_packet_unpack_dictionary(side_metadata, size, frame_md);
>>>   }
>>> +static const struct {
>>> +    enum AVPacketSideDataType packet;
>>> +    enum AVFrameSideDataType frame;
>>> +} sd_global_map[] = {
>>> +    { AV_PKT_DATA_REPLAYGAIN ,                
>>> AV_FRAME_DATA_REPLAYGAIN },
>>> +    { AV_PKT_DATA_SPHERICAL,                  
>>> AV_FRAME_DATA_SPHERICAL },
>>> +    { AV_PKT_DATA_STEREO3D,                   AV_FRAME_DATA_STEREO3D },
>>> +    { AV_PKT_DATA_AUDIO_SERVICE_TYPE,         
>>> AV_FRAME_DATA_AUDIO_SERVICE_TYPE },
>>> +    { AV_PKT_DATA_MASTERING_DISPLAY_METADATA, 
>>> AV_FRAME_DATA_MASTERING_DISPLAY_METADATA },
>>> +    { AV_PKT_DATA_CONTENT_LIGHT_LEVEL,        
>>> AV_FRAME_DATA_CONTENT_LIGHT_LEVEL },
>>> +    { AV_PKT_DATA_ICC_PROFILE,                
>>> AV_FRAME_DATA_ICC_PROFILE },
>>> +    { AV_PKT_DATA_DYNAMIC_HDR10_PLUS,         
>>> AV_FRAME_DATA_DYNAMIC_HDR_PLUS },
>>> +};
>>> +
>>>   int ff_decode_frame_props_from_pkt(const AVCodecContext *avctx,
>>>                                      AVFrame *frame, const AVPacket 
>>> *pkt)
>>>   {
>>> @@ -1429,18 +1443,10 @@ int ff_decode_frame_props_from_pkt(const 
>>> AVCodecContext *avctx,
>>>           enum AVPacketSideDataType packet;
>>>           enum AVFrameSideDataType frame;
>>>       } sd[] = {
>>> -        { AV_PKT_DATA_REPLAYGAIN ,                
>>> AV_FRAME_DATA_REPLAYGAIN },
>>>           { AV_PKT_DATA_DISPLAYMATRIX,              
>>> AV_FRAME_DATA_DISPLAYMATRIX },
>>
>> Why are you leaving displaymatrix out?
> 
> See the code patch 10/11 removes. Display matrix in global side data is 
> apparently not meant to be propagated to frames. If i move this line to 
> the global map, a couple tests change their output due to new side data 
> in frames.
> If you want that to happen (After all, the CLI is not the only library 
> user), I'd rather remove the skipping code in the CLI first, with the 
> relevant test refs changes, so this patch is not the one to change them.

Actually, both tests use ffprobe, which never injected stream side data 
into packets before this set, so the ffmpeg code removed in 10/11 makes 
no difference.

Adding DISPLAYMATRIX to the global map here will still mean ffmpeg.c 
will pass stream side data to decoded frames, though, even if no test 
covers it. So it would be nice to know why was it being skipped.

> 
>>
>> Also, what happens if the same side data is present at both global and
>> packet level? Won't you get two instances in the frame?
>> I think the correct behaviour would be that packet overrides global.
> 
> I'm keeping the behavior from before this set intact with this at 
> Andreas' request.
> In a previous iteration i ensured packet side data would replace global 
> side data in the output frames, but Andreas asked me to not do that, as 
> a decoder could add that same side data type and it should have priority 
> over container level side data (All entries will be added to the frame 
> anyway).