-
Notifications
You must be signed in to change notification settings - Fork 125
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
force key frame implementation for vaapi #437
Conversation
@@ -545,6 +547,10 @@ func (e *encoderVP8) Controller() codec.EncoderController { | |||
return e | |||
} | |||
|
|||
func (e *encoderVP8) ForceKeyFrame() { |
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.
This interface has been replaced by func Controller() codec.EncoderController
https://github.com/pion/mediadevices/pull/338/files#diff-f1af839aa40e8d1553814897ef277f31f625ed6daf6e6f44bfeb950905efabb8R122-R146
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.
I don't understand, this function method already exists and returns the encoder itself.
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.
I also don't remember what I wanted to point and old CI log is gone.
The interface seems be correct.
Codecov ReportPatch has no changes to coverable lines. 📢 Thoughts on this report? Let us know!. |
pkg/codec/vaapi/vp8.go
Outdated
@@ -545,6 +547,10 @@ func (e *encoderVP8) Controller() codec.EncoderController { | |||
return e | |||
} | |||
|
|||
func (e *encoderVP8) ForceKeyFrame() { | |||
e.forceKeyFrame = true |
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.
It may cause data race if ForceKeyFrame()
is called from other goroutine during Read()
?
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.
I've changed the type to use an atomic.Bool
instead. This way, there is no data race possible.
pkg/codec/vaapi/vp8.go
Outdated
@@ -315,7 +317,7 @@ func (e *encoderVP8) Read() ([]byte, func(), error) { | |||
|
|||
e.frParam.data.framerate = C.uint(e.rate.Calc()) | |||
|
|||
if kf { | |||
if kf || e.forceKeyFrame { |
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.
forceKeyFrame
isn't reset, so every frames after ForceKeyFrame()
will be keyframe.
Hi @EmrysMyrddin. It looks like this one is close. Do you still want to get this in? |
8eb5e86
to
410e856
Compare
@edaniels I've merged master and applied suggestions if you want :-) |
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.
LGTM. Thanks!
@@ -100,6 +101,8 @@ type encoderVP8 struct { | |||
|
|||
rate *framerateDetector | |||
|
|||
forceKeyFrame atomic.Bool |
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.
Need to bump minimal supported go version to 1.19
Line 3 in b4770e5
go 1.13 |
Description
Force key frame implementation for the VAAPI in both VP8 and VP9 mode.