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

[James Almer]

On 4/1/2021 7:07 AM, Anton Khirnov wrote:
> Quoting James Almer (2021-03-25 14:37:20)
>> On 3/25/2021 8:55 AM, Nicolas George wrote:
>>
>> Same situation as av_add_index_entry(). And if you look at the signature
>> of the (3) function in my last proposal, i kept the av_index_* namespace
>> free precisely in case a new API in the future needs to be added for
>> this reason, for both add() and get(). One that could work directly with
>> the AVIndexEntry struct after the internal entries array was redesigned,
>> perhaps using AVTreeNode, so returned pointers or entries are safer to
>> handle.
> 
> On the topic of av_add_index_entry() - is there any reason that function
> should be public? Seems like it's internal-use only.

We could deprecate it and make it internal, if it's not used by callers.
It may have been added back when things were devised as being more user 
controlled, which ultimately didn't happen, or just made public without 
giving it much thought.

> 
>>
>>>
>>> Option (4) has the obvious practical drawback that misusing the API
>>> causes undefined behavior.
>>>
>>> The constraint of using a pointer immediately on risk of undefined
>>> behavior is actually a frequent one, in FFmpeg but also in C at large:
>>> gethosbtyname, localtime, etc.
>>>
>>> For me, that makes it approximately on par with the risk of messing the
>>> order of the many arguments.
>>>
>>> Which leaves more typing, NULL checks overhead or useless variables
>>> (still more typing).
>>>
>>> It is tiny, I have no trouble admitting, but it is tiny in favor of one
>>> solution.
>>>
>>> If you do not agree with these estimates, please explain exactly where.
>>
>> I don't know if you remember how in this one imgutils patch i sent some
>> time ago i was against functions with tons of output parameters, because
>> i considered it ugly and typical of enterprise software API. That hasn't
>> changed. But here, being the exact counterpart of an existing add()
>> function put it above the other approach i dislike slightly more of
>> returning an internal pointer and not being able to tell the user just
>> what may invalidate it.
>>
>>>
>>>> If some other developer wants to chime in and comment which approach they
>>>> prefer, then that would be ideal.
> 
> FWIW in this specific case exporting a short-lived pointer seems less
> problematic than the other options.
> 
> But on the other hand I wonder about exporting AVIndexEntry exactly as
> is internally. Are all these fields useful or even meaningful to
> external callers?
> Perhaps we could make a new struct that would export only those fields
> people actually use. And have the new API return a pointer to something
> like AVFormatInternal.index_entry_for_the_caller.

There are five fields, pos, timestamp, flags, size, and min_distance. 
The first three are the most important, with timestamps and flags IMO 
being the only useful ones for users building their own index entry list 
for example to display each seek point to the user in a player's seek bar.
Pos doesn't seem like it would be useful outside of the internal seeking 
methods, size seems to be the least useful of them all, and min_distance 
i can't say, since it probably is mainly useful to also for the internal 
seeking methods.

So instead of making a new struct, or build a second array internally, 
we could maybe just return those two fields?

> 
> Also a naming note - I'd prefer the function names to start with
> avformat, so it's clearer where they belong. "index" can mean many
> different things.