From 92de141b9794e55585d0692bc4c3e745bac63214 Mon Sep 17 00:00:00 2001
From: Juliusz Chroboczek <jch@irif.fr>
Date: Sat, 2 May 2020 22:26:09 +0200
Subject: [PATCH] Rework sending of NACKs.

---
 client.go                       |  6 +++---
 packetcache/packetcache.go      | 30 ++++++++++++++++--------------
 packetcache/packetcache_test.go | 24 +++++++++++++++++-------
 3 files changed, 36 insertions(+), 24 deletions(-)

diff --git a/client.go b/client.go
index 7dc00f1..93101fe 100644
--- a/client.go
+++ b/client.go
@@ -349,9 +349,9 @@ func upLoop(conn *upConnection, track *upTrack) {
 
 		first := track.cache.Store(packet.SequenceNumber, buf[:bytes])
 		if packet.SequenceNumber-first > 24 {
-			first, bitmap := track.cache.BitmapGet()
-			if bitmap != ^uint16(0) {
-				err := conn.sendNACK(track, first, ^bitmap)
+			found, first, bitmap := track.cache.BitmapGet()
+			if found {
+				err := conn.sendNACK(track, first, bitmap)
 				if err != nil {
 					log.Printf("%v", err)
 				}
diff --git a/packetcache/packetcache.go b/packetcache/packetcache.go
index 702c87d..d03d0d7 100644
--- a/packetcache/packetcache.go
+++ b/packetcache/packetcache.go
@@ -59,16 +59,6 @@ func (cache *Cache) set(seqno uint16) {
 		return
 	}
 
-	if seqno == cache.first {
-		cache.bitmap >>= 1
-		cache.first += 1
-		for (cache.bitmap & 1) == 1 {
-			cache.bitmap >>= 1
-			cache.first += 1
-		}
-		return
-	}
-
 	if seqno-cache.first < 32 {
 		cache.bitmap |= (1 << uint16(seqno-cache.first))
 		return
@@ -140,16 +130,28 @@ func (cache *Cache) Get(seqno uint16) []byte {
 	return nil
 }
 
-// Shift 17 bits out of the bitmap, return first index and remaining 16.
-func (cache *Cache) BitmapGet() (uint16, uint16) {
+// Shift 17 bits out of the bitmap.  Return a boolean indicating if any
+// were 0, the index of the first 0 bit, and a bitmap indicating any
+// 0 bits after the first one.
+func (cache *Cache) BitmapGet() (bool, uint16, uint16) {
 	cache.mu.Lock()
 	defer cache.mu.Unlock()
 
 	first := cache.first
-	bitmap := uint16((cache.bitmap >> 1) & 0xFFFF)
+	bitmap := (^cache.bitmap) & 0x1FFFF
 	cache.bitmap >>= 17
 	cache.first += 17
-	return first, bitmap
+
+	if bitmap == 0 {
+		return false, first, 0
+	}
+
+	for bitmap & 1 == 0 {
+		bitmap >>= 1
+		first++
+	}
+
+	return true, first, uint16(bitmap >> 1)
 }
 
 func (cache *Cache) GetStats(reset bool) (uint32, uint32, uint32, uint32) {
diff --git a/packetcache/packetcache_test.go b/packetcache/packetcache_test.go
index f932526..a58862c 100644
--- a/packetcache/packetcache_test.go
+++ b/packetcache/packetcache_test.go
@@ -109,10 +109,13 @@ func TestBitmapGet(t *testing.T) {
 
 	pos := uint16(42)
 	for cache.bitmap != 0 {
-		first, bitmap := cache.BitmapGet()
+		found, first, bitmap := cache.BitmapGet()
 		if first < pos || first >= pos+64 {
 			t.Errorf("First is %v, pos is %v", first, pos)
 		}
+		if !found {
+			t.Fatalf("Didn't find any 0 bits")
+		}
 		value >>= (first - pos)
 		pos = first
 		if (value & 1) != 0 {
@@ -120,11 +123,14 @@ func TestBitmapGet(t *testing.T) {
 		}
 		value >>= 1
 		pos += 1
-		if bitmap != uint16(value&0xFFFF) {
-			t.Errorf("Got %b, expected %b", bitmap, (value & 0xFFFF))
+		for bitmap != 0 {
+			if uint8(bitmap & 1) == uint8(value & 1) {
+				t.Errorf("Bitmap mismatch")
+			}
+			bitmap >>= 1
+			value >>= 1
+			pos += 1
 		}
-		value >>= 16
-		pos += 16
 	}
 	if value != 0 {
 		t.Errorf("Value is %v", value)
@@ -143,9 +149,13 @@ func TestBitmapPacket(t *testing.T) {
 		}
 	}
 
-	first, bitmap := cache.BitmapGet()
+	found, first, bitmap := cache.BitmapGet()
 
-	p := rtcp.NackPair{first, rtcp.PacketBitmap(^bitmap)}
+	if !found {
+		t.Fatalf("Didn't find any 0 bits")
+	}
+
+	p := rtcp.NackPair{first, rtcp.PacketBitmap(bitmap)}
 	pl := p.PacketList()
 
 	for _, s := range pl {