Message ID | 20210323183622.2219-1-jamrial@gmail.com |
---|---|
State | Accepted |
Headers | show |
Series | [FFmpeg-devel] avformat/utils: add helper functions to retrieve index entries from an AVStream | expand |
Context | Check | Description |
---|---|---|
andriy/x86_make | success | Make finished |
andriy/x86_make_fate | success | Make fate finished |
andriy/PPC64_make | success | Make finished |
andriy/PPC64_make_fate | success | Make fate finished |
James Almer (12021-03-23): > Signed-off-by: James Almer <jamrial@gmail.com> > --- > Following "lavf: move AVStream.*index_entries* to AVStreamInternal", it was > revealed that some library users apparently found these fields useful and were > accessing them despite not being public. > This patch introduces a few functions to recover this functionality in a proper > and supported way. > > We can't simply return a const pointer to the corresponding AVIndexEntry in the > array because it may change at any time by for example calling > av_add_index_entry(), so it's either a copy of the AVIndexEntry, or adding half > a dozen output parameters to the function signature for each field in > AVIndexEntry. We can return a const pointer to the AVIndexEntry if we document it: Warning: the returned pointer is only valid until the next operation that may modify the AVStream or the AVFormatContext it belongs to. Accessing it after av_read_frame() or other functions results in undefined behavior. Users can still copy it immediately if they need it longer. > > libavformat/avformat.h | 39 +++++++++++++++++++++++++++++++++++++++ > libavformat/utils.c | 32 ++++++++++++++++++++++++++++++++ > 2 files changed, 71 insertions(+) > > diff --git a/libavformat/avformat.h b/libavformat/avformat.h > index 765bc3b6f5..ac8b57149e 100644 > --- a/libavformat/avformat.h > +++ b/libavformat/avformat.h > @@ -2754,6 +2754,45 @@ int av_find_default_stream_index(AVFormatContext *s); > */ > int av_index_search_timestamp(AVStream *st, int64_t timestamp, int flags); > > +/** > + * Get the index entry count for the given AVStream. > + * > + * @param st stream > + * @return the number of index entries in the stream > + */ > +int av_index_get_entries_count(AVStream *st); > + > +/** > + * Get the AVIndexEntry corresponding to the given index. > + * > + * @param st stream containing the requested AVIndexEntry > + * @param index_entry pointer to an AVIndexEntry where to store the entry. On failure, > + * or if the index entry could not be found, this struct will remain > + * untouched > + * @param idx the desired index > + * @return >= 0 on success. AVERROR(ENOENT) if no entry for the requested index could > + be found. other negative AVERROR codes on failure. > + */ > +int av_index_get_entry(AVStream *st, AVIndexEntry *index_entry, int idx); > + > +/** > + * Get the AVIndexEntry corresponding to the given timestamp. > + * > + * @param st stream containing the requested AVIndexEntry > + * @param index_entry pointer to an AVIndexEntry where to store the entry. On failure, > + * or if the index entry could not be found, this struct will remain > + * untouched > + * @param timestamp timestamp to retrieve the index entry for > + * @param flags if AVSEEK_FLAG_BACKWARD then the returned entry will correspond > + * to the timestamp which is <= the requested one, if backward > + * is 0, then it will be >= > + * if AVSEEK_FLAG_ANY seek to any frame, only keyframes otherwise > + * @return >= 0 on success. AVERROR(ENOENT) if no entry for the requested timestamp could > + * be found. other negative AVERROR codes on failure. > + */ > +int av_index_get_entry_from_timestamp(AVStream *st, AVIndexEntry *index_entry, > + int64_t wanted_timestamp, int flags); > + > /** > * Add an index entry into a sorted list. Update the entry if the list > * already contains it. > diff --git a/libavformat/utils.c b/libavformat/utils.c > index f31826f2ea..4e7a8bf23e 100644 > --- a/libavformat/utils.c > +++ b/libavformat/utils.c > @@ -2129,6 +2129,38 @@ int av_index_search_timestamp(AVStream *st, int64_t wanted_timestamp, int flags) > wanted_timestamp, flags); > } > > +int av_index_get_entries_count(AVStream *st) > +{ > + return st->internal->nb_index_entries; > +} > + > +int av_index_get_entry(AVStream *st, AVIndexEntry *index_entry, int idx) > +{ > + if (idx < 0) > + return AVERROR(EINVAL); Make idx unsigned. > + if (idx >= st->internal->nb_index_entries) > + return AVERROR(ENOENT); > + > + memcpy(index_entry, &st->internal->index_entries[idx], sizeof(*index_entry)); Why not using an assignment? But this makes sizeof(AVIndexEntry) part of the ABI. Probably not what we want. > + > + return 0; > +} > + > +int av_index_get_entry_from_timestamp(AVStream *st, AVIndexEntry *index_entry, > + int64_t wanted_timestamp, int flags) > +{ > + int idx = ff_index_search_timestamp(st->internal->index_entries, > + st->internal->nb_index_entries, > + wanted_timestamp, flags); > + > + if (idx < 0) > + return AVERROR(ENOENT); > + > + memcpy(index_entry, &st->internal->index_entries[idx], sizeof(*index_entry)); Same, of course. > + > + return 0; > +} > + > static int64_t ff_read_timestamp(AVFormatContext *s, int stream_index, int64_t *ppos, int64_t pos_limit, > int64_t (*read_timestamp)(struct AVFormatContext *, int , int64_t *, int64_t )) > { Regards,
On 3/23/2021 3:47 PM, Nicolas George wrote: > James Almer (12021-03-23): >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> Following "lavf: move AVStream.*index_entries* to AVStreamInternal", it was >> revealed that some library users apparently found these fields useful and were >> accessing them despite not being public. >> This patch introduces a few functions to recover this functionality in a proper >> and supported way. >> > >> We can't simply return a const pointer to the corresponding AVIndexEntry in the >> array because it may change at any time by for example calling >> av_add_index_entry(), so it's either a copy of the AVIndexEntry, or adding half >> a dozen output parameters to the function signature for each field in >> AVIndexEntry. > > We can return a const pointer to the AVIndexEntry if we document it: > > Warning: the returned pointer is only valid until the next > operation that may modify the AVStream or the AVFormatContext it > belongs to. Accessing it after av_read_frame() or other > functions results in undefined behavior. > > Users can still copy it immediately if they need it longer. I really don't like the idea of returning a pointer to some offset within an internal struct that may either start pointing at some other entry or even to freed memory, especially when the alternative of giving the user a copy of a single entry is trivial to do and actually safe. > >> >> libavformat/avformat.h | 39 +++++++++++++++++++++++++++++++++++++++ >> libavformat/utils.c | 32 ++++++++++++++++++++++++++++++++ >> 2 files changed, 71 insertions(+) >> >> diff --git a/libavformat/avformat.h b/libavformat/avformat.h >> index 765bc3b6f5..ac8b57149e 100644 >> --- a/libavformat/avformat.h >> +++ b/libavformat/avformat.h >> @@ -2754,6 +2754,45 @@ int av_find_default_stream_index(AVFormatContext *s); >> */ >> int av_index_search_timestamp(AVStream *st, int64_t timestamp, int flags); >> >> +/** >> + * Get the index entry count for the given AVStream. >> + * >> + * @param st stream >> + * @return the number of index entries in the stream >> + */ >> +int av_index_get_entries_count(AVStream *st); >> + >> +/** >> + * Get the AVIndexEntry corresponding to the given index. >> + * >> + * @param st stream containing the requested AVIndexEntry >> + * @param index_entry pointer to an AVIndexEntry where to store the entry. On failure, >> + * or if the index entry could not be found, this struct will remain >> + * untouched >> + * @param idx the desired index >> + * @return >= 0 on success. AVERROR(ENOENT) if no entry for the requested index could >> + be found. other negative AVERROR codes on failure. >> + */ >> +int av_index_get_entry(AVStream *st, AVIndexEntry *index_entry, int idx); >> + >> +/** >> + * Get the AVIndexEntry corresponding to the given timestamp. >> + * >> + * @param st stream containing the requested AVIndexEntry >> + * @param index_entry pointer to an AVIndexEntry where to store the entry. On failure, >> + * or if the index entry could not be found, this struct will remain >> + * untouched >> + * @param timestamp timestamp to retrieve the index entry for >> + * @param flags if AVSEEK_FLAG_BACKWARD then the returned entry will correspond >> + * to the timestamp which is <= the requested one, if backward >> + * is 0, then it will be >= >> + * if AVSEEK_FLAG_ANY seek to any frame, only keyframes otherwise >> + * @return >= 0 on success. AVERROR(ENOENT) if no entry for the requested timestamp could >> + * be found. other negative AVERROR codes on failure. >> + */ >> +int av_index_get_entry_from_timestamp(AVStream *st, AVIndexEntry *index_entry, >> + int64_t wanted_timestamp, int flags); >> + >> /** >> * Add an index entry into a sorted list. Update the entry if the list >> * already contains it. >> diff --git a/libavformat/utils.c b/libavformat/utils.c >> index f31826f2ea..4e7a8bf23e 100644 >> --- a/libavformat/utils.c >> +++ b/libavformat/utils.c >> @@ -2129,6 +2129,38 @@ int av_index_search_timestamp(AVStream *st, int64_t wanted_timestamp, int flags) >> wanted_timestamp, flags); >> } >> >> +int av_index_get_entries_count(AVStream *st) >> +{ >> + return st->internal->nb_index_entries; >> +} >> + >> +int av_index_get_entry(AVStream *st, AVIndexEntry *index_entry, int idx) >> +{ > >> + if (idx < 0) >> + return AVERROR(EINVAL); > > Make idx unsigned. Ok. > >> + if (idx >= st->internal->nb_index_entries) >> + return AVERROR(ENOENT); >> + > >> + memcpy(index_entry, &st->internal->index_entries[idx], sizeof(*index_entry)); > > Why not using an assignment? > > But this makes sizeof(AVIndexEntry) part of the ABI. Probably not what > we want. The struct itself says nothing about it not being part of the ABI, and we usually document when that's the case to let the user know. That being said, no public function until now used that struct as parameter, so to not effectively tie it to the ABI if that's undesired, i could instead make index_entry a pointer to pointer, do an av_memdup(), and document that the user owns the returned entry and must free it after using it. > >> + >> + return 0; >> +} >> + >> +int av_index_get_entry_from_timestamp(AVStream *st, AVIndexEntry *index_entry, >> + int64_t wanted_timestamp, int flags) >> +{ >> + int idx = ff_index_search_timestamp(st->internal->index_entries, >> + st->internal->nb_index_entries, >> + wanted_timestamp, flags); >> + >> + if (idx < 0) >> + return AVERROR(ENOENT); >> + > >> + memcpy(index_entry, &st->internal->index_entries[idx], sizeof(*index_entry)); > > Same, of course. > >> + >> + return 0; >> +} >> + >> static int64_t ff_read_timestamp(AVFormatContext *s, int stream_index, int64_t *ppos, int64_t pos_limit, >> int64_t (*read_timestamp)(struct AVFormatContext *, int , int64_t *, int64_t )) >> { > > 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-request@ffmpeg.org with subject "unsubscribe". >
James Almer (12021-03-23): > I really don't like the idea of returning a pointer to some offset within an > internal struct that may either start pointing at some other entry or even > to freed memory, especially when the alternative of giving the user a copy > of a single entry is trivial to do and actually safe. It is already what the users who accessed the field directly did. And I am pretty sure I can find examples where we do similar things in the code base. And it is exactly what we are doing when we let users access fields directly. And the problem of sizeof(AVIndexEntry) part of the ABI just disappears if we do that. May I suggest that you recalibrate your dislike? Regards,
Nicolas George (12021-03-23): > And it is exactly what we are doing when we let users access fields > directly. I mean: AVStream **streams = ctx->streams; av_read_frame(ctx, &packet); AVStream *stream = streams[packet.stream_index]; That should work, right? Regards,
On 3/23/2021 4:07 PM, Nicolas George wrote: > James Almer (12021-03-23): >> I really don't like the idea of returning a pointer to some offset within an >> internal struct that may either start pointing at some other entry or even >> to freed memory, especially when the alternative of giving the user a copy >> of a single entry is trivial to do and actually safe. > > It is already what the users who accessed the field directly did. Users accessed st->index_entries, or fields like st->index_entries[idx].timestamp. If the array was reallocated by lavf, st->index_entries is made to point to the new array, and accessing it or fields at offsets within it still worked as expected and gave you the information related to the index you requested. If a function returns a pointer to some offset within an internal array, and said array changes or disappears, the pointer returned is now pointing to who knows what, and that's unsafe. A copy is trivial and safe. > > And I am pretty sure I can find examples where we do similar things in > the code base. 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. > > And it is exactly what we are doing when we let users access fields > directly. See above. st->index_entries will always point to the current valid array, even if it was reallocated. > > And the problem of sizeof(AVIndexEntry) part of the ABI just disappears > if we do that. It also disappears if we return a AVIndexEntry the user must free. > > May I suggest that you recalibrate your dislike? No, I'll not stop disliking this practice, for the reasons i stated above. And you'll not find me asking you to recalibrate you like, only explain why i think this new API should not use that approach. > > 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-request@ffmpeg.org with subject "unsubscribe". >
On 3/23/2021 4:11 PM, Nicolas George wrote: > Nicolas George (12021-03-23): >> And it is exactly what we are doing when we let users access fields >> directly. > > I mean: > > AVStream **streams = ctx->streams; > av_read_frame(ctx, &packet); > AVStream *stream = streams[packet.stream_index]; > > That should work, right? 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. You should always use ctx->streams directly. > > 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-request@ffmpeg.org with subject "unsubscribe". >
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 ;-Þ > > 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. > You should always use ctx->streams directly. I am using ctx->streams directly. More accurately, we should always use ctx->streams *immediately*. 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. Regards,
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, >
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,
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();
James Almer (12021-03-24): > 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. What you say is true, but it is absolutely not a problem if the user access the pointer immediately, before calling any other function, just like it is supposed to do when derefencing ctx->streams. > 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. I will not block it as I would block the version with dynamic allocation, but you will need a little more work to convince me that this is better than just returning the pointer. You make more work for yourself and more work for the user, you have to have a good reason for it. Regards,
On 3/24/2021 4:48 PM, Nicolas George wrote: > James Almer (12021-03-24): >> 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. > > What you say is true, but it is absolutely not a problem if the user > access the pointer immediately, before calling any other function, just > like it is supposed to do when derefencing ctx->streams. > >> 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. > > I will not block it as I would block the version with dynamic > allocation, but you will need a little more work to convince me that > this is better than just returning the pointer. You make more work for > yourself and more work for the user, you have to have a good reason for > it. I think it's clear by now that nothing i could say will convince you it's better to not return a pointer to an internal array when there are safer alternatives, and i already gave my reasons why, none of which satisfied you, so i don't see the point in keeping this discussion going. If some other developer wants to chime in and comment which approach they prefer, then that would be ideal. I have no hurry to push this, and I'd rather know which direction to go before i write a v3, so it can wait. > > 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-request@ffmpeg.org with subject "unsubscribe". >
James Almer (12021-03-24): > I think it's clear by now that nothing i could say will convince you it's > better to not return a pointer to an internal array when there are safer > alternatives, and i already gave my reasons why, none of which satisfied > you, so i don't see the point in keeping this discussion going. I find this comment quite offensive. You did not manage to convince me because your arguments have not been up to the task. Do not try to push the fault on me, and I will refrain from accusing you of not taking my arguments into account. Coming to an agreement is a process, it requires both parts to refine their arguments progressively. This is a matter of choosing the least of several drawbacks. So let us compare the drawbacks and not muddle things further. For me: 1. having a dynamic allocation is way way worse than 2. having sizeof(AVIndexEntry) in the ABI, which is somewhat worse than 3. having a function with many arguments, which is a tiny bit worse than 4. having a "use this pointer immediately" constraint. We agree except on 3>4, so let us focus on that. Option (3) has these practical drawbacks: Many arguments involves more typing and the risk of messing with the order and getting invalid values. It also requires redesigning the API if we add fields and exporting them is useful. And it requires either the overhead of NULL checks or the caller declaring unneeded variables. Option (4) has the obvious practical drawback that misusing the API causes undefined behavior. The constraint of using a pointer immediately on risk of undefined behavior is actually a frequent one, in FFmpeg but also in C at large: gethosbtyname, localtime, etc. For me, that makes it approximately on par with the risk of messing the order of the many arguments. Which leaves more typing, NULL checks overhead or useless variables (still more typing). It is tiny, I have no trouble admitting, but it is tiny in favor of one solution. If you do not agree with these estimates, please explain exactly where. > If some other developer wants to chime in and comment which approach they > prefer, then that would be ideal. Indeed. Regards,
On 25 Mar 2021, at 12:55, Nicolas George wrote: > James Almer (12021-03-24): >> I think it's clear by now that nothing i could say will convince you it's >> better to not return a pointer to an internal array when there are safer >> alternatives, and i already gave my reasons why, none of which satisfied >> you, so i don't see the point in keeping this discussion going. > > I find this comment quite offensive. You did not manage to convince me > because your arguments have not been up to the task. Do not try to push > the fault on me, and I will refrain from accusing you of not taking my > arguments into account. Coming to an agreement is a process, it requires > both parts to refine their arguments progressively. > > This is a matter of choosing the least of several drawbacks. So let us > compare the drawbacks and not muddle things further. > > For me: > > 1. having a dynamic allocation is way way worse than > 2. having sizeof(AVIndexEntry) in the ABI, which is somewhat worse than > 3. having a function with many arguments, which is a tiny bit worse than > 4. having a "use this pointer immediately" constraint. > > We agree except on 3>4, so let us focus on that. > > Option (3) has these practical drawbacks: Many arguments involves more > typing and the risk of messing with the order and getting invalid > values. It also requires redesigning the API if we add fields and > exporting them is useful. And it requires either the overhead of NULL > checks or the caller declaring unneeded variables. > > Option (4) has the obvious practical drawback that misusing the API > causes undefined behavior. > > The constraint of using a pointer immediately on risk of undefined > behavior is actually a frequent one, in FFmpeg but also in C at large: > gethosbtyname, localtime, etc. > > For me, that makes it approximately on par with the risk of messing the > order of the many arguments. > > Which leaves more typing, NULL checks overhead or useless variables > (still more typing). > > It is tiny, I have no trouble admitting, but it is tiny in favor of one > solution. > > If you do not agree with these estimates, please explain exactly where. I disagree with your ordering too, for me 4. is clearly worse than 3. here, as the harm that can be done by mixing up arguments (in this case) is less than use of a possibly invalid pointer. And mixed up arguments would probably be noticed easier when testing than reads of possibly invalid memory. Even when documenting the constraint of immediately using the pointer, it could easily be overlooked/forgotten. It does not even has to be me forgetting the constraint, but could be someone else changing the code in the future, adding some API call before the pointer is used, unaware of the possible consequences and that could invalidate the pointer (if I understand James explanation correctly). This could go unnoticed easily. So IMO having a function with many arguments seems like a better and safer tradeoff in this case to me… > >> If some other developer wants to chime in and comment which approach they >> prefer, then that would be ideal. > > Indeed. > > Regards, > > -- > Nicolas George > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On 3/25/2021 8:55 AM, Nicolas George wrote: > James Almer (12021-03-24): >> I think it's clear by now that nothing i could say will convince you it's >> better to not return a pointer to an internal array when there are safer >> alternatives, and i already gave my reasons why, none of which satisfied >> you, so i don't see the point in keeping this discussion going. > > I find this comment quite offensive. You did not manage to convince me > because your arguments have not been up to the task. Do not try to push > the fault on me, and I will refrain from accusing you of not taking my > arguments into account. Coming to an agreement is a process, it requires > both parts to refine their arguments progressively. It was not meant to be offensive. As i said above i have no other argument against your approach to this function than what i already said, hence why anything i might add will be more or less repeating myself. > > This is a matter of choosing the least of several drawbacks. So let us > compare the drawbacks and not muddle things further. > > For me: > > 1. having a dynamic allocation is way way worse than > 2. having sizeof(AVIndexEntry) in the ABI, which is somewhat worse than > 3. having a function with many arguments, which is a tiny bit worse than > 4. having a "use this pointer immediately" constraint. > > We agree except on 3>4, so let us focus on that. > > Option (3) has these practical drawbacks: Many arguments involves more > typing and the risk of messing with the order and getting invalid > values. It also requires redesigning the API if we add fields and > exporting them is useful. And it requires either the overhead of NULL > checks or the caller declaring unneeded variables. Same situation as av_add_index_entry(). And if you look at the signature of the (3) function in my last proposal, i kept the av_index_* namespace free precisely in case a new API in the future needs to be added for this reason, for both add() and get(). One that could work directly with the AVIndexEntry struct after the internal entries array was redesigned, perhaps using AVTreeNode, so returned pointers or entries are safer to handle. > > Option (4) has the obvious practical drawback that misusing the API > causes undefined behavior. > > The constraint of using a pointer immediately on risk of undefined > behavior is actually a frequent one, in FFmpeg but also in C at large: > gethosbtyname, localtime, etc. > > For me, that makes it approximately on par with the risk of messing the > order of the many arguments. > > Which leaves more typing, NULL checks overhead or useless variables > (still more typing). > > It is tiny, I have no trouble admitting, but it is tiny in favor of one > solution. > > If you do not agree with these estimates, please explain exactly where. I don't know if you remember how in this one imgutils patch i sent some time ago i was against functions with tons of output parameters, because i considered it ugly and typical of enterprise software API. That hasn't changed. But here, being the exact counterpart of an existing add() function put it above the other approach i dislike slightly more of returning an internal pointer and not being able to tell the user just what may invalidate it. > >> If some other developer wants to chime in and comment which approach they >> prefer, then that would be ideal. > > Indeed. > > 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-request@ffmpeg.org with subject "unsubscribe". >
On Thu, Mar 25, 2021 at 02:21:43PM +0100, Marvin Scholz wrote: > On 25 Mar 2021, at 12:55, Nicolas George wrote: > > James Almer (12021-03-24): > >> I think it's clear by now that nothing i could say will convince you it's > >> better to not return a pointer to an internal array when there are safer > >> alternatives, and i already gave my reasons why, none of which satisfied > >> you, so i don't see the point in keeping this discussion going. > > > > I find this comment quite offensive. You did not manage to convince me > > because your arguments have not been up to the task. Do not try to push > > the fault on me, and I will refrain from accusing you of not taking my > > arguments into account. Coming to an agreement is a process, it requires > > both parts to refine their arguments progressively. > > > > This is a matter of choosing the least of several drawbacks. So let us > > compare the drawbacks and not muddle things further. > > > > For me: > > > > 1. having a dynamic allocation is way way worse than > > 2. having sizeof(AVIndexEntry) in the ABI, which is somewhat worse than > > 3. having a function with many arguments, which is a tiny bit worse than > > 4. having a "use this pointer immediately" constraint. > > > > We agree except on 3>4, so let us focus on that. > > > > Option (3) has these practical drawbacks: Many arguments involves more > > typing and the risk of messing with the order and getting invalid > > values. It also requires redesigning the API if we add fields and > > exporting them is useful. And it requires either the overhead of NULL > > checks or the caller declaring unneeded variables. > > > > Option (4) has the obvious practical drawback that misusing the API > > causes undefined behavior. > > > > The constraint of using a pointer immediately on risk of undefined > > behavior is actually a frequent one, in FFmpeg but also in C at large: > > gethosbtyname, localtime, etc. > > > > For me, that makes it approximately on par with the risk of messing the > > order of the many arguments. > > > > Which leaves more typing, NULL checks overhead or useless variables > > (still more typing). > > > > It is tiny, I have no trouble admitting, but it is tiny in favor of one > > solution. > > > > If you do not agree with these estimates, please explain exactly where. > > I disagree with your ordering too, > for me 4. is clearly worse than 3. here, as the harm that can be > done by mixing up arguments (in this case) is less than use of a > possibly invalid pointer. If you mess the order of the arguments in an unlucky way or forgot to initialize one, that's also undefined behavior--though you may get warnings. > And mixed up arguments would probably be noticed easier when testing > than reads of possibly invalid memory. Every operation is potentially invalid unless you make sure that you keep the constraints given by the API. That's the end of the story. Returning to the previous streams example, you also have to make sure, you do not call avformat_free_context() somewhere before accessing AVStream, because that would invalidate that. You can do it easily without getting any warnings or errors. Why nobody complains about this? (Because we are not in Rust but in C.) > Even when documenting the constraint of immediately using the pointer, > it could easily be overlooked/forgotten. Just like everything else. > It does not even has to be me forgetting the constraint, but could be > someone else changing the code in the future, adding some API call > before the pointer is used, unaware of the possible consequences and > that could invalidate the pointer (if I understand James explanation > correctly). This could go unnoticed easily. Keep away the cat from the code and sit somebody to the chair who used to read the documentation and familiar with the code he modifies. Anyway, if you modify index entries while iterating, then you may miss some entries, or count one twice, who knows. So it is the violation of the API anyways, and you will be happy when you receive an invalid memory access or a fatal av_free() error that will save you days of debugging. > So IMO having a function with many arguments seems like a better and > safer tradeoff in this case to me… Safe-safe... we are still in C, homeland of UB. Your program will not get magically more or less safer depending on what a single FFmpeg API function returns, if you or your colleague (or >me< or >anybody<) never used the standard library before and/or does not read the documentation. Let's investigate a different thing that have not been mentioned before: What if AVIndexEntry changes? Fields ADDED/User INTERESTED: (3): Must use the new av_get_index_entry2(); have to be very careful about the order of parameters. (4): AVIndexEntry.new_field. Fields ADDED/User NOT interested: (3): Deprecation warning for av_get_index_entry(); will have to use function with the new signature, passing NULL, taking care of the order of arguments. (4): - Fields REMOVED/User was INTERESTED: (3): Potentially deprecation warning for av_get_index_entry?(); have to use function with the new signature, taking care of the order of arguments. (4): Deprecation warning for the field; have to use a different field or something. ((If field access would be hidden behind a function call deprecation may not be even necessary in all cases.)) Fields REMOVED/User not INTERESTED: (3): Potentially deprecation warning for av_get_index_entry?(); have to use function with the new signature, taking care of the order of arguments, again. (4): - Old (3) methods surely cause API bloat. Changes in fundamental fields require major changes in all interfaces since old values may not be easily substituted by anything other. In every other case (4) seems a better choice. So IMO const AVIndexEntry *av_get_index_entry(AVStream, unsigned) has the advantage that: - Simple: Implementation is one line, no branches, no memcpy, no AVERROR()s that you 100% not interested (for this function) and can be more easily maintained. - Superset of the "many arguments" proposal: If user wants some fields to live longer, it can copy it... without branches. - Returned object can be extended with functions. (Computed values.) At the end, if you take some random pointer and you expect it too stay valid for some unspecified time, yes, you may face some unexpected result. There are objects that stay usable until the corresponding free() call and there are objects that stay usable until some other condition. You have to look up the API documentation for the function signature, reading what it does, does this function really does what I need and lastly for the correct usage. For me, (4) seems much simpler and simpler things are always a better choice in the long run. -- zsugabubus P.S.: It probably will not be an issue with AVIndexEntry, but a complex structure with pointers would be even more hassle using (3), because output variables would require special memdup()ing and free()ing. Going down the rabbit hole, how many objects should be created at the end? I think this approach is not viable in big so why should it be used in small.
Quoting James Almer (2021-03-25 14:37:20) > On 3/25/2021 8:55 AM, Nicolas George wrote: > > Same situation as av_add_index_entry(). And if you look at the signature > of the (3) function in my last proposal, i kept the av_index_* namespace > free precisely in case a new API in the future needs to be added for > this reason, for both add() and get(). One that could work directly with > the AVIndexEntry struct after the internal entries array was redesigned, > perhaps using AVTreeNode, so returned pointers or entries are safer to > handle. On the topic of av_add_index_entry() - is there any reason that function should be public? Seems like it's internal-use only. > > > > > Option (4) has the obvious practical drawback that misusing the API > > causes undefined behavior. > > > > The constraint of using a pointer immediately on risk of undefined > > behavior is actually a frequent one, in FFmpeg but also in C at large: > > gethosbtyname, localtime, etc. > > > > For me, that makes it approximately on par with the risk of messing the > > order of the many arguments. > > > > Which leaves more typing, NULL checks overhead or useless variables > > (still more typing). > > > > It is tiny, I have no trouble admitting, but it is tiny in favor of one > > solution. > > > > If you do not agree with these estimates, please explain exactly where. > > I don't know if you remember how in this one imgutils patch i sent some > time ago i was against functions with tons of output parameters, because > i considered it ugly and typical of enterprise software API. That hasn't > changed. But here, being the exact counterpart of an existing add() > function put it above the other approach i dislike slightly more of > returning an internal pointer and not being able to tell the user just > what may invalidate it. > > > > >> If some other developer wants to chime in and comment which approach they > >> prefer, then that would be ideal. FWIW in this specific case exporting a short-lived pointer seems less problematic than the other options. But on the other hand I wonder about exporting AVIndexEntry exactly as is internally. Are all these fields useful or even meaningful to external callers? Perhaps we could make a new struct that would export only those fields people actually use. And have the new API return a pointer to something like AVFormatInternal.index_entry_for_the_caller. Also a naming note - I'd prefer the function names to start with avformat, so it's clearer where they belong. "index" can mean many different things.
On 4/1/2021 7:07 AM, Anton Khirnov wrote: > Quoting James Almer (2021-03-25 14:37:20) >> On 3/25/2021 8:55 AM, Nicolas George wrote: >> >> Same situation as av_add_index_entry(). And if you look at the signature >> of the (3) function in my last proposal, i kept the av_index_* namespace >> free precisely in case a new API in the future needs to be added for >> this reason, for both add() and get(). One that could work directly with >> the AVIndexEntry struct after the internal entries array was redesigned, >> perhaps using AVTreeNode, so returned pointers or entries are safer to >> handle. > > On the topic of av_add_index_entry() - is there any reason that function > should be public? Seems like it's internal-use only. We could deprecate it and make it internal, if it's not used by callers. It may have been added back when things were devised as being more user controlled, which ultimately didn't happen, or just made public without giving it much thought. > >> >>> >>> Option (4) has the obvious practical drawback that misusing the API >>> causes undefined behavior. >>> >>> The constraint of using a pointer immediately on risk of undefined >>> behavior is actually a frequent one, in FFmpeg but also in C at large: >>> gethosbtyname, localtime, etc. >>> >>> For me, that makes it approximately on par with the risk of messing the >>> order of the many arguments. >>> >>> Which leaves more typing, NULL checks overhead or useless variables >>> (still more typing). >>> >>> It is tiny, I have no trouble admitting, but it is tiny in favor of one >>> solution. >>> >>> If you do not agree with these estimates, please explain exactly where. >> >> I don't know if you remember how in this one imgutils patch i sent some >> time ago i was against functions with tons of output parameters, because >> i considered it ugly and typical of enterprise software API. That hasn't >> changed. But here, being the exact counterpart of an existing add() >> function put it above the other approach i dislike slightly more of >> returning an internal pointer and not being able to tell the user just >> what may invalidate it. >> >>> >>>> If some other developer wants to chime in and comment which approach they >>>> prefer, then that would be ideal. > > FWIW in this specific case exporting a short-lived pointer seems less > problematic than the other options. > > But on the other hand I wonder about exporting AVIndexEntry exactly as > is internally. Are all these fields useful or even meaningful to > external callers? > Perhaps we could make a new struct that would export only those fields > people actually use. And have the new API return a pointer to something > like AVFormatInternal.index_entry_for_the_caller. There are five fields, pos, timestamp, flags, size, and min_distance. The first three are the most important, with timestamps and flags IMO being the only useful ones for users building their own index entry list for example to display each seek point to the user in a player's seek bar. Pos doesn't seem like it would be useful outside of the internal seeking methods, size seems to be the least useful of them all, and min_distance i can't say, since it probably is mainly useful to also for the internal seeking methods. So instead of making a new struct, or build a second array internally, we could maybe just return those two fields? > > Also a naming note - I'd prefer the function names to start with > avformat, so it's clearer where they belong. "index" can mean many > different things.
James Almer (12021-04-01): > Pos doesn't seem like it would be useful outside of the internal seeking > methods, size seems to be the least useful of them all pos and size seem very useful to me for an application that wants to make quick graphs of (time, bitrate). As a general rule "foobar does not seem useful for users" is usually a lack of imagination from the developer. > So instead of making a new struct, or build a second array internally, we > could maybe just return those two fields? Why do you want to complicate things? Just return the darn pointer and trust the users to use it properly to access the fields they need, without speculating. Regards,
On 4/1/2021 11:29 AM, Nicolas George wrote: > James Almer (12021-04-01): >> Pos doesn't seem like it would be useful outside of the internal seeking >> methods, size seems to be the least useful of them all > > pos and size seem very useful to me for an application that wants to > make quick graphs of (time, bitrate). > > As a general rule "foobar does not seem useful for users" is usually a > lack of imagination from the developer. Yet the line has to be drawn somewhere. Otherwise public headers would be chock full of structs and/or fields normally kept in internal headers because "Someone may find them useful". Look at adts_parser, av_adts_header_parse() returns only two fields from AACADTSHeaderInfo. Could the rest be useful for some users? Probably, people can surely come up with all kinds of scenarios where such fields could be useful, like you and i did for AVIndexEntry ones, but in this case a decision ultimately was made that only samples and frames would be made available. > >> So instead of making a new struct, or build a second array internally, we >> could maybe just return those two fields? > > Why do you want to complicate things? Just return the darn pointer and > trust the users to use it properly to access the fields they need, > without speculating. Please refrain from making such accusations. I want to not return a volatile pointer if i can avoid it, because no matter how many warnings you add or how much you trust the user, it's an unsafe practice. But if both you and Anton consider it the less problematic solution, then fine, lets go with it.
James Almer (12021-04-01): > Yet the line has to be drawn somewhere. Otherwise public headers would be > chock full of structs and/or fields normally kept in internal headers > because "Someone may find them useful". You are right, the line has to be drawn somewhere. I posit that the best place to draw the line is defined by laziness: let us make things private by default; but if something needs to be public, then let us make it public in the simplest possible way. We need to think ahead about what would be a maintenance or compatibility hassle, but apart from that, we should not spend time deciding which bit needs to be public and which bit needs to be private. We just chose the simplest solution. > Please refrain from making such accusations. I insist: the direct consequence of what you want is to complicate things, therefore I consider it is fair to write that in this particular instance "you want to complicate things". But more importantly, it is not an accusation: I state it only in the hope that it will be useful, to you and to the project. Still in the hope to be useful, I will elaborate. I believe I have observed a bias in your policy towards the project. Knowing that it might just be imagination plus confirmation bias from me, I still believe it is useful to let you know about it. It is easier to spot biases in others than in oneself. The bias I believe you have is to underestimate mundane complexity: the cost of typing a few more keys (or to have to rely on advanced features of an editor); the cost of reading longer lines of code; the cost of remembering the order of the arguments of a function; the cost of remembering in which half of a structure that has been split a particular field has arrived, etc. All this is tiny, but it is everywhere, and tiny times many, it starts to accumulate, and it makes working on the project that less fun. This is not an accusation, I just suggest that you think about it. > I want to not return a volatile pointer if i can avoid it, because no > matter how many warnings you add or how much you trust the user, it's > an unsafe practice. But if both you and Anton consider it the less > problematic solution, then fine, lets go with it. I will pile up on this: In C every function that accepts a pointer as argument or that returns a pointer comes with caveats about the validity of the pointer. Just a few common cases: some functions will free their arguments with generic free(); some functions will free them with a specific handler; some functions will keep the pointer and expect it to stay valid; some functions will demand that you free what they return, again, with generic or specific handlers. The list goes on and on and on. There are no exception: when you meet a new function with pointers, you cannot guess the hypotheses it makes about the pointers (unless it is very similar to a function you already know, and even then you can have bad surprises), you have to look at the doc. And if you do not, you will get segfaults or memory leaks or memory corruption. And every C programmer knows it. Possibly not in as many words, but what I wrote just now will be no surprise for any C programmer skilled enough to use the FFmpeg libraries. The constraint "only valid until the next function call on the same structure" is not the most common, nor the least dangerous, but it is not exceptional at all. And in this case, it is by far both the most simple and the most efficient solution. Regards,
diff --git a/libavformat/avformat.h b/libavformat/avformat.h index 765bc3b6f5..ac8b57149e 100644 --- a/libavformat/avformat.h +++ b/libavformat/avformat.h @@ -2754,6 +2754,45 @@ int av_find_default_stream_index(AVFormatContext *s); */ int av_index_search_timestamp(AVStream *st, int64_t timestamp, int flags); +/** + * Get the index entry count for the given AVStream. + * + * @param st stream + * @return the number of index entries in the stream + */ +int av_index_get_entries_count(AVStream *st); + +/** + * Get the AVIndexEntry corresponding to the given index. + * + * @param st stream containing the requested AVIndexEntry + * @param index_entry pointer to an AVIndexEntry where to store the entry. On failure, + * or if the index entry could not be found, this struct will remain + * untouched + * @param idx the desired index + * @return >= 0 on success. AVERROR(ENOENT) if no entry for the requested index could + be found. other negative AVERROR codes on failure. + */ +int av_index_get_entry(AVStream *st, AVIndexEntry *index_entry, int idx); + +/** + * Get the AVIndexEntry corresponding to the given timestamp. + * + * @param st stream containing the requested AVIndexEntry + * @param index_entry pointer to an AVIndexEntry where to store the entry. On failure, + * or if the index entry could not be found, this struct will remain + * untouched + * @param timestamp timestamp to retrieve the index entry for + * @param flags if AVSEEK_FLAG_BACKWARD then the returned entry will correspond + * to the timestamp which is <= the requested one, if backward + * is 0, then it will be >= + * if AVSEEK_FLAG_ANY seek to any frame, only keyframes otherwise + * @return >= 0 on success. AVERROR(ENOENT) if no entry for the requested timestamp could + * be found. other negative AVERROR codes on failure. + */ +int av_index_get_entry_from_timestamp(AVStream *st, AVIndexEntry *index_entry, + int64_t wanted_timestamp, int flags); + /** * Add an index entry into a sorted list. Update the entry if the list * already contains it. diff --git a/libavformat/utils.c b/libavformat/utils.c index f31826f2ea..4e7a8bf23e 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -2129,6 +2129,38 @@ int av_index_search_timestamp(AVStream *st, int64_t wanted_timestamp, int flags) wanted_timestamp, flags); } +int av_index_get_entries_count(AVStream *st) +{ + return st->internal->nb_index_entries; +} + +int av_index_get_entry(AVStream *st, AVIndexEntry *index_entry, int idx) +{ + if (idx < 0) + return AVERROR(EINVAL); + if (idx >= st->internal->nb_index_entries) + return AVERROR(ENOENT); + + memcpy(index_entry, &st->internal->index_entries[idx], sizeof(*index_entry)); + + return 0; +} + +int av_index_get_entry_from_timestamp(AVStream *st, AVIndexEntry *index_entry, + int64_t wanted_timestamp, int flags) +{ + int idx = ff_index_search_timestamp(st->internal->index_entries, + st->internal->nb_index_entries, + wanted_timestamp, flags); + + if (idx < 0) + return AVERROR(ENOENT); + + memcpy(index_entry, &st->internal->index_entries[idx], sizeof(*index_entry)); + + return 0; +} + static int64_t ff_read_timestamp(AVFormatContext *s, int stream_index, int64_t *ppos, int64_t pos_limit, int64_t (*read_timestamp)(struct AVFormatContext *, int , int64_t *, int64_t )) {
Signed-off-by: James Almer <jamrial@gmail.com> --- Following "lavf: move AVStream.*index_entries* to AVStreamInternal", it was revealed that some library users apparently found these fields useful and were accessing them despite not being public. This patch introduces a few functions to recover this functionality in a proper and supported way. We can't simply return a const pointer to the corresponding AVIndexEntry in the array because it may change at any time by for example calling av_add_index_entry(), so it's either a copy of the AVIndexEntry, or adding half a dozen output parameters to the function signature for each field in AVIndexEntry. libavformat/avformat.h | 39 +++++++++++++++++++++++++++++++++++++++ libavformat/utils.c | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+)