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