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

[James Almer]

On 3/23/2021 4:40 PM, Nicolas George wrote:
> James Almer (12021-03-23):
>> I recall many people have said before that just because it was done before
>> doesn't mean it should be done again. That's how bad practices spread.
> 
> I will happily concede you this. All the more happily that I will keep
> it warm to serve it back when you oppose to one of my creative API
> inventions for the sake of consistency ;-Þ

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.

> 
>>> 	AVStream **streams = ctx->streams;
>>> 	av_read_frame(ctx, &packet);
>>> 	AVStream *stream = streams[packet.stream_index];
>> No, avformat_new_stream() will reallocate that array, so if av_read_frame()
>> can allocate new streams (I think AVFMT_NOHEADER formats do that) then that
>> may just crash.
> 
> This is exactly why I chose this example.

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.

> 
>> You should always use ctx->streams directly.
> 
> 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.

> 
> And that's exactly the same with giving access to the internal
> structure: they use the fields *immediately*, and everything is fine.
> 
> What you are defending is equivalent to defending
> avformat_get_stream(ctx, idx) just to prevent users from accessing
> ctx->streams directly because they may keep a pointer to an outdated
> array. Note that in my proposal, the constraint is clearly documented.
> This is not the case, currently, for ctx->streams.
> 
> Furthermore, returning a pointer avoids copying the fields that the user
> will not use. It is minuscule, of course. But it is still one more
> argument for returning the pointer.
> 
> We have to choose between having an API proof against users who do not
> read the doc and try to guess what they can get away with and having an
> API that is efficient and convenient for careful users, we cannot have
> both.
> 
> I vote we choose the second, because the first is not really possible.

By returning a pointer the user has to free, they will get leaks if they 
don't read the doc. 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.

> 
> Regards,
>