[FFmpeg-devel] [PATCH 3/8] ffmpeg: add support for muxing AVStreamGroups

[James Almer]

On 12/11/2023 8:48 AM, Anton Khirnov wrote:
> Quoting James Almer (2023-12-05 23:43:57)
>> Starting with IAMF support.
>>
>> Signed-off-by: James Almer <jamrial at gmail.com>
>> ---
>>   fftools/ffmpeg.h          |   2 +
>>   fftools/ffmpeg_mux_init.c | 335 ++++++++++++++++++++++++++++++++++++++
>>   fftools/ffmpeg_opt.c      |   2 +
>>   3 files changed, 339 insertions(+)
> 
> Missing documentation.

Will do.

> 
>> +static int of_add_groups(Muxer *mux, const OptionsContext *o)
>> +{
>> +    AVFormatContext *oc = mux->fc;
>> +    int ret;
>> +
>> +    /* process manually set groups */
>> +    for (int i = 0; i < o->nb_stream_groups; i++) {
>> +        AVDictionary *dict = NULL, *tmp = NULL;
>> +        const AVDictionaryEntry *e;
>> +        AVStreamGroup *stg = NULL;
>> +        int type;
>> +        const char *token;
>> +        char *str, *ptr = NULL;
>> +        const AVOption opts[] = {
>> +            { "type", "Set group type", offsetof(AVStreamGroup, type), AV_OPT_TYPE_INT,
>> +                    { .i64 = 0 }, 0, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM, "type" },
>> +                { "iamf_audio_element",    NULL, 0, AV_OPT_TYPE_CONST,
>> +                    { .i64 = AV_STREAM_GROUP_PARAMS_IAMF_AUDIO_ELEMENT },    .unit = "type" },
>> +                { "iamf_mix_presentation", NULL, 0, AV_OPT_TYPE_CONST,
>> +                    { .i64 = AV_STREAM_GROUP_PARAMS_IAMF_MIX_PRESENTATION }, .unit = "type" },
>> +            { NULL },
>> +        };
>> +         const AVClass class = {
>> +            .class_name = "StreamGroupType",
>> +            .item_name  = av_default_item_name,
>> +            .option     = opts,
>> +            .version    = LIBAVUTIL_VERSION_INT,
>> +        };
>> +        const AVClass *pclass = &class;
>> +
>> +        str = av_strdup(o->stream_groups[i].u.str);
>> +        if (!str)
>> +            goto end;
>> +
>> +        token = av_strtok(str, ",", &ptr);
>> +        if (token) {
> 
> Too many indentation levels, move this whole block into a separate
> function.
> 
>> +            ret = av_dict_parse_string(&dict, token, "=", ":", AV_DICT_MULTIKEY);
>> +            if (ret < 0) {
>> +                av_log(mux, AV_LOG_ERROR, "Error parsing group specification %s\n", token);
>> +                goto end;
>> +            }
>> +
>> +            // "type" is not a user settable option in AVStreamGroup
> 
> This comment confuses me.

AVStreamGroup.type is not setteable through AVOptions, but it of course 
needs to be supported by the CLI. So i catch it and remove it from the 
dict that will be used for avformat_stream_group_create().

I can change the comment to "'type' is not a user settable AVOption".

> 
>> +            e = av_dict_get(dict, "type", NULL, 0);
>> +            if (!e) {
>> +                av_log(mux, AV_LOG_ERROR, "No type define for Steam Group %d\n", i);
> 
> 1) Steam
> 2) defined? Or maybe specified.

Will change to specified.

> 3) Print the string, not the index.
> 
>> +                ret = AVERROR(EINVAL);
>> +                goto end;
>> +            }
>> +
>> +            ret = av_opt_eval_int(&pclass, opts, e->value, &type);
>> +            if (ret < 0 || type == AV_STREAM_GROUP_PARAMS_NONE) {
>> +                av_log(mux, AV_LOG_ERROR, "Invalid group type \"%s\"\n", e->value);
>> +                goto end;
>> +            }
>> +
>> +            av_dict_copy(&tmp, dict, 0);
>> +            stg = avformat_stream_group_create(oc, type, &tmp);
>> +            if (!stg) {
>> +                ret = AVERROR(ENOMEM);
>> +                goto end;
>> +            }
>> +            av_dict_set(&tmp, "type", NULL, 0);
>> +
>> +            e = NULL;
>> +            while (e = av_dict_get(dict, "st", e, 0)) {
>> +                unsigned int idx = strtol(e->value, NULL, 0);
>> +                if (idx >= oc->nb_streams) {
>> +                    av_log(mux, AV_LOG_ERROR, "Invalid stream index %d\n", idx);
>> +                    ret = AVERROR(EINVAL);
>> +                    goto end;
>> +                }
> 
> This block seems confused about signedness of e->value.

You mean change %d to %u?

> 
>> +                avformat_stream_group_add_stream(stg, oc->streams[idx]);
> 
> Unchecked return value.
> 
>