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

[Anton Khirnov <anton@khirnov.net>]

Hi,
Quoting James Almer (2023-10-30 16:23:52)
> Signed-off-by: James Almer <jamrial@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.

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

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