Conversation
AkimioJR
commented
Feb 7, 2026
AkimioJR
commented
- 通过 cgo.Handle 传递回调函数，减少读写锁开销
- 使用switch替换if-else组合
There was a problem hiding this comment.
Pull request overview
该 PR 旨在优化 Go 绑定层回调传递方式：用 cgo.Handle 直接把 Go 回调对象作为 user_data 传给 C 层，从而移除全局回调表与读写锁开销，并顺带将 if-else 事件分发改为 switch。
Changes:
- 使用
cgo.Handle替代全局map[int]Callback+RWMutex的回调管理方式 goCallback中用switch分发事件类型，简化控制流- 在
Destroy/Play中增加对cgo.Handle的Delete()生命周期管理
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| callback := cgo.Handle(uintptr(userData)).Value().(Callback) | ||
|
|
||
| switch event { |
There was a problem hiding this comment.
cgo.Handle(uintptr(userData)).Value().(Callback) will panic if userData is nil/0, if the handle has already been deleted, or if the stored value is a nil interface / wrong type. Panics crossing the cgo boundary will crash the process. Add guards (e.g., return early on userData == nil, use a safe type assertion, and consider recovering from runtime/cgo: misuse of an invalid Handle).
| if player.cbHandle != 0 { | ||
| player.cbHandle.Delete() | ||
| } | ||
| player.cbHandle = cgo.NewHandle(callback) | ||
|
|
||
| C.crp_play(player.handle, curl, &coption, C.crp_callback(C.goCallback), unsafe.Pointer(uintptr(id))) | ||
| C.crp_play(player.handle, curl, &coption, C.crp_callback(C.goCallback), unsafe.Pointer(player.cbHandle)) |
There was a problem hiding this comment.
Deleting and replacing cbHandle inside Play can leave the native player still invoking callbacks with the old userData (e.g., if a previous stream is still running, or if StreamPuller::start refuses to start when already playing and keeps the old callback/userData). In that case goCallback will receive a deleted handle and panic. Ensure the native side is fully stopped before deleting the existing handle (e.g., call Stop() first and only delete after stop completes), or avoid deleting until you know no more callbacks can occur.
| if player.cbHandle != 0 { | ||
| player.cbHandle.Delete() | ||
| } | ||
| player.cbHandle = cgo.NewHandle(callback) | ||
|
|
||
| C.crp_play(player.handle, curl, &coption, C.crp_callback(C.goCallback), unsafe.Pointer(uintptr(id))) | ||
| C.crp_play(player.handle, curl, &coption, C.crp_callback(C.goCallback), unsafe.Pointer(player.cbHandle)) |
There was a problem hiding this comment.
There is existing Go test coverage for Play, but the new cgo.Handle lifecycle behavior (replacing/deleting handles, repeated Play calls, Play after Stop, etc.) isn’t covered. Adding a test that calls Play twice (or Play → Stop → Play) and asserts no panic/crash would help prevent regressions in handle lifetime management.
| player.cbHandle = cgo.NewHandle(callback) | ||
|
|
||
| C.crp_play(player.handle, curl, &coption, C.crp_callback(C.goCallback), unsafe.Pointer(uintptr(id))) | ||
| C.crp_play(player.handle, curl, &coption, C.crp_callback(C.goCallback), unsafe.Pointer(player.cbHandle)) |
There was a problem hiding this comment.
unsafe.Pointer(player.cbHandle) is not a safe/valid way to pass a cgo.Handle through C void*. cgo.Handle is a defined uintptr type; this conversion will not compile and/or will be incorrect on some platforms. Convert via uintptr explicitly (and keep the contract that C treats it as an opaque value and only passes it back).
| C.crp_play(player.handle, curl, &coption, C.crp_callback(C.goCallback), unsafe.Pointer(player.cbHandle)) | |
| C.crp_play(player.handle, curl, &coption, C.crp_callback(C.goCallback), unsafe.Pointer(uintptr(player.cbHandle))) |
