Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix a bug in AbsCpatureTimeExtension #248

Merged
merged 1 commit into from
Mar 15, 2024
Merged

Fix a bug in AbsCpatureTimeExtension #248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions abscapturetimeextension.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would something like:

duration := time.Duration(offset/(1<<32))*time.Second + time.Duration((offset%(1<<32)))*1e9/(1<<32))*time.Nanosecond

work instead to be branchless?

Copy link
Member Author

@enobufs enobufs Mar 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @kevmo314 for your comment.
To accurately handle negative durations in the NTP offset conversion, the 64-bit value must undergo a 2's complement operation (^(value) + 1), the example you showed wouldn't work. I think it is challenging to implement without conditional branching imo. (FYI: We found, based on a benchmark, the unary operation (val = -val) is faster (Go does it well apparently) than ^(val) + 1. )

If you had a better solution that works, please let me know!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good thanks for trying!

if negative {
duration = -duration
}
return &duration
}

Expand All @@ -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
Comment on lines 96 to 97
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, could we replace & 0xFFFFFFFF with % (1 << 32) to fix the issue?

offset := (lsb << 32) | msb
if negative {
offset = -offset
}
return &AbsCaptureTimeExtension{
Timestamp: toNtpTime(captureTime),
EstimatedCaptureClockOffset: &offset,
Expand Down
106 changes: 73 additions & 33 deletions abscapturetimeextension_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}
})
}
Loading