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

[Michael Niedermayer]

On Sat, Jan 12, 2008 at 09:27:19PM +0000, Paul Kelly wrote:
> On Sat, 12 Jan 2008, Michael Niedermayer wrote:
>
>> On Mon, Jan 07, 2008 at 08:10:17AM +0000, Paul Kelly wrote:
>> [...]
>>>  /**
>>> + * 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."
>
> OK - I've simplified that doxygen comment and put the mention of halving 
> the size back into the function definition as a comment - is that 
> acceptable style? The FFmpeg source does seem pretty sparse on comments but 
> I guess the philosophy is to simplify the code as much as possible so 
> comments aren't needed..
>
>> [...]
>>> +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
>
> OK I've removed them and made the function void. I thought it would save 
> executing some code but I guess the performance benefit is insignificant 
> compared to decoding video.
>
> I think given Mans' comment that some streamed sources do support seeking, 
> the other patch is not such a good idea. So now I'd prefer to see this one 
> go in :)
[...]
> @@ -477,6 +477,8 @@
>       * demuxing: set by user
>       */
>      enum CodecID subtitle_codec_id;
> +
> +    unsigned int max_index_size; /**< Maximum memory to use per stream for timestamp index */

the comment should follow the same syntax as the ones above that is
specifying who sets it for demuxing/muxing


[...]
>  /**
> + * Ensures the index uses less memory than the maximum specified in
> + * AVFormatContext.max_index_size, by discarding entries if it grows
> + * too large.
> + */
> +void ff_reduce_index(AVFormatContext *s, int stream_index);

this needs at least a note that it is not part of the public API


> +
> +/**
>   * Add a index entry into a sorted list updateing if it is already there.
>   *
>   * @param timestamp timestamp in the timebase of the given stream
> Index: libavformat/utils.c
> ===================================================================
> --- libavformat/utils.c	(revision 11518)
> +++ libavformat/utils.c	(working copy)
> @@ -324,6 +324,7 @@
>  {"year", "set the year", OFFSET(year), FF_OPT_TYPE_INT, DEFAULT, INT_MIN, INT_MAX, E},
>  {"analyzeduration", "how many microseconds are analyzed to estimate duration", OFFSET(max_analyze_duration), FF_OPT_TYPE_INT, 3*AV_TIME_BASE, 0, INT_MAX, D},
>  {"cryptokey", "decryption key", OFFSET(key), FF_OPT_TYPE_BINARY, 0, 0, 0, D},
> +{"maxindexsize", "max memory used for timestamp index (per stream)", OFFSET(max_index_size), FF_OPT_TYPE_INT, INT_MAX, 0, INT_MAX, D},

i would prefer a shorter name, maybe "maxindex"


[...]
> +        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;

int i;
for(i=0; 2*i<st->nb_index_entries; i++)
    st->index_entries[i]= st->index_entries[2*i];
st->nb_index_entries= i;

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

Republics decline into democracies and democracies degenerate into
despotisms. -- Aristotle
-------------- 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/20080113/bad0a171/attachment.pgp>