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,


_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Reply via email to