---------- 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
