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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel