Re: [FFmpeg-devel] [PoC][PATCH 08/14] avutil/frame: add a param change side data type

[James Almer <jamrial@gmail.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 5420 bytes --]

On 1/29/2025 9:29 AM, Andreas Rheinhardt wrote:
> James Almer:
>> Similar in purpose as the packet side data of the same name, but for encoders.
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>> An example of RefCount used for side data, including a copy and uninit callback
>> to handle allocations within the entry.
>>
>>   libavutil/frame.h     | 14 ++++++++++++++
>>   libavutil/side_data.c | 23 +++++++++++++++++++++++
>>   2 files changed, 37 insertions(+)
>>
>> diff --git a/libavutil/frame.h b/libavutil/frame.h
>> index 1feb11506a..c677407781 100644
>> --- a/libavutil/frame.h
>> +++ b/libavutil/frame.h
>> @@ -243,8 +243,22 @@ enum AVFrameSideDataType {
>>        * The data is an int storing the view ID.
>>        */
>>       AV_FRAME_DATA_VIEW_ID,
>> +
>> +    /**
>> +     * A side data used to signal an encoder that certain parameters changed.
>> +     * The payload is an AVParamChange struct.
>> +     */
>> +    AV_FRAME_DATA_PARAM_CHANGE,
>>   };
>>   
>> +typedef struct AVParamChange {
>> +    /**
>> +     * A dictionary filled with options to be passed to an already initialized
>> +     * encoder. May contain global and encoder private options.
>> +     */
>> +    AVDictionary *opts;
>> +} AVParamChange;
>> +
>>   enum AVActiveFormatDescription {
>>       AV_AFD_SAME         = 8,
>>       AV_AFD_4_3          = 9,
>> diff --git a/libavutil/side_data.c b/libavutil/side_data.c
>> index 597228baf1..c156d2bef8 100644
>> --- a/libavutil/side_data.c
>> +++ b/libavutil/side_data.c
>> @@ -63,6 +63,24 @@ typedef struct FFSideDataDescriptor {
>>       size_t size;
>>   } FFSideDataDescriptor;
>>   
>> +/* Type specific refstruct callbacks */
>> +
>> +static int copy_param_change(void *_dst, const void *_src)
>> +{
>> +    const AVParamChange *src = _src;
>> +    AVParamChange *dst = _dst;
>> +    int ret = av_dict_copy(&dst->opts, src->opts, 0);
>> +    if (ret < 0)
>> +        av_dict_free(&dst->opts);
>> +    return ret;
>> +}
>> +
>> +static void uninit_param_change(AVRefStructOpaque opaque, void *obj)
>> +{
>> +    AVParamChange *param = obj;
>> +    av_dict_free(&param->opts);
>> +}
>> +
>>   static const FFSideDataDescriptor sd_props[] = {
>>       [AV_FRAME_DATA_PANSCAN]                     = { .p = { "AVPanScan",                                    AV_SIDE_DATA_PROP_STRUCT | AV_SIDE_DATA_PROP_SIZE_DEPENDENT },
>>                                                       .size = sizeof(AVPanScan) },
>> @@ -88,6 +106,11 @@ static const FFSideDataDescriptor sd_props[] = {
>>       [AV_FRAME_DATA_DOVI_METADATA]               = { .p = { "Dolby Vision Metadata",                        AV_SIDE_DATA_PROP_COLOR_DEPENDENT } },
>>       [AV_FRAME_DATA_LCEVC]                       = { .p = { "LCEVC NAL data",                               AV_SIDE_DATA_PROP_SIZE_DEPENDENT } },
>>       [AV_FRAME_DATA_VIEW_ID]                     = { .p = { "View ID" } },
>> +    [AV_FRAME_DATA_PARAM_CHANGE]                = { .p = { "Param Change",                                 AV_SIDE_DATA_PROP_STRUCT },
>> +                                                    .props = FF_SIDE_DATA_PROP_REFSTRUCT,
>> +                                                    .copy = copy_param_change,
>> +                                                    .uninit = uninit_param_change,
>> +                                                    .size = sizeof(AVParamChange) },
>>       [AV_FRAME_DATA_STEREO3D]                    = { .p = { "Stereo 3D",                                    AV_SIDE_DATA_PROP_GLOBAL | AV_SIDE_DATA_PROP_STRUCT },
>>                                                       .size = sizeof(AVStereo3D) },
>>       [AV_FRAME_DATA_REPLAYGAIN]                  = { .p = { "AVReplayGain",                                 AV_SIDE_DATA_PROP_GLOBAL | AV_SIDE_DATA_PROP_STRUCT },
> 
> I dislike the whole approach: Frame side data is supposed to contain
> side-data that applies to the frame; yet a lot of encoder options which
> will ultimately be added to this side data (if it gets committed) are
> not frame properties at all.

Fair enough.

> 
> Furthermore, this approach necessitates checking every frame (even of
> encoders that do not support parameter changes) for the existence of
> said side-data, although this side data will not exist in the

There's no need to check them. I just copied the check done for the 
packet side data of the same name, which aborts if it's present when it 
shouldn't. We can always just silently ignore it instead if the encoder 
doesn't care about it.

> overwhelming majority of cases. Why not add a new function for this
> task? (This does not imply that I am in favor of the reconfiguration
> approach at all. I'm still leaning towards "close+reopen".)
Like i said, there are some cases where it's better to do a graceful 
re-initialization for the sake of compression if you only care about 
changing a few values. If we can implement a way to do it that's not 
invasive, then it's IMO worth giving the option.
Right now, libx264 does it silently if it detects changes to the avctx 
or the private struct, which is not ok as it gives the user the 
impression it can be done for every encoder, when that's clearly not the 
case and can even result in crazy behavior for some.

I'll look into making it a function later.


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

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