[FFmpeg-devel] [PATCH 10/18] lavf: move AVStream.*index_entries* to AVStreamInternal

[Michael Niedermayer]

On Wed, Nov 18, 2020 at 06:49:10PM -0300, James Almer wrote:
> On 11/18/2020 6:41 PM, Michael Niedermayer wrote:
> > On Mon, Nov 16, 2020 at 09:46:01PM -0300, James Almer wrote:
> > > On 11/16/2020 12:25 PM, Hendrik Leppkes wrote:
> > > > On Mon, Nov 16, 2020 at 4:20 PM James Almer <jamrial at gmail.com> wrote:
> > > > > 
> > > > > On 11/16/2020 3:01 AM, Anton Khirnov wrote:
> > > > > > Quoting Xiang, Haihao (2020-11-16 06:16:55)
> > > > > > > 
> > > > > > > This change breaks the compiling of gst-libav (
> > > > > > > https://gitlab.freedesktop.org/gstreamer/gst-libav), I filed
> > > > > > > https://trac.ffmpeg.org/ticket/8988 to track this regression.
> > > > > > 
> > > > > > This is not a regression, it's a bug in gst-libav. These fields are
> > > > > > private and have always been private, applications accessing them are
> > > > > > broken.
> > > > > 
> > > > > Looking at that ticket, it seems gst-libav wants the timestamp for a
> > > > > given index entry. Unless this can already be achieved in some other
> > > > > way, it could maybe be implemented cleanly with a new public function
> > > > > (We have av_index_search_timestamp() to fetch an index from a timestamp
> > > > > after all).
> > > > 
> > > > I also like being able to access the index, it can be used by players
> > > > to efficiently offer seeking straight to keyframes (eg. index
> > > > entries), which basically requires iterating over the full index and
> > > > getting the full information (timestamp, flags at least)
> > > > 
> > > > - Hendrik
> > > 
> > > What do you suggest, implement something like what you added to you
> > > LAVFilters fork? I don't know if returning a pointer to a given AVIndexEntry
> > > is ideal. If that were the case, then it wouldn't be an internal field.
> > > 
> > > Perhaps something like
> > > 
> > 
> > > int av_index_get_entries_count(AVStream *st);
> > > int av_index_get_timestamp(AVStream *st, int64_t *timestamp, int *flags, int
> > > idx);
> > 
> > If the (only) goal is to export the whole list then a better option might be
> > to use an opaque state and some sort of iterator.
> 
> The index entries are in a flat array. A function like what i suggested
> above would simply do st->internal->index_entries[idx].

Yes, if we never want to change the internal data structure, this makes sense.
But then if you never want to change the internal data structure then
allowing direct access could improve performance for some apps potentially

on this topic, a flat array has issues in some circumstances
repeatedly inserting of new elements is one, it is not fast unless the
elements are only added at the end


> 
> > Because the suggestion requires N index based lookups and this is only fast
> > with some conceivable implementations.
> > for example consider an index stored in some sort of tree structure
> > a index based lookup might be O(log n) while one step iterating over all
> > elements could be O(1)
> > 
> > AVIndexEntry *av_index_iterate(AVStream *st, AVIndexIterator **i);
> > 
> > and
> > 
> > AVIndexIterator *i = NULL;
> > AVIndexEntry *entry;
> > for (entry = av_index_iterate(st, &i); i; entry = av_index_iterate(st, &i))
> >      ...
> 
> But do we want to return a pointer to an entry in the index array, or just
> some of the fields for a given entry?
> As i said above, if the former was intended, then it wouldn't have been an
> internal field all this time to being with.

I dont know but a struct seems simpler

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many that live deserve death. And some that die deserve life. Can you give
it to them? Then do not be too eager to deal out death in judgement. For
even the very wise cannot see all ends. -- Gandalf
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20201120/2c9e014e/attachment.sig>