[FFmpeg-devel] [PATCH 5/7] avcodec/encode: add a new param change side data value that takes a dictionary

[James Almer]

On 1/27/2025 10:26 AM, Andreas Rheinhardt wrote:
> James Almer:
>> Allow the caller to pass a dictionary with key/value pairs to set encoder
>> private options for reinitialization.
>> AVCodecContext global options are not affected. For that, specific
>> AVSideDataParamChangeFlags should be used, as is currently the case for
>> dimensions and sample rate.
>>
>> Signed-off-by: James Almer <jamrial at gmail.com>
>> ---
>>   libavcodec/encode.c | 21 +++++++++++++++++++++
>>   libavutil/defs.h    |  1 +
>>   2 files changed, 22 insertions(+)
>>
>> diff --git a/libavcodec/encode.c b/libavcodec/encode.c
>> index 187b4015f1..c14ab65509 100644
>> --- a/libavcodec/encode.c
>> +++ b/libavcodec/encode.c
>> @@ -20,12 +20,14 @@
>>   
>>   #include "libavutil/attributes.h"
>>   #include "libavutil/avassert.h"
>> +#include "libavutil/avstring.h"
>>   #include "libavutil/channel_layout.h"
>>   #include "libavutil/emms.h"
>>   #include "libavutil/frame.h"
>>   #include "libavutil/imgutils.h"
>>   #include "libavutil/internal.h"
>>   #include "libavutil/mem.h"
>> +#include "libavutil/opt.h"
>>   #include "libavutil/pixdesc.h"
>>   #include "libavutil/samplefmt.h"
>>   
>> @@ -101,6 +103,25 @@ static int apply_param_change(AVCodecContext *avctx, const AVFrame *frame)
>>           avctx->width  = frame->width;
>>           avctx->height = frame->height;
>>       }
>> +    if (flags & AV_SIDE_DATA_PARAM_CHANGE_DICT) {
>> +        AVDictionary *dict = NULL;
>> +        int left = av_strnlen(gbc.buffer, bytestream2_get_bytes_left(&gbc));
>> +        if (!left)> +            goto fail;
>> +        ret = av_dict_parse_string(&dict, gbc.buffer, "=", ":", 0);
>> +        if (ret < 0) {
>> +            av_log(avctx, AV_LOG_ERROR, "Invalid string");
>> +            goto fail2;
> 
> Potential leak of dict here and below.
> 
>> +        }
>> +        bytestream2_skip(&gbc, left + 1);
>> +
>> +        if (avctx->codec->priv_class) {
>> +            ret = av_opt_set_dict(avctx->priv_data, &dict);
>> +            if (ret < 0)
>> +                goto fail2;
> 
> 1. You are simply setting the options directly, without letting the
> codec know what the changed options are. This might make implementing
> this easy for the users you add in this patchset, but this may come back
> to bite us in the future.

I can make the dict an argument to FFCodec.reconf() to let the encoder 
handle it if you prefer.

> 2. What happens in case of error? The encoder state may be inconsistent.
> Is the user allowed to send more frames or is the user just supposed to
> free this AVCodecContext?
> 
>> +        }
>> +        av_dict_free(&dict);
>> +    }
>>   
>>       if (flags) {
>>           ret = 0;
>> diff --git a/libavutil/defs.h b/libavutil/defs.h
>> index f09fb5efd2..e1de680f2f 100644
>> --- a/libavutil/defs.h
>> +++ b/libavutil/defs.h
>> @@ -125,6 +125,7 @@ enum AVMediaType {
>>   enum AVSideDataParamChangeFlags {
>>       AV_SIDE_DATA_PARAM_CHANGE_SAMPLE_RATE    = 0x0004,
>>       AV_SIDE_DATA_PARAM_CHANGE_DIMENSIONS     = 0x0008,
>> +    AV_SIDE_DATA_PARAM_CHANGE_DICT           = 0x0016,
> 
> This does not explain the layout of this side data.

That's done in the side data type entry, like the two other flags.

fwiw: This approach (reusing the host-agnostic bitstream param change 
side data implementation from AVPacketSideData) was disliked by Lynne 
and Nicolas, so i sent a different one in a new patchset.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature.asc
Type: application/pgp-signature
Size: 495 bytes
Desc: OpenPGP digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20250127/96d27b2d/attachment.sig>