[FFmpeg-devel] [PATCH] Add and use av_fast_padded_malloc.

[Reimar Döffinger]

On Tue, Jan 17, 2012 at 08:55:40PM +0100, Michael Niedermayer wrote:
> On Tue, Jan 17, 2012 at 08:52:49PM +0100, Reimar Döffinger wrote:
> > On Tue, Jan 17, 2012 at 08:36:20PM +0100, Michael Niedermayer wrote:
> > > On Tue, Jan 17, 2012 at 07:16:47PM +0100, Reimar Döffinger wrote:
> > > > On Tue, Jan 17, 2012 at 06:17:44PM +0100, Michael Niedermayer wrote:
> > > > > On Tue, Jan 17, 2012 at 08:08:48AM +0100, Reimar Döffinger wrote:
> > > > > > On Tue, Jan 17, 2012 at 04:26:43AM +0100, Michael Niedermayer wrote:
> > > > > > > On Tue, Jan 17, 2012 at 12:18:17AM +0100, Reimar Döffinger wrote:
> > > > > > > > The same as av_fast_malloc but uses av_mallocz and keeps extra
> > > > > > > > always-0 padding.
> > > > > > > > This does not mean the memory will be 0-initialized after each call,
> > > > > > > > but actually only after each growth of the buffer.
> > > > > > > > However this makes sure that
> > > > > > > > a) all data anywhere in the buffer is always initialized
> > > > > > > > b) the padding is always 0
> > > > > > > > c) the user does not have to bother with adding the padding themselves
> > > > > > > > Fixes another valgrind warning about use of uninitialized data,
> > > > > > > > this time with fate-vsynth1-jpegls.
> > > > > > > > 
> > > > > > > > Signed-off-by: Reimar Döffinger <Reimar.Doeffinger at gmx.de>
> > > > > > > 
> > > > > > > LGTM
> > > > > > 
> > > > > > I wonder if it would be better to make the size of the padding an
> > > > > > argument.
> > > > > > Because I also needed below patch.
> > > > > > Though not having to specify the padding size in almost all cases
> > > > > > might be worth having to memset manually in a rare few.
> > > > > > Note that I think the "si" code that tries to detect if a memset
> > > > > > is necessary is completely broken and I have some doubts trying
> > > > > > to avoid it will give any relevant speedup (how often will the
> > > > > > size be exactly the same?)
> > > > > 
> > > > > it should be exactly the same unless the buffer size is increased
> > > > > which should be only O(log n) times
> > > > 
> > > > As you pointed out not long ago, if the padding has to be 0 the buffer
> > > > size is irrelevant, the actual size has to be the same (and unless you
> > > > clear a lot more in some cases or do some clever tracking you have to
> > > > run the memset both when the size increases or decreases).
> > > 
> > > Our h264 decoder uses the standard bitreader so its ok if the zeros
> > > are farther away it cant get stuck before
> > 
> > In that case the extra memset should not be necessary at all,
> > if the last n bytes of the buffer are never written to,
> > the padded_malloc function will always result in them being 0.
> > It will also make sure that some padding directly after the actually
> > requested size are 0, which would cost a bit performance, do you think
> > this could be an issue here?
> 
> i dont think a O(log n) times called memset 0 is a big issue

The point is that av_fast_padded_malloc actually calls memset basically
every time (on the padding), it only skips it O(log n) times when the
buffer is reallocated.
I mostly intended it for once-per-frame allocations and other not
_highly_ performance critical cases, giving instead hard promises
(there will be a 0'd padding right after the requested size).