[PATCH 2/4] mgcp/rtp: Add counter for invalid RTP timestamp deltas

[Jacob Erlbeck]

On 11/21/2013 08:30 PM, Holger Hans Peter Freyther wrote:
> On Thu, Nov 21, 2013 at 07:05:43PM +0100, Jacob Erlbeck wrote:
> 
> Thank you for the patch. I have one wish for you to move the
> code to separate method (with the side effect on the data structure).

Ok, see remark at the end of the mail.

> 
>> +	} else {
> 
> I would prefer if we could move this code into another method and
> re-use it later on. Sure this requires to pass "Input"/"Output" into
> the function and the right counters. But I think I prefer this over
> the code clone.
> 
> You just happened to be late to if/elseif/else and I think it is
> getting a bit crowded in this method.

It does ;-)

> 
> 
>> +	/* Check again, whether the timestamps are still valid */
>> +	if (last_tsdelta && seq != state->max_seq) {
> 
> seq == state->max_seq => retransmission or just the guard against
> the division by 0?

The latter, but dSeqNo == 0 => dTS == 0 should be checked, too.

> 
> 
>> +		tsdelta = (timestamp - state->last_timestamp) /
>> +			(seq - state->max_seq);
> 
> okay subtle change as the timestamp_offset has already been applied? My
> idea of taking this code out might be difficult. Can you try anyway?

Yes, I didn't find a clean solution without code duplication yesterday.
But I think it should be possible by moving this into a separate
function which does checking and tsdelta computation. This makes it
easier to improve the tests (e.g. the dSeqNo check above or more complex
packet duration stuff) and should help to clean up mgcp_patch_and_count,
too.

I'll update the patch accordingly.

Jacob