[FFmpeg-devel] [PATCH][RFC] avcodec/avutil: Add timeline side data

[Alexander Strasser]

Hi Derek,

I am happy to see someone trying to extend FFmpeg to support these kind
of features in a deeper way. Good luck on this journey!

Below I am mostly commenting on typos and rather minor things.

So you should for now mostly ignore my comments, but it is easier to
write them now that I read the patch. Just saying taking immediate action
in this early stage when things will probably be dropped or modified could
result in unneeded extra work.


On 2018-03-27 20:44 +0100, Derek Buitenhuis wrote:
> Signed-off-by: Derek Buitenhuis <derek.buitenhuis at gmail.com>
> ---
>  libavcodec/avcodec.h |   8 +++
>  libavutil/timeline.h | 160 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 168 insertions(+)
>  create mode 100644 libavutil/timeline.h
> 
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 50c34db..6f54495 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -1358,6 +1358,14 @@ enum AVPacketSideDataType {
>      AV_PKT_DATA_ENCRYPTION_INFO,
>  
>      /**
> +     * This side data contains timeline entries for a given stream. This type
> +     * will only apear as stream side data.
> +     *
> +     * The format is not part of the ABI, use av_timeline_* method to access.
> +     */
> +    AV_PKT_DATA_TIMELINE,
> +
> +    /**
>       * The number of side data types.
>       * This is not part of the public API/ABI in the sense that it may
>       * change when new side data types are added.
> diff --git a/libavutil/timeline.h b/libavutil/timeline.h
> new file mode 100644
> index 0000000..f1f3e1b
> --- /dev/null
> +++ b/libavutil/timeline.h
> @@ -0,0 +1,160 @@
> +/*
> + * Copyright (C) 2018 Derek Buitenhuis
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#ifndef AVUTIL_TIMELINE_H
> +#define AVUTIL_TIMELINE_H
> +
> +#include <stddef.h>
> +#include <stdint.h>
> +
> +#include "rational.h"
> +
> +typedef struct AVTimelineEntry {
> +    /**
> +     * The start time of the given timeline entry, in stream timebase units.
> +     * If this value is AV_NOPTS_VALUE, you must display black for the duration
> +     * of the entry. For audio, silence must be played.
> +     */
> +    int64_t start;
> +
> +    /**
> +     * The duration of the given timeline entry, in steam timebase units.
                                                       ^^^^^
stream


> +     */
> +    int64_t duration;
> +
> +    /**
> +     * The rate at which this entry should be played back. The value is a multipier
                                                                             ^^^^^^^^^
multiplier


> +     * of the stream's rate, for example: 1.2 means play back this entry at 1.2x speed.
> +     * If this value is 0, then the first sample (located at 'start') must be displayed
> +     * for the duration of the entry.
> +     */
> +    AVRational media_rate;

Wouldn't it better be called rate_mult or rate_multiplier or rate_factor
or something in that fashion?

Nit: As this member is of type AVRational it might be better to word
it more precisely like "if the numerator is 0".


> +} AVTimelineEntry;
> +
> +/**
> + * Describes a timeline for a stream in terms of edits/entries. Each entry must be
> + * played back in order, according to the information in each. Each stream may have
> + * multiple timelines which need to be correlated between different streams.
> + */
> +typedef struct AVTimeline {
> +    /**
> +     * The ID of a given timeline. Since each stream may have multiple timelines
> +     * defined, this value is used to correlated different streams' timelines
                                         ^^^^^^^^^^
correlate?


> +     * which should be used together. For example, if one has two streams,
> +     * one video, and one audio, with two timelines each, the timelines
> +     * with matching IDs should be used in conjuction, to assure everything
                                              ^^^^^^^^^^
conjunction


> +     * is in sync and matches. The concept is similar to that of EditionUID
> +     * in Matroska.
> +     */
> +    uint32_t id;
> +
> +    /**
> +     * An in-order array of entries for the given timeline.
> +     * Each entry contains information on which samples to display for a
> +     * particular edit.
> +     */
> +    AVTimelineEntry *entries;
> +
> +    /**
> +     * Number of entries in the timeline.
> +     */
> +    size_t entry_count;

I think usually we prefix these with nb_ e.g. nb_entries
This comment also applies to all _count suffixed names
following later in this patch.


> +} AVTimeline;
> +
> +typedef struct AVTimelineList {

Would it be better to name it AVTimelineArray?


> +    /**
> +     * An array of timelines associated with the stream.
> +     */
> +    AVTimeline *timelines;
> +
> +    /**
> +     * Then number of timelines associated with the stream.
          ^^^^
Should probably just be dropped


> +     */
> +    size_t timeline_count;
> +} AVTimelineList;
> +
> +/**
> + * Allocates an AVTimeline strcture with the requested number of entires.
                              ^^^^^^^^
structure

> + *
> + * @param entry_count The number of entries in the timeline.
> + *
> + * @return The new AVTimeline structure, or NULL on error.
> + */
> +AVTimeline *av_timeline_alloc(size_t entry_count);

The allocated entries will be uninitialized?
Either way it's probably worth it to document it.


> +
> +
> +/**
> + * Frees an AVTimeline structure and its members.
> + *
> + * @param timeline The AVTimeline structure to free.
> + */
> +void av_timeline_free(AVTimeline *timeline);

Is passing a NULL pointer OK?
Would some other signature and semantic similar to av_freep be better?


> +
> +/**
> + * Allocates an AVTimeline strcture with room for the request number of timelines.
                              ^^^^^^^^                   ^^^^^^^
structure, requested

Should it say AVTimelineList instead of AVTimeline ?


> + *
> + * @param timeline_count The number of entries in the timeline.

  The number of _time lines_ that can be stored in the list

or

  The number of AVTimeline objects/instances that can be stored in the array


> + *
> + * @return The new AVTimelineList structure, or NULL on error.
> + */
> +AVTimelineList *av_timeline_list_alloc(size_t timeline_count);
> +
> +
> +/**
> + * Frees an AVTimelineList structure and its timelines, and their entries.
> + *
> + * @param timeline_list The AVTimelineList structure to free.
> + */
> +void av_timeline_list_free(AVTimeline *timeline_list);
> +
> +/**
> + * Allocates a new AVTimelineList structure and copies the data from an
> + * existing structure, allocating new members.
> + *
> + * @param timeline_list The existing AVTimelineList structure to copy data from.
> + *
> + * @return The new AVTimelineList structure, or NULL on error.
> + */
> +AVTimelineList *av_timeline_list_clone(const AVTimelineList *timeline_list);
> +
> +/**
> + * Creates a copy of the AVTimelineList that is contained in the given side
> + * data. The resulting struct should be passed to av_timeline_list_free()
> + * when done.
> + *
> + * @param side_data The side data array.
> + * @param side_data_size The size of the side data array.
> + *
> + * @return The new AVTimelineList structure, or NULL on error.
> + */
> +AVTimelineList *av_timeline_list_get_side_data(const uint8_t *side_data, size_t side_data_size);

The name av_timeline_list_get_side_data seems a bit odd to me.

Maybe it is in line with other functions, then it might be just
fine. To me something like

  av_timeline_list_create_from_side_data

or similar would be more concise.


> +
> +/**
> + * Allocates and initializes side data that holds a copy of the given timeline
> + * list. The resulting pointer should be either freed using av_free() or given
> + * to av_packet_add_side_data().
> + *
> + * @param timeline_list The AVTimelineList to put into side data.
> + * @param side_data_size A pointer to where the size can be filled in.
> + *
> + * @return The new side-data pointer, or NULL.
> + */
> +uint8_t *av_timeline_list_add_side_data(const AVTimelineList *timeline_list, size_t *side_data_size);

As above this function name doesn't sound very intuitive to me. I didn't
check if we have similar naming patterns around already. So it might
be consistent; I don't know.

> +
> +#endif /* AVUTIL_TIMELINE_H */


Looks quite OK as an initial sketch for an API. Though as already
mentioned I just read your proposal and I do not have deep knowledge
about the topic itself. From my experience the more subtle details 
tend to pop up when actually implementing the thing and they usually
leak into the API or show its deficiencies.


  Alexander