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

Derek Buitenhuis derek.buitenhuis at gmail.com
Wed Mar 28 02:33:18 EEST 2018


On 3/27/2018 11:46 PM, Alexander Strasser wrote:
>> +     * 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?

The name is taken directly from the ISOBMFF and MOV spec, which is where this
field will mostly come from. I figured changing it might be needless renaming.
>> +    /**
>> +     * 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.

I modeled this after the recently pushed encryption side data. That is, I wanted
to be consistent with other side data APIs. It uses the _count suffix rather than
an nb_ prefix. I can use whichever people prefer.


>> +} AVTimeline;
>> +
>> +typedef struct AVTimelineList {
> 
> Would it be better to name it AVTimelineArray?

I'd prefer to use whichever is convention in FFmpeg's codebase.


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

OK.

>> +/**
>> + * 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?

This is another thing where I tried to match how other side data APIs
like encryption had their APIs, and this is how they did it.


>> +
>> +/**
>> + * Allocates an AVTimeline strcture with room for the request number of timelines.
>                               ^^^^^^^^                   ^^^^^^^
> Should it say AVTimelineList instead of AVTimeline ?

Yeah.

>> + *
>> + * @param timeline_count The number of entries in the timeline.
> 
>   The number of _time lines_ that can be stored in the list


Yeah, copypaste fail. Woops.




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

I would rather keep this name since it is consistent with how we namespace, and
name the other side data APIs
>> +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.

Ditto.

Cheers,
- Derek


More information about the ffmpeg-devel mailing list