-
Notifications
You must be signed in to change notification settings - Fork 47
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
Add method to expose underlying Frame C struct #44
Conversation
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.
Since we can't use CGO in tests, could you add a simple test require.NotNil(t, f1.UnsafePointer())
here
frame.go
Outdated
@@ -213,3 +213,7 @@ func (f *Frame) Unref() { | |||
func (f *Frame) MoveRef(src *Frame) { | |||
C.av_frame_move_ref(f.c, src.c) | |||
} | |||
|
|||
func (f *Frame) Inner() unsafe.Pointer { |
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.
Could you rename this function to UnsafePointer
?
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.
As discussed below, if we were to return *C.struct_AVFrame
, do you still recommend calling the method UnsafePointer
? I do not have a particular preference.
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.
Thing is I want developers to understand that once they use this method they're on their own and this library could present chaotic behavior. Therefore I want to prefix this new method with Unsafe
, and thought Pointer
was clear enough to explain what it does. If you have another idea for the suffix, let me know.
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.
That was a fair consideration. Again, I don't have a particular preference, so let's go with UnsafePointer
.
frame.go
Outdated
@@ -213,3 +213,7 @@ func (f *Frame) Unref() { | |||
func (f *Frame) MoveRef(src *Frame) { | |||
C.av_frame_move_ref(f.c, src.c) | |||
} | |||
|
|||
func (f *Frame) Inner() unsafe.Pointer { |
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.
Is there a reason why we wouldn't want to return a *C.struct_AVFrame
here? 🤔
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 guess it was just a matter of my preference. Now that I think about it, returning *C.struct_AVFrame
does offer more clarity and type safety. So let's go with that?
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 honestly don't know whether returning *C.struct_AVFrame
is a bad idea, but it feels better so let's go with that 👍
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.
SGTM
@asticode All fixes implemented |
Thanks for the PR! ❤️ |
This PR address issue #43