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