[FFmpeg-devel] [PATCH] avformat/utils: add helper functions to retrieve index entries from an AVStream

[James Almer]

On 3/23/2021 3:47 PM, Nicolas George wrote:
> James Almer (12021-03-23):
>> Signed-off-by: James Almer <jamrial at gmail.com>
>> ---
>> Following "lavf: move AVStream.*index_entries* to AVStreamInternal", it was
>> revealed that some library users apparently found these fields useful and were
>> accessing them despite not being public.
>> This patch introduces a few functions to recover this functionality in a proper
>> and supported way.
>>
> 
>> We can't simply return a const pointer to the corresponding AVIndexEntry in the
>> array because it may change at any time by for example calling
>> av_add_index_entry(), so it's either a copy of the AVIndexEntry, or adding half
>> a dozen output parameters to the function signature for each field in
>> AVIndexEntry.
> 
> We can return a const pointer to the AVIndexEntry if we document it:
> 
> 	Warning: the returned pointer is only valid until the next
> 	operation that may modify the AVStream or the AVFormatContext it
> 	belongs to. Accessing it after av_read_frame() or other
> 	functions results in undefined behavior.
> 
> Users can still copy it immediately if they need it longer.

I really don't like the idea of returning a pointer to some offset 
within an internal struct that may either start pointing at some other 
entry or even to freed memory, especially when the alternative of giving 
the user a copy of a single entry is trivial to do and actually safe.

> 
>>
>>   libavformat/avformat.h | 39 +++++++++++++++++++++++++++++++++++++++
>>   libavformat/utils.c    | 32 ++++++++++++++++++++++++++++++++
>>   2 files changed, 71 insertions(+)
>>
>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
>> index 765bc3b6f5..ac8b57149e 100644
>> --- a/libavformat/avformat.h
>> +++ b/libavformat/avformat.h
>> @@ -2754,6 +2754,45 @@ int av_find_default_stream_index(AVFormatContext *s);
>>    */
>>   int av_index_search_timestamp(AVStream *st, int64_t timestamp, int flags);
>>   
>> +/**
>> + * Get the index entry count for the given AVStream.
>> + *
>> + * @param st stream
>> + * @return the number of index entries in the stream
>> + */
>> +int av_index_get_entries_count(AVStream *st);
>> +
>> +/**
>> + * Get the AVIndexEntry corresponding to the given index.
>> + *
>> + * @param st          stream containing the requested AVIndexEntry
>> + * @param index_entry pointer to an AVIndexEntry where to store the entry. On failure,
>> + *                    or if the index entry could not be found, this struct will remain
>> + *                    untouched
>> + * @param idx         the desired index
>> + * @return >= 0 on success. AVERROR(ENOENT) if no entry for the requested index could
>> +           be found. other negative AVERROR codes on failure.
>> + */
>> +int av_index_get_entry(AVStream *st, AVIndexEntry *index_entry, int idx);
>> +
>> +/**
>> + * Get the AVIndexEntry corresponding to the given timestamp.
>> + *
>> + * @param st          stream containing the requested AVIndexEntry
>> + * @param index_entry pointer to an AVIndexEntry where to store the entry. On failure,
>> + *                    or if the index entry could not be found, this struct will remain
>> + *                    untouched
>> + * @param timestamp   timestamp to retrieve the index entry for
>> + * @param flags       if AVSEEK_FLAG_BACKWARD then the returned entry will correspond
>> + *                    to the timestamp which is <= the requested one, if backward
>> + *                    is 0, then it will be >=
>> + *                    if AVSEEK_FLAG_ANY seek to any frame, only keyframes otherwise
>> + * @return >= 0 on success. AVERROR(ENOENT) if no entry for the requested timestamp could
>> + *          be found. other negative AVERROR codes on failure.
>> + */
>> +int av_index_get_entry_from_timestamp(AVStream *st, AVIndexEntry *index_entry,
>> +                                      int64_t wanted_timestamp, int flags);
>> +
>>   /**
>>    * Add an index entry into a sorted list. Update the entry if the list
>>    * already contains it.
>> diff --git a/libavformat/utils.c b/libavformat/utils.c
>> index f31826f2ea..4e7a8bf23e 100644
>> --- a/libavformat/utils.c
>> +++ b/libavformat/utils.c
>> @@ -2129,6 +2129,38 @@ int av_index_search_timestamp(AVStream *st, int64_t wanted_timestamp, int flags)
>>                                        wanted_timestamp, flags);
>>   }
>>   
>> +int av_index_get_entries_count(AVStream *st)
>> +{
>> +    return st->internal->nb_index_entries;
>> +}
>> +
>> +int av_index_get_entry(AVStream *st, AVIndexEntry *index_entry, int idx)
>> +{
> 
>> +    if (idx < 0)
>> +        return AVERROR(EINVAL);
> 
> Make idx unsigned.

Ok.

> 
>> +    if (idx >= st->internal->nb_index_entries)
>> +        return AVERROR(ENOENT);
>> +
> 
>> +    memcpy(index_entry, &st->internal->index_entries[idx], sizeof(*index_entry));
> 
> Why not using an assignment?
> 
> But this makes sizeof(AVIndexEntry) part of the ABI. Probably not what
> we want.

The struct itself says nothing about it not being part of the ABI, and 
we usually document when that's the case to let the user know.
That being said, no public function until now used that struct as 
parameter, so to not effectively tie it to the ABI if that's undesired, 
i could instead make index_entry a pointer to pointer, do an 
av_memdup(), and document that the user owns the returned entry and must 
free it after using it.

> 
>> +
>> +    return 0;
>> +}
>> +
>> +int av_index_get_entry_from_timestamp(AVStream *st, AVIndexEntry *index_entry,
>> +                                      int64_t wanted_timestamp, int flags)
>> +{
>> +    int idx = ff_index_search_timestamp(st->internal->index_entries,
>> +                                        st->internal->nb_index_entries,
>> +                                        wanted_timestamp, flags);
>> +
>> +    if (idx < 0)
>> +        return AVERROR(ENOENT);
>> +
> 
>> +    memcpy(index_entry, &st->internal->index_entries[idx], sizeof(*index_entry));
> 
> Same, of course.
> 
>> +
>> +    return 0;
>> +}
>> +
>>   static int64_t ff_read_timestamp(AVFormatContext *s, int stream_index, int64_t *ppos, int64_t pos_limit,
>>                                    int64_t (*read_timestamp)(struct AVFormatContext *, int , int64_t *, int64_t ))
>>   {
> 
> Regards,
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>