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

From: James Almer <jamrial@gmail.com>
To: ffmpeg-devel@ffmpeg.org
Subject: Re: [FFmpeg-devel] [PATCH 09/11] avcodec/decode: check for global side data in AVCodecContext side data
Date: Tue, 3 Oct 2023 15:46:10 -0300
Message-ID: <8144112d-046f-46a8-9ee2-dbc26cdb6edf@gmail.com> (raw)
In-Reply-To: <169633649730.6638.7006599604326857996@lain.khirnov.net>

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@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.

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