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

[James Almer <jamrial@gmail.com>]

On 11/21/2023 6:57 PM, Marton Balint wrote:
> 
> 
> On Tue, 21 Nov 2023, James Almer wrote:
> 
>> Signed-off-by: James Almer <jamrial@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...

Will do.

> 
> [...]
> 
>> --- 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?

I copied the doxy from the relevant AVStream field. It may be bogus 
there though, seeing for example how the mp4 muxer has an option to use 
the ids when muxing, but ultimately trusting them and not changing them.

Can remove that part here and just leave it as may be set by the user.

> 
> [...]
> 
>> 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...

It's the same in AVStream. And why couldn't a module access it for 
logging or similar? It's internal, not exposed, and the group is 
strictly a part of its parent AVFormatContext.

> 
> Regards,
> Marton
> _______________________________________________
> 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".
_______________________________________________
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".