From 222ce5d1a86ad0f5be2ba1db40c4c2a4a36b064e Mon Sep 17 00:00:00 2001 From: Raja Subramanian Date: Sun, 23 Oct 2022 22:46:01 +0530 Subject: [PATCH] Notify max layer taking into account overshoot. (#1117) * Notify max layer taking into account overshoot. An attempt to handle case of FF stopping layer 0, but not layer 1. When max subscribed layer is layer 0, server allows overshoot to layer 1 to ensure continued streaming when the channel is not congested. But, dynacast could have reported maximum subscribed layer as layer 0. This is a very simple attempt to address that by taking overshoot into account. Needs testing if this works well or not. NOTE: When subsriber/down track is unmuted, it will report the max subscribed layer as the max required layer. In those cases, if the client does not start layer 0, there will still be an issue. IOW, server is not keeping track of client behaviour that the client has stopped layer 0 and publishing only layer 1. Server is just accommodating overshoot with this change. * Fix a couple of tests and change reflect.DeepEqual -> require.Equal as much as possible --- pkg/sfu/forwarder.go | 2 +- pkg/sfu/forwarder_test.go | 45 ++++++++++++++++++++------------------- pkg/sfu/sequencer_test.go | 5 ++--- pkg/sfu/vp8munger_test.go | 12 +++++------ 4 files changed, 32 insertions(+), 32 deletions(-) diff --git a/pkg/sfu/forwarder.go b/pkg/sfu/forwarder.go index 2118d2594..7e4abf688 100644 --- a/pkg/sfu/forwarder.go +++ b/pkg/sfu/forwarder.go @@ -1462,7 +1462,7 @@ func (f *Forwarder) getTranslationParamsVideo(extPkt *buffer.ExtPacket, layer in // if f.ddLayerSelector != nil { // f.ddLayerSelector.SelectLayer(f.currentLayers) // } - if f.currentLayers.Spatial == f.maxLayers.Spatial { + if f.currentLayers.Spatial >= f.maxLayers.Spatial { tp.isSwitchingToMaxLayer = true } } diff --git a/pkg/sfu/forwarder_test.go b/pkg/sfu/forwarder_test.go index d9cd50a9d..f229e97ab 100644 --- a/pkg/sfu/forwarder_test.go +++ b/pkg/sfu/forwarder_test.go @@ -1,7 +1,6 @@ package sfu import ( - "reflect" "testing" "github.com/pion/webrtc/v3" @@ -1209,7 +1208,7 @@ func TestForwarderGetTranslationParamsAudio(t *testing.T) { } actualTP, err := f.GetTranslationParams(extPkt, 0) require.NoError(t, err) - require.True(t, reflect.DeepEqual(expectedTP, *actualTP)) + require.Equal(t, expectedTP, *actualTP) require.True(t, f.started) require.Equal(t, f.lastSSRC, params.SSRC) @@ -1219,7 +1218,7 @@ func TestForwarderGetTranslationParamsAudio(t *testing.T) { } actualTP, err = f.GetTranslationParams(extPkt, 0) require.NoError(t, err) - require.True(t, reflect.DeepEqual(expectedTP, *actualTP)) + require.Equal(t, expectedTP, *actualTP) // out-of-order packet not in cache should be dropped params = &testutils.TestExtPacketParams{ @@ -1236,7 +1235,7 @@ func TestForwarderGetTranslationParamsAudio(t *testing.T) { } actualTP, err = f.GetTranslationParams(extPkt, 0) require.NoError(t, err) - require.True(t, reflect.DeepEqual(expectedTP, *actualTP)) + require.Equal(t, expectedTP, *actualTP) // padding only packet in order should be dropped params = &testutils.TestExtPacketParams{ @@ -1251,7 +1250,7 @@ func TestForwarderGetTranslationParamsAudio(t *testing.T) { } actualTP, err = f.GetTranslationParams(extPkt, 0) require.NoError(t, err) - require.True(t, reflect.DeepEqual(expectedTP, *actualTP)) + require.Equal(t, expectedTP, *actualTP) // in order packet should be forwarded params = &testutils.TestExtPacketParams{ @@ -1271,7 +1270,7 @@ func TestForwarderGetTranslationParamsAudio(t *testing.T) { } actualTP, err = f.GetTranslationParams(extPkt, 0) require.NoError(t, err) - require.True(t, reflect.DeepEqual(expectedTP, *actualTP)) + require.Equal(t, expectedTP, *actualTP) // padding only packet after a gap should be forwarded params = &testutils.TestExtPacketParams{ @@ -1290,7 +1289,7 @@ func TestForwarderGetTranslationParamsAudio(t *testing.T) { } actualTP, err = f.GetTranslationParams(extPkt, 0) require.NoError(t, err) - require.True(t, reflect.DeepEqual(expectedTP, *actualTP)) + require.Equal(t, expectedTP, *actualTP) // out-of-order should be forwarded using cache params = &testutils.TestExtPacketParams{ @@ -1310,7 +1309,7 @@ func TestForwarderGetTranslationParamsAudio(t *testing.T) { } actualTP, err = f.GetTranslationParams(extPkt, 0) require.NoError(t, err) - require.True(t, reflect.DeepEqual(expectedTP, *actualTP)) + require.Equal(t, expectedTP, *actualTP) // switching source should lock onto the new source, but sequence number should be contiguous params = &testutils.TestExtPacketParams{ @@ -1330,7 +1329,7 @@ func TestForwarderGetTranslationParamsAudio(t *testing.T) { } actualTP, err = f.GetTranslationParams(extPkt, 0) require.NoError(t, err) - require.True(t, reflect.DeepEqual(expectedTP, *actualTP)) + require.Equal(t, expectedTP, *actualTP) require.Equal(t, f.lastSSRC, params.SSRC) } @@ -1368,7 +1367,7 @@ func TestForwarderGetTranslationParamsVideo(t *testing.T) { require.NoError(t, err) require.Equal(t, expectedTP, *actualTP) - // although target layer matches, not a key frame, so should drop and ask to send PLI + // although target layer matches, not a key frame, so should drop f.targetLayers = VideoLayers{ Spatial: 0, Temporal: 1, @@ -1398,6 +1397,7 @@ func TestForwarderGetTranslationParamsVideo(t *testing.T) { } extPkt, _ = testutils.GetTestExtPacketVP8(params, vp8) expectedTP = TranslationParams{ + isSwitchingToMaxLayer: true, rtp: &TranslationParamsRTP{ snOrdering: SequenceNumberOrderingContiguous, sequenceNumber: 23333, @@ -1423,7 +1423,7 @@ func TestForwarderGetTranslationParamsVideo(t *testing.T) { } actualTP, err = f.GetTranslationParams(extPkt, 0) require.NoError(t, err) - require.True(t, reflect.DeepEqual(expectedTP, *actualTP)) + require.Equal(t, expectedTP, *actualTP) require.True(t, f.started) require.Equal(t, f.lastSSRC, params.SSRC) @@ -1433,7 +1433,7 @@ func TestForwarderGetTranslationParamsVideo(t *testing.T) { } actualTP, err = f.GetTranslationParams(extPkt, 0) require.NoError(t, err) - require.True(t, reflect.DeepEqual(expectedTP, *actualTP)) + require.Equal(t, expectedTP, *actualTP) // out-of-order packet not in cache should be dropped params = &testutils.TestExtPacketParams{ @@ -1449,7 +1449,7 @@ func TestForwarderGetTranslationParamsVideo(t *testing.T) { } actualTP, err = f.GetTranslationParams(extPkt, 0) require.NoError(t, err) - require.True(t, reflect.DeepEqual(expectedTP, *actualTP)) + require.Equal(t, expectedTP, *actualTP) // padding only packet in order should be dropped params = &testutils.TestExtPacketParams{ @@ -1463,7 +1463,7 @@ func TestForwarderGetTranslationParamsVideo(t *testing.T) { } actualTP, err = f.GetTranslationParams(extPkt, 0) require.NoError(t, err) - require.True(t, reflect.DeepEqual(expectedTP, *actualTP)) + require.Equal(t, expectedTP, *actualTP) // in order packet should be forwarded params = &testutils.TestExtPacketParams{ @@ -1499,7 +1499,7 @@ func TestForwarderGetTranslationParamsVideo(t *testing.T) { } actualTP, err = f.GetTranslationParams(extPkt, 0) require.NoError(t, err) - require.True(t, reflect.DeepEqual(expectedTP, *actualTP)) + require.Equal(t, expectedTP, *actualTP) // temporal layer higher than target, should be dropped params = &testutils.TestExtPacketParams{ @@ -1529,7 +1529,7 @@ func TestForwarderGetTranslationParamsVideo(t *testing.T) { } actualTP, err = f.GetTranslationParams(extPkt, 0) require.NoError(t, err) - require.True(t, reflect.DeepEqual(expectedTP, *actualTP)) + require.Equal(t, expectedTP, *actualTP) // RTP sequence number and VP8 picture id should be contiguous after dropping higher temporal layer picture params = &testutils.TestExtPacketParams{ @@ -1580,7 +1580,7 @@ func TestForwarderGetTranslationParamsVideo(t *testing.T) { } actualTP, err = f.GetTranslationParams(extPkt, 0) require.NoError(t, err) - require.True(t, reflect.DeepEqual(expectedTP, *actualTP)) + require.Equal(t, expectedTP, *actualTP) // padding only packet after a gap should be forwarded params = &testutils.TestExtPacketParams{ @@ -1599,7 +1599,7 @@ func TestForwarderGetTranslationParamsVideo(t *testing.T) { } actualTP, err = f.GetTranslationParams(extPkt, 0) require.NoError(t, err) - require.True(t, reflect.DeepEqual(expectedTP, *actualTP)) + require.Equal(t, expectedTP, *actualTP) // out-of-order should be forwarded using cache, even if it is padding only params = &testutils.TestExtPacketParams{ @@ -1618,7 +1618,7 @@ func TestForwarderGetTranslationParamsVideo(t *testing.T) { } actualTP, err = f.GetTranslationParams(extPkt, 0) require.NoError(t, err) - require.True(t, reflect.DeepEqual(expectedTP, *actualTP)) + require.Equal(t, expectedTP, *actualTP) // switching SSRC (happens for new layer or new track source) // should lock onto the new source, but sequence number should be contiguous @@ -1651,6 +1651,7 @@ func TestForwarderGetTranslationParamsVideo(t *testing.T) { extPkt, _ = testutils.GetTestExtPacketVP8(params, vp8) expectedTP = TranslationParams{ + isSwitchingToMaxLayer: true, rtp: &TranslationParamsRTP{ snOrdering: SequenceNumberOrderingContiguous, sequenceNumber: 23338, @@ -1676,7 +1677,7 @@ func TestForwarderGetTranslationParamsVideo(t *testing.T) { } actualTP, err = f.GetTranslationParams(extPkt, 1) require.NoError(t, err) - require.True(t, reflect.DeepEqual(expectedTP, *actualTP)) + require.Equal(t, expectedTP, *actualTP) require.Equal(t, f.lastSSRC, params.SSRC) } @@ -1869,7 +1870,7 @@ func TestForwardGetPaddingVP8(t *testing.T) { IsKeyFrame: true, } blankVP8 := f.GetPaddingVP8(true) - require.True(t, reflect.DeepEqual(expectedVP8, *blankVP8)) + require.Equal(t, expectedVP8, *blankVP8) // getting padding with no frame end needed, should get next picture id expectedVP8 = buffer.VP8{ @@ -1888,5 +1889,5 @@ func TestForwardGetPaddingVP8(t *testing.T) { IsKeyFrame: true, } blankVP8 = f.GetPaddingVP8(false) - require.True(t, reflect.DeepEqual(expectedVP8, *blankVP8)) + require.Equal(t, expectedVP8, *blankVP8) } diff --git a/pkg/sfu/sequencer_test.go b/pkg/sfu/sequencer_test.go index f0fb9a54c..5ab8b4c06 100644 --- a/pkg/sfu/sequencer_test.go +++ b/pkg/sfu/sequencer_test.go @@ -144,7 +144,7 @@ func Test_packetMeta_VP8(t *testing.T) { IsKeyFrame: true, } unpackedVP8 := p.unpackVP8() - require.True(t, reflect.DeepEqual(expectedVP8, unpackedVP8)) + require.Equal(t, expectedVP8, unpackedVP8) // short picture id and no TL0PICIDX vp8 = &buffer.VP8{ @@ -181,6 +181,5 @@ func Test_packetMeta_VP8(t *testing.T) { IsKeyFrame: true, } unpackedVP8 = p.unpackVP8() - require.True(t, reflect.DeepEqual(expectedVP8, unpackedVP8)) - + require.Equal(t, expectedVP8, unpackedVP8) } diff --git a/pkg/sfu/vp8munger_test.go b/pkg/sfu/vp8munger_test.go index e5bf69e3c..667608db8 100644 --- a/pkg/sfu/vp8munger_test.go +++ b/pkg/sfu/vp8munger_test.go @@ -216,7 +216,7 @@ func TestOutOfOrderPictureId(t *testing.T) { tp, err = v.UpdateAndGet(extPkt, SequenceNumberOrderingGap, 2) require.NoError(t, err) require.NotNil(t, tp) - require.True(t, reflect.DeepEqual(tpExpected, *tp)) + require.Equal(t, tpExpected, *tp) // all three, the last, the current and the in-between should have been added to missing picture id cache value, ok := v.PictureIdOffset(13467) @@ -255,7 +255,7 @@ func TestOutOfOrderPictureId(t *testing.T) { tp, err = v.UpdateAndGet(extPkt, SequenceNumberOrderingOutOfOrder, 2) require.NoError(t, err) require.NotNil(t, tp) - require.True(t, reflect.DeepEqual(tpExpected, *tp)) + require.Equal(t, tpExpected, *tp) } func TestTemporalLayerFiltering(t *testing.T) { @@ -363,7 +363,7 @@ func TestGapInSequenceNumberSamePicture(t *testing.T) { } tp, err := v.UpdateAndGet(extPkt, SequenceNumberOrderingContiguous, 2) require.NoError(t, err) - require.True(t, reflect.DeepEqual(*tp, tpExpected)) + require.Equal(t, tpExpected, *tp) // telling there is a gap in sequence number will add pictures to missing picture cache tpExpected = TranslationParamsVP8{ @@ -385,7 +385,7 @@ func TestGapInSequenceNumberSamePicture(t *testing.T) { } tp, err = v.UpdateAndGet(extPkt, SequenceNumberOrderingGap, 2) require.NoError(t, err) - require.True(t, reflect.DeepEqual(*tp, tpExpected)) + require.Equal(t, tpExpected, *tp) value, ok := v.PictureIdOffset(13467) require.True(t, ok) @@ -437,7 +437,7 @@ func TestUpdateAndGetPadding(t *testing.T) { HeaderSize: 6, IsKeyFrame: true, } - require.True(t, reflect.DeepEqual(expectedVP8, *blankVP8)) + require.Equal(t, expectedVP8, *blankVP8) // getting padding with new picture blankVP8 = v.UpdateAndGetPadding(true) @@ -456,7 +456,7 @@ func TestUpdateAndGetPadding(t *testing.T) { HeaderSize: 6, IsKeyFrame: true, } - require.True(t, reflect.DeepEqual(expectedVP8, *blankVP8)) + require.Equal(t, expectedVP8, *blankVP8) } func TestVP8PictureIdWrapHandler(t *testing.T) {