-
Notifications
You must be signed in to change notification settings - Fork 123
[RSDK-6469] Set x264 bitrate based on resolution and fps #4769
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 6 commits
7eaf687
1780a0c
11f81ae
3573204
4bbc895
e789a17
c11fb11
86affca
9f8c7f7
422a5e4
590fd98
02246d4
8419c01
80fe632
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -6,6 +6,17 @@ import ( | |||||||
"go.viam.com/rdk/logging" | ||||||||
) | ||||||||
|
||||||||
const ( | ||||||||
encodeCompressionRatio = 0.15 // bits per pixel when encoded | ||||||||
// For very small resolutions, we need to ensure that the vbv buffer size is large enough to | ||||||||
// handle frame bursts. This is the minimum bitrate that we can use without causing the encoder | ||||||||
// to spew out warnings about the buffer size being too small. | ||||||||
minBitrate = 300_000 // 300kbps | ||||||||
// Setting a reasonable max bitrate to prevent the encoder from using too much bandwidth. | ||||||||
// 4K resolution at 20fps is around 24.8Mbps. | ||||||||
maxBitrate = 25_000_000 // 25Mbps | ||||||||
) | ||||||||
|
||||||||
// DefaultStreamConfig configures x264 as the encoder for a stream. | ||||||||
var DefaultStreamConfig gostream.StreamConfig | ||||||||
|
||||||||
|
@@ -27,3 +38,15 @@ func (f *factory) New(width, height, keyFrameInterval int, logger logging.Logger | |||||||
func (f *factory) MIMEType() string { | ||||||||
return "video/H264" | ||||||||
} | ||||||||
|
||||||||
// calcBitrateFromResolution calculates the bitrate based on the given resolution and framerate. | ||||||||
func calcBitrateFromResolution(width, height int, framerate float32) int { | ||||||||
bitrate := int(float32(width) * float32(height) * framerate * encodeCompressionRatio) | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does bitrate scale linearly with width and height? Apparently the answer is complicated https://www.reddit.com/r/explainlikeimfive/comments/hlrwcu/eli5_what_is_the_relationship_between_bit_rate/ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am currently scaling the bitrate linearly with the number of pixels. Interesting graphs in the thread that show logarithmic quality gains with increasing bitrate for a given size/fps. Will try to verify that we are hitting these sweet-spots. The amount of scene change also comes into play here so it gets quite complicated. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you look at the example bitrates in the description we seem to be pretty bang on to the recommended settings linked in the jira ticket: https://support.google.com/youtube/answer/2853702?hl=en There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could result in a float? Does that break the stream sever? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||||||||
if bitrate < minBitrate { | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good, added comment. |
||||||||
return minBitrate | ||||||||
} | ||||||||
if bitrate > maxBitrate { | ||||||||
return maxBitrate | ||||||||
} | ||||||||
return bitrate | ||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -507,11 +507,16 @@ func (server *Server) AddNewStreams(ctx context.Context) error { | |
server.logger.Warn("video streaming not supported on Windows yet") | ||
break | ||
} | ||
framerate, err := server.getFramerateFromCamera(name) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will this new logic break streaming for cameras that don't provide framerate in getProperties? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will add comment here -- encoder pipeline will default to 30fps in bitrate calculation (the same as the key-frame interval) if it cant pick up framerate. |
||
if err != nil { | ||
server.logger.Debugf("error getting framerate from camera %q: %v", name, err) | ||
} | ||
// We walk the updated set of `videoSources` and ensure all of the sources are "created" and | ||
// "started". | ||
config := gostream.StreamConfig{ | ||
Name: name, | ||
VideoEncoderFactory: server.streamConfig.VideoEncoderFactory, | ||
TargetFrameRate: framerate, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if this is 0? Can we add a test? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Will see about adding a test for this. |
||
} | ||
// Call `createStream`. `createStream` is responsible for first checking if the stream | ||
// already exists. If it does, it skips creating a new stream and we continue to the next source. | ||
|
@@ -758,6 +763,18 @@ func (server *Server) startAudioStream(ctx context.Context, source gostream.Audi | |
}) | ||
} | ||
|
||
func (server *Server) getFramerateFromCamera(name string) (int, error) { | ||
cam, err := camera.FromRobot(server.robot, name) | ||
if err != nil { | ||
return 0, fmt.Errorf("failed to get camera from robot: %w", err) | ||
} | ||
props, err := cam.Properties(context.Background()) | ||
if err != nil { | ||
return 0, fmt.Errorf("failed to get camera properties: %w", err) | ||
} | ||
return int(props.FrameRate), nil | ||
} | ||
|
||
// GenerateResolutions takes the original width and height of an image and returns | ||
// a list of the original resolution with 4 smaller width/height options that maintain | ||
// the same aspect ratio. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't comment above - but I just realized - if the initial resolution is odd - x264 could break could we add a check for the initial resolution not being odd as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call -- added dimension check to
NewEncoder
.