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

[brechtvanlommel at gmail.com]

> 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.

I think this is ok behavior for the operator. Maybe the 'new' function
in the API could leave this as NULL (rna_NodeTree_node_new), but I also
don't feel very strongly about that, don't think it's an issue for
scripts in practice.

> 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 =\

Ah right, this checks if it's a file, can leave it as is then.

> 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.

Right, it doesn't seem like it would be much of an issue in practice.

> 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.

Ok, it wasn't clear to me how this particular function related to the
compositing problem in the bug report.

> 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).

The threading issue I'm referring to is mostly when you use pixels from
the ibuf after getting it, since the cache could free it. I.e. one
thread might get an ibuf, another might get a different ibuf, forcing
the first ibuf out of the cache and freeing it.

Anyway, this might not happen in practice in this code so it's a minor
issue. If I wrote this code I'd put the locks in the tracking code to
indicate that's it's because of the the logic there that this is thread
safe, but it's your choice.


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