[FFmpeg-soc] [soc]: r5826 - in seek2010: . seek2010.patch

[Michael Chinen]

On Thu, Jun 17, 2010 at 8:45 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Wed, Jun 16, 2010 at 10:50:06PM +0200, Michael Chinen wrote:
>> On Wed, Jun 16, 2010 at 10:07 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
>> > On Wed, Jun 09, 2010 at 03:42:09PM +0200, mchinen wrote:
>> >> Author: mchinen
>> >> Date: Wed Jun  9 15:42:08 2010
>> >> New Revision: 5826
>> >>
>> >> Log:
>> >> creating seek2010 dir for my soc proj and adding current patch
>> > [...]
>> >
>> >> +Index: libavformat/avformat.h
>> >> +===================================================================
>> >> +--- libavformat/avformat.h   (revision 23548)
>> >> ++++ libavformat/avformat.h   (working copy)
>> >> +@@ -390,6 +390,21 @@
>> >> +     int min_distance;         /**< Minimum distance between this and the previous keyframe, used to avoid unneeded searching. */
>> >> + } AVIndexEntry;
>> >> +
>> >> ++#define AV_SEEKTABLE_BUILDING   0x0001
>> >> ++#define AV_SEEKTABLE_CBR        0x0002
>> >> ++#define AV_SEEKTABLE_FINISHED   0x0004
>> >> ++#define AV_SEEKTABLE_COPIED     0x0008
>> >
>> > missing documentation
>> >
>> >
>> > [...]
>> >
>> >> +@@ -531,6 +546,9 @@
>> >> +      * Number of frames that have been demuxed during av_find_stream_info()
>> >> +      */
>> >> +     int codec_info_nb_frames;
>> >> ++
>> >> ++    /* new av_seek_frame() support */
>> >> ++    AVSeekTable seek_table;
>> >> + } AVStream;
>> >
>> > we alraedy have a table for seeking, that is AVStream.index_entries
>> > why do you add a second table?
>> This one is a complete index table that will be saved/loaded/built on
>> a different thread during decoding.  I made a new one mostly to make
>> sure I didn't cause regression bugs.  Also currently lots of demuxers
>> change the state of that index table during normal read/parse.  Once
>> the complete table is built, the idea is to stop using the old
>> index_entries one and use the complete table for seeking.  Is this
>> okay?
>
> lets assume you have a seperate demuxer for buildng the index, then this one
> already has its own AVStream.index_entries which it also should already fill
> during demuxing.
> only thing left is moving it over to AVStream.index_entries of the main
> demuxer once its done.
> am i missing something?
>
> this does appear simpler and simpler to implement to me

I've thought more about this and I think there are tradeoffs either way:
With using a seperate demuxer and moving over:
-have to go through all demuxer specific parse code that modifies the
index and change it to not do this after moving the table over
-some demuxers (avidec.c) appear to use the last index in
index_entries to find the last timestamp in read_packet. There may be
a few places where demuxers expect the index_entries to be something
that isn't the complete index table.
-implementing the moving over in the parallel use case will require
more than mutexing the entries array since we will also have invalid
indexes returned by av_index_search_timestamp.


With using AVSeekTable struct inside AVStream:
-more memory is used
-AVStream has one more thing in it (although either way we will need
the flags variable in AVSeekTable to exist somewhere).
-seek functions need to be modified to use this structure instead of
index_entries. (this is already done in utils.c and some demuxers,
although they need to be optimized)

I agree that it would be nicer from an interface standpoint, not to
introduce a new data structure, but it doesn't seem as simple to do it
that way.  You guys definitely know the code better than I do.  Let me
know what I'm missing.

Michael