On Sat, 2007-07-28 at 14:18 +0200, Branko Kokanovic wrote:
> hi all,
> if anyone is interested, I did this for myself and now, with little
> make-up-for-public modifications, this filter ends up here.
> Few notes: I used NMS, I hope I did everything right.
Looks 95% Ok and welll-written, just a few nitpicks that I'd like
to point out.
All of them are very minor things that I've not properly explained
in docs, so is mostly my bad :)
> Only
> preprocessing is possible, not post. It converts from YUV to RGB and
> other way round, and works only with RGB internally, I don't know if
> it slows down transcode because of this,
Indeed colorspace conversion slow down things, the idea for long long
term is to avoid most (ideally: all) of them.
It is also possible (that in theory is true) that multiple colorspace
conversion also degrade output quality due to rounding errors and
error propagations.
> I really, really hope this filter can not be achieved with
> combinations of other transcode commands:)
There is -l option, but at first glance your code looks more general.
> Dunno, maybe it will help someone (sorry if this filter is not in
> scope of transcode 'duties', but I think with transcode is always 'the
> more the better' philosophy:). If you think it's OK, i'm planning to
> do similar filters - kaleidoscope, blur and sepia.
I've nothing against this in principle, but I'm almost 100% focus on
make 1.1.0 finally out so I can't expend much time here.
If new code does NOT duplicate existing functionalities, yes, I also
agree in 'the more the better' philosophy...
...mostly. :)
Here it comes another problem: maintenance.
transcode isn't straightforward to mantain due to huge combination of
input/output/filter combinations. See recent ImageMagick issues for
example. Current development team has frequently spare time problems,
so maintenance easily becomes a problem, even more than in other
(free) software projects.
> Actually, I don't need these ones for myself, but I learned NMS
> (at least I think so:)
Looks like you learned pretty well :)
Of course, if you found some documentation lacking or unclear please
don't hesitate to point it out.
> and a bit of transcode internals writing this (it should be easy now -
> shame to waste all those acquired knowledge on only one filter:), so
> if anyone is interested in kaleidoscope/blur/sepia filter - tell.
if your are interested, you have time and stuff, there is a lot of
filters that needs to be ported to NMS and would be really *GREAT*
if you like to port some of them (or just one :) ).
Just knock me if you're interested :)
+++
Quick code review follows:
> /*************************************************************************/
>
> typedef enum { Mirror_LTR=0, Mirror_RTL, Mirror_TTB, Mirror_BTT}
directions;
It is preferable that:
- constants will be all UPPERCASE
- typedefs have Capitalized Initialz
> static int mirror_fini(TCModuleInstance *self)
> {
> MirrorData *md = NULL;
> TC_MODULE_SELF_CHECK(self, "fini");
>
> md = self->userdata;
>
> tcv_free(md->tcvhandle);
Better if this should be conditional:
if (md->tcvhandle)
tcv_free(md->tcvhandle);
> static int mirror_configure(TCModuleInstance *self,
> const char *options, vob_t *vob)
> {
> MirrorData *md = NULL;
>
> TC_MODULE_SELF_CHECK(self, "configure");
>
> md = self->userdata;
>
> /* checks for RGB or YUV */
> if ((vob->im_v_codec != CODEC_YUV) && (vob->im_v_codec !=
CODEC_RGB)){
> tc_log_error(MOD_NAME, "This filter is only capable of RGB and
YUV mode");
> return TC_ERROR;
> }
> /* setup defaults */
> md->dir = Mirror_LTR;
> md->codec = vob->im_v_codec;
> if (vob->im_v_codec == CODEC_YUV){
> if (!(md->tcvhandle = tcv_init())) {
better to not have assignements in if() conditions (but that is
discouraged, not forbidden):
md->tcvhandle = tcv_init();
if (!md->tcvhandle) { ...
> return(-1);
always preferred to use symbolic constants:
return TC_ERROR;
> char buf[1024];
TC_BUF_MAX?
(libtc/libtc.h)
> if (verbose >= TC_STATS) {
> tc_log_info(MOD_NAME, "options=%s", options);
> }
> ret=optstr_get(options, "dir","%[^:]",buf);
> if (ret>0){
> if (strcmp(buf,"ltr")==0) md->dir=Mirror_LTR;
> else if (strcmp(buf,"rtl")==0) md->dir=Mirror_RTL;
> else if (strcmp(buf,"ttb")==0) md->dir=Mirror_TTB;
> else if (strcmp(buf,"btt")==0) md->dir=Mirror_BTT;
> }
> optstr_get(options, "range", "%u-%u/%d", &md->start,
&md->end, &md->step);
> }
>
> if (md->start % md->step == 0) md->boolstep = 0;
> else md->boolstep = 1;
nitpick^2 (ok, that is annoying but is needed):
This is not consistent with our STYLE guidelines
> if (optstr_lookup(param, "help")) {
> *value = mirror_help;
> }else if (optstr_lookup(param, "dir")) {
> switch (md->dir){
> case Mirror_LTR: *value=mirror_ltr; break;
> case Mirror_RTL: *value=mirror_rtl; break;
> case Mirror_TTB: *value=mirror_ttb; break;
> case Mirror_BTT: *value=mirror_btt; break;
> }
> }
same as above
> static int mirror_filter_video(TCModuleInstance *self, vframe_list_t
*frame)
> {
> MirrorData *md = NULL;
> int w,h,i,j;
>
> TC_MODULE_SELF_CHECK(self, "filer_video");
> TC_MODULE_SELF_CHECK(frame, "filer_video");
>
> md = self->userdata;
> if (md->start <= frame->id && frame->id <= md->end && frame->id%
md->step == md->boolstep) { /* in range */
> /* converting YUV->RGB */
> if (md->codec==CODEC_YUV){
> if (!tcv_convert(md->tcvhandle, frame->video_buf,
frame->video_buf, frame->v_width, frame->v_height,
> IMG_YUV_DEFAULT, IMG_RGB24)){
> tc_log_error(MOD_NAME, "cannot convert YUV stream to
RGB format !");
> return -1;
same as above:
return TC_ERROR;
(ditto for STYLE)
>
> /*************************************************************************/
>
> static const TCCodecID mirror_codecs_in[] = {
> TC_CODEC_ANY, TC_CODEC_ERROR
> };
> static const TCCodecID mirror_codecs_out[] = {
> TC_CODEC_ANY, TC_CODEC_ERROR
> };
Given the filter descripton, I think is better to have
s/TC_CODEC_ANY/TC_CODEC_RGB24, TC_CODEC_YUV420P/
> /*************************************************************************/
>
> static int mirror_get_config(TCModuleInstance *self, char *options)
> {
> MirrorData *md = NULL;
> char b[128];
TC_BUF_MIN?
(libtc/libtc.h)
Bests,
--
Francesco Romani // Ikitt
