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

[Anton Khirnov <anton@khirnov.net>]

Quoting James Almer (2023-12-05 23:43:57)
> Starting with IAMF support.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  fftools/ffmpeg.h          |   2 +
>  fftools/ffmpeg_mux_init.c | 335 ++++++++++++++++++++++++++++++++++++++
>  fftools/ffmpeg_opt.c      |   2 +
>  3 files changed, 339 insertions(+)

Missing documentation.

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

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

> +                avformat_stream_group_add_stream(stg, oc->streams[idx]);

Unchecked return value.


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