---------- Oorspronkelijk bericht ----------
Van: "Youness Alaoui" <[EMAIL PROTECTED]>
Onderwerp: Re: [Amsn-devel] Optimized capture extension
Datum: zondag 22 januari 2006 23:34
> Summary of changes to capture.c:
> - Capture_Grab: the first half of it ended up to be totally different
> from the
> original. I made it run faster by changing it such that it would not
call
> setformat before every grab. That accounts for at least 90% of the
> performance improvement. But then, In a flash of brightness of mind, I
> saw a
> bug in the code: it wouldn't handle switching resolution anymore. So I
> fixed
> that, by moving some code around within the first half of the
function.
> Then
> it supported switching resolution again, but while testing another bug
> appeared: when in low resolution it would repeatedly change to high
> resolution every 3rd frame, and immediately back to low res. Which of
> course
> also brought it back to a much lower framerate. So I needed to change
> some
> more to fix that.
yes, great! I totally forgot about that.. I wanted to store the last
attributes used and set the new attributes only if they changed.. never
had time then it got erased from my TODO (my memory).. thing is, it was
the simplest to make it work, and we needed to make it work for LOW
resolution cams (or allow low res setting if the user chose to)... if
you
made it change resolution if needed only, then great (I though a simple
global variable "current-res" set to LOW or HIGH (more of a boolean
instead of string) and set the new resolution only if the resolution
argument is different from current-res.)
That gets quite close to how I did it. Just no need for the 'current-res'
thing. It is kept in the captureItem. That was done already. There was
just
no if around the setformat call, and the resolution wasn't read correctly
from the CaptureItem structure when using a converter.
> - Capture_AccessSettings: is no more. Capture_SetAttribute and
> Capture_GetAttribute took its place. Those are much less if-sprinkled
> than
> the original, and thus should be a little faster. Also, and to be
honest
> that
> was the most important reason for this change, it looks a lot cleaner.
> Also
> the getting and setting of attributes is separated this way, which I
> regard
> as good common practice. Also I applied the patch I made when we were
> investigating the pwc segfault. That patch makes it use libng to get a
> pointer to an attribute structure, rather than having it muddle with
> lists by
> itself. The same patch also applies to Capture_Open.
aie aie aie!! and that was what I was all proud of!!!! I changed 10
functions into one! and I needed to put them all into one function
because
I had to read the attributes, then change only what I needed to change,
then set the whole capabilities back.. it seems it's not needed with
libng
(since it does it itself, gets/change/sets) since we don't set the whole
capabilities, we only set an attribute.. in that case, you're excused,
it's great this way :P
You can be relaxed, I won't change it back to 10 functions! :-P
> - Capture_Init: Removed the repeating code to register the proc with
the
> TCL
> interpreter. Replaced it by a loop that iterates over an array, which
is
> statically defined at the beginning of the file. Changed the required
> Tcl and
> Tk version to 8.4, because we do not support 8.3 anymore. Removed a
few
> Tk8.3
> specific things elsewhere in the file (in Capture_Grab I think).
looks nice this way, thanks :) but no, I think we should keep 8.3
support
because in case some other application wants to use the extension for a
8.3-compatible software, they'll be able to do it without moidfying the
code (and changing a requirement in the code shouldn't be done by
someone
not knowing the code).. anyways,the code should work for 8.3, so it
should
stay that way... apart from that, 8.3 support is indeed dropped by aMSN
takes care of checking that all by itself!
Okay, I will revert the removal of Tcl/Tk8.3 support.
> - Capture_Open: Just code cleaning. Biggest change is the moving away
of
> the
> code that sets up the colorspace to use, which is hardly
interdependent
> to
> the rest of the function. It is now in a statically defined function
> called
> set_color_conv. This makes Capture_Open a lot cleaner. While using
gdb I
> saw
> gcc inlines it again.
cool :)
> - Throughout the file: changed
> if (debug) {
> ....
> }
>
> to:
> #ifdef DEBUG
> ....
> #endif
>
> The latter makes sure that debug code is removed if not compiling in
> debug
> mode. The former depends on the compiler being so smart to detect that
> debug
> is static, and not written to, so it will alwas be zero if not in
debug
> mode.
> I suspect that most of the time that will end up with the code being
in
> the
> executable, so during execution, debug will be evalued every time,
> unnecessarily, which is a peformance penalty (not a huge one of
course,
> but
> the little ones also count!)
yes, indeed, I agree.. although I don't like #ifdef dependant code that
much, but usually for #ifdef DEBUG, it's perfect!
That is exactly as I think about it!
> - I chose one of the code indentation styles used, and applied it
> thoughout
> the file.
it's the indentation I use :D
:-) I looked at it at a glance, and this one seemed the one used most,
so I
chose to use that.
> - removed Tcl_AppendResult("Wrong # of args //etc.
> instead of that: Tcl_WrongNumArgs
yes!!!! I think it was already done but only in one proc, not all (or in
one proc of another of our extensions)
Indeed, there was one function doing this already, I did it for the
others.
> - Many more subtle changes for cleaning and/or optimization. 100% safe
> fixes.
> (Did a mathematical proof of the equality in functionality between the
> original and what I made it if applicable!)
>
> In capture.h just added some comments, and removed some unnecessary
> stuff.
> Also some code layout changes to make it real clean.
cool
> Okay, that was it... (Yeah, if I say huge diff, then it IS a huge diff
> ;-) )
> So, please test it as much as possible. Expecally when it is about
what
> is in
> Capture_Grab. It is the very core of what capture.c is about, and it
was
> changed most dramatically of all.
yes, that's also what I'm worried about.. especially for some drivers..
some will not work with high res, some will say they work with high res,
but when grabbing, they'll fail and accept low res only... anyways,
we'll
see!
In fact, I was doubting for some time if I would remove the support for
automatically changing resolution if grab fails. I decided to keep it,
but do
it a better way than it was done before. But still I am not content, I'm
sure
it could be better still, but I need some time to think about it.
> BTW: it does work with pwc, I use that driver myself, remember?
memtest86 crashed when testing my memory.. so sorry, but I usually don't
remember such details.. hehe but cool if it works with pwc.. are u using
pwcx ? or using the low resolution driver ?
None of these! I use the improved pwc that has a built-in replacement for
pwcx.
> Harry
ok, now, non-inline comments.. I just took a look at your code, looks
really great, like the many comments (don't remember if they were
already
there though...) and the modularity and code cleaning you added... one
thing I didn't like was :
if (-1 != new_value)
or something like that.. I hate when people do that.. I think it's more
intuitive, easy to read to just use
if (new_value != -1)
Actually it also isn't my custom to do it the 'if (-1 != new_value)'
-way. It
is more like having copied the style from existing code in the file.
anyways, apart from that, it all looks good, but you know, I just took a
quick glance at the code, so maybe there are some weird things I didn't
see yet... in any case, I trust your code, and we'll see the result..
your
code review will be replaced by user's feedback instead!
Thanks for all the explanation and for the contribution!
:-) I'm happy I finally had the time to do some coding for aMSN!
Started feeling a 'much talk, no code' guy already ;-)
-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log
files
for problems? Stop! Download the new AJAX search engine that makes
searching your log files as easy as surfing the web. DOWNLOAD SPLUNK!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=103432&bid=230486&dat=121642
_______________________________________________
Amsn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/amsn-devel
