[FFmpeg-soc] [soc]: r5831 - seek2010/seek2010.patch

[Michael Chinen]

Hi,

On Wed, Jun 16, 2010 at 9:47 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Tue, Jun 15, 2010 at 06:52:35PM +0200, mchinen wrote:
>> Author: mchinen
>> Date: Tue Jun 15 18:52:35 2010
>> New Revision: 5831
>>
>> Log:
>> adding thread-safe (optional flag) support for av_build_index
>
> why isnt it thread safe to begin with?
After thinking about it, thread-safe clearly isn't the right word.
Using this flag will let you call av_build_index on an AVFormatContext
in the background, while some other thread read/decodes the same
context, without messing up the ByteIOContext and priv_data in
between.
This was a use-case I discussed a bit with Baptiste for editors and
players who want to build an index, but have the client able to use
the decoded data as soon as possible.

I'll think of a better name or take suggestions.  'parallel' sounds good to me.

I had a chat with Ronald yesterday about this and I should discuss
this feature on the list.  Because av* lets the users pick when the
read/parse/decode happens, and these modify the ByteIOContext, packet
list, and priv_data, we would need a lot more than a mutex to be able
to read packets on two threads from the same stream.  So I thought to
use a copy of the streams/codecs instead.  Ronald pointed out this
isn't compatible with some inputs so I'll have to return error for
some.  But in this case we can return errors and the client can fall
back to the standard non-parallel av_build_index.

>
>
>>
>> Modified:
>>    seek2010/seek2010.patch
>>
>> Modified: seek2010/seek2010.patch
>> ==============================================================================
>> --- seek2010/seek2010.patch   Sat Jun 12 19:10:27 2010        (r5830)
>> +++ seek2010/seek2010.patch   Tue Jun 15 18:52:35 2010        (r5831)
>> @@ -1,19 +1,19 @@
>>  Index: ffplay.c
>>  ===================================================================
>> ---- ffplay.c (revision 23548)
>> +--- ffplay.c (revision 23615)
>>  +++ ffplay.c (working copy)
>>  @@ -2501,6 +2501,8 @@
>>           goto fail;
>>       }
>>
>> -+    av_build_index(ic, 0);
>> ++    av_build_index(ic, AV_BUILD_INDEX_THREADSAFE);
>>  +
>
> trailing whitespace is forbidden in ffmpeg svn
> and so are tabs
ok, sorry.

> [...]
>
>> @@ -178,12 +178,13 @@ Index: libavformat/avformat.h
>>  + * Part of the new seeking api.  incomplete.
>>  + */
>>  +int av_build_index(AVFormatContext *s, int flags);
>> ++#define AV_BUILD_INDEX_THREADSAFE 0x0001
>>  +
>>  +/**
>>    * Ensures the index uses less memory than the maximum specified in
>>    * AVFormatContext.max_index_size by discarding entries if it grows
>>    * too large.
>
> this is missing documentation
>
>
> [...]
>> @@ -574,36 +575,84 @@ Index: libavformat/utils.c
>>  +        s->streams[i]->seek_table.flags |= AV_SEEKTABLE_COPIED;
>>  +    }
>>  +    } else {
>> ++    AVFormatContext *build_ic;
>>  +        AVPacket pkt;
>>  +
>> -+        /* default table generation behavior from av_seek_frame_generic */
>> -+    /* TODO: see why s->data_offset is the file length for avi/mp4 and others */
>> -+
>> -+    /* use an independent file pointer so that we can use this call in multithreaded contexts*/
>> -+    /* not complete yet - see av_read_packet to see how we need to swap out the old file pointers*/
>> -+    ByteIOContext* pb;
>> -+
>>  +    printf("SEEK_TABLE_DEBUG: building index from scratch\n");
>>  +
>> -+        if ((ret=url_fopen(&pb, s->filename, URL_RDONLY)) < 0) {
>> -+        return ret;
>> ++    /* if the client needs it to be threadsafe, create a new format context to read from. */
>> ++    if(flags & AV_BUILD_INDEX_THREADSAFE) {
>
>> ++        printf("SEEK_TABLE_DEBUG: making thread-safe copy of streams\n");
>
> you cant do printf() in libav*
Noted.  These are temporary, for debugging purposes.  They're still
helpful for me so I hope its okay in soc svn patches.  I can take them
out soon.
> also patches of patches are totally unreadable
Yes, diffs of patches aren't that nice and new to me.  So what is
preferred if my output is patches that go to soc svn? I could make a
new patch file for each commit, but then the soc svn mail won't show
what I modified this commit, requiring you guys to do a manual diff.

Michael