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

[Paul Kelly]

On Sat, 5 Jan 2008, Michael Niedermayer wrote:
> On Fri, Jan 04, 2008 at 11:12:09PM +0000, Paul Kelly wrote:
>>
[...]
>> OK, first attempt at a patch is attached. Is the general idea OK? I'm not
>> sure where the check for !url_is_streamed() should go so I haven't included
>> it - possibly in av_new_stream()?
>>
>> Paul
>
>> Index: libavformat/avformat.h
>> ===================================================================
>> --- libavformat/avformat.h	(revision 11408)
>> +++ libavformat/avformat.h	(working copy)
>> @@ -342,6 +342,7 @@
>>                                      support seeking natively */
>>      int nb_index_entries;
>>      unsigned int index_entries_allocated_size;
>> +    unsigned int max_index_size; /**< max memory to use for index when demuxing */
>>
>>      int64_t nb_frames;                 ///< number of frames in this stream if known or 0
>>
>
> /**
> * Stream structure.
> * New fields can be added to the end with minor version bumps.
>                                  ^^^

Ah yes I meant to ask about that. I wasn't sure if it was more important 
to have the related fields next to each other so it was obvious they were 
related to a human reading through the struct definition, or to avoid 
version increases if at all possible. Now I have the answer.

> * Removal, reordering and changes to existing fields require a major
> * version bump.
> * sizeof(AVStream) must not be used outside libav*.
> */
> typedef struct AVStream {
>
> also iam slightly thinking that this would belong more to AVFormatContext

Yes it seemed to me originally that it should be in AVFormatContext; 
that's why I mentioned the index_built member there that seemed relevant 
but unused. Logically, the index should be valid for the whole container 
and it did not make as much sense to me to have separate indexes for each 
stream. But that is the way it is implemented currently and the 
AVFormatContext is not passed to av_add_index_entry() so I couldn't see 
how it could be easily accessed. I thought it was most in keeping with the 
existing code to add it to AVStream...

> or what is the use case of having max_index_size differ between streams

I can't think of any huge advantages; perhaps the user might want to set 
the index size to 0 on all but one stream to save memory (hmm, actually 
this is an argument in favour of having one index for the whole file!). As 
I said the main reason was simply to try and fit in well with the existing 
code.

> and the user should be able to set it from the command line

Yes that is important - I haven't looked at ffmpeg yet to see how I would 
do this but I agree it is needed.

> if it were in AVFormatContext you only would have to add a single line to
> the AVOption array in libavformat/utils.c

Would that make it easier/simpler to specify it from the ffmpeg command 
line? I will read up AVOption to see how I could use it and how the option 
should be passed to av_add_index_entry().

> [...]
>> +        for(in= 0; in < st->nb_index_entries; in+= 2)
>> +            memmove(&st->index_entries[out++], &st->index_entries[in], sizeof(AVIndexEntry));
>
> for(in=0; in<st->nb_index_entries; in+= 2)
>    st->index_entries[out++]= st->index_entries[in];

Of course. Much nicer.

Paul