Avoid RTPBuffer allocation#390
Conversation
Codecov Report❌ Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -36,15 +46,11 @@ func NewRTPBuffer(size uint16) (*RTPBuffer, error) { | |||
| } | |||
| allowedSizes = append(allowedSizes, 1<<i) | |||
There was a problem hiding this comment.
Maybe we can also avoid allocating allowedSizes every time we call IsBufferSizeValid. Maybe just on the error path?
There was a problem hiding this comment.
Nice! fixed.
| }, nil | ||
| } | ||
|
|
||
| func IsBufferSizeValid(size uint16) (bool, error) { |
There was a problem hiding this comment.
Does this need to be public and why we can't just return error, bool seems to be redundant here.
There was a problem hiding this comment.
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.
arjunshajitech
force-pushed
the
fix-new-interceptor
branch
from
b93395d to
d976f64
Compare
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.
arjunshajitech
force-pushed
the
fix-new-interceptor
branch
from
d976f64 to
b78d61e
Compare
|
|
||
| if !correctSize { | ||
| return nil, fmt.Errorf("%w: %d is not a valid size, allowed sizes: %v", ErrInvalidSize, size, allowedSizes) | ||
| if err := rtpbuffer.IsBufferSizeValid(size); err != nil { |
There was a problem hiding this comment.
This now uses rtpbuffer.IsBufferSizeValid to check if we can create a receiveLog. What if we ever change the conditions for only one of them? Could we rename the IsBufferSizeValid function to IsPowerOfTwo and move it out of the rtpbuffer package? That way, it would be clear what it does, and if we change the conditions of either receiveLog or RTPBuffer, it would not affect the other.
3 participants
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 #...