Francesco:
Thanks for the comments and compliments.
>- The filter, while well written, is not compliant with our STYLE
guidelines
(see transcode source root directory/STYLE).
Yes, I thought the STYLE issue would arise. Well, "If in Rome, do
as the Romans do", so I'll get it somewhat compliant. As a possible
compromise, could a set of "astyle" options be devised that would be
compliant with your style guidelines? (For example "astyle
--style=linux"). That would allow me to convert the code to my
formatting tastes for editing, (I'm an old timer, I get confused easily
when the brackets aren't where I like them), then run "astyle" before
submitting back. It might even be a good thing to run the entire code
base over with astyle. Get everything consistent.
>- 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 :) )
Yes, I agree. It's just a function of time...
>- A few symbols of transcode can be used directly, so for example
tc_malloc + memset -> tc_zalloc; TRUE/FALSE -> TC_TRUE/TC_FALSE...
Ok.
>- constantize as much pointers as is possible!
Low on my priority list... It's a good thing to do, but a pain.
> - 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!
I don't know the difference between the "old" and "new" module
system, but I will move all the globals into a struct. Everything will
be made static.
What is NMS?
> - always prefer enums to #defines whenever feasible.
Will do.
> I'd like to suggest a different source arrangement, feel free to
ignore it or comment/flame/whatever
At the present time, I do want the filter to be able to run the
analysis automatically, so I agree with moving the functionality to
filter/yait_core.c (which allows a common yait.h). However, I'm not
sure I want to drop the tcyait executable. Being a perfectionist (and
control freak), I will want to scrutinize the analysis for each
particular encoding, and possibly re-run the analysis with different
options, or make manual corrections to both the log and ops file to get
it "just perfect". Again, I would not want to have to regenerate the
delta information each time. So, I would like to see tools/tcyait.c
stay (permanently), although, it would simply be a command line argument
parser and would use filter/yait_core.c for all its functionality.
I think I will merge in the analysis portion at this time and have
it run by default, but provide a filter option that will allow the
intermediate log file to be generated, allowing tcyait to be used if
desired.
How about this? The filter could be normally run with say:
Pass 1: -J yait=analyze (-y null)
- compute delta information, analyze it, and create the
.ops file
Pass 2 (3): -J yait=ops
- encode the clip using an .ops file
But, pass 1 can also be run with:
-J yait=log
- just generate the delta information log file, allowing
tcyait to be used to create the .ops file.
or
-J yait=analyze:log
- do the analysis, but also generate a log file so that
further tweaking is possible using tcyait.
I think I like that. Thoughts?
> Oops, forgot to add: I attach a reviewed version of filter_yait.c
that addresses most of STYLE issues outlined in the previous mail. Feel
free to drop it, flame it or take as a basis for further improvements.
Ok, I'll have a look at it. (I haven't yet).
> PS: could you please elaborate the tcdecode bug mentioned on source?
This is a problem that has plagued me for some time. I'm not sure
if the problem is transcode itself, or in some support library used by
transcode. When specifying a frame range, transcode hangs at exit time,
and the filter close operation never gets called. (Very rarely this can
happen even when not specifying a frame range). This problem affects me
for all versions of transcode, v0.6.14, v1.0.2, and v1.1.0. I've seen
bug posts describing this exact problem and it seems to be gentoo
specific. Perhaps something to do with running native posix thread
library only. Very painful. dvdrip, for example, can never grab a
frame without hanging, so I have to run the commands externally and send
transcode a quit signal to get it to exit. Here is a typical run:
[encoder.c] encoder last frame finished (201/201): 0:16:11, ( 0| 0| 1)
[encoder.c] export terminated - buffer(s) empty
[encoder.c] encoder closed
[decoder.c] import stop requested by client=-1212643664
(main=-1212643664) import status=1
[decoder.c] A/V import canceled (-1212643664) (-1212643664)
[decoder.c] video thread exit (ret_code=0) (status_code=4294967295)
[decoder.c] audio thread exit (ret_code=0) (status_code=4294967295)
[decoder.c] vframe_list_lock=0
[decoder.c] aframe_list_lock=0
Hangs forever at this point, no cpu used. Sending a ^C gives:
[transcode] (sighandler) SIGINT received
[transcode] (sighandler) import cancellation submitted
[transcode] (sighandler) export cancellation submitted
And repeats the above for each signal received.
Running a debug build (-g) from ddd is even worse. ddd can
sometimes spin at exit time printing an error message repeatedly
(consuming 100% cpu). Of course, I can't reproduce this at the moment
to give you the error (figures). Does any of this ring a bell? Anyway,
that's why the flush was done in the filter, so that I lost at most 5
frames when I had to kill transcode.
Allan
PS:
Have a look at tcyait.c when time provides, but be aware that the
drop frame balancing was (badly) broken and I have re-written it. (I
also added an option to control the window size used when trying to
balance drop frames). As well, I'm still looking into a case where the
interlace pattern is not being detected properly, so there is more work
to be done there as well. I'll roll those changes up with the analysis
merge. Might be a couple of weeks... Or more...
PPS:
I'm currently trying to complete my filter/extsub enhancements.
Unfortunately, I could not keep the functionality backwards compatible,
so decided to just rework the filter options as I saw fit. One can now
specify opacity and render colors for all three subtitle indices,
anti-alias, zoom, crop the subtitle image, etc. The question is, what
do I call it now? I can't replace the existing filter_extsub, or I'm
going to break things (like dvdrip), and I'm sure you don't want to see
filter_extsub2.c (ever again), so what can you suggest?
extsub/filter_nextsub? (Next extsub, new extsub)? It still should
reside in the extsub directory as it uses the same support code,
subproc.c, etc.
Francesco Romani wrote:
On Sat, 3 Mar 2007 08:59:53 +0100
Francesco Romani <[EMAIL PROTECTED]> wrote:
OK, I finally found some time to take a good glance at the code.
Oops, forgot to add: I attach a reviewed version of filter_yait.c that
addresses most of STYLE issues outlined in the previous mail.
Feel free to drop it, flame it or take as a basis for further improvements.
(note: TC_BUF_LINE symbol is yet private on my tree, it will be committed
ASAP, it is just a
#define TC_BUF_LINE 256)
PS: could you please elaborate the tcdecode bug mentioned on source?
I can recall it now honestly. Filing a bug on tcfoundry.hostme.it/mantis
will be the optimum :)
Bests,
