[FFmpeg-devel] [PATCH 1/5] avformat: introduce AVStreamGroup

[Marton Balint]

On Tue, 21 Nov 2023, James Almer wrote:

> Signed-off-by: James Almer <jamrial at gmail.com>
> ---
> libavformat/avformat.c | 178 +++++++++++++++++++++++++++++++++++++++--
> libavformat/avformat.h | 163 +++++++++++++++++++++++++++++++++++++
> libavformat/dump.c     |  33 ++++++--
> libavformat/internal.h |  33 ++++++++
> libavformat/options.c  |  90 +++++++++++++++++++++
> 5 files changed, 486 insertions(+), 11 deletions(-)
>
> diff --git a/libavformat/avformat.c b/libavformat/avformat.c
> index 5b8bb7879e..4e31924c71 100644
> --- a/libavformat/avformat.c
> +++ b/libavformat/avformat.c

[...]

> @@ -493,6 +524,46 @@ static int match_stream_specifier(const AVFormatContext *s, const AVStream *st,
>                 match = 0;
>             if (nopic && (st->disposition & AV_DISPOSITION_ATTACHED_PIC))
>                 match = 0;
> +        } else if (*spec == 'g' && *(spec + 1) == ':') {
> +            int64_t group_idx = -1, group_id = -1;
> +            int found = 0;
> +            char *endptr;
> +            spec += 2;
> +            if (*spec == '#' || (*spec == 'i' && *(spec + 1) == ':')) {
> +                spec += 1 + (*spec == 'i');
> +                group_id = strtol(spec, &endptr, 0);
> +                if (spec == endptr || (*endptr && *endptr++ != ':'))
> +                    return AVERROR(EINVAL);
> +                spec = endptr;
> +            } else {
> +                group_idx = strtol(spec, &endptr, 0);
> +                /* Disallow empty id and make sure that if we are not at the end, then another specifier must follow. */
> +                if (spec == endptr || (*endptr && *endptr++ != ':'))
> +                    return AVERROR(EINVAL);
> +                spec = endptr;
> +            }
> +            if (match) {
> +                if (group_id > 0) {
> +                    for (unsigned i = 0; i < s->nb_stream_groups; i++) {
> +                        if (group_id == s->stream_groups[i]->id) {
> +                            group_idx = i;
> +                            break;
> +                        }
> +                    }
> +			    }
> +                if (group_idx < 0 || group_idx > s->nb_stream_groups)
> +                    return AVERROR(EINVAL);
> +                for (unsigned j = 0; j < s->stream_groups[group_idx]->nb_streams; j++) {
> +                    if (st->index == s->stream_groups[group_idx]->streams[j]->index) {
> +                        found = 1;
> +                        if (g)
> +                            *g = s->stream_groups[group_idx];
> +                        break;
> +                    }
> +                }
> +            }
> +            if (!found)
> +                match = 0;

The documentation for the stream specifier changes are missing from
doc/fftools-common-opts.texi. You should update relevant docs preferably
in this patch...

[...]

> --- a/libavformat/avformat.h
> +++ b/libavformat/avformat.h
> @@ -1018,6 +1018,77 @@ typedef struct AVStream {
>     int pts_wrap_bits;
> } AVStream;
>
> +enum AVStreamGroupParamsType {
> +    AV_STREAM_GROUP_PARAMS_NONE,
> +};
> +
> +typedef struct AVStreamGroup {
> +    /**
> +     * A class for @ref avoptions. Set by avformat_stream_group_create().
> +     */
> +    const AVClass *av_class;
> +
> +    void *priv_data;
> +
> +    /**
> +     * Group index in AVFormatContext.
> +     */
> +    unsigned int index;
> +
> +    /**
> +     * Group type-specific group ID.
> +     *
> +     * decoding: set by libavformat
> +     * encoding: may set by the user, replaced by libavformat if left unset
> +     */
> +    int64_t id;

I don't quite get the encoding case where libavformat replaces the unset 
ID... Also, what is unset? 0 or -1? Because as far as I see 0 is the 
default, maybe -1 is better if unset has some special meaning?

[...]

> diff --git a/libavformat/internal.h b/libavformat/internal.h
> index 7702986c9c..c6181683ef 100644
> --- a/libavformat/internal.h
> +++ b/libavformat/internal.h
> @@ -202,6 +202,7 @@ typedef struct FFStream {
>      */
>     AVStream pub;
>
> +    AVFormatContext *fmtctx;
>     /**
>      * Set to 1 if the codec allows reordering, so pts can be different
>      * from dts.
> @@ -427,6 +428,26 @@ static av_always_inline const FFStream *cffstream(const AVStream *st)
>     return (const FFStream*)st;
> }
>
> +typedef struct FFStreamGroup {
> +    /**
> +     * The public context.
> +     */
> +    AVStreamGroup pub;
> +
> +    AVFormatContext *fmtctx;

If the only purpose of storing this is sanity checking if the user calls 
API with matching stream and stream group, then maybe better to make it 
ptrdiff_t, so nobody will try to misuse this to access the format context 
or do logging, etc...

Regards,
Marton