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

[Michael Niedermayer]

On Mon, Jan 07, 2008 at 08:10:17AM +0000, Paul Kelly wrote:
> On Mon, 7 Jan 2008, Michael Niedermayer wrote:
>> On Mon, Jan 07, 2008 at 01:36:39AM +0100, Baptiste Coudurier wrote:
>>> Paul Kelly wrote:
>>>> [...]
>>>>
>>>> +    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()
>
> Do you mean to call ff_reduce_index() before av_add_index_entry() in 
> individual demuxers, if it's appropriate to how they use the index? The 
> attached patch does that, but only for AVFMT_GENERIC_INDEX and the mpegps 
> demuxer (to solve my specific problem). If it's the right approach I could 
> try adding it to the other demuxers too, but it might be better if someone 
> more familiar with how they work did that.
>
> Paul

[...]
>  /**
> + * Check the size of the index for the given stream against the maximum
> + * specified in the AVFormatContext. If adding another entry would go
> + * over the limit, halve the size of the index by removing every second
> + * entry.
> + * 
> + * @return < 0 if no more entries may be added to the index or the stream is invalid
> + */

Trailing whitespace, and it should be more generically described, not
mentioning the precisse implementation.
Like, "ensures that the index uses less mem than what is specified as
maximimum in AVFormatContext.max_index_size. If the index is too large
it will discard entries."


[...]
> +int ff_reduce_index(AVFormatContext *s, int stream_index)
> +{
> +    AVStream *st;
> +    unsigned int max_entries;
> +

> +    if(stream_index < 0 || stream_index >= s->nb_streams)
> +        return -1;

i think it can be assumed that the parameters are valid


> +    st= s->streams[stream_index];
> +
> +    max_entries= s->max_index_size / sizeof(AVIndexEntry);

> +    if(max_entries == 0)
> +        return -1;

I dont see what harm a single entry would do with max_entries == 0.
So this and the return checking seems unneeded

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

Let us carefully observe those good qualities wherein our enemies excel us
and endeavor to excel them, by avoiding what is faulty, and imitating what
is excellent in them. -- Plutarch
-------------- 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/20080112/544a580e/attachment.pgp>