[Bf-codereview] MovieCache implementation (issue 5283049)

[g.ulairi at gmail.com]

On 2011/10/15 17:55:32, brechtvl wrote:
> There's something wrong with the new files, they don't diff correctly
and inline
> comments don't work there, maybe because this was uploaded from a git
checkout?

Strange. Worked for ages before and i've checked locally with applying
patch on recent svn trunk and it was fine =\

> Anyway, here are some comments:


> BKE_moviecache.h:

> I'd put this code in the imbuf/ module as IMB_moviecache.h. It doesn't
really
> depend on anything in blenkernel.
> There are also functions IMB_cache_limiter_* in imbuf but they seem to
be
> unused, might be good to remove that system now.


Yes, sounds reasonable. Just wasn't sure which dependencies are
weightful. Discussed in IRC and it's fine to move.

> Typo: stroting => storing

Fixed

> moviecache.c:

> if(ibuf->rect_float)
>      channel_size= sizeof(float);

> This should also be += ?

Yeah, stupid mistake.

> } else if(cache) {

> This if(cache) isn't needed.

Right. probably forgot to update it after some of improvements/merges.
Fixed now.

> MEM_freeN(frames);

> This should be moved outside the if(), it will leak with totseg 0.

Well, in theory yes. Just it's currently wasn't possible to create cache
with zero amount of cached frames sequence. Corrected.


http://codereview.appspot.com/5283049/diff/1/source/blender/blenkernel/intern/seqcache.c
> File source/blender/blenkernel/intern/seqcache.c (right):


http://codereview.appspot.com/5283049/diff/1/source/blender/blenkernel/intern/seqcache.c#newcode51
> source/blender/blenkernel/intern/seqcache.c:51: struct MovieCache
*moviecache =
> NULL;
> Suggest to make this static, better not have this in global namespace.

Yeah, missed this.

http://codereview.appspot.com/5283049/