Skip to content

Conversation

@arjunshajitech
Copy link
Contributor

Description

Previously, NewInterceptor validated the RTP buffer size by calling rtpbuffer.NewRTPBuffer
this resulted in unnecessary memory allocations during interceptor construction, even though
the buffer itself was not used at that stage.

Reference issue

Fixes #...

@codecov
Copy link

codecov bot commented Dec 17, 2025

Codecov Report

❌ Patch coverage is 75.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.54%. Comparing base (2d87b58) to head (b78d61e).

Files with missing lines Patch % Lines
internal/rtpbuffer/rtpbuffer.go 83.33% 1 Missing and 1 partial ⚠️
pkg/nack/receive_log.go 0.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #390      +/-   ##
==========================================
- Coverage   79.67%   79.54%   -0.13%     
==========================================
  Files          84       84              
  Lines        4182     4176       -6     
==========================================
- Hits         3332     3322      -10     
- Misses        679      681       +2     
- Partials      171      173       +2     
Flag Coverage Δ
go 79.54% <75.00%> (-0.13%) ⬇️

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.

Copy link
Member

@JoTurk JoTurk left a comment

Choose a reason for hiding this comment

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

cool change, thank you.

Comment on lines 39 to 47
allowedSizes := make([]uint16, 0)
correctSize := false
for i := 0; i < 16; i++ {
if size == 1<<i {
correctSize = true

break
}
allowedSizes = append(allowedSizes, 1<<i)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can also avoid allocating allowedSizes every time we call IsBufferSizeValid. Maybe just on the error path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice! fixed.

}, nil
}

func IsBufferSizeValid(size uint16) (bool, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be public and why we can't just return error, bool seems to be redundant here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although returning an error would be sufficient, the function must be public because it is called from the NACK packet code in the RTP buffer.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, thank you.

Previously, NewInterceptor validated the RTP
buffer size by calling rtpbuffer.NewRTPBuffer
this resulted in unnecessary memory allocations
during interceptor construction, even though
the buffer itself was not used at that stage.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants