Repository: pion/rtp
Pull request #248 (2024-03-09 23:00:55)

Fix a bug in AbsCpatureTimeExtension
#### Description
* Small modifications to the following method to handle a negative captureClockOffset value correctly.
    - EstimatedCaptureClockOffsetDuration
    - NewAbsCaptureTimeExtensionWithCaptureClockOffset
* Added a unit test to verify the fix

#### Reference issue
Fixes #247


codecov[bot]: ## [Codecov](https://app.codecov.io/gh/pion/rtp/pull/248?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 86.98%. Comparing base [(`78da5a2`)](https://app.codecov.io/gh/pion/rtp/commit/78da5a2169f8f40819ba7847d62c3e71abb75549?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) to head [(`af67ac0`)](https://app.codecov.io/gh/pion/rtp/pull/248?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).


<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master     #248      +/-   ##
==========================================
- Coverage   87.41%   86.98%   -0.43%     
==========================================
  Files          21       21              
  Lines        2113     2129      +16     
==========================================
+ Hits         1847     1852       +5     
- Misses        230      238       +8     
- Partials       36       39       +3     
```

| [Flag](https://app.codecov.io/gh/pion/rtp/pull/248/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | Coverage Δ | |
|---|---|---|
| [go](https://app.codecov.io/gh/pion/rtp/pull/248/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | `?` | |
| [wasm](https://app.codecov.io/gh/pion/rtp/pull/248/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | `86.98% <100.00%> (+0.09%)` | :arrow_up: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#carryforward-flags-in-the-pull-request-comment) to find out more.


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/pion/rtp/pull/248?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).


Patch:
diff --git a/abscapturetimeextension.go b/abscapturetimeextension.go
index 56b783d..5e96cff 100644
--- a/abscapturetimeextension.go
+++ b/abscapturetimeextension.go
@@ -66,7 +66,15 @@ func (t AbsCaptureTimeExtension) EstimatedCaptureClockOffsetDuration() *time.Dur
 		return nil
 	}
 	offset := *t.EstimatedCaptureClockOffset
+	negative := false
+	if offset < 0 {
+		offset = -offset
+		negative = true
+	}
 	duration := time.Duration(offset/(1<<32))*time.Second + time.Duration((offset&0xFFFFFFFF)*1e9/(1<<32))*time.Nanosecond
+	if negative {
+		duration = -duration
+	}
 	return &duration
 }
 
@@ -80,9 +88,17 @@ func NewAbsCaptureTimeExtension(captureTime time.Time) *AbsCaptureTimeExtension
 // NewAbsCaptureTimeExtensionWithCaptureClockOffset makes new AbsCaptureTimeExtension from time.Time and a clock offset.
 func NewAbsCaptureTimeExtensionWithCaptureClockOffset(captureTime time.Time, captureClockOffset time.Duration) *AbsCaptureTimeExtension {
 	ns := captureClockOffset.Nanoseconds()
+	negative := false
+	if ns < 0 {
+		ns = -ns
+		negative = true
+	}
 	lsb := (ns / 1e9) & 0xFFFFFFFF
 	msb := (((ns % 1e9) * (1 << 32)) / 1e9) & 0xFFFFFFFF
 	offset := (lsb << 32) | msb
+	if negative {
+		offset = -offset
+	}
 	return &AbsCaptureTimeExtension{
 		Timestamp:                   toNtpTime(captureTime),
 		EstimatedCaptureClockOffset: &offset,


Test patch:
diff --git a/abscapturetimeextension_test.go b/abscapturetimeextension_test.go
index 15c434a..038e677 100644
--- a/abscapturetimeextension_test.go
+++ b/abscapturetimeextension_test.go
@@ -9,38 +9,78 @@ import (
 )
 
 func TestAbsCaptureTimeExtension_Roundtrip(t *testing.T) {
-	t0 := time.Now()
-	e1 := NewAbsCaptureTimeExtension(t0)
-	b1, err1 := e1.Marshal()
-	if err1 != nil {
-		t.Fatal(err1)
-	}
-	var o1 AbsCaptureTimeExtension
-	if err := o1.Unmarshal(b1); err != nil {
-		t.Fatal(err)
-	}
-	dt1 := o1.CaptureTime().Sub(t0).Seconds()
-	if dt1 < -0.001 || dt1 > 0.001 {
-		t.Fatalf("timestamp differs, want %v got %v (dt=%f)", t0, o1.CaptureTime(), dt1)
-	}
-	if o1.EstimatedCaptureClockOffsetDuration() != nil {
-		t.Fatalf("duration differs, want nil got %d", o1.EstimatedCaptureClockOffsetDuration())
-	}
+	t.Run("positive captureClockOffset", func(t *testing.T) {
+		t0 := time.Now()
+		e1 := NewAbsCaptureTimeExtension(t0)
+		b1, err1 := e1.Marshal()
+		if err1 != nil {
+			t.Fatal(err1)
+		}
+		var o1 AbsCaptureTimeExtension
+		if err := o1.Unmarshal(b1); err != nil {
+			t.Fatal(err)
+		}
+		dt1 := o1.CaptureTime().Sub(t0).Seconds()
+		if dt1 < -0.001 || dt1 > 0.001 {
+			t.Fatalf("timestamp differs, want %v got %v (dt=%f)", t0, o1.CaptureTime(), dt1)
+		}
+		if o1.EstimatedCaptureClockOffsetDuration() != nil {
+			t.Fatalf("duration differs, want nil got %d", o1.EstimatedCaptureClockOffsetDuration())
+		}
 
-	e2 := NewAbsCaptureTimeExtensionWithCaptureClockOffset(t0, 1250*time.Millisecond)
-	b2, err2 := e2.Marshal()
-	if err2 != nil {
-		t.Fatal(err2)
-	}
-	var o2 AbsCaptureTimeExtension
-	if err := o2.Unmarshal(b2); err != nil {
-		t.Fatal(err)
-	}
-	dt2 := o1.CaptureTime().Sub(t0).Seconds()
-	if dt2 < -0.001 || dt2 > 0.001 {
-		t.Fatalf("timestamp differs, want %v got %v (dt=%f)", t0, o2.CaptureTime(), dt2)
-	}
-	if *o2.EstimatedCaptureClockOffsetDuration() != 1250*time.Millisecond {
-		t.Fatalf("duration differs, want 250ms got %d", *o2.EstimatedCaptureClockOffsetDuration())
-	}
+		e2 := NewAbsCaptureTimeExtensionWithCaptureClockOffset(t0, 1250*time.Millisecond)
+		b2, err2 := e2.Marshal()
+		if err2 != nil {
+			t.Fatal(err2)
+		}
+		var o2 AbsCaptureTimeExtension
+		if err := o2.Unmarshal(b2); err != nil {
+			t.Fatal(err)
+		}
+		dt2 := o1.CaptureTime().Sub(t0).Seconds()
+		if dt2 < -0.001 || dt2 > 0.001 {
+			t.Fatalf("timestamp differs, want %v got %v (dt=%f)", t0, o2.CaptureTime(), dt2)
+		}
+		if *o2.EstimatedCaptureClockOffsetDuration() != 1250*time.Millisecond {
+			t.Fatalf("duration differs, want 250ms got %d", *o2.EstimatedCaptureClockOffsetDuration())
+		}
+	})
+
+	// This test can verify the for for the issue 247
+	t.Run("negative captureClockOffset", func(t *testing.T) {
+		t0 := time.Now()
+		e1 := NewAbsCaptureTimeExtension(t0)
+		b1, err1 := e1.Marshal()
+		if err1 != nil {
+			t.Fatal(err1)
+		}
+		var o1 AbsCaptureTimeExtension
+		if err := o1.Unmarshal(b1); err != nil {
+			t.Fatal(err)
+		}
+		dt1 := o1.CaptureTime().Sub(t0).Seconds()
+		if dt1 < -0.001 || dt1 > 0.001 {
+			t.Fatalf("timestamp differs, want %v got %v (dt=%f)", t0, o1.CaptureTime(), dt1)
+		}
+		if o1.EstimatedCaptureClockOffsetDuration() != nil {
+			t.Fatalf("duration differs, want nil got %d", o1.EstimatedCaptureClockOffsetDuration())
+		}
+
+		e2 := NewAbsCaptureTimeExtensionWithCaptureClockOffset(t0, -250*time.Millisecond)
+		b2, err2 := e2.Marshal()
+		if err2 != nil {
+			t.Fatal(err2)
+		}
+		var o2 AbsCaptureTimeExtension
+		if err := o2.Unmarshal(b2); err != nil {
+			t.Fatal(err)
+		}
+		dt2 := o1.CaptureTime().Sub(t0).Seconds()
+		if dt2 < -0.001 || dt2 > 0.001 {
+			t.Fatalf("timestamp differs, want %v got %v (dt=%f)", t0, o2.CaptureTime(), dt2)
+		}
+		if *o2.EstimatedCaptureClockOffsetDuration() != -250*time.Millisecond {
+			t.Fatalf("duration differs, want -250ms got %v", *o2.EstimatedCaptureClockOffsetDuration())
+		}
+	})
 }