[libav-bugs] [Bug 525] Optimize memory management for mp4 muxer
bugzilla-daemon at aruru.libav.org
bugzilla-daemon at aruru.libav.org
Sun Jun 2 12:27:02 CEST 2013
--- Comment #3 from andysem at mail.ru 2013-06-02 12:27:02 CEST ---
(In reply to comment #2)
> The changes in general look useful, thank you! We'd normally prefer to get the
> changes split separately (as a series of git patches), but if you don't want to
> take the time to do that, we can probably do it as well.
All these changes are related and some of them depend on others. And I don't
use git in my setup, so I'd prefer to work on a single patch.
> What's your real name,
> so that we can give proper attribution to you in the git author field, btw?
> Storing the capacity of some arrays to avoid reallocing is a nice change, but
> it would be better to keep the naming of the capacity variables consistent
> between the two.
Sure, makes sense. I've updated the patch.
> What's the reason for changing MOV_INDEX_CLUSTER_SIZE?
The previous value seems to be unnecessarily high. 16384 makes it allocate
memory for cluster array in 655360 byte steps, which is more than 0.5 megs.
Each entry in the cluster array corresponds to a single audio or video frame,
so for typical media of 25 fps and 20 ms audio frame size we fill this array in
75 elements per second. With 10 second fragments we use in our application, we
should be using somewhere around 750 elements max of this buffer (maybe a
little more, since we set to split fragments at keyframes) because the array
starts to get filled from the beginning on every fragment. So 1024 looks like
the more appropriate value.
I understand that 10 second fragments is our choice, but in conjunction with
memory pooling for the cluster array this constant plays less role than before.
Even if longer fragments are used, the initial array capacity adjustment with
reallocations will only happen for the first fragment and then the memory will
be reused. The upside is that the final capacity will be closer to the actual
fragment size, so less memory will be wasted.
The change may worsen performance a little for non-fragmented mp4 files, since
it will do reallocations more often. But I think, if there's a need to do
anything about it then another allocation strategy should probably be
considered in this case (like double the capacity on each reallocation, up to
some reasonable limit, and only then make the growth linear). I didn't address
this case because I didn't need it and I didn't think that 1024 was
unacceptably low for it as well.
> For the new avio function, Anton suggested that it might be better to add a
> function that creates a new AVIOContext from an existing malloced buffer - that
> would perhaps be a more generic and hopefully more widely useful function.
> Since that's part of the public API, it needs to be discussed properly before
> applied. But with that, you'd end up freeing/mallocing the AVIOContext every
> now and then. Is that less of a problem with your allocator, since it's just an
> allocation of the same size instead of reallocs, perhaps causing less
Passing a pre-allocated buffer to the AVIOContext factory function might be a
good idea, but as I understand this will not eliminate the need in
avio_reset_dyn_buf or similar function. The dyn_buf may need to grow the passed
buffer (thus it will call av_realloc on it, which already forces the user to
use av_malloc to initially allocate the buffer). After reallocation the buffer
pointer changes, so it has to be obtained again when the user wants to use the
written data. The method is also needed to obtain the size of the buffer.
What I would suggest it to introduce 3 methods: to obtain pointer, size and to
reset the buffer (to avoid pointlessly reallocating it; not sure if it will
pose a problem for tcmalloc, but there really is no need for it). The only
thing I'm not sure of is that tail padding avio_close_dyn_buf appends to the
end of the buffer. I don't know if it's needed and when to add it in the
3-method solution (maybe add a 4-th method to pad the buffer when needed?).
This was the main reason I ended up with the single avio_reset_dyn_buf
Configure bugmail: https://bugzilla.libav.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching all bug changes.
More information about the libav-bugs