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

[James Almer]

On 3/24/2021 12:15 PM, Nicolas George wrote:
> James Almer (12021-03-23):
>> If your creative API invention is better, then i have no problem with it
>> even if it goes against existing conventions.
>>
>> Which for that matter reminds me that i changed my mind regarding your
>> refcounted proposal, since the alternative of adding an AVRefCount struct
>> other structs can then use has proven to be kinda ugly/unwieldy, at least in
>> the example implementation i saw. So there you have it.
> 
> Thank you very much for letting me know. I appreciate a lot.
> 
> I hope I will manage to convince you that we need a convenient and
> efficient string API too, even if that means quite a bit of code to
> implement it.

I have no opinion on a strings API.

> 
>> Why did you choose an example that can crash? To show why the warning in the
>> documentation would be needed? Because i'm not against documenting details
>> about this approach whenever used.
> 
> My point is that we accepts in our API constructs that are somewhat
> tricky to use, with constraints that are not checked by the compiler and
> that the user is responsible for meeting, and that can cause crashes,
> for the sake of efficiency and simplicity.
> 
> It is a good thing, I would not have it any other way.
> 
>>> I am using ctx->streams directly. More accurately, we should always use
>>> ctx->streams *immediately*.
>                   ^^^^^^^^^^^^^
>>
>> You did not use it directly since you accessed the copy pointer you made two
>> lines above instead, and you did not use that copy immediately since you
>                                                       ^^^^^^^^^^^
>> first called av_read_frame(), which may have invalidated it.
> 
> As I said, the important word is "immediately", not "directly". But no
> matter.
> 
>> By returning a pointer the user has to free, they will get leaks if they
>> don't read the doc.
> 
> Sorry, I had not understood that you were really considering returning a
> dynamically allocated structure, I thought you mentioned the solution as
> a bad idea.

I don't recall saying that, or at least not in this thread. It's 
definitely not my preferred solution for the reasons you state below, 
which is why i didn't do it at first. But in my first reply i suggested 
it as an alternative to ensure sizeof(AVIndexEntry) would not be 
effectively tied to the ABI, and then sent a v2 of this patch 
implementing it.

> 
> Remember, we should always have this guiding principle in mind: Do not
> use dynamic allocations if we can make it work without.
> 
> And in this case, this is not theoretical. A file with frequent
> keyframes can have thousands of entries, and some applications will want
> to walk the entire array, which is currently very fast. Adding a
> function call in the middle will cause some overhead, but not that much.
> Adding a dynamic allocation, on the other hand, would make it
> sluggishly slow.
> 
>> So I'm not making this function lazy-user proof, I'm
>> trying to not give them a pointer to an offset within a volatile internal
>> array they have no business accessing.
> 
> But WHY? What is so bad with this:
> 
> 	while ((entry = av_index_get_entry(st, idx++)))
> 	    do_some_math(entry->timestamp, entry->pos);
> 	do_some_more_math();
> 
> ? No dynamic allocation, no sizeof(AVIndexEntry) creeping into the ABI,
> almost as fast as direct access, almost as simple.
> 
> This is by far the simplest and most efficient solution for both you and
> our users.
> 
> So why reject it?

Because it's not even a pointer that's guaranteed to be valid or point 
to valid data until the next call to one specific function or set of 
functions (Your example is basically av_dict_get(), where only calls to 
av_dict_set*() on a AVDictionary you own will make previously returned 
AVDictionaryEntry invalid), and instead it's a pointer that may or may 
not be valid after the next call to potentially *any* lavf function, 
because it could be modified by reading the header, reading a packet, 
and maybe more cases, all depending on demuxer, and after which it could 
still be pointing to the desired entry, or to some other entry, or to 
freed memory.

In any case, would a

int av_get_index_entry(AVStream *st, int idx, int64_t *pos,
                        int64_t *timestamp, int *size,
                        int *distance, int *flags);

implementation, which is basically the inverse of av_add_index_entry(), 
be ok instead? No AVIndexEntry usage, only the fields the caller cares 
about can be returned, and no "This pointer will autodestruct in five 
seconds" situation.

Following your example above, it would look like

while (!av_get_index_entry(st, idx++, &pos, &timestamp,
                            NULL, NULL, NULL))
     do_some_math(timestamp, pos);
do_some_more_math();