[Bf-codereview] Camera tracking integration (issue 5285047)

[g.ulairi at gmail.com]

Fixed most of issues reported by Campbell and Brecht. Not sure when
updated patchset would be commited, but there are some comments i wand
to get answers.


http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/intern/rna_tracking.c
File source/blender/makesrna/intern/rna_tracking.c (right):

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/intern/rna_tracking.c#newcode426
source/blender/makesrna/intern/rna_tracking.c:426: prop=
RNA_def_property(srna, "enabled", PROP_BOOLEAN, PROP_NONE);
Hrm. It's current naming convention vs. how artists habited to name some
option.. I can see "enable" in some places too, but if you think it's
not nice it can be renamed.

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/intern/rna_tracking.c#newcode485
source/blender/makesrna/intern/rna_tracking.c:485: prop=
RNA_def_property(srna, "markers_count", PROP_INT, PROP_NONE);
Question was more about if i should use such callback or not. Added it
to be sure..

http://codereview.appspot.com/5285047/diff/32002/source/blender/makesrna/intern/rna_tracking.c#newcode678
source/blender/makesrna/intern/rna_tracking.c:678: prop=
RNA_def_property(srna, "is_reconstructed", PROP_BOOLEAN, PROP_NONE);
Well, "valid" doesn't make much sense ad bit flag of MovieTracking.
It'll make sense if it'll be flag of MovieTracking->reconstrucion. I
just need to know if reconstruction was performed or not (display some
extra information like average error for reconstructed movies)

http://codereview.appspot.com/5285047/diff/32002/source/blender/nodes/composite/nodes/node_composite_movieclip.c
File source/blender/nodes/composite/nodes/node_composite_movieclip.c
(right):

http://codereview.appspot.com/5285047/diff/32002/source/blender/nodes/composite/nodes/node_composite_movieclip.c#newcode166
source/blender/nodes/composite/nodes/node_composite_movieclip.c:166:
node->id= G.main->movieclip.first;
Well, i'll agree and disagree with you here. It's nice to have core
functions have determined behavior, but in this case it's was question
of "determined behavior of core functions" vs. "concentrating node's
logic in it's implementation file". And i'm not sure that automated
creation can cause issues here. Most probably you'll just set
node.movieclip to needed source and wouldn't make any checks here.
There's already some confusing things with setting scene to RenderLayer
node in node_add_node and setting other settings to some node types in
ED_node_set_active. Also, i can't find operator for adding node -- stack
tells this function was called from ui_apply_but_funcs_after (via some
other functions). If we'll add this movieclip setting to node_add_node
it wouldn't "solve" issue with that automated creation, if we'll add
this setting to other function we'll have:
- Functions which weren't supposed to do such kind of things are
changing node settings
- De-centralization of node settings stuff. Different settings for
different nodes in different situations would happen from totally
different parts of code. I don't like this =\

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/intern/depsgraph.c
File source/blender/blenkernel/intern/depsgraph.c (right):

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/intern/depsgraph.c#newcode2034
source/blender/blenkernel/intern/depsgraph.c:2034: /* also all objects
which are parented to tracking data should be re-calculated */
Hrm, yes it looks crappy. Can't remember why this was added, probably
for some crappy constraints setup. Can't make things failed without this
crappy code, so lets delete it and see if it'll cause problems.

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/intern/movieclip.c
File source/blender/blenkernel/intern/movieclip.c (right):

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/intern/movieclip.c#newcode82
source/blender/blenkernel/intern/movieclip.c:82: strncpy(num,
full_name+head_len, numlen);
Argh, always forgot about this.

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/intern/movieclip.c#newcode415
source/blender/blenkernel/intern/movieclip.c:415: /* exists? */
Was thinking about this. BLI_exists behaves a bit differently:
- AFAIK, it doesn't check if file name passed to it is a file or it's a
directory
- It doesn't check if file can be read (e.g. you've got permission to
read it).
Not sure if it's so critical, and it's also how add_image works =\

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/intern/movieclip.c#newcode459
source/blender/blenkernel/intern/movieclip.c:459: static void
real_ibuf_size(MovieClip *clip, MovieClipUser *user, ImBuf *ibuf, int
*width, int *height)
Was thinking about this. In fact, it's not so big issue doe to tracks
coords are in unified space (0..1). Or normalized, as you want.
It can be original size stored in MovieClip. Easy to change and it's not
so hight priority, imo.

P.S. You're commonly using movies and they'll fail if it's original size
is not a multiple of 16 (see #26837 i.e. ;))

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/intern/movieclip.c#newcode531
source/blender/blenkernel/intern/movieclip.c:531:
imb_freerectfloatImBuf(ibuf);
Did you read comment above? It _is_ bug, and i know about it. It was
_me_ who reported it months ago. This workaround can be removed when
that bug is fixed, but until this the whole clip editor becomes almost
useless when starting working with compositor.

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/intern/movieclip.c#newcode581
source/blender/blenkernel/intern/movieclip.c:581:
BLI_lock_thread(LOCK_MOVIECLIP);
Hrm. In fact problem is not cause by cache size. The only issue i can
see here is that IMB_freeImBuf isn't therad-safe. If we'll add
IMB_freeImBuf_threadsafe function which will just call IMB_freeImBuf
from critical section and switch movie/sequencer to use this function,
this stuff would be totally thread safe.
Not sure if there's more simple solutions for this, but don't think it's
really critical issue. The same thing can happen with sequencer before
my refactor here (aka it's not new issue).

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/intern/movieclip.c#newcode1002
source/blender/blenkernel/intern/movieclip.c:1002: void
unlink_movieclip(Main *bmain, MovieClip *clip)
You're right. Totally forgot to maintain this function.

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/intern/tracking.c
File source/blender/blenkernel/intern/tracking.c (right):

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/intern/tracking.c#newcode772
source/blender/blenkernel/intern/tracking.c:772: *fp= (0.2126*rrgbf[0] +
0.7152*rrgbf[1] + 0.0722*rrgbf[2]);
Right. Strange that still nobody noticed this. Does it mean float
buffers aren't needed when doing motion tracking? =\

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/intern/tracking.c#newcode1500
source/blender/blenkernel/intern/tracking.c:1500: static unsigned char
*get_ucharbuf(ImBuf *ibuf)
Right.

http://codereview.appspot.com/5285047/