Ok, a bit deeper review.
First of all. Please, follow coding style! If surrounding code
use 3-space indent (most of sipXmediaLib code), use the same,
if it use 4-space indent, then use it too. For new files we recommend
to use 3-space indent and *never use tabs*. Tabs are simply
prohibited in all source files. If you see one - send me a patch
to get rid of it. That's usual rule for cross-platform and multi-IDE code.
Also follow placing of { and } (always on a new line in our projects)
and commenting style (use // instead of /* */). In general - your
code mustn't anyhow differ from surrounding code in its look.
Following is a review of your chanegs:
--- 10793MprDecode.h Fri Oct 31 15:57:12 2008
+++ MprDecode.h Fri Oct 31 15:29:36 2008
+int mSSRC_Change_Buffer[2] ; //SSRCs current / previous
First take:
I can't see why do you store this in Decoder. This variables have
nothing to do with it and should be moved to MprFromNet.
Second take:
We already have a code to detect SSRC change, so there is no
need to do the work twice. Just insert reset code to appropriate
places in MprFromNet, like this:
if (!mPrefSsrcValid) {
(probably it would be just enough to insert it here).
+void resetDejitter(); //don't know if it is really necessary...
+void setmIsStreamInitialized(UtlBoolean initialized);
To maintain good OO-style and hide underlying details (and
reduce code size and possibility of coding errors) you have to
join these two functions into single "one void reset()" which
resets the whole internal state of the Decoder.
On Sat, Nov 1, 2008 at 6:12 PM, Alexander Chemeris
<[EMAIL PROTECTED]> wrote:
> Thanks for your patch, I'll look into in a short and probably check it in,
> But I'd ask you to NOT insert comments like this:
> + //Paulo: For SSRC change...
> + //End Paulo
> It can be clearly seen what you've changed from a diff. And svn is used
> to help us identify who did what in the past. Such comments just make
> the code dirty. Imagine every developer would mark his code, and then
> every one who change few lines mark them, and so on.. that would be
> a hell to read.
>
> On Fri, Oct 31, 2008 at 6:51 PM, Paulo Vicentini
> <[EMAIL PROTECTED]> wrote:
>> Attached is a hack to deal with SSRC change (unicast)
>>
>> b.r.
>>
>> Paulo
>>
>> On Wed, Oct 29, 2008 at 12:57 AM, Alexander Chemeris
>> <[EMAIL PROTECTED]> wrote:
>>>
>>> On Wed, Oct 29, 2008 at 3:11 AM, Paul Whitfield
>>> <[EMAIL PROTECTED]> wrote:
>>> > Paulo Vicentini wrote:
>>> >>
>>> >> Yeah, DEcoder is considering the same stream , but stream has changed:
>>> >>
>>> >> if (mIsStreamInitialized == FALSE) ---> use previous
>>> >> mStreamState...but
>>> >> we have other one
>>> >>
>>> >> Paulo
>>> >>
>>> >>
>>> >
>>> > I have encountered exactly the same issue in a different setup.
>>> > Two independent SSRC on the same connection.
>>> >
>>> > This is slightly worse for me as both SSRC's are active at the same time
>>> > (a simulated multicast connection).
>>>
>>> I must check in multicast changes before the end of the week, so hold
>>> your breath ;)
>>> If it is "simulated multicast" you'll have to tweak default settings a bit
>>> to enable multiple SSRC handling for unicast streams. But that won't
>>> be a bi deal really.
>>>
>>>
>>>
>>> --
>>> Regards,
>>> Alexander Chemeris.
>>>
>>> SIPez LLC.
>>> SIP VoIP, IM and Presence Consulting
>>> http://www.SIPez.com
>>> tel: +1 (617) 273-4000
>>
>>
>> _______________________________________________
>> sipxtapi-dev mailing list
>> [email protected]
>> List Archive: http://list.sipfoundry.org/archive/sipxtapi-dev/
>>
>
>
>
> --
> Regards,
> Alexander Chemeris.
>
> SIPez LLC.
> SIP VoIP, IM and Presence Consulting
> http://www.SIPez.com
> tel: +1 (617) 273-4000
>
--
Regards,
Alexander Chemeris.
SIPez LLC.
SIP VoIP, IM and Presence Consulting
http://www.SIPez.com
tel: +1 (617) 273-4000
_______________________________________________
sipxtapi-dev mailing list
[email protected]
List Archive: http://list.sipfoundry.org/archive/sipxtapi-dev/
