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

[Anton Khirnov]

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.

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

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.

-- 
Anton Khirnov