On Tue, 2008-09-16 at 12:44 +0200, Georg Martius wrote:
> > > I am happy for further comments.
OK, after a deeper (even if not complete) read of the sources, I can add
the following suggestion/notes. Don't take them too seriously, I think
the average code quality it is good and I think the plugin is very
promising. This email is terse only because my lack of time :)
OK, now let's roll
- always take an eye on libtc, it contains handy functions and wrappers
(and growing) which covers most usage patterns in transcode, like
malloc + memset(), better snprintf, strlcat + strlcpy and so on.
There also are functions to allocate properly sized frames and so on.
- always use stdint types (unsigned char -> uint8_t)
this can sound like nitpicking (and for unsigned chars it is quite like
that), but if use stdint becomes an habit, it will save some headaches
when dealing with portability (I'm thinking to avilib and it's doubtful
64 bit safeness, not to mention some other tricky import layer parts.
Anyway.)
- I don't get why you're placing the filters into TC_PREVIEW slot.
We should really avoid that, this slot is designed to fit only preview
filters like pv, preview, sdlview and not anything else.
- If init/fini module functions do not do anything special (and it's
like that in 90% of the cases), it is better to use the generic
functions autogenerated by TC_MODULE_GENERIC_INIT/TC_MODULE_GENERIC_FINI
pair (see tcmodule-plugin.h)
- the following construct doesn't sounds really good:
if (strlen(td->vob->video_in_file) < 250) {
tc_snprintf(td->input, TC_BUF_LINE, "%s.trf",
td->vob->video_in_file);
} else {
tc_log_warn(MOD_NAME, "input name too long, using default `%s'",
DEFAULT_TRANS_FILE_NAME);
tc_snprintf(td->input, TC_BUF_LINE, "transforms.dat");
}
(filter_transform.c:532 and filter_stabilize.c:658)
the consolidate convention is to use temporary files by default into the
current directory, and optionally in an user-configurable (via a filter
option) path.
Also consider that input file can reside in a read-only media.
- It is preferred (at very least by me) to avoid as much as is possible
if() into the inner loop function (filter_video), by using function
pointers whenerver feasible (very likely it is).
- even not required by API, always try to make multiple _stop() calls
idempotent, this will lead to more future-proof code.
For example:
acceptable:
static int transform_stop(TCModuleInstance *self)
{
TransformData *td = NULL;
TC_MODULE_SELF_CHECK(self, "stop");
td = self->userdata;
if (td->src) {
tc_free(td->src);
}
/*...*/
better:
static int transform_stop(TCModuleInstance *self)
{
TransformData *td = NULL;
TC_MODULE_SELF_CHECK(self, "stop");
td = self->userdata;
if (td->src) {
tc_free(td->src);
td->src = NULL; /* NOTE THAT! */
}
/*...*/
Last but not least, wile ASM-powered code is always warmly welcome, I'm
quite confident that it is feasible to achieve even significant speedups
using only plain but clever C code, by improving the algorhytm used
making it more cache friendly and so on. There is some enlightening
papers (freely avalaible) on this topic written by Ulrich Drepper or
Fefe (fefe.de).
Bests,
--
Francesco Romani // Ikitt
