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

[Michael Chinen]

On Fri, Jun 18, 2010 at 4:44 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Fri, Jun 18, 2010 at 02:16:43AM +0200, Michael Chinen wrote:
>> 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
>
> if a demuxer did that, that would implicate that it keeps changing the
> index after it read through the file, this makes no sense and sounds
> like a bug.
I thought it can happen in some cases.  AVStream.max_index_entries
imposes a size limit based on on the index_entries array.  The default
value is 2^20 bytes, which may or may not hold all index entries.
When the limit is reached ff_reduce_index throws away old ones.  This
is done in av_read_frame_internal for formats that use a generic
index.  But we can just check the flag to see if we've built a
complete table and should ignore this limit.

>
>> -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.
>
> avidec alraedy reads the whole index for normal files. background index
> building would only be used for truncated avi files


>
>
>> -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.
>
> you wont even need a mutex
> just put the index at some point that is accessable to the main demuxer
> when its finished building
> and the main demuxer then checks if thats non NULL and moves it over
> that way the normal index is only accessed by the main demuxer
> this can be implemented without a mutex
Thanks, that's a good solution.  I think i'll just put the check/move
in the seek function since that's the first point in code where we can
use the new table.
index_entries is also a public structure - is it mostly safe to say
that clients haven't been using it or there is some doc saying not to?
(as this would break under the move)

In any case thanks.  I'll begin to move things over to the original
index_entries.

Michael