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

[brechtvanlommel at gmail.com]

Blenkernel changes review.


http://codereview.appspot.com/5285047/diff/26003/source/blender/makesdna/DNA_movieclip_types.h
File source/blender/makesdna/DNA_movieclip_types.h (right):

http://codereview.appspot.com/5285047/diff/26003/source/blender/makesdna/DNA_movieclip_types.h#newcode91
source/blender/makesdna/DNA_movieclip_types.h:91: int ok;							/* 1
means scopes are ok and re-calculaito is unneeded */
typo: re-calculaito

http://codereview.appspot.com/5285047/diff/32002/source/blender/blenkernel/BKE_tracking.h
File source/blender/blenkernel/BKE_tracking.h (right):

http://codereview.appspot.com/5285047/diff/32002/source/blender/blenkernel/BKE_tracking.h#newcode103
source/blender/blenkernel/BKE_tracking.h:103: /* Distoriton/Undistortion
*/
Distoriton => Distortion

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 */
This doesn't seem right, looks like it's bypassing the depsgraph system
and just recalculating these objects always, even when nothing related
changed?

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);
Use BLI_strncpy, strncpy does not do null termination of strings if they
are too long.

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/intern/movieclip.c#newcode262
source/blender/blenkernel/intern/movieclip.c:262: /* regular moive cache
*/
moive => movie

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/intern/movieclip.c#newcode415
source/blender/blenkernel/intern/movieclip.c:415: /* exists? */
BLI_exists can be used instead.

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)
If the size of the original image is not a multiple of 4, this isn't
accurate for proxies. Not sure if this would be a problem in practice.

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/intern/movieclip.c#newcode531
source/blender/blenkernel/intern/movieclip.c:531:
imb_freerectfloatImBuf(ibuf);
I think the problem here may be that IMB_dupImBuf does not copy the
ibuf->profile flag. This looks like a bug in that function and might
cause problems in other places as well.

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);
This lock doesn't really make the movie cache thread safe? The reason
being that after return the ibuf, that ibuf might still get removed from
the cache in another thread. If the cache is large enough it's just less
likely to happen.

It seems the tracking code calls this from threads, if they're all
requesting the same ibuf, it would be ok. But in that case the lock
should be placed there rather than here, because it would only be
threadsafe because of the logic there.

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)
This doesn't seem to set all clip references to NULL. Scene, constraints
and 3d view bgpic are not checked, maybe more. It's not really required
for a datablock to support this, but if it is it should be complete.

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/intern/movieclip.c#newcode1008
source/blender/blenkernel/intern/movieclip.c:1008: /* text space */
text space => clip space

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#newcode730
source/blender/blenkernel/intern/tracking.c:730: float *rrgbf=
ibuf->rect_float + pixel*4;
tmpibuf => ibuf

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/intern/tracking.c#newcode732
source/blender/blenkernel/intern/tracking.c:732: *fp= (0.2126*rrgbf[0] +
0.7152*rrgbf[1] + 0.0722*rrgbf[2])/255;
This division by 255 seems wrong.

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/intern/tracking.c#newcode752
source/blender/blenkernel/intern/tracking.c:752: unsigned char *pixels,
*fp;
Small nitpick, fp is usually used for floats, cp for chars.

http://codereview.appspot.com/5285047/diff/18002/source/blender/blenkernel/intern/tracking.c#newcode770
source/blender/blenkernel/intern/tracking.c:770: float *rrgbf=
ibuf->rect_float + pixel*4;
ibuf => tmpibuf

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]);
Missing scaling here, use FTOCHAR() here.

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)
The code here and in get_search_bytebuf could be deduplicated, also
contains same errors.

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