Avoid RTPBuffer allocation - #390
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #390 +/- ##
==========================================
- Coverage 81.52% 81.33% -0.20%
==========================================
Files 88 89 +1
Lines 4656 4650 -6
==========================================
- Hits 3796 3782 -14
- Misses 684 688 +4
- Partials 176 180 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 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.
b93395d to
d976f64
Compare
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.
There was a problem hiding this comment.
@mengelbart I’ve updated it to use a separate IsPowerOfTwo helper instead of rtpbuffer.IsBufferSizeValid
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.
b78d61e to
c3955b7
Compare
c3955b7 to
8e2a58f
Compare
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 #...