Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
33 changes: 18 additions & 15 deletions internal/rtpbuffer/rtpbuffer.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,7 @@
// Package rtpbuffer provides a buffer for storing RTP packets
package rtpbuffer

import (
"fmt"
)
import "fmt"

const (
// Uint16SizeHalf is half of a math.Uint16.
Expand All @@ -26,25 +24,30 @@ type RTPBuffer struct {

// NewRTPBuffer constructs a new RTPBuffer.
func NewRTPBuffer(size uint16) (*RTPBuffer, error) {
allowedSizes := make([]uint16, 0)
correctSize := false
if err := IsBufferSizeValid(size); err != nil {
return nil, err
}

return &RTPBuffer{
packets: make([]*RetainablePacket, size),
size: size,
}, nil
}

func IsBufferSizeValid(size uint16) error {
for i := 0; i < 16; i++ {
if size == 1<<i {
correctSize = true

break
return nil
}
allowedSizes = append(allowedSizes, 1<<i)
}

if !correctSize {
return nil, fmt.Errorf("%w: %d is not a valid size, allowed sizes: %v", ErrInvalidSize, size, allowedSizes)
// Only build allowedSizes if we actually need the error
allowedSizes := make([]uint16, 0)
for i := 0; i < 16; i++ {
allowedSizes = append(allowedSizes, 1<<i)
}

return &RTPBuffer{
packets: make([]*RetainablePacket, size),
size: size,
}, nil
return fmt.Errorf("%w: %d is not a valid size, allowed sizes: %v", ErrInvalidSize, size, allowedSizes)
}

// Add places the RetainablePacket in the RTPBuffer.
Expand Down
3 changes: 2 additions & 1 deletion pkg/nack/generator_interceptor.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"time"

"github.com/pion/interceptor"
"github.com/pion/interceptor/internal/rtpbuffer"
"github.com/pion/logging"
"github.com/pion/rtcp"
)
Expand Down Expand Up @@ -39,7 +40,7 @@ func (g *GeneratorInterceptorFactory) NewInterceptor(_ string) (interceptor.Inte
}
}

if _, err := newReceiveLog(generatorInterceptor.size); err != nil {
if err := rtpbuffer.IsBufferSizeValid(generatorInterceptor.size); err != nil {
return nil, err
}

Expand Down
16 changes: 2 additions & 14 deletions pkg/nack/receive_log.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
package nack

import (
"fmt"
"sync"

"github.com/pion/interceptor/internal/rtpbuffer"
Expand All @@ -20,19 +19,8 @@ type receiveLog struct {
}

func newReceiveLog(size uint16) (*receiveLog, error) {
allowedSizes := make([]uint16, 0)
correctSize := false
for i := 6; i < 16; i++ {
if size == 1<<i {
correctSize = true

break
}
allowedSizes = append(allowedSizes, 1<<i)
}

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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

return nil, err
}

return &receiveLog{
Expand Down
2 changes: 1 addition & 1 deletion pkg/nack/responder_interceptor.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ func (r *ResponderInterceptorFactory) NewInterceptor(_ string) (interceptor.Inte
responderInterceptor.packetFactory = rtpbuffer.NewPacketFactoryCopy()
}

if _, err := rtpbuffer.NewRTPBuffer(responderInterceptor.size); err != nil {
if err := rtpbuffer.IsBufferSizeValid(responderInterceptor.size); err != nil {
return nil, err
}

Expand Down
Loading