From d9e682a0d253faf793e283db952931d0e42aebb8 Mon Sep 17 00:00:00 2001 From: Raja Subramanian Date: Mon, 22 May 2023 18:46:56 +0530 Subject: [PATCH] Fix unwrap (#1729) * Fix unwrap An out-or-order packet wrapping back after a wrap around had already happened was not using proper cycle ounter to calculate unerapped value. * update mediatransportutil --- go.mod | 2 +- go.sum | 4 +-- pkg/sfu/utils/wraparound.go | 15 ++++++++-- pkg/sfu/utils/wraparound_test.go | 50 +++++++++++++++++++++++++------- 4 files changed, 56 insertions(+), 15 deletions(-) diff --git a/go.mod b/go.mod index f916462b6..c8784cc15 100644 --- a/go.mod +++ b/go.mod @@ -17,7 +17,7 @@ require ( github.com/hashicorp/golang-lru/v2 v2.0.2 github.com/jxskiss/base62 v1.1.0 github.com/livekit/mageutil v0.0.0-20230125210925-54e8a70427c1 - github.com/livekit/mediatransportutil v0.0.0-20230517210015-117bec6a19a8 + github.com/livekit/mediatransportutil v0.0.0-20230522130635-cc5a3793d7b5 github.com/livekit/protocol v1.5.7-0.20230518171313-8999a6b785c9 github.com/livekit/psrpc v0.3.1-0.20230502152150-df9dd21fba11 github.com/mackerelio/go-osstat v0.2.4 diff --git a/go.sum b/go.sum index cf0f2536e..4ddfa5abb 100644 --- a/go.sum +++ b/go.sum @@ -119,8 +119,8 @@ github.com/lithammer/shortuuid/v4 v4.0.0 h1:QRbbVkfgNippHOS8PXDkti4NaWeyYfcBTHtw github.com/lithammer/shortuuid/v4 v4.0.0/go.mod h1:Zs8puNcrvf2rV9rTH51ZLLcj7ZXqQI3lv67aw4KiB1Y= github.com/livekit/mageutil v0.0.0-20230125210925-54e8a70427c1 h1:jm09419p0lqTkDaKb5iXdynYrzB84ErPPO4LbRASk58= github.com/livekit/mageutil v0.0.0-20230125210925-54e8a70427c1/go.mod h1:Rs3MhFwutWhGwmY1VQsygw28z5bWcnEYmS1OG9OxjOQ= -github.com/livekit/mediatransportutil v0.0.0-20230517210015-117bec6a19a8 h1:YgBDljjYPJc57sSwaoyUgiviThQDyS7SyWsXJSRsZH8= -github.com/livekit/mediatransportutil v0.0.0-20230517210015-117bec6a19a8/go.mod h1:MRc0zSOSzXuFt0X218SgabzlaKevkvCckPgBEoHYc34= +github.com/livekit/mediatransportutil v0.0.0-20230522130635-cc5a3793d7b5 h1:Pifg96a/cWZ9tyEk/41LzPmkww7GqBhzJ8E+jgxmQvQ= +github.com/livekit/mediatransportutil v0.0.0-20230522130635-cc5a3793d7b5/go.mod h1:MRc0zSOSzXuFt0X218SgabzlaKevkvCckPgBEoHYc34= github.com/livekit/protocol v1.5.7-0.20230518171313-8999a6b785c9 h1:i61dBfZbe4MSF+5EVv9/kwVPq9pj1bWciBolSajl374= github.com/livekit/protocol v1.5.7-0.20230518171313-8999a6b785c9/go.mod h1:nJvfqOFq0yenjwaJR0K5PCGf/6tbDts9QZ8bts+RBvk= github.com/livekit/psrpc v0.3.1-0.20230502152150-df9dd21fba11 h1:VS23iVQu/TNiLEM5XjbBSY28+B6nSewjKWPDbieg0Ho= diff --git a/pkg/sfu/utils/wraparound.go b/pkg/sfu/utils/wraparound.go index e6c8bbf2c..ea9a09887 100644 --- a/pkg/sfu/utils/wraparound.go +++ b/pkg/sfu/utils/wraparound.go @@ -93,17 +93,24 @@ func (w *WrapAround[T, ET]) GetExtendedHighest() ET { } func (w *WrapAround[T, ET]) maybeAdjustStart(val T) (isRestart bool, preExtendedStart ET, extendedVal ET) { + isWrapBack := func() bool { + return ET(w.highest) < (w.fullRange>>1) && ET(val) >= (w.fullRange>>1) + } + // re-adjust start if necessary. The conditions are // 1. Not seen more than half the range yet // 1. wrap around compared to start and not completed a half cycle, sequences like (10, 65530) in uint16 space // 2. no wrap around, but out-of-order compared to start and not completed a half cycle , sequences like (10, 9), (65530, 65528) in uint16 space + cycles := w.cycles totalNum := w.GetExtendedHighest() - w.GetExtendedStart() + 1 if totalNum > (w.fullRange >> 1) { - extendedVal = ET(w.cycles)*w.fullRange + ET(val) + if isWrapBack() { + cycles-- + } + extendedVal = ET(cycles)*w.fullRange + ET(val) return } - cycles := w.cycles if val-w.start > T(w.fullRange>>1) { // out-of-order with existing start => a new start isRestart = true @@ -115,6 +122,10 @@ func (w *WrapAround[T, ET]) maybeAdjustStart(val T) (isRestart bool, preExtended cycles = 0 } w.start = val + } else { + if isWrapBack() { + cycles-- + } } extendedVal = ET(cycles)*w.fullRange + ET(val) return diff --git a/pkg/sfu/utils/wraparound_test.go b/pkg/sfu/utils/wraparound_test.go index 828242f87..e9b6bd7a2 100644 --- a/pkg/sfu/utils/wraparound_test.go +++ b/pkg/sfu/utils/wraparound_test.go @@ -77,6 +77,21 @@ func TestWrapAroundUint16(t *testing.T) { highest: 10, extendedHighest: (1 << 16) + 10, }, + // out of order with highest, wrap back, but no restart + { + name: "out of order - no restart", + input: (1 << 16) - 3, + updated: wrapAroundUpdateResult[uint32]{ + IsRestart: false, + PreExtendedStart: 0, + PreExtendedHighest: (1 << 16) + 10, + ExtendedVal: (1 << 16) - 3, + }, + start: (1 << 16) - 12, + extendedStart: (1 << 16) - 12, + highest: 10, + extendedHighest: (1 << 16) + 10, + }, // duplicate should return same as highest { name: "duplicate", @@ -95,32 +110,47 @@ func TestWrapAroundUint16(t *testing.T) { // a significant jump in order should not reset start { name: "big in-order jump", - input: 1 << 15, + input: (1 << 15) - 10, updated: wrapAroundUpdateResult[uint32]{ IsRestart: false, PreExtendedStart: 0, PreExtendedHighest: (1 << 16) + 10, - ExtendedVal: (1 << 16) + (1 << 15), + ExtendedVal: (1 << 16) + (1 << 15) - 10, }, start: (1 << 16) - 12, extendedStart: (1 << 16) - 12, - highest: 1 << 15, - extendedHighest: (1 << 16) + (1 << 15), + highest: (1 << 15) - 10, + extendedHighest: (1 << 16) + (1 << 15) - 10, }, // now out-of-order should not reset start as half the range has been seen { name: "out-of-order after half range", - input: (1 << 15) - 1, + input: (1 << 15) - 11, updated: wrapAroundUpdateResult[uint32]{ IsRestart: false, PreExtendedStart: 0, - PreExtendedHighest: (1 << 16) + (1 << 15), - ExtendedVal: (1 << 16) + (1 << 15) - 1, + PreExtendedHighest: (1 << 16) + (1 << 15) - 10, + ExtendedVal: (1 << 16) + (1 << 15) - 11, }, start: (1 << 16) - 12, extendedStart: (1 << 16) - 12, - highest: 1 << 15, - extendedHighest: (1 << 16) + (1 << 15), + highest: (1 << 15) - 10, + extendedHighest: (1 << 16) + (1 << 15) - 10, + }, + // wrap back out-of-order + { + name: "wrap back out-of-order after half range", + input: (1 << 16) - 1, + updated: wrapAroundUpdateResult[uint32]{ + IsRestart: false, + PreExtendedStart: 0, + PreExtendedHighest: (1 << 16) + (1 << 15) - 10, + ExtendedVal: (1 << 16) - 1, + }, + start: (1 << 16) - 12, + extendedStart: (1 << 16) - 12, + highest: (1 << 15) - 10, + extendedHighest: (1 << 16) + (1 << 15) - 10, }, // in-order, should update highest { @@ -129,7 +159,7 @@ func TestWrapAroundUint16(t *testing.T) { updated: wrapAroundUpdateResult[uint32]{ IsRestart: false, PreExtendedStart: 0, - PreExtendedHighest: (1 << 16) + (1 << 15), + PreExtendedHighest: (1 << 16) + (1 << 15) - 10, ExtendedVal: (1 << 16) + (1 << 15) + 3, }, start: (1 << 16) - 12,