On Mon, 26 Feb 2007 19:34:27 -0500
"Allan N. Snider" <[EMAIL PROTECTED]> wrote:
[...]
> This functionality may already exist in other packages (AviSynth
> perhaps?), but it's not in transcode, so this could be a useful
> addition. If there is enough interest, it would be nice to integrate it
> into dvd::rip.
OK, I finally found some time to take a good glance at the code.
I've seen filter part mostly (not the tcyait tool), and the code itself
looks good. I've no problem for inclusion (Andrew?).
Anyway, there are some (not critical) issues that must be addressed.
Don't get me wrong they will NOT deny the inclusion, they will just delay it.
So (not in priority order):
- The filter, while well written, is not compliant with our STYLE guidelines
(see transcode source root directory/STYLE).
- It will be better to add documentation for functions (see again STYLE)
used internally by filter, even if function does `trivial' things (in that
case docs will be trivial too :) )
- A few symbols of transcode can be used directly, so for example
tc_malloc + memset -> tc_zalloc; TRUE/FALSE -> TC_TRUE/TC_FALSE...
- constantize as much pointers as is possible!
- the proposed filter uses old module system, but that isn't a big deal, yet.
If is possible, module private variables should be packed into a struct
(to make future transition to NMS easier without significant pain today).
And they must be made static!
- always prefer enums to #defines whenever feasible.
Regarding the presence of tcyait tool, I fully agree with the rationale and
I don't see as a problem to have it into codebase temporarily.
I'd like to suggest a different source arrangement, feel free to ignore it
or comment/flame/whatever
1) keep filter_yait.c as it is now, but moving macros/constants into a
separate header file, sai yait.h
2) move the decision logic, that is bundled in tools/tcyait.c, in a
separate file under filter/, say filter/yait_core.c
3) both filter_yait.c and yait_core.c will include the same yait.h,
so macros/constants will not be duplicated as it is now partly done
(see filter_yait.c and tcyait.c)
4) in the long shot, once yait frame decision algorithms are tuned,
filter_yait.so will be built including filter_yait.c and yait_core.c,
and log will be produced at TC_FILTER_CLOSE stage, as you already outlined.
5) during tuning phases, filter_yait.so is built using filter_yait.c only,
while tcyait will include tools/tcyait.c and filter/yait_core.c
To be more clear, a couple of makefile snippets using new source code
arrangements
filter/Makefile.am, now:
[...]
filter_yait_la_SOURCES = filter_yait.c # yait_core.c
filter_yait_la_LDFLAGS = -module -avoid-version
filter_yait_la_LIBADD = -lm
[...]
filter/Makefile.am, once yait tuned:
[...]
filter_yait_la_SOURCES = filter_yait.c yait_core.c
filter_yait_la_LDFLAGS = -module -avoid-version
filter_yait_la_LIBADD = -lm
[...]
tools/Makefile.am, now:
tcyait_SOURCES = \
tcyait.c ../filter/yait_core.c
tcyait_LDADD = \
-lm
tools/Makefile.am, once yait tuned:
#tcyait_SOURCES = \
# tcyait.c ../filter/yait_core.c
#tcyait_LDADD = \
# -lm
(tcyait will just go away, or will be identical to former)
Supposed advantages of this approach:
1) less code duplication
2) things are more future-proof (If, of course, I've understood correctly the
yait filter future :) )
3) no relevant complexity added
Thoughts?
Best regards,
--
Francesco Romani - Ikitt ['people always complain, no matther what you do']
IM contact : (email first, Antispam default deny!) icq://27-83-87-867
tiny homepage : http://fromani.exit1.org (see IDEAS if you want send code!)
known bugs : http://tcfoundry.hostme.it/mantis (EXPERIMENTAL)
