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, ×tamp,
NULL, NULL, NULL))
do_some_math(timestamp, pos);
do_some_more_math();
_______________________________________________
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".
