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

[Nicolas George]

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.

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

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?

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20210324/1359343c/attachment.sig>