From 56424985df20785c13f00f82c61ea51a7b0840d3 Mon Sep 17 00:00:00 2001
From: Raja Subramanian <raja.gobi@tutanota.com>
Date: Wed, 4 Jan 2023 23:06:50 +0530
Subject: [PATCH] Do not get tripped by default values. (#1284)

* Do not get tripped by default values.

The following scenario declared the second message a dupe incorrectly
- UpdateSubscription{subscribe: true}: This message initialized quality
  to default which is livekit.VideoQuality_LOW
- UpdateTrackSettings{quality: livekit.VideoQuality_LOW} - this one got
  tripped as duplicate because the previous message initialized quality
  to LOW.

Fix it by recording whether track settings have been seen.

no auto subscribe + quality setting to LOW test failed before this
change and passes with this change.

* patch all track setting fields
---
 pkg/rtc/signaldeduper/subscriptiondeduper.go  | 57 +++++++++++++------
 .../signaldeduper/subscriptiondeduper_test.go | 15 ++++-
 2 files changed, 54 insertions(+), 18 deletions(-)

diff --git a/pkg/rtc/signaldeduper/subscriptiondeduper.go b/pkg/rtc/signaldeduper/subscriptiondeduper.go
index c8622a1f9..d1569b86d 100644
--- a/pkg/rtc/signaldeduper/subscriptiondeduper.go
+++ b/pkg/rtc/signaldeduper/subscriptiondeduper.go
@@ -17,31 +17,38 @@ const (
 // --------------------------------------------------
 
 type subscriptionSetting struct {
-	isEnabled bool
-	quality   livekit.VideoQuality
-	width     uint32
-	height    uint32
-	fps       uint32
+	isEnabled         bool
+	trackSettingsSeen bool
+	quality           livekit.VideoQuality
+	width             uint32
+	height            uint32
+	fps               uint32
 }
 
-func subscriptionSettingFromUpdateSubscription(us *livekit.UpdateSubscription) *subscriptionSetting {
-	return &subscriptionSetting{
-		isEnabled: us.Subscribe,
+func subscriptionSettingFromUpdateSubscription(us *livekit.UpdateSubscription, existing *subscriptionSetting) *subscriptionSetting {
+	var ss subscriptionSetting
+	if existing != nil {
+		ss = *existing
 	}
+	ss.isEnabled = us.Subscribe
+	return &ss
+
 }
 
 func subscriptionSettingFromUpdateTrackSettings(uts *livekit.UpdateTrackSettings) *subscriptionSetting {
 	return &subscriptionSetting{
-		isEnabled: !uts.Disabled,
-		quality:   uts.Quality,
-		width:     uts.Width,
-		height:    uts.Height,
-		fps:       uts.Fps,
+		isEnabled:         !uts.Disabled,
+		trackSettingsSeen: true,
+		quality:           uts.Quality,
+		width:             uts.Width,
+		height:            uts.Height,
+		fps:               uts.Fps,
 	}
 }
 
 func (s *subscriptionSetting) Equal(other *subscriptionSetting) bool {
 	return s.isEnabled == other.isEnabled &&
+		s.trackSettingsSeen == other.trackSettingsSeen &&
 		s.quality == other.quality &&
 		s.width == other.width &&
 		s.height == other.height &&
@@ -111,8 +118,15 @@ func (s *SubscriptionDeduper) updateSubscriptionsFromUpdateSubscription(
 		}
 	}
 
-	newSetting := subscriptionSettingFromUpdateSubscription(us)
 	for trackID := range trackIDs {
+		var existingSetting *subscriptionSetting
+		existingState := s.getSubscriptionState(participantKey, trackID)
+		if existingState != nil {
+			existingSetting = existingState.setting
+		}
+
+		newSetting := subscriptionSettingFromUpdateSubscription(us, existingSetting)
+
 		isTrackDupe := s.detectDupe(participantKey, trackID, newSetting)
 		if !isTrackDupe {
 			isDupe = false
@@ -160,8 +174,7 @@ func (s *SubscriptionDeduper) detectDupe(
 	updatedSetting *subscriptionSetting,
 ) bool {
 	isDupe := true
-	participantSubscriptions := s.getOrCreateParticipantSubscriptions(participantKey)
-	state := participantSubscriptions[trackID]
+	state := s.getSubscriptionState(participantKey, trackID)
 	if state == nil || !state.setting.Equal(updatedSetting) {
 		// new track seen or subscription setting change
 		state = &subscriptionState{
@@ -177,8 +190,18 @@ func (s *SubscriptionDeduper) detectDupe(
 	}
 
 	if !isDupe {
-		participantSubscriptions[trackID] = state
+		s.setSubscriptionState(participantKey, trackID, state)
 	}
 
 	return isDupe
 }
+
+func (s *SubscriptionDeduper) getSubscriptionState(participantKey livekit.ParticipantKey, trackID livekit.TrackID) *subscriptionState {
+	participantSubscriptions := s.getOrCreateParticipantSubscriptions(participantKey)
+	return participantSubscriptions[trackID]
+}
+
+func (s *SubscriptionDeduper) setSubscriptionState(participantKey livekit.ParticipantKey, trackID livekit.TrackID, state *subscriptionState) {
+	participantSubscriptions := s.getOrCreateParticipantSubscriptions(participantKey)
+	participantSubscriptions[trackID] = state
+}
diff --git a/pkg/rtc/signaldeduper/subscriptiondeduper_test.go b/pkg/rtc/signaldeduper/subscriptiondeduper_test.go
index eaf52f9ad..c3d4536eb 100644
--- a/pkg/rtc/signaldeduper/subscriptiondeduper_test.go
+++ b/pkg/rtc/signaldeduper/subscriptiondeduper_test.go
@@ -74,8 +74,21 @@ func TestSubscriptionDeduper(t *testing.T) {
 		}
 		require.True(t, sd.Dedupe("p0", us))
 
-		// update track settings, should not be a dupe
+		// update track settings with quality, should not be a dupe
 		uts := &livekit.SignalRequest{
+			Message: &livekit.SignalRequest_TrackSetting{
+				TrackSetting: &livekit.UpdateTrackSettings{
+					TrackSids: []string{
+						"p1.track1",
+					},
+					Quality: livekit.VideoQuality_LOW,
+				},
+			},
+		}
+		require.False(t, sd.Dedupe("p0", uts))
+
+		// update track settings with dimensions, should not be a dupe
+		uts = &livekit.SignalRequest{
 			Message: &livekit.SignalRequest_TrackSetting{
 				TrackSetting: &livekit.UpdateTrackSettings{
 					TrackSids: []string{