On Mon, 2008-09-15 at 08:26 +0200, Georg Martius wrote: > Hi, Hi, thanks for your patches!
> please find the patch for moving the code to the coding style. Created with > cvs diff in the filter/stabilize directory. Yup, applied. Mostly OK, but there is still some code that should be updated. No hurry for that. > I also abstracted the linked list implementation as earlier suggested. Since > the file is not yet in the CVS it is attached too. Eventually this might be > moved to libtc. Yes, I plan to add it to libtc because we already have at least another piece of code (cfgfile.c) needing it a list implementation. I'll spend some time thinking about the API which fit best with TC ecosystem, but it will not be very different from yours linkedlist. > I have some questions: > How is the help for filters supposed to work? I figured that I can use > tcmodinfo. But filters like filter_levels suggests that I can pass the option > help and it will print the help message, but it does not. For new-style modules (even if used through compatibility layer, as for filters), the former behaviour has changed. To query the module parameters, or to see the default values of them, the user must use the tcmodinfo tool: $ tcmodinfo -i levels for configuring parameters, the module (of course) still accept options $ transcode [...] -J levels=pre=1 The core idea is that one shouldn't be _forced_ to perform fake runs of transcode just to inspect the module characteristics: use tcmodinfo (as the executable name suggests!) separately instead. > I am also confused about the logging and verbosity. > The functions tc_log_XXX do not filter but only format. That means that I > always have to write if(verbose & TC_INFO) tc_log_info(.....)? Yes you shall go this way until tc_log_* infrastructure receive further enhancements (the timeframe is next major release). > What about changelog and todos for the plugin? Just embed them in the main source, there is no official guidelines for that, every module goes on it's own way. > I am happy for further comments. I'm still reading your code, so I haven't much more to say :) Just a word of warning: while I _personally_ like glib/gtk-isms (oop-isms?) like the following #define NEW(type,cnt) (type*)tc_malloc(sizeof(type)*cnt) there is _NO_ general consensus about such things in the transcode-dev environment, nor anyone like them. So it is best to avoid them. PS: the cast after the malloc is useless and it should be removed Bests, -- Francesco Romani // Ikitt
