[FFmpeg-devel] MPEG-PS demuxer index memory usage

[Michael Niedermayer]

On Mon, Jan 07, 2008 at 01:36:39AM +0100, Baptiste Coudurier wrote:
> Hi guys,
> 
> Paul Kelly wrote:
> > On Sat, 5 Jan 2008, Michael Niedermayer wrote:
> > 
> >> On Sat, Jan 05, 2008 at 05:14:21PM +0000, Paul Kelly wrote:
> >>> On Sat, 5 Jan 2008, Michael Niedermayer wrote:
> >>>
> >>>> also iam slightly thinking that this would belong more to
> >>>> AVFormatContext
> >>>> or what is the use case of having max_index_size differ between streams
> >>>> and the user should be able to set it from the command line
> >>>> if it were in AVFormatContext you only would have to add a single
> >>>> line to
> >>>> the AVOption array in libavformat/utils.c
> >>>
> >>> Ok I understand now about the AVOption array - it's a very neat idea
> >>> and I
> >>> agree adding it there and putting max_index_size in AVFormatContext is a
> >>> more logical and elegant solution than putting it in AVStream.
> > [...]
> >>> If that's the case then the only way I can see that having it in
> >>> AVFormatContext would work would be to rewrite av_add_index_entry()
> >>> to take
> >>> a pointer to an AVFormatContext, and change everywhere it is called
> >>> in all
> >>> the demuxers -
> >>
> >> yes, if its in AVFormatContext av_add_index_entry() and all calls to it
> >> need a tiny change.
> > 
> > OK, the attached patch changes av_add_index_stream() to take a pointer
> > to an AVFormatContext and a stream index instead of a pointer to an
> > AVStream (in a similar manner to av_seek_frame_binary()), and changes
> > all calls to av_add_index_entry() accordingly.
> > 
> > The new struct member max_index_size is added to the end of
> > AVFormatContext, and the AVOption array has a new entry to allow it to
> > be set from the command line.
> > 
> > Things I'm still not sure about:
> > * Should there be also an FF_OPT_TYPE_UINT, 

no


> > or is setting max_index_size
> > to INT_MAX as the default (as I have done) OK?

yes


> > * Should the option be marked as relevant to encoding as well as
> > decoding (not sure how to do that) - while changing all the calls to
> > av_add_index_entry() some of the calls to it appeared to be in muxers as
> > well as demuxers (example: nutenc.c)

as baptiste already realized some demuxers (as well as all muxers) depend on
the index, so no the option is not relevant to encoding


> > 
> > [...]
> >  
> > +    max_entries= s->max_index_size / sizeof(AVIndexEntry);
> > +    if(max_entries == 0)
> > +        return -1;
> > +    if((unsigned)st->nb_index_entries >= max_entries){
> > +        int in, out= 0;
> > +        /* Halve the size of the index by removing every second entry */
> > +        for(in=0; in<st->nb_index_entries; in+= 2)
> > +            st->index_entries[out++]= st->index_entries[in];
> > +        st->nb_index_entries= out;
> > +    }
> > +
> >      entries = av_fast_realloc(st->index_entries,
> 
> Just a note to remember that mov demuxer is building index containing
> all samples, and absolutely need all index entries to seek and demux, so
> you cannot remove any index entry with the current implementation.

yes, right, i should have realized that earlier ...
so i guess the best would be an explicitly called ff_reduce_index() which
does the above. This also safes us from changing all av_add_index_entry()


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

No snowflake in an avalanche ever feels responsible. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080107/a7253862/attachment.pgp>