JitterBuffer: Improve performance for SampleBuilder use #292

thatsnotright · 2024-11-26T23:51:46Z

Description

This PR Should hopefully improve the performance for use by SampleBuilder and in the general case.

@at-wat If you have time would you be able to test this PR and the corresponding branch I created in the webrtc repository? pion/webrtc#2959

Thank you!

codecov · 2024-11-26T23:52:42Z

Codecov Report

❌ Patch coverage is 83.50000% with 66 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.60%. Comparing base (3cb438f) to head (7e03435).

Files with missing lines	Patch %	Lines
pkg/jitterbuffer/rbtree.go	84.26%	45 Missing and 11 partials ⚠️
pkg/jitterbuffer/jitter_buffer.go	61.53%	5 Missing ⚠️
pkg/jitterbuffer/receiver_interceptor.go	80.00%	4 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #292      +/-   ##
==========================================
- Coverage   78.64%   78.60%   -0.04%     
==========================================
  Files          82       83       +1     
  Lines        5113     5497     +384     
==========================================
+ Hits         4021     4321     +300     
- Misses        921      995      +74     
- Partials      171      181      +10

Flag	Coverage Δ
go	`78.60% <83.50%> (-0.04%)`	⬇️
wasm	`76.20% <83.50%> (+0.08%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

at-wat · 2024-11-27T04:24:32Z

I'll try it later!

at-wat · 2024-12-10T09:19:48Z

@thatsnotright I tested this branch locally in my usecase and confirmed that SampleBuilder's performance is good! (mostly same CPU/memory usage as the latest release)

thatsnotright · 2024-12-11T12:03:23Z

@thatsnotright I tested this branch locally in my usecase and confirmed that SampleBuilder's performance is good! (mostly same CPU/memory usage as the latest release)

@at-wat Thank you for taking the time to test, I appreciate your time! I'll clean up the code a little and make some test coverage improvements and then work with @Sean-Der on next steps!

Rework the jitterbuffer for SampleBuilder use Allow Skipping packets

at-wat · 2025-09-24T01:17:56Z

@thatsnotright could you add a benchmark test and post a comparison between the master branch and this PR?

at-wat

Please consider adding a benchmark test to compare with the master branch.
It would be better to have bench test cases for some different amount of unordered packet.

PriorityQueue is now unused but the code is remained. It should be removed.

at-wat · 2025-09-24T01:29:41Z

pkg/jitterbuffer/jitter_buffer_test.go

+func safeUint16(i int) uint16 {
+	if i < 0 {
+		return 0
+	}
+	if i > math.MaxUint16 {
+		return math.MaxUint16
+	}
+
+	return uint16(i)
+}
+
+func safeUint32(i int) uint32 {
+	if i < 0 {
+		return 0
+	}
+	if i > math.MaxInt32 {
+		return math.MaxUint32
+	}
+
+	return uint32(i)
+}


They seems to be used to clamp sequence number and timestamp.
Is it correct to clamp instead of rolling over?
e.g. testing the sequence of (0xFFFE, 0xFFFF, 0xFFFF, 0xFFFF) doesn't make sense and should be (0xFFFE, 0xFFFF, 0x0001, 0x0002)

at-wat · 2025-09-24T01:32:25Z

pkg/jitterbuffer/jitter_buffer_test.go

+}
+
+func TestJitterBufferInOrderPackets(t *testing.T) {
 	assert := assert.New(t)


I think overriding a package name by a local variable makes confusion

at-wat · 2025-09-24T01:56:31Z

pkg/jitterbuffer/receiver_interceptor_test.go

+	}
+
+	// Wait for buffer to transition to emitting state
+	for jb.state == Buffering {


I guess this should be accessed with lock

at-wat · 2025-09-24T02:00:36Z

pkg/jitterbuffer/rbtree_test.go

+				tree.Clear()
+				_, err := tree.Pop()
+				assert.Error(err)
+				assert.Contains(err.Error(), "priority not found")


Should be checked by assert.ErrorIs

at-wat · 2025-09-24T02:03:59Z

pkg/jitterbuffer/rbtree_test.go

+	tests := []struct {
+		name     string
+		ops      func(*RBTree)
+		validate func(*testing.T, *RBTree)
+	}{


IMO, these test cases don't share almost any code and doesn't make sense to be a table driven. Simply writing t.Run("TableRotation", ...) { would be better

at-wat · 2025-09-24T02:10:39Z

pkg/jitterbuffer/rbtree.go

+	if diff > 32768 {
+		return -1
+	}
+	if diff < -32768 {
+		return 1
+	}
+	if diff < 0 {
+		return -1
+	}
+	if diff > 0 {
+		return 1
+	}


Suggested change

if diff > 32768 {

return -1

}

if diff < -32768 {

return 1

}

if diff < 0 {

return -1

}

if diff > 0 {

return 1

}

switch {

case diff > 32768:

return -1

case diff < -32768:

return 1

case diff < 0:

return -1

case diff > 0:

return 1

}

at-wat · 2025-09-24T02:14:54Z

pkg/jitterbuffer/rbtree.go

+// RBTree structure is a red-black tree for fast access based on priority.
+type RBTree struct {
+	root   *rbnode
+	length uint16
+}
+
+// NewTree creates a new red-black tree.
+func NewTree() *RBTree {
+	return &RBTree{}
+}


Having public RBTree in jitterbuffer package looks not proper.
(Users shouldn't have to take care of the internal implementation)
I think it should be an internal sub-package.

at-wat · 2025-09-24T02:20:21Z

pkg/jitterbuffer/receiver_interceptor.go

+		if err != nil {
+			if errors.Is(err, ErrNotFound) {
+				if i.skipMissingPackets {
+					i.log.Warn("Skipping missing packet")


I guess this may spam the logs. Maybe it should be debug level?

at-wat · 2025-09-24T02:37:52Z

pkg/jitterbuffer/rbtree.go

+}
+
+// Pop will remove the root in the queue.
+func (t *RBTree) Pop() (*rtp.Packet, error) {


Does the root node mean the approx. median of the stored priority (sequence number)?
I'm not sure popping this makes sense.

thatsnotright requested a review from at-wat November 26, 2024 23:51

thatsnotright force-pushed the rob/jb_perf branch 2 times, most recently from ea085bb to ab615fe Compare February 26, 2025 00:11

thatsnotright force-pushed the rob/jb_perf branch 4 times, most recently from 521c4f0 to 339f38e Compare April 2, 2025 00:36

thatsnotright force-pushed the rob/jb_perf branch from 339f38e to 6d35fb4 Compare April 15, 2025 00:10

thatsnotright force-pushed the rob/jb_perf branch 3 times, most recently from 3ff714b to 6982678 Compare April 29, 2025 00:46

thatsnotright force-pushed the rob/jb_perf branch 3 times, most recently from 7639fd0 to 72641d8 Compare May 7, 2025 00:21

thatsnotright force-pushed the rob/jb_perf branch from 72641d8 to e99f243 Compare May 22, 2025 00:21

thatsnotright force-pushed the rob/jb_perf branch 5 times, most recently from e7aab86 to 8e6a4ed Compare May 29, 2025 00:46

thatsnotright force-pushed the rob/jb_perf branch 6 times, most recently from 9e75f82 to 77fa007 Compare June 11, 2025 00:12

thatsnotright force-pushed the rob/jb_perf branch 3 times, most recently from 6572218 to 2de1b4e Compare June 11, 2025 12:41

thatsnotright force-pushed the rob/jb_perf branch from 91acafb to d2a5878 Compare September 11, 2025 23:22

thatsnotright marked this pull request as ready for review September 11, 2025 23:29

thatsnotright and others added 2 commits September 11, 2025 19:30

Jitter Buffer Improvements

f865529

Rework the jitterbuffer for SampleBuilder use Allow Skipping packets

JitterBuffer: Fix lint issues

7e03435

thatsnotright force-pushed the rob/jb_perf branch from d2a5878 to 7e03435 Compare September 11, 2025 23:30

at-wat reviewed Sep 24, 2025

View reviewed changes

JitterBuffer: Improve performance for SampleBuilder use #292

Are you sure you want to change the base?

JitterBuffer: Improve performance for SampleBuilder use #292

Uh oh!

Conversation

thatsnotright commented Nov 26, 2024

Description

Uh oh!

codecov bot commented Nov 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

at-wat commented Nov 27, 2024

Uh oh!

at-wat commented Dec 10, 2024

Uh oh!

thatsnotright commented Dec 11, 2024

Uh oh!

at-wat commented Sep 24, 2025

Uh oh!

at-wat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

codecov bot commented Nov 26, 2024 •

edited

Loading