[FFmpeg-devel] [PATCH v3 1/3] [PoC]avformat: introduce AVStreamGroup

[James Almer]

On 11/6/2023 8:37 AM, Anton Khirnov wrote:
> Hi,
> Quoting James Almer (2023-10-30 16:23:52)
>> Signed-off-by: James Almer <jamrial at gmail.com>
>> ---
>> No changes since last version. I'm resending this for the IAMF demuxer.
>>
>> I need opinions or reviews for this. We need to get this right from the start
>> and i don't want to push something that will afterwards be considered unoptimal
> 
> The API generally looks good to me.
> 
>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
>> index 9e7eca007e..9b2ee7ff14 100644
>> --- 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 on group creation.
>> +     */
>> +    const AVClass *av_class;
>> +
>> +    void *priv_data;
>> +
>> +    /**
>> +     * Group index in AVFormatContext.
>> +     */
>> +    unsigned int index;
>> +
>> +    /**
>> +     * Group-specific group ID.
> 
> type-specific?
> 
>> +     *
>> +     * decoding: set by libavformat
>> +     * encoding: set by the user, replaced by libavformat if left unset
> 
> 'may be set' is the terminology we use for optional values
> 
>> +     */
>> +    int64_t id;
>> +
>> +    /**
>> +     * Group-specific type
> 
> drop '-specific', I don't see what it adds to the meaning.
> 
>> +     *
>> +     * decoding: set by libavformat on group creation
>> +     * encoding: set by the user on group creation
> 
> Could just say 'set by avformat_stream_group_create()', the user should
> never set it manually.
> 
>> +     */
>> +    enum AVStreamGroupParamsType type;
>> +
>> +    /**
>> +     * Group-specific type parameters
> 
> type-specific?
> 
>> +     */
>> +    union {
>> +        uintptr_t dummy; // Placeholder
>> +    } params;
>> +
>> +    /**
>> +     * Metadata that applies to the whole file.
> 
> s/file/group
> 
>> +     *
>> +     * - demuxing: set by libavformat in avformat_open_input()
> 
> Set on group creation, that does not have to be in
> avformat_open_input().
> 
>> +     * - muxing: may be set by the caller before avformat_write_header()
>> +     *
>> +     * Freed by libavformat in avformat_free_context().
>> +     */
>> +    AVDictionary *metadata;
>> +
>> +    /**
>> +     * Number of elements in AVStreamGroup.streams.
>> +     *
>> +     * Set by avformat_stream_group_add_stream() must not be modified by any other code.
>> +     */
>> +    unsigned int nb_streams;
>> +
>> +    /**
>> +     * A list of streams in the group. New entries are created with
>> +     * avformat_stream_group_add_stream().
>> +     *
>> +     * - demuxing: entries are created by libavformat in avformat_open_input().
>> +     *             If AVFMTCTX_NOHEADER is set in ctx_flags, then new entries may also
>> +     *             appear in av_read_frame().
> 
> We might want to make this type-specific, i.e. some group types are
> guaranteed to never change.
> 
>> +     * - muxing: entries are created by the user before avformat_write_header().
>> +     *
>> +     * Freed by libavformat in avformat_free_context().
>> +     */
>> +    const AVStream **streams;
> 
> Is this const useful? I imagine some code might reasonably want to
> modify the stream through this pointer.

True. Will change.

> 
>> @@ -608,6 +629,18 @@ void ff_free_stream(AVStream **st);
>>    */
>>   void ff_remove_stream(AVFormatContext *s, AVStream *st);
>>   
>> +/**
>> + * Frees a stream group without modifying the corresponding AVFormatContext.
>> + * Must only be called if the latter doesn't matter or if the stream
>> + * is not yet attached to an AVFormatContext.
>> + */
>> +void ff_free_stream_group(AVStreamGroup **pstg);
> 
> ff_stream_group_free()
> 
> Also, I'd prefer a dedicated header for this, internal.h is an
> abomination.

Should i move the AVStream related functions to it too?